Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2300403003:
DevTools: patch browser's Chrome version into Chrome user agents for emulation (Closed)
|
Created:
4 years, 3 months ago by luoe Modified:
4 years, 3 months ago CC:
chenwilliam, chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: patch browser's Chrome version into Chrome user agents for emulation
User agents are defined by hard coded strings for emulation mode in DevTools.
This CL patches all preset user agents in emulated_devices that include
"Chrome/" and those in the "Chrome" section in the network conditions drawer to
the version read from the browser's navigator.userAgent.
BUG=460059
Committed: https://crrev.com/999f86be50685adb30c783cdf30e97f8044234a3
Cr-Commit-Position: refs/heads/master@{#419834}
Patch Set 1 #
Total comments: 4
Patch Set 2 : use sprintf, default chrome version for fallback #
Total comments: 4
Patch Set 3 : tests, escape, nullcheck #
Total comments: 5
Patch Set 4 : regex nit #Patch Set 5 : Bake version at build time #Patch Set 6 : Removed old stuff #
Total comments: 8
Patch Set 7 : address comments #
Total comments: 2
Patch Set 8 : reorder imports #Patch Set 9 : address comments #Patch Set 10 : rebase for nav.ua method #Patch Set 11 : Simpler navigator.userAgent method #
Total comments: 14
Patch Set 12 : address comments #
Total comments: 8
Patch Set 13 : common > sdk, address comments #
Total comments: 1
Patch Set 14 : revert to sprintf %s #Patch Set 15 : update test #Patch Set 16 : renames #Messages
Total messages: 49 (19 generated)
Description was changed from ========== version patcher BUG= ========== to ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 ==========
luoe@chromium.org changed reviewers: + allada@chromium.org, lushnikov@chromium.org
allada@: regex review lushnikov@: general review
Description was changed from ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 ========== to ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices that include "Chrome/" and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 ==========
The regex looks good though. https://codereview.chromium.org/2300403003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:18: return uaString.replace(sectionRegex, "$1" + currentVersion); Lets not use a regex replace for this, lets use: return String.sprintf(uaString, currentVersion); And change WebInspector.NetworkConfigView._userAgentGroups like this: {title: "Chrome \u2014 Android Mobile", value: "Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/%s Mobile Safari/537.36"} It looks like String.sprintf() is safe if not all params are present in string (ie: no %s in format string). https://codereview.chromium.org/2300403003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:18: return uaString.replace(sectionRegex, "$1" + currentVersion); How are we handling cases where the user wants to emulate a specific version of chrome and puts the version in as a custom string? I assume user input values we do not want to alter? Maybe allow the user to use %s to substitute the chrome version?... maybe not?
https://codereview.chromium.org/2300403003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:18: return uaString.replace(sectionRegex, "$1" + currentVersion); Okay, it now uses %s in the strings, as it's more explicit what we're replacing. https://codereview.chromium.org/2300403003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:18: return uaString.replace(sectionRegex, "$1" + currentVersion); Maybe not in this CL :)
https://codereview.chromium.org/2300403003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:16: if (!currentVersion || uaString.indexOf("%s") === -1) Can we add a test to make sure if a user adds a custom user agent with %s in it, it will not change their user agent? https://codereview.chromium.org/2300403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:32: var sectionRegex = new RegExp("(?:^|[^\\w])" + sectionName + "/([^\\s]+)"); var sectionRegex = new RegExp("(?:^|[^\\w])" + sectionName.escapeForRegExp() + "/([^\\s]+)", "ig"); I assume we should replace all instances of "Chrome/" and make it case insensitive?
https://codereview.chromium.org/2300403003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:16: if (!currentVersion || uaString.indexOf("%s") === -1) On 2016/09/02 21:08:05, Blaise wrote: > Can we add a test to make sure if a user adds a custom user agent with %s in it, > it will not change their user agent? Done. https://codereview.chromium.org/2300403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:32: var sectionRegex = new RegExp("(?:^|[^\\w])" + sectionName + "/([^\\s]+)"); escapeForRegExp: done This function just wants to extract THE value associated with "Chrome/", so I just want the one "Chrome/", no need for "ig" flags.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:29: // e.g. _getVersion("Chrome") === "1.2.3.4" when user agent is: "Chrome/1.2.3.4" I think doesn't work for content shell. Can we generalize it?
I think the regex is fine lgtm. https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:33: var sectionRegex = new RegExp("(?:^|[^\\w])" + sectionName.escapeForRegExp() + "/([^\\s]+)"); nit: lets use "(?:^|\\W)" and "/(\\S+)"
https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:29: // e.g. _getVersion("Chrome") === "1.2.3.4" when user agent is: "Chrome/1.2.3.4" On 2016/09/02 23:42:45, dgozman wrote: > I think doesn't work for content shell. Can we generalize it? Can you show me how this breaks? I patched a branch from today, built content_shell, and it seems to extract the "99.77.34.5" out of "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/99.77.34.5 Safari/537.36". https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:33: var sectionRegex = new RegExp("(?:^|[^\\w])" + sectionName.escapeForRegExp() + "/([^\\s]+)"); On 2016/09/02 23:48:29, Blaise wrote: > nit: lets use "(?:^|\\W)" and "/(\\S+)" Done.
https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:29: // e.g. _getVersion("Chrome") === "1.2.3.4" when user agent is: "Chrome/1.2.3.4" On 2016/09/03 00:08:15, luoe wrote: > On 2016/09/02 23:42:45, dgozman wrote: > > I think doesn't work for content shell. Can we generalize it? > > Can you show me how this breaks? > > I patched a branch from today, built content_shell, and it seems to extract the > "99.77.34.5" out of "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, > like Gecko) Chrome/99.77.34.5 Safari/537.36". I was thinking that content shell has different UA string. My bad. Since we don't run frontend in other browsers, this is fine for now, but I'd actually prefer to generate some constants at a build time instead of adding so much code here. WDYT?
Redone at build time. CurrentBrowserVersion.js is the template that gets the substituted version numbers. BrowserVersionInfo.js is the utility to patch user agents.
https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/BUILD.gn (right): https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/BUILD.gn:7: import("//chrome/version.gni") This looks like a layering violation. Ain't any presubmit complaining? https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:6: DefaultChromeVersion: "54.0.2834.0", Let's have this in a default CurrentBrowserVersion.js, with generated one overriding it. https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:23: return String.sprintf(uaString, currentVersion); I think instead of replacing the version inside, we'd rather generate the useragent string as a whole. https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:105: groupElement.appendChild(new Option(userAgentVersion.title, userAgentVersion.value)); Why not patch right here with a single call?
https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/BUILD.gn (right): https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/BUILD.gn:7: import("//chrome/version.gni") On 2016/09/07 22:06:28, dgozman wrote: > This looks like a layering violation. Ain't any presubmit complaining? Ninja and presubmit are fine with it, no complaints. //chrome/version and //third_party/WebKit/Source/core/core.gni both depend on //build/config/chrome_build.gni. What's the right way to do this? https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:6: DefaultChromeVersion: "54.0.2834.0", On 2016/09/07 22:06:28, dgozman wrote: > Let's have this in a default CurrentBrowserVersion.js, with generated one > overriding it. Done. https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:23: return String.sprintf(uaString, currentVersion); On 2016/09/07 22:06:28, dgozman wrote: > I think instead of replacing the version inside, we'd rather generate the > useragent string as a whole. I don't think we have sufficient information to generate the entire user-agent for most emulated devices. We want the Nexus 7 user-agent to use the current Chrome version, but Chrome doesn't know what the latest Nexus build numbers are, for example. Unless there's some way to generate all the other user agents too? https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2300403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:105: groupElement.appendChild(new Option(userAgentVersion.title, userAgentVersion.value)); On 2016/09/07 22:06:28, dgozman wrote: > Why not patch right here with a single call? I think it makes a readable, self-contained unit that can be reused if we ever want to iterate through UserAgentGroups() in other places. However, we're only using it on creation, so I'm fine with moving it here.
gentle ping
luoe@chromium.org changed reviewers: + brettw@chromium.org
brettw@: I want to let DevTools know about the Chrome version at build time by updating the third_party/.../devtools/BUILD.gn file to depend on //chrome/version.gni. However, this would introduce a layering violation, as dgozman@ pointed out. I found a #TODO in one of your commits that seems to have the same situation: https://chromium.googlesource.com/chromium/src/+/f48fe876ae18ebe0a639eb271c3b... What do you think is best to do in this case? Is it fine to introduce this dependency and add a TODO?
On 2016/09/12 18:22:58, luoe wrote: > brettw@: I want to let DevTools know about the Chrome version at build time by > updating the third_party/.../devtools/BUILD.gn file to depend on > //chrome/version.gni. However, this would introduce a layering violation, as > dgozman@ pointed out. > > I found a #TODO in one of your commits that seems to have the same situation: > https://chromium.googlesource.com/chromium/src/+/f48fe876ae18ebe0a639eb271c3b... > > What do you think is best to do in this case? Is it fine to introduce this > dependency and add a TODO? Yeah, probably that's the best thing for now.
https://codereview.chromium.org/2300403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/BUILD.gn (right): https://codereview.chromium.org/2300403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/BUILD.gn:7: import("//chrome/version.gni") These should be sorted like we do in C++.
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'd take a shot at a one line fix :-)
it is easy to show that regex-based solution is at least as reliable: if it stops working, gen-based is no longer working as well. both rely on hard-coded UA structure...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@dgozman, @pfeldman: I rewrote BrowserVersionInfo.js to be simpler, PTAL. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... https://codereview.chromium.org/2300403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/BUILD.gn (right): https://codereview.chromium.org/2300403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/BUILD.gn:7: import("//chrome/version.gni") On 2016/09/13 22:43:47, brettw (ping on IM after 24h) wrote: > These should be sorted like we do in C++. Done.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
Let us shrink it further to a few lines. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:5: WebInspector.BrowserVersionInfo = { This is no longer browser version info, also you don't need a new file for a 3 line function and put it into common. This can be a part of "emulation" module. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:6: DefaultChromeVersion: "54.0.2834.0", I'd leave these hardcoded in the config view and only replace them with current if regex matches. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:15: var sectionRegex = new RegExp("(?:^|\\W)Chrome/(\\S+)"); var regex = /Chrome\/[\d.]+/; var version = navigator.userAgent.match(regex)[0]; return version ? uaString.replace(regex, version) : uaString; https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:230: "user-agent": "Mozilla/5.0 (Linux; Android 4.4.2; Nexus 4 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/%s Mobile Safari/537.36", Leave those as is... https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:108: userAgentValue = WebInspector.BrowserVersionInfo.patchUserAgentWithChromeVersion(userAgentValue); You can unconditionally do that.
Thanks for the review. Instead of marking locations with '%s', it now calls replace twice, once for 'Chrome' and once for 'CriOS'. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:5: WebInspector.BrowserVersionInfo = { There are 2 places where we could use this patching: emulated_devices/module.json (device mode) network/networkConfigView.js (the drawer) Since network doesn't depend on emulation, isn't this necessary? https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:6: DefaultChromeVersion: "54.0.2834.0", On 2016/09/14 21:47:09, pfeldman wrote: > I'd leave these hardcoded in the config view and only replace them with current > if regex matches. Done. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:15: var sectionRegex = new RegExp("(?:^|\\W)Chrome/(\\S+)"); Regex: I think the "(?:^|\\W)" is warranted so that we don't match on "XChrome", for example. Also, \S should be more flexible than \d. We aren't guaranteed that the version value is only numbers. Could change in the future, similar to "Mobile/13B143". match: If there's no match, ua.match(regex) === null. A change in UA format could throw an error in DevTools if this isn't checked. replace: String.sprintf() gives us an added check, asserting that the string doesn't have more than one '%s' With replace, we should now do 2 replacements, one for 'Chrome' and one for 'CriOS'. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:230: "user-agent": "Mozilla/5.0 (Linux; Android 4.4.2; Nexus 4 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/%s Mobile Safari/537.36", On 2016/09/14 21:47:09, pfeldman wrote: > Leave those as is... Done. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:108: userAgentValue = WebInspector.BrowserVersionInfo.patchUserAgentWithChromeVersion(userAgentValue); On 2016/09/14 21:47:10, pfeldman wrote: > You can unconditionally do that. Done.
luoe@chromium.org changed reviewers: - brettw@chromium.org, dgozman@chromium.org
Updated reviewer list.
https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:5: WebInspector.BrowserVersionInfo = { "sdk" would be the common denominator then, you can put it into WebInspector.MultitargetNetworkManager. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:15: var sectionRegex = new RegExp("(?:^|\\W)Chrome/(\\S+)"); On 2016/09/14 22:49:13, luoe wrote: > Regex: I think the "(?:^|\\W)" is warranted so that we don't match on "XChrome", > for example. - If XChrome gets into the UA string, this means that you no longer emulate it properly - there is no XChrome in your template that you paste version into. > Also, \S should be more flexible than \d. We aren't guaranteed > that the version value is only numbers. Same here, if version gets characters, your pattern will no longer stand and will likely need an update. > match: If there's no match, ua.match(regex) === null. A change in UA format > could throw an error in DevTools if this isn't checked. Sure, the null check is necessary. > replace: String.sprintf() gives us an added check, asserting that the string > doesn't have more than one '%s' Which string? There are no longer strings with %s. > With replace, we should now do 2 replacements, one for 'Chrome' and one for > 'CriOS'. Could you elaborate? https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:230: "user-agent": "Mozilla/5.0 (Linux; Android 4.4.2; Nexus 4 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2860.0 Mobile Safari/537.36", There is no longer a point in keeping them up-to-date, you can revert this change. https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:107: WebInspector.DeviceModeModel._defaultMobileUserAgent = "Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2860.0 Mobile Safari/537.36"; ditto https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:142: result.userAgent = /** @type {string} */ (parseValue(json, "user-agent", "string")); use intermediate variable. https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:184: {title: "Chrome \u2014 Android Mobile", value: "Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2860.0 Mobile Safari/537.36"}, ditto
I spoke with Blaise offline about explicitly marking '%s'. If it's important to be able to keep old versions like Opera 37 consistent, we either need to avoid updating them or whitelist the places we want to update with '%s'. PTAL https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js (right): https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:5: WebInspector.BrowserVersionInfo = { On 2016/09/15 21:44:06, pfeldman wrote: > "sdk" would be the common denominator then, you can put it into > WebInspector.MultitargetNetworkManager. Done. https://codereview.chromium.org/2300403003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/BrowserVersionInfo.js:15: var sectionRegex = new RegExp("(?:^|\\W)Chrome/(\\S+)"); On 2016/09/15 21:44:06, pfeldman wrote: > On 2016/09/14 22:49:13, luoe wrote: > > Regex: I think the "(?:^|\\W)" is warranted so that we don't match on > "XChrome", > > for example. > > - If XChrome gets into the UA string, this means that you no longer emulate it > properly - there is no XChrome in your template that you paste version into. If there is an XChrome in the staleAgent, we will update it when we shouldn't. If there is an XChrome in the frontend UA, we will extract the wrong value. What we gain by using "(?:^|\\W)" is we avoid these bad cases: (1) frontEndAgent: "GoogleChrome/bad" staleAgent: "Chrome/x", staleAgent.replace(): "Chrome/bad" (2) frontEndAgent: "Chrome/bad" staleAgent: "XChrome/x", staleAgent.replace(): "Chrome/bad" I don't think we lose anything, and we don't need to update the regex if in the future either agent gets XChrome. > > > Also, \S should be more flexible than \d. We aren't guaranteed > > that the version value is only numbers. > > Same here, if version gets characters, your pattern will no longer stand and > will likely need an update. I think the pattern will still work? I tested it and it will replace the staleAgent with the current version even if the current version contains characters. The $1 in the .replace() means replace only the part of the string inside the 2nd capturing group. > > > match: If there's no match, ua.match(regex) === null. A change in UA format > > could throw an error in DevTools if this isn't checked. > > Sure, the null check is necessary. > > > replace: String.sprintf() gives us an added check, asserting that the string > > doesn't have more than one '%s' > > Which string? There are no longer strings with %s. Acknowledged. > > > With replace, we should now do 2 replacements, one for 'Chrome' and one for > > 'CriOS'. > > Could you elaborate? For Chrome on iPhone and iPad, the user agents use 'CriOS' instead of 'Chrome'. Therefore, to keep those up to date without explicitly marking them with '%s' we need to replace stale instances of both keywords. Also, replacing all user agents with 'Chrome' means that things like 'Opera 37', an agent that we intentionally hard coded at version 37, will have an auto-updating 'Chrome/' number. Isn't this a valid reason to put '%s' back in? https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:230: "user-agent": "Mozilla/5.0 (Linux; Android 4.4.2; Nexus 4 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2860.0 Mobile Safari/537.36", On 2016/09/15 21:44:06, pfeldman wrote: > There is no longer a point in keeping them up-to-date, you can revert this > change. Done. https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:107: WebInspector.DeviceModeModel._defaultMobileUserAgent = "Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2860.0 Mobile Safari/537.36"; On 2016/09/15 21:44:07, pfeldman wrote: > ditto Done. https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:142: result.userAgent = /** @type {string} */ (parseValue(json, "user-agent", "string")); On 2016/09/15 21:44:07, pfeldman wrote: > use intermediate variable. Done. https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/2300403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:184: {title: "Chrome \u2014 Android Mobile", value: "Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2860.0 Mobile Safari/537.36"}, On 2016/09/15 21:44:07, pfeldman wrote: > ditto Done.
https://codereview.chromium.org/2300403003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2300403003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:709: var chromeRegex = new RegExp("((?:^|\\W)Chrome/)(\\S+)"); So you want Opera here as well? Or array of "Chrome"m "CriOS" and "Opera" that you would iterate over?
After an offline discussion, returned to the explicit '%s' method of marking places that need updating. Also, some agents have been renamed: Firefox 46 >> Firefox Opera 37 >> Opera Opera 12 >> Opera (Presto) PTAL
lgtm
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org Link to the patchset: https://codereview.chromium.org/2300403003/#ps300001 (title: "renames")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices that include "Chrome/" and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 ========== to ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices that include "Chrome/" and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices that include "Chrome/" and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 ========== to ========== DevTools: patch browser's Chrome version into Chrome user agents for emulation User agents are defined by hard coded strings for emulation mode in DevTools. This CL patches all preset user agents in emulated_devices that include "Chrome/" and those in the "Chrome" section in the network conditions drawer to the version read from the browser's navigator.userAgent. BUG=460059 Committed: https://crrev.com/999f86be50685adb30c783cdf30e97f8044234a3 Cr-Commit-Position: refs/heads/master@{#419834} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/999f86be50685adb30c783cdf30e97f8044234a3 Cr-Commit-Position: refs/heads/master@{#419834} |
