You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by klopfdreh <gi...@git.apache.org> on 2018/04/12 07:09:21 UTC
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
GitHub user klopfdreh opened a pull request:
https://github.com/apache/wicket/pull/275
[WICKET-6544] mobile browser detection is improved (with YAUAA)
Hi all,
here my 5 cent regarding the browser detection with https://github.com/nielsbasjes/yauaa
you have to extend the test to see the real benefit.
kind regards
Tobias
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/klopfdreh/wicket browser_version_detection
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/wicket/pull/275.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #275
----
commit 96bab261b5c9e96393737444ac9601eda97ef7bc
Author: Tobias Soloschenko <ts...@...>
Date: 2018-04-12T07:05:19Z
Browser / Version detection
----
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/275
I believe we can add browser detection as wicketstuff module and/or as confluence example
Should be easy and useful
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
Due to the vote that Wicket is going to drop the support of Browser detection I am closing this branch right now.
@solomax - yes why not a wicketstuff module. 👍
/closed
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181286230
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -193,165 +198,117 @@ protected String getRemoteAddr(RequestCycle requestCycle)
*/
private void init()
{
- setInternetExplorerProperties();
- setOperaProperties();
- setMozillaProperties();
- setKonquerorProperties();
- setChromeProperties();
- setEdgeProperties();
- setSafariProperties();
+ nl.basjes.parse.useragent.UserAgent parsedUserAgent = UAA.parse(getUserAgent());
+ setInternetExplorerProperties(parsedUserAgent);
+ setKonquerorProperties(parsedUserAgent);
+ setMozillaProperties(parsedUserAgent);
+ setOperaProperties(parsedUserAgent);
+ setChromeProperties(parsedUserAgent);
+ setEdgeProperties(parsedUserAgent);
+ setSafariProperties(parsedUserAgent);
--- End diff --
Hi @solomax - yes indeed - it was due to the refactoring and those methods were present before - I just refactored them.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181328485
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -46,14 +46,20 @@
/**
* The user agent string from the User-Agent header, app. Theoretically, this might differ from
- * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property, which is
- * not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is set.
+ * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property,
+ * which is not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is
+ * set.
*/
private final String userAgent;
/** Client properties object. */
private final ClientProperties properties;
+ private final static UserAgentAnalyzer UAA = UserAgentAnalyzer.newBuilder()
--- End diff --
Have a look at the recent changes. A lot of stuff changed already and I think it is ok like it is now (except the tests)
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/275
@klopfdreh can you create wicketstuff module?
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/275
My concerns still hold :(
Most importantly: with that single UserAgentAnalyzer instance you've introduced a bottleneck for all concurrent request that need to gatherExtendedInfo().
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181297750
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
Will try to provide patch later tonight
Have to do day time job right now :(
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r184288167
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -133,24 +140,99 @@ public final String getUserAgent()
}
/**
- * returns the user agent string (lower case).
+ * Initializes the {@link WebClientInfo} user agent detection. This can be overridden to choose
+ * a different detection as YAUAA (https://github.com/nielsbasjes/yauaa) - if you do so, you
+ * might exclude the maven dependency from your project in favor of a different framework.
+ */
--- End diff --
No, you can't exclude the dependency - with UserAgentAnalyzer in the signature of #detectBrowserProperties(UserAgentAnalyzer) you will get a NoClassDefFoundError if the dependency is not present.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181282313
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -193,165 +198,117 @@ protected String getRemoteAddr(RequestCycle requestCycle)
*/
private void init()
{
- setInternetExplorerProperties();
- setOperaProperties();
- setMozillaProperties();
- setKonquerorProperties();
- setChromeProperties();
- setEdgeProperties();
- setSafariProperties();
+ nl.basjes.parse.useragent.UserAgent parsedUserAgent = UAA.parse(getUserAgent());
+ setInternetExplorerProperties(parsedUserAgent);
+ setKonquerorProperties(parsedUserAgent);
+ setMozillaProperties(parsedUserAgent);
+ setOperaProperties(parsedUserAgent);
+ setChromeProperties(parsedUserAgent);
+ setEdgeProperties(parsedUserAgent);
+ setSafariProperties(parsedUserAgent);
--- End diff --
All these methods looks very much the same ....
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181295733
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
Yes that is true, but I think if you change the code / the browser detection you are going to investigate why the test is failing and fix the integration as long as every test is back working again.
@BeforeClass is not working, because WicketTestCase and at this stage no Application is attached to the main thread.
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/275
OK :) I'll merge my changes
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181224559
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -46,14 +46,20 @@
/**
* The user agent string from the User-Agent header, app. Theoretically, this might differ from
- * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property, which is
- * not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is set.
+ * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property,
+ * which is not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is
+ * set.
*/
private final String userAgent;
/** Client properties object. */
private final ClientProperties properties;
+ private final static UserAgentAnalyzer UAA = UserAgentAnalyzer.newBuilder()
--- End diff --
UserAgentAnalyzer isn't reentrant. Since it is expensive to create one, instances have to be pooled.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181294275
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
All subsequent lines will not be executed in case of failure ....
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r182993206
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
Sorry for delay, here is the code works for me:
```
private static ThreadLocal<UserAgentAnalyzer> localAnalyzer = new ThreadLocal<UserAgentAnalyzer>();
@Before
public void before()
{
requestCycleMock = mock(RequestCycle.class);
webRequest = mock(ServletWebRequest.class);
when(requestCycleMock.getRequest()).thenReturn(webRequest);
servletRequest = mock(HttpServletRequest.class);
when(webRequest.getContainerRequest()).thenReturn(servletRequest);
if (localAnalyzer.get() == null) {
WebClientInfo webClientInfo = new WebClientInfo(requestCycleMock, "test");
webClientInfo.gatherExtendedInfo();
localAnalyzer.set(Application.get().getMetaData(WebClientInfo.UAA_META_DATA_KEY));
} else {
Application.get().setMetaData(WebClientInfo.UAA_META_DATA_KEY, localAnalyzer.get());
}
}
@AfterClass
public static void staticAfter()
{
localAnalyzer.set(null);
}
```
Here are the measurment results:
```
Combined tests
real 0m24.123s
user 1m30.748s
sys 0m1.804s
Individual tests (after above changes were applied)
real 0m22.844s
user 1m23.948s
sys 0m1.832s
```
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181293718
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
You will see if something fails in the test results btw.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh closed the pull request at:
https://github.com/apache/wicket/pull/275
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by nielsbasjes <gi...@git.apache.org>.
Github user nielsbasjes commented on the issue:
https://github.com/apache/wicket/pull/275
@klopfdreh
A few points about the slow startup.
When you explicitly specify which fields you really need the system will completely 'not load' the rules that have no impact. Especially if you do not need the DeviceBrand/DeviceName the impact will be quite large. You can simply pass something like this .withField("AgentName") .withField("DeviceClass") .withField("AgentVersion") and it will have a big impact on both startup times and run times for a not yet cached parse.
I had a look at your project and with this patch the startup time drops to less than 1.5 seconds.
```
diff --git pom.xml pom.xml
index 00cc8f8..7910bd9 100644
--- pom.xml
+++ pom.xml
@@ -146,7 +146,7 @@
<dependency>
<groupId>nl.basjes.parse.useragent</groupId>
<artifactId>yauaa</artifactId>
- <version>4.2</version>
+ <version>4.5</version>
</dependency>
<dependency>
<groupId>com.google.code.findbugs</groupId>
diff --git wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java
index 5a02a3d..1e488e9 100644
--- wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java
+++ wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java
@@ -151,6 +151,9 @@ public class WebClientInfo extends ClientInfo
{
userAgentAnalyzer = UserAgentAnalyzer.newBuilder()
.hideMatcherLoadStats()
+ .dropTests()
+ .withField("AgentName")
+ .withField("AgentVersion")
.withCache(25000)
.build();
Application.get().setMetaData(UAA_META_DATA_KEY, userAgentAnalyzer);
@@ -217,7 +220,7 @@ public class WebClientInfo extends ClientInfo
*/
protected void setBrowserVersion(nl.basjes.parse.useragent.UserAgent parsedUserAgent)
{
- String value = parsedUserAgent.get("AgentVersion").getValue();
+ String value = parsedUserAgent.getValue("AgentVersion");
if (!"Hacker".equals(value))
{
properties.setBrowserVersion(value);
```
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181293658
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
This might slow down the test execution because of the init of YAUAA - try it out.
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/275
Hi Niels,
thanks for chiming in, but just today we decided to drop all code related to user agent detection, as Wicket itself doesn't need/utilizes it.
Actually your remarks affirm my position that user agent detection is hard business and Wicket shouldn't hide the details and configuration behind our very broad API.
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
What about merging this? Or do we proceed with this PR after the release @bitstorm ?
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181286646
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -46,14 +46,20 @@
/**
* The user agent string from the User-Agent header, app. Theoretically, this might differ from
- * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property, which is
- * not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is set.
+ * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property,
+ * which is not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is
+ * set.
*/
private final String userAgent;
/** Client properties object. */
private final ClientProperties properties;
+ private final static UserAgentAnalyzer UAA = UserAgentAnalyzer.newBuilder()
--- End diff --
The initialization is expensive, but the UserAgentAnalyzer is thread safe so it can be reused. Maybe we are able to use MetaDataKey
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
To create UserAgentAnalyzer takes rather a long time. Maybe we are able to find a way to resolve the bottleneck, but my concerns to implement the browser detection in such a bad way like it was before also still hold.
Also when I see the benchmarks I would consider to just inform about the bottleneck and keep it that way, or to turn it gatherExtendedInfo() off if 0.011ms are to much.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r184343253
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -133,24 +140,99 @@ public final String getUserAgent()
}
/**
- * returns the user agent string (lower case).
+ * Initializes the {@link WebClientInfo} user agent detection. This can be overridden to choose
+ * a different detection as YAUAA (https://github.com/nielsbasjes/yauaa) - if you do so, you
+ * might exclude the maven dependency from your project in favor of a different framework.
+ */
--- End diff --
Ok, then keep the dependency and just override it. We have to adjust the documentation here.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181282823
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -193,165 +198,117 @@ protected String getRemoteAddr(RequestCycle requestCycle)
*/
private void init()
{
- setInternetExplorerProperties();
- setOperaProperties();
- setMozillaProperties();
- setKonquerorProperties();
- setChromeProperties();
- setEdgeProperties();
- setSafariProperties();
+ nl.basjes.parse.useragent.UserAgent parsedUserAgent = UAA.parse(getUserAgent());
+ setInternetExplorerProperties(parsedUserAgent);
+ setKonquerorProperties(parsedUserAgent);
+ setMozillaProperties(parsedUserAgent);
+ setOperaProperties(parsedUserAgent);
+ setChromeProperties(parsedUserAgent);
+ setEdgeProperties(parsedUserAgent);
+ setSafariProperties(parsedUserAgent);
--- End diff --
can be replaced with
String agent = parsedUserAgent.getValue("AgentName");
properties.setBrowserKonqueror(UserAgent.KONQUEROR.getUaStrings().contains(agent));
properties.setBrowserChrome(UserAgent.CHROME.getUaStrings().contains(agent));
.................
setBrowserVersion(parsedUserAgent);
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r182987156
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -143,215 +150,133 @@ private String getUserAgentStringLc()
}
/**
- * When using ProxyPass, requestCycle().getHttpServletRequest(). getRemoteAddr() returns the IP
- * of the machine forwarding the request. In order to maintain the clients ip address, the
- * server places it in the <a
- * href="http://httpd.apache.org/docs/2.2/mod/mod_proxy.html#x-headers">X-Forwarded-For</a>
- * Header.
- *
- * Proxies may also mask the original client IP with tokens like "hidden" or "unknown".
- * If so, the last proxy ip address is returned.
- *
- * @param requestCycle
- * the request cycle
- * @return remoteAddr IP address of the client, using the X-Forwarded-For header and defaulting
- * to: getHttpServletRequest().getRemoteAddr()
+ * Initializes the {@link WebClientInfo} user agent detection. This can be overridden to choose
+ * a different detection as YAUAA (https://github.com/nielsbasjes/yauaa) - if you do so, you
+ * might exclude the maven dependency from your project in favor of a different framework.
*/
- protected String getRemoteAddr(RequestCycle requestCycle)
+ public void gatherExtendedInfo()
{
- ServletWebRequest request = (ServletWebRequest)requestCycle.getRequest();
- HttpServletRequest req = request.getContainerRequest();
- String remoteAddr = request.getHeader("X-Forwarded-For");
-
- if (remoteAddr != null)
- {
- if (remoteAddr.contains(","))
- {
- // sometimes the header is of form client ip,proxy 1 ip,proxy 2 ip,...,proxy n ip,
- // we just want the client
- remoteAddr = Strings.split(remoteAddr, ',')[0].trim();
- }
- try
- {
- // If ip4/6 address string handed over, simply does pattern validation.
- InetAddress.getByName(remoteAddr);
- }
- catch (UnknownHostException e)
- {
- remoteAddr = req.getRemoteAddr();
- }
- }
- else
+ UserAgentAnalyzer userAgentAnalyzer = Application.get().getMetaData(UAA_META_DATA_KEY);
+ if (userAgentAnalyzer == null)
{
- remoteAddr = req.getRemoteAddr();
+ userAgentAnalyzer = UserAgentAnalyzer.newBuilder()
+ .hideMatcherLoadStats()
+ .withCache(25000)
+ .build();
+ Application.get().setMetaData(UAA_META_DATA_KEY, userAgentAnalyzer);
}
- return remoteAddr;
+ detectBrowserProperties(userAgentAnalyzer);
}
/**
- * Initialize the client properties object
+ * Detects browser properties like versions or the type of the browser and applies them to the
+ * {@link ClientProperties}, override this method if there are errors within the browser /
+ * version detection due to newer browsers
+ *
+ * @param userAgentAnalyzer
+ * the user agent analyzer to detect browsers and versions
*/
- private void init()
+ protected void detectBrowserProperties(UserAgentAnalyzer userAgentAnalyzer)
{
- setInternetExplorerProperties();
- setOperaProperties();
- setMozillaProperties();
- setKonquerorProperties();
- setChromeProperties();
- setEdgeProperties();
- setSafariProperties();
- log.debug("determined user agent: {}", properties);
- }
+ nl.basjes.parse.useragent.UserAgent parsedUserAgent = userAgentAnalyzer
+ .parse(getUserAgent());
+ String userAgentName = parsedUserAgent.getValue("AgentName");
- /**
- * sets the konqueror specific properties
- */
- private void setKonquerorProperties()
- {
- properties.setBrowserKonqueror(UserAgent.KONQUEROR.matches(getUserAgent()));
+ // Konqueror
+ properties.setBrowserKonqueror(UserAgent.KONQUEROR.getUaStrings().contains(userAgentName));
- if (properties.isBrowserKonqueror())
- {
- // e.g.: Mozilla/5.0 (compatible; Konqueror/4.2; Linux) KHTML/4.2.96 (like Gecko)
- setMajorMinorVersionByPattern("konqueror/(\\d+)\\.(\\d+)");
- }
- }
+ // Chrome
+ properties.setBrowserChrome(UserAgent.CHROME.getUaStrings().contains(userAgentName));
- /**
- * sets the chrome specific properties
- */
- private void setChromeProperties()
- {
- properties.setBrowserChrome(UserAgent.CHROME.matches(getUserAgent()));
+ // Edge
+ properties.setBrowserEdge(UserAgent.EDGE.getUaStrings().contains(userAgentName));
- if (properties.isBrowserChrome())
- {
- // e.g.: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/534.24 (KHTML, like Gecko)
-// Chrome/12.0.702.0 Safari/534.24
- setMajorMinorVersionByPattern("chrome/(\\d+)\\.(\\d+)");
- }
- }
+ // Safari
+ properties.setBrowserSafari(UserAgent.SAFARI.getUaStrings().contains(userAgentName));
- /**
- * sets the Edge specific properties
- */
- private void setEdgeProperties()
- {
- properties.setBrowserEdge(UserAgent.EDGE.matches(getUserAgent()));
+ // Opera
+ properties.setBrowserOpera(UserAgent.OPERA.getUaStrings().contains(userAgentName));
+
+ // Internet Explorer
+ properties.setBrowserInternetExplorer(
+ UserAgent.INTERNET_EXPLORER.getUaStrings().contains(userAgentName));
- if (properties.isBrowserEdge())
+ // FireFox
+ boolean isFireFox = UserAgent.FIREFOX.getUaStrings().contains(userAgentName);
+ if (isFireFox)
{
- // e.g.: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko)
- // Chrome/52.0.2743.116 Safari/537.36 Edge/15.15063
- setMajorMinorVersionByPattern("edge/(\\d+)\\.(\\d+)");
+ properties.setBrowserMozillaFirefox(true);
+ properties.setBrowserMozilla(true);
}
- }
-
- /**
- * sets the safari specific properties
- */
- private void setSafariProperties()
- {
- properties.setBrowserSafari(UserAgent.SAFARI.matches(getUserAgent()));
-
- if (properties.isBrowserSafari())
+ else
{
- String userAgent = getUserAgentStringLc();
--- End diff --
`getUserAgentStringLc` is private and not used anymore
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by nielsbasjes <gi...@git.apache.org>.
Github user nielsbasjes commented on the issue:
https://github.com/apache/wicket/pull/275
On a more general note: I think the DeviceClass field might be useful for Wicket as it indicates if the device is a phone, tablet or desktop.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181293350
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -147,6 +149,93 @@ private String getUserAgentStringLc()
return (getUserAgent() != null) ? getUserAgent().toLowerCase() : "";
}
+ /**
+ * Initializes the {@link WebClientInfo} user agent detection. This can be overridden to choose
+ * a different detection as YAUAA (https://github.com/nielsbasjes/yauaa) - if you do so, you
+ * might exclude the maven dependency from your project in favor of a different framework.
+ */
+ public void initialize()
+ {
+ UserAgentAnalyzer userAgentAnalyzer = Application.get().getMetaData(UAA_META_DATA_KEY);
+ if (userAgentAnalyzer == null)
+ {
+ userAgentAnalyzer = UserAgentAnalyzer.newBuilder()
+ .hideMatcherLoadStats()
+ .withCache(25000)
+ .build();
+ Application.get().setMetaData(UAA_META_DATA_KEY, userAgentAnalyzer);
+ }
+ detectBrowserProperties(userAgentAnalyzer);
+ }
+
+ /**
+ * Detects browser properties like versions or the type of the browser and applies them to the
+ * {@link ClientProperties}, override this method if there are errors within the browser /
+ * version detection due to newer browsers
+ *
+ * @param userAgentAnalyzer
+ * the user agent analyzer to detect browsers and versions
+ */
+ protected void detectBrowserProperties(UserAgentAnalyzer userAgentAnalyzer)
+ {
+
+ nl.basjes.parse.useragent.UserAgent parsedUserAgent = userAgentAnalyzer
+ .parse(getUserAgent());
+ String userAgentName = parsedUserAgent.getValue("AgentName");
+
+ // Konqueror
+ properties.setBrowserKonqueror(UserAgent.KONQUEROR.getUaStrings().contains(userAgentName));
+
+ // Chrome
+ properties.setBrowserChrome(UserAgent.CHROME.getUaStrings().contains(userAgentName));
+
+ // Edge
+ properties.setBrowserEdge(UserAgent.EDGE.getUaStrings().contains(userAgentName));
+
+ // Safari
+ properties.setBrowserSafari(UserAgent.SAFARI.getUaStrings().contains(userAgentName));
+
+ // Opera
+ properties.setBrowserOpera(UserAgent.OPERA.getUaStrings().contains(userAgentName));
+
+ // Internet Explorer
+ properties.setBrowserInternetExplorer(
+ UserAgent.INTERNET_EXPLORER.getUaStrings().contains(userAgentName));
+
+ // FireFox
+ boolean isFireFox = UserAgent.FIREFOX.getUaStrings()
+ .contains(parsedUserAgent.getValue("AgentName"));
+ if (isFireFox)
+ {
+ properties.setBrowserMozillaFirefox(true);
+ properties.setBrowserMozilla(true);
+ }
+ else
+ {
+ properties.setBrowserMozilla(
+ UserAgent.MOZILLA.getUaStrings().contains(parsedUserAgent.getValue("AgentName")));
--- End diff --
`userAgentName`
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181296712
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
May you have any other good idea to keep the tests individual.
Just send a patch to me if something would fit the requirement and I am going to integrate it.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181293591
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
I would rather have all these as individual tests, to be able to see which one was failed
And run all of them regardless failure state of others
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
Following changes have been made:
* The init() method has been changed into gatherExtendedInfo() which is only be executed if org.apache.wicket.settings.RequestCycleSettings.setGatherExtendedBrowserInfo(boolean) is set to true
* MetaDataKey has been used to initialize YAUAA
* All browser specific methods have been moved into detectBrowserProperties
* gatherExtendedInfo / detectBrowserProperties are protected / public so that the user may override them
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
I send you an invitation to the repo in which the branch of the PR is located. just push to it and merge if you are done. I think we are on a good way with this integration.
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
Hey @solomax - sadly I am totally busy right now, but I try my best to change this.
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181303063
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
Hehe me too 😄 - no hurry, because I think this is going to be released in a further version.
---
[GitHub] wicket issue #275: [WICKET-6544] mobile browser detection is improved (with ...
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on the issue:
https://github.com/apache/wicket/pull/275
Hi @solomax - thanks a lot for the review. Because I am not able to work on it currently I am going to grant you access to my repo for further commits. Is this ok for you?
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181293318
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -147,6 +149,93 @@ private String getUserAgentStringLc()
return (getUserAgent() != null) ? getUserAgent().toLowerCase() : "";
}
+ /**
+ * Initializes the {@link WebClientInfo} user agent detection. This can be overridden to choose
+ * a different detection as YAUAA (https://github.com/nielsbasjes/yauaa) - if you do so, you
+ * might exclude the maven dependency from your project in favor of a different framework.
+ */
+ public void initialize()
+ {
+ UserAgentAnalyzer userAgentAnalyzer = Application.get().getMetaData(UAA_META_DATA_KEY);
+ if (userAgentAnalyzer == null)
+ {
+ userAgentAnalyzer = UserAgentAnalyzer.newBuilder()
+ .hideMatcherLoadStats()
+ .withCache(25000)
+ .build();
+ Application.get().setMetaData(UAA_META_DATA_KEY, userAgentAnalyzer);
+ }
+ detectBrowserProperties(userAgentAnalyzer);
+ }
+
+ /**
+ * Detects browser properties like versions or the type of the browser and applies them to the
+ * {@link ClientProperties}, override this method if there are errors within the browser /
+ * version detection due to newer browsers
+ *
+ * @param userAgentAnalyzer
+ * the user agent analyzer to detect browsers and versions
+ */
+ protected void detectBrowserProperties(UserAgentAnalyzer userAgentAnalyzer)
+ {
+
+ nl.basjes.parse.useragent.UserAgent parsedUserAgent = userAgentAnalyzer
+ .parse(getUserAgent());
+ String userAgentName = parsedUserAgent.getValue("AgentName");
+
+ // Konqueror
+ properties.setBrowserKonqueror(UserAgent.KONQUEROR.getUaStrings().contains(userAgentName));
+
+ // Chrome
+ properties.setBrowserChrome(UserAgent.CHROME.getUaStrings().contains(userAgentName));
+
+ // Edge
+ properties.setBrowserEdge(UserAgent.EDGE.getUaStrings().contains(userAgentName));
+
+ // Safari
+ properties.setBrowserSafari(UserAgent.SAFARI.getUaStrings().contains(userAgentName));
+
+ // Opera
+ properties.setBrowserOpera(UserAgent.OPERA.getUaStrings().contains(userAgentName));
+
+ // Internet Explorer
+ properties.setBrowserInternetExplorer(
+ UserAgent.INTERNET_EXPLORER.getUaStrings().contains(userAgentName));
+
+ // FireFox
+ boolean isFireFox = UserAgent.FIREFOX.getUaStrings()
+ .contains(parsedUserAgent.getValue("AgentName"));
--- End diff --
`userAgentName`
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181293918
--- Diff: wicket-core/src/test/java/org/apache/wicket/protocol/http/request/WebClientInfoTest.java ---
@@ -59,9 +58,52 @@ public void before()
}
/**
- * Test IE 6.x user-agent strings
+ * Test check check all user agents
*/
@Test
+ public void testBrowsersAndVersionsOfUserAgents() {
+ // Internet Explorers
--- End diff --
You can move init to `@BeforeClass` .....
---
[GitHub] wicket pull request #275: [WICKET-6544] mobile browser detection is improved...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/275#discussion_r181308489
--- Diff: wicket-core/src/main/java/org/apache/wicket/protocol/http/request/WebClientInfo.java ---
@@ -46,14 +46,20 @@
/**
* The user agent string from the User-Agent header, app. Theoretically, this might differ from
- * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property, which is
- * not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is set.
+ * {@link org.apache.wicket.protocol.http.ClientProperties#isNavigatorJavaEnabled()} property,
+ * which is not set until an actual reply from a browser (e.g. using {@link BrowserInfoPage} is
+ * set.
*/
private final String userAgent;
/** Client properties object. */
private final ClientProperties properties;
+ private final static UserAgentAnalyzer UAA = UserAgentAnalyzer.newBuilder()
--- End diff --
UserAgentAnalyzer#parse(UserAgent) is synchronized, please check its code. This is even mentioned on the project home page.
---