Task/new osgi annotations #312
Conversation
| import org.osgi.framework.Constants; | ||
| import org.osgi.service.component.annotations.*; |
There was a problem hiding this comment.
Please don't use * imports.
core/worker/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.osgi</groupId> | ||
| <artifactId>org.osgi.service.metatype.annotations</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Dependency
<dependency>
<groupId>org.apache.felix</groupId>
<artifactId>org.apache.felix.scr.annotations</artifactId>
</dependency>is probably no longer necessary here?
| description = "Url to selenium grid hub. When null local Chrome driver will be used. Local Chrome driver does not work on Linux", | ||
| value = DEFAULT_SELENIUM_GRID_URL) | ||
| private String seleniumGridUrl; | ||
| private ChromeWebDriverFactoryConf chromeWebDriverFactoryConf; |
There was a problem hiding this comment.
In PRs for other modules I've seen convention of naming those properties config. Could we please keep it also here?
| public static final String PATH_DESC = "Custom path to driver binary"; | ||
|
|
||
| public static final String SELENIUM_GRID_URL = "seleniumGridUrl"; | ||
| public static final String LOG_FILE_PATH_LABEL = "Path to firefox error log"; | ||
|
|
||
| public static final String SELENIUM_GRID_URL_LABEL = "Selenium grid URL"; |
There was a problem hiding this comment.
It look that those constants are only used in WebDriverProviderConf. I believe they should be defined here.
This WebDriverHelper look like generic class - so there should be no constants that have Firefox or Chrome words in it.
| @@ -1,81 +1,58 @@ | |||
| /** | |||
| * AET | |||
| * | |||
| * <p> | |||
There was a problem hiding this comment.
Please remove this formatting changes form license header.
| } | ||
|
|
||
| public WebCommunicationWrapper createWebDriverWithProxy(String preferredWebDriver, String proxyName) | ||
| public WebCommunicationWrapper createWebDriverWithProxy(String preferredWebDriver, |
There was a problem hiding this comment.
For the future could you please don't change formatting on the lines that didn't change during the refactoring?
Please create additional PR to fix only formatting.
|
|
||
| @Property(name = DEFAULT_WEB_DRIVER_NAME, label = "Default Web Driver name", value = "ff") | ||
| private String defaultWebDriverName; | ||
| private WebDriverProviderConfig webDriverProviderConfig; |
There was a problem hiding this comment.
In PRs for other modules I've seen convention of naming those properties config. Could we please keep it also here and other classes in this PR?
There was a problem hiding this comment.
You mean changing this to
private WebDriverProviderConfig config?
There was a problem hiding this comment.
Yes, and all other XYZConfig fields in other services.
core/worker/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.osgi</groupId> | ||
| <artifactId>org.osgi.service.metatype.annotations</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
You can also remove:
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.commons.osgi</artifactId>
</dependency>There was a problem hiding this comment.
No, I can't - WebDriverHelper uses PropertiesUtil from this package.
There was a problem hiding this comment.
Maybe -> return StringUtils.defaultString(properties.get(name), defaultValue); at https://github.com/Cognifide/aet/blob/a024722d58c9704720199560a19ae4897058f555/core/worker/src/main/java/com/cognifide/aet/worker/drivers/WebDriverHelper.java#L39
There was a problem hiding this comment.
Is this method used (WebDriverHelper.getProp)? Some of this method references were removed from ChromeWebDriverFactory.activate.
Description
Migrate Felix annotations to OSGi in Worker module
Motivation and Context
Screenshots (if appropriate):
Types of changes
Checklist:
I hereby agree to the terms of the AET Contributor License Agreement.