Issue 29454678: Issue 5085 - Add edgeInfo.js template for edge specific builds (Closed)
|
Created:
June 2, 2017, 4 p.m. by Jon Sonesen Modified:
June 8, 2017, 4:30 p.m. Reviewers:
Sebastian Noack CC:
kzar, Thomas Greiner Visibility:
Public. |
DescriptionIssue 5085 - Add edgeInfo.info template for edge specific builds
Patch Set 1 : #
Total comments: 2
Patch Set 2 : remove chrome specific code from edge template, and edge specific from chrome #
Total comments: 7
Patch Set 3 : readd chrome logic, remove more chrome specific code from edge tmpl #
Total comments: 4
Patch Set 4 : fix regexp in edge tmpl, remove platformVersion from chrome tmpl #
Total comments: 4
Patch Set 5 : remove unecessary variable #
MessagesTotal messages: 15
Perhaps the commit message can be improved, let me know what you think, Thanks!
On 2017/06/02 16:05:37, Jon Sonesen wrote: > Perhaps the commit message can be improved, let me know what you think, Thanks! Ah well I see I called it edgeInfo.info XD
https://codereview.adblockplus.org/29454678/diff/29454684/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29454684/templates/edgeInfo.... templates/edgeInfo.js.tmpl:21: if (app != "Mozilla" && app != "AppleWebKit" && app != "Safari") This logic (and most of the code below) is specific to Chrome, hence redundant here. Also mind the other way around, and remove the Edge specific logic from chromeInfo.js.tmpl.
https://codereview.adblockplus.org/29454678/diff/29454684/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29454684/templates/edgeInfo.... templates/edgeInfo.js.tmpl:21: if (app != "Mozilla" && app != "AppleWebKit" && app != "Safari") On 2017/06/06 16:36:38, Sebastian Noack wrote: > This logic (and most of the code below) is specific to Chrome, hence redundant > here. > > Also mind the other way around, and remove the Edge specific logic from > chromeInfo.js.tmpl. I see, yeah I though I had removed the stuff from the chrome template but I didn't read the comments here thoroughly enough, my bad
https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... templates/edgeInfo.js.tmpl:22: // not a Chromium-based UA, probably modifed by the user Not sure if I should change the comment, seems it should have chromium replaed with edge?
https://codereview.adblockplus.org/29454678/diff/29458565/templates/chromeInf... File templates/chromeInfo.js.tmpl (left): https://codereview.adblockplus.org/29454678/diff/29458565/templates/chromeInf... templates/chromeInfo.js.tmpl:20: if (app == "Chrome") This part is still relevant on Chrome, only the app == "Edge" branch isn't. https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... templates/edgeInfo.js.tmpl:32: exports.application = application; Since this code is only bundled with the build for Microsoft Edge, there is no way that the application (this code is running on) is anything. If the user agent string indicates some other browser, it is wrong. So just hard-code "edge" here, and "edgehtml" for the platform below. Then the logic above can be simplified to merely extract the version out of the user agent string, using an inlined regexp, no need for a loop.
https://codereview.adblockplus.org/29454678/diff/29458565/templates/chromeInf... File templates/chromeInfo.js.tmpl (left): https://codereview.adblockplus.org/29454678/diff/29458565/templates/chromeInf... templates/chromeInfo.js.tmpl:20: if (app == "Chrome") On 2017/06/07 11:14:13, Sebastian Noack wrote: > This part is still relevant on Chrome, only the app == "Edge" branch isn't. Acknowledged. https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... templates/edgeInfo.js.tmpl:32: exports.application = application; On 2017/06/07 11:14:13, Sebastian Noack wrote: > Since this code is only bundled with the build for Microsoft Edge, there is no > way that the application (this code is running on) is anything. If the user > agent string indicates some other browser, it is wrong. So just hard-code "edge" > here, and "edgehtml" for the platform below. > > Then the logic above can be simplified to merely extract the version out of the > user agent string, using an inlined regexp, no need for a loop. Acknowledged. What about the logic if platform version is null (where my comment above is)? would this just be wrong and we would want it to fail? Should it be removed?
https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... templates/edgeInfo.js.tmpl:32: exports.application = application; On 2017/06/07 11:41:38, Jon Sonesen wrote: > On 2017/06/07 11:14:13, Sebastian Noack wrote: > > Since this code is only bundled with the build for Microsoft Edge, there is no > > way that the application (this code is running on) is anything. If the user > > agent string indicates some other browser, it is wrong. So just hard-code > "edge" > > here, and "edgehtml" for the platform below. > > > > Then the logic above can be simplified to merely extract the version out of > the > > user agent string, using an inlined regexp, no need for a loop. > > Acknowledged. What about the logic if platform version is null (where my comment > above is)? would this just be wrong and we would want it to fail? Should it be > removed? If the regular expression fails we should still set "platformVersion" to "0" to indicate that it is unknown, "platform" should still be "edgehtml".
https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458565/templates/edgeInfo.... templates/edgeInfo.js.tmpl:32: exports.application = application; On 2017/06/07 11:51:46, Sebastian Noack wrote: > On 2017/06/07 11:41:38, Jon Sonesen wrote: > > On 2017/06/07 11:14:13, Sebastian Noack wrote: > > > Since this code is only bundled with the build for Microsoft Edge, there is > no > > > way that the application (this code is running on) is anything. If the user > > > agent string indicates some other browser, it is wrong. So just hard-code > > "edge" > > > here, and "edgehtml" for the platform below. > > > > > > Then the logic above can be simplified to merely extract the version out of > > the > > > user agent string, using an inlined regexp, no need for a loop. > > > > Acknowledged. What about the logic if platform version is null (where my > comment > > above is)? would this just be wrong and we would want it to fail? Should it be > > removed? > > If the regular expression fails we should still set "platformVersion" to "0" to > indicate that it is unknown, "platform" should still be "edgehtml". Acknowledged.
https://codereview.adblockplus.org/29454678/diff/29458612/templates/chromeInf... File templates/chromeInfo.js.tmpl (left): https://codereview.adblockplus.org/29454678/diff/29458612/templates/chromeInf... templates/chromeInfo.js.tmpl:16: { The "platform" variable defined above isn't necessary anymore. You can inline/hard-code "chromium" at the bottom in the only case where this variable is used anymore. https://codereview.adblockplus.org/29454678/diff/29458612/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458612/templates/edgeInfo.... templates/edgeInfo.js.tmpl:8: let [app, platformVersion] = /(\S+)\/(\S+)(?:\s*\(.*?\))?/g.exec(navigator.userAgent); Obviously, you didn't test that code. At very least you could have tried out the regexp on a real user agent string, if you don't care to run MS Edge: https://stackoverflow.com/questions/30591706/what-is-the-user-agent-string-na...
https://codereview.adblockplus.org/29454678/diff/29458612/templates/chromeInf... File templates/chromeInfo.js.tmpl (left): https://codereview.adblockplus.org/29454678/diff/29458612/templates/chromeInf... templates/chromeInfo.js.tmpl:16: { On 2017/06/07 13:26:12, Sebastian Noack wrote: > The "platform" variable defined above isn't necessary anymore. You can > inline/hard-code "chromium" at the bottom in the only case where this variable > is used anymore. Acknowledged. https://codereview.adblockplus.org/29454678/diff/29458612/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458612/templates/edgeInfo.... templates/edgeInfo.js.tmpl:8: let [app, platformVersion] = /(\S+)\/(\S+)(?:\s*\(.*?\))?/g.exec(navigator.userAgent); On 2017/06/07 13:26:13, Sebastian Noack wrote: > Obviously, you didn't test that code. At very least you could have tried out the > regexp on a real user agent string, if you don't care to run MS Edge: > https://stackoverflow.com/questions/30591706/what-is-the-user-agent-string-na... My apologies, I made a mistake in my testing and just was checking the regex passed but not paying attention to the values. Definitely will be more careful.
https://codereview.adblockplus.org/29454678/diff/29458638/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458638/templates/edgeInfo.... templates/edgeInfo.js.tmpl:9: let match = /\bEdge\/(\S+(?:\.\d+)?)\b/.exec(navigator.userAgent); \S matches any non-whiespace characters, but we really just want numbers here. https://codereview.adblockplus.org/29454678/diff/29458638/templates/edgeInfo.... templates/edgeInfo.js.tmpl:11: platformVersion = match[1]; If you assign directly to exports.platformVersion here, we won't need a temporary variable. I don't have a strong opinion which is better, but I'd prefer to remain consistent with how we do it already in other info modules: https://hg.adblockplus.org/buildtools/file/6ab9eb46db2a/templates/geckoInfo.j...
https://codereview.adblockplus.org/29454678/diff/29458638/templates/edgeInfo.... File templates/edgeInfo.js.tmpl (right): https://codereview.adblockplus.org/29454678/diff/29458638/templates/edgeInfo.... templates/edgeInfo.js.tmpl:9: let match = /\bEdge\/(\S+(?:\.\d+)?)\b/.exec(navigator.userAgent); On 2017/06/08 08:52:30, Sebastian Noack wrote: > \S matches any non-whiespace characters, but we really just want numbers here. Done. https://codereview.adblockplus.org/29454678/diff/29458638/templates/edgeInfo.... templates/edgeInfo.js.tmpl:11: platformVersion = match[1]; On 2017/06/08 08:52:30, Sebastian Noack wrote: > If you assign directly to exports.platformVersion here, we won't need a > temporary variable. I don't have a strong opinion which is better, but I'd > prefer to remain consistent with how we do it already in other info modules: > https://hg.adblockplus.org/buildtools/file/6ab9eb46db2a/templates/geckoInfo.j... Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
