Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing some device ambiguity issues #1578

Merged
merged 1 commit into from
Sep 7, 2017

Conversation

jaydiablo
Copy link
Contributor

I was doing some analysis on the devices in browscap and found a few cases where the same token may represent different devices on the same platform.

This revealed some ambiguity issues that I figured I’d resolve (and a type-o for a device in the IE Mobile files).

@@ -160,7 +160,7 @@
"properties": {
"Device_Type": "Mobile Phone",
"Device_Code_Name": "801E",
"Device_Name": "One M7",
"Device_Name": "One M7 LTE",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an 801N apparently that isn't LTE (just 3G), so I figured it would be useful to add this in the name.

@@ -1057,7 +1057,7 @@
"properties": {
"Device_Type": "Mobile Phone",
"Device_Code_Name": "M7",
"Device_Name": "One",
"Device_Name": "One M7",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the M7 is just "HTC One", but I thought it was helpful to rename this to prevent any confusion.

@@ -597,19 +597,6 @@
"isTablet": "false"
}
},
"SonyEricsson LT22i": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any evidence that there was a SonyEricsson version of this in the UA strings, all of them were just "LT22i", and google searches seem to point to the Sony brand rather than SonyEricsson.

@@ -51,6 +64,19 @@
"isTablet": "true"
}
},
"Motorola MZ615": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two devices are the Wifi version of the Xoom 2 and Xoom 2 Media Edition, I did end up using one of these in these changes, the other was added for future use (perhaps).

@@ -76,5 +102,18 @@
"isMobileDevice": "true",
"isTablet": "true"
}
},
"Motorola Xoom 2 ME": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases there is just "Xoom 2 ME" in the UA, so this is a general purpose device where we can't decipher if it's the Wifi or 3G version.

@@ -810,7 +810,7 @@
"Vodafone SmartTab II 10": "Lenovo SmartTab II 10",
"X10": "SonyEricsson X10",
"XOLO A700": "XOLO A700",
"XOOM 2 ME": "Motorola MZ608",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed any of these that didn't have the explicit model name to the generic "Xoom 2" and "Xoom 2 ME" device.

@@ -1426,9 +1426,10 @@
]
},
{
"match": "Mozilla/5.0 (#PLATFORM#XOOM 2 Build/*#DEVICE#) applewebkit* (*khtml*like*gecko*) Version/#MAJORVER#.#MINORVER#*Safari*",
"match": "Mozilla/5.0 (#PLATFORM#XOOM 2* Build/*#DEVICE#*) applewebkit* (*khtml*like*gecko*) Version/#MAJORVER#.#MINORVER#*Safari*",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In many cases though, the model number appears in the build string, so I did adjust these in some cases where there was a test with that sort of pattern.

@@ -493,7 +493,7 @@
"HTC_DesireS": "HTC Desire S",
"HTC_Sensation": "HTC Z710",
"HTC_Sensation_Z710e": "HTC Z710e",
"HTC_SensationXE_Beats": "HTC Z710",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Z710 is just "HTC Sensation", so doesn't seem like it should be the device used for this token.

@@ -47,7 +47,7 @@
"SAMSUNG-GT-3210": "Samsung GT-3210",
"SAMSUNG-GT-B3310": "Samsung GT-B3310",
"SAMSUNG-GT-B3410": "Samsung GT-B3410",
"SAMSUNG-GT-B7610": "Samsung GT-B6710",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have been a type-o.

@codecov
Copy link

codecov bot commented Sep 7, 2017

Codecov Report

Merging #1578 into master will increase coverage by <.01%.
The diff coverage is 48.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1578      +/-   ##
===========================================
+ Coverage      41.8%   41.8%   +<.01%     
  Complexity      763     763              
===========================================
  Files          1540    1540              
  Lines         53239   53246       +7     
  Branches      21224   21227       +3     
===========================================
+ Hits          22255   22260       +5     
- Misses        30984   30986       +2
Impacted Files Coverage Δ Complexity Δ
...s/user-agents/browsers/ie-mobile/iemobile-7-0.json 13.21% <0%> (ø) 0 <0> (ø) ⬇️
.../browsers/android-webview/android-webview-1-0.json 7.01% <0%> (ø) 0 <0> (ø) ⬇️
...s/user-agents/browsers/ie-mobile/iemobile-7-6.json 4.54% <0%> (ø) 0 <0> (ø) ⬇️
...wsers/android-webview/android-webview-1-5-2-1.json 30.56% <0%> (ø) 0 <0> (ø) ⬇️
...r-agents/browsers/chrome/chrome-android-43-on.json 80.1% <100%> (ø) 0 <0> (ø) ⬇️
...gents/browsers/chrome/chrome-android-18-to-27.json 81.49% <100%> (ø) 0 <0> (ø) ⬇️
.../browsers/android-webview/android-webview-4-0.json 57.63% <25%> (ø) 0 <0> (ø) ⬇️
.../browsers/android-browser/android-browser-4-0.json 88.02% <80%> (-0.07%) 0 <0> (ø)
...gents/browsers/chrome/chrome-android-28-to-42.json 94.28% <88.88%> (-0.09%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8d0415...54ccdfd. Read the comment docs.

@mimmi20 mimmi20 self-assigned this Sep 7, 2017
@mimmi20 mimmi20 added this to the 6025 milestone Sep 7, 2017
@mimmi20 mimmi20 merged commit 97ca3ec into browscap:master Sep 7, 2017
@jaydiablo jaydiablo deleted the device-ambiguity branch September 23, 2017 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants