We have a working version of UA overrides with go faster. This bug will be used to get that merged into m-c and shipped to users.

What timeline are you currently looking at? You notified us pretty early on for this, and we had a follow-up meeting[0]. Do you still need a QA resource? Thanks! [0] https://pad.mocotoolsprod.net/p/cbs2hKntP9

Hey Cory, We have a team work week Oct 17 - 21 in Taipei to push towards this. Let me follow up after that and we'll have a better idea of timing (but within Q4, ideally).

Comment on attachment 8815274 [details] [diff] [review] Import sources of the WebCompat Go Faster add-on V1 This is the patch that imports current state of our add-on from GitHub into mozilla-central. It only includes the ability to override user agents at this moment, as specified in [0]. We've sent an "Intent to implement"-like email back in June, see [1]. QA is already involved and they will start working on a testing plan soon. Felipe, in the meantime, we'd really appreciate some feedback from a person outside of our team. Can you have a look at our implementation? :) Since it might not be too obvious, let me explain what we are doing here: In `browser/extensions/webcompat/content/data/ua_overrides.js`, we specify a list of websites that qualify for a user agent override by their base domain. In addition, we can provide a function to check the exact URL (the user agent will only be overriden if the function returns true) and a function that returns the user agent override that will get applied. You'll see there is an override for webcompat.com in place that overrides our UA to a Chrome one. With this patch applied, you'll get a link to the Chrome add-on in the header of webcompat.com instead of the Firefox extension. This, obviously, will get removed before landing the patch, but QA needs something to test and it should make reviewing it easier as well. We check for available overrides in a `http-on-useragent-request` observer. To avoid looking up the same URL multiple times, we set up a map to cache URI -> overrides. In addition to overriding the UA, we also send out a notification to web developers by adding a statement into the developer console, which is why the embedded webextension is in there (turns out it's incredibly difficult to log into the devtools console from within the observer...). In the future, we're going to add more details to that notification, but for now, this suits our version 1. We'd appreciate any feedback on our implementation as we intent to land it into central soon. [0]: https://wiki.mozilla.org/Compatibility/Go_Faster_Addon/Version_1 [1]: https://groups.google.com/d/msg/mozilla.dev.platform/la__EONSOxI/tlXGBOeQBAAJ

Comment on attachment 8815274 [details] [diff] [review] Import sources of the WebCompat Go Faster add-on V1 Review of attachment 8815274 [details] [diff] [review]: ----------------------------------------------------------------- Given you asked in #developers, I took a look. Can someone explain why we're nesting a webextension here? Do we anticipate we'll ever be able to implement this fully within webextensions? If not, what do we gain? The comment just mentions console logging, for which you can use Services.console.logStringMessage(), I think, or Console.jsm if you want something slightly fancier. ::: browser/extensions/webcompat/bootstrap.js @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +/* globals XPCOMUtils, Services, UAOverrider UAOverrides */ You shouldn't need this if you're using the relevant mozilla eslint plugin @@ +7,5 @@ > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +const UA_ENABLE_PREF_NAME = "extensions.webcompat.perform_ua_overrides"; > + > +XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); No point lazily importing Services.jsm - it will have been loaded here already, and you call into it as soon as startup() is called anyway. @@ +9,5 @@ > +const UA_ENABLE_PREF_NAME = "extensions.webcompat.perform_ua_overrides"; > + > +XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "UAOverrider", "chrome://webcompat/content/lib/ua_overrider.js"); > +XPCOMUtils.defineLazyModuleGetter(this, "UAOverrides", "chrome://webcompat/content/data/ua_overrides.js"); If these are js modules, they should have the jsm extension. @@ +32,5 @@ > +function startup({webExtension}) { // eslint-disable-line no-unused-vars > + // Intentionally set the preference to true on every browser restart to > + // avoid site breakage by accidentally toggled preferences or by leaving > + // it off after debugging a site. > + Services.prefs.setBoolPref(UA_ENABLE_PREF_NAME, true); This feels yucky. Also, the correct thing to do is likely to call clearUserPref, not setBoolPref. But in terms of the yucky-ness, perhaps it would make more sense if there was a clear way to turn off the web-compat stuff for "current process lifetime only", effectively? @@ +38,5 @@ > + > + overrider = new UAOverrider(UAOverrides); > + overrider.init(); > + > + // Init webExtension to listen tab update status What does this sentence mean? @@ +44,5 @@ > + const {browser} = api; > + // tabUpdateHandler receives tab updated event from WebExtension tablog.js > + // While tab status changes to loading, tablog.js queries this URI is overrided or not. > + // tabUpdateHandler uses sendResponse sends result to tablog.js > + // tablog.js can determine to print web console log or not. These comments could also use polish... ::: browser/extensions/webcompat/content/data/ua_overrides.js @@ +37,5 @@ > + * } > + * ``` > + */ > + > +const UAOverrides = [ // eslint-disable-line ?? @@ +50,5 @@ > + baseDomain: "webcompat.com", > + uaTransformer: (originalUA) => { > + let prefix = originalUA.substr(0, originalUA.indexOf(")")+1); > + return `${prefix} AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36` > + } Nit: missing trailing comma @@ +51,5 @@ > + uaTransformer: (originalUA) => { > + let prefix = originalUA.substr(0, originalUA.indexOf(")")+1); > + return `${prefix} AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36` > + } > + } and here. ::: browser/extensions/webcompat/jar.mn @@ +2,5 @@ > +% content webcompat %content/ > + content/ (content/*) > + > +[features/webcompat@mozilla.org] webextension.jar: > +% override webcompat %/ This is a strange instruction. Recursive overrides in chrome.manifest files didn't use to work. Did that change? What's this trying to accomplish? ::: browser/extensions/webcompat/test/browser_check_installed.js @@ +7,5 @@ > +"use strict"; > + > +add_task(function* installed() { > + let addon = yield new Promise( > + (resolve) => AddonManager.getAddonByID("webcompat@mozilla.org", resolve) Please use `hg mv` to move files to make it possible to see diffs of the test files. ::: browser/extensions/webcompat/test/browser_overrider.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* globals XPCOMUtils, UAOverrider, IOService */ Again, shouldn't need this. ::: browser/extensions/webcompat/webextension/manifest.json @@ +1,2 @@ > +{ > + Nit: remove empty line.

(In reply to :Gijs Kruitbosch from comment #5) > > Can someone explain why we're nesting a webextension here? Do we anticipate > we'll ever be able to implement this fully within webextensions? If not, > what do we gain? The comment just mentions console logging, for which you > can use Services.console.logStringMessage(), I think, or Console.jsm if you > want something slightly fancier. Using WebExtension here is the only way I found to insert log message to Web console, not Browser console. Reason is to let web developer or user find this page is modified for compatibility reason. If it's in Browser console, it's not easy to associate the message with tab.

Thank you *very much* for your feedback, Gijs! > Do we anticipate we'll ever be able to implement this fully within > webextensions? We do not. We need an observer on `http-on-useragent-request` to change the User Agent header before the request gets send and I highly doubt there will be an API for that anytime soon. Eric already outlined the reason why we embedded the web extension here, but I'd like to add some words. Services.console.logStringMessage() and Console.jsm called from inside the extension or from inside the observer results in the output being logged into the Browser Console. The main target for this console logging, however, are the sites developer, so we need to log into the contents console. We could log into the content console if we get a hand on an nsIWindow from inside the observer, however, there seems to be no easy way of doing that. The non-webextension way would be to attach a listener to all load events, check their URLs and add a console message to the corresponding window if the URL matches what we have in our overrides list. Embedding a web extension allows us to re-use the stuff that's already there, and it won't break with e10s. The non-webext implementation of that would be a bit more complex and nicer to read. In any way, this is likely to be just a temporary workaround while we are working on a nicer way to add console notifications. That, in fact, is already on our roadmap for the second version. I agree that the comments are hard to understand - I'll work on that as well as on the other remarks you had. > You shouldn't need this if you're using the relevant mozilla eslint plugin I've included the `eslint-plugin-mozilla` and included `mozilla` in the plugins array in eslintrc. However, eslint still complains that these objects are undefined. The docs inside gecko.readthedocs.io at [0] don't talk much about setting it up - do you have more information on that? > ::: browser/extensions/webcompat/content/data/ua_overrides.js > @@ +37,5 @@ > > + * } > > + * ``` > > + */ > > + > > +const UAOverrides = [ // eslint-disable-line > > ?? Do you mind explaining what's your point here? If the eslint comment bothers you: Since, without the eslint plugin, it does not recognize the EXPORTED_SYMBOLS line, so the `UAOverrides` constant would be unused in eslints perspective. I assume this is fixed if we get the eslint plugin running though. [0]: http://gecko.readthedocs.io/en/latest/tools/lint/linters/eslint-plugin-mozilla.html

Comment on attachment 8815274 [details] [diff] [review] Import sources of the WebCompat Go Faster add-on V1 Review of attachment 8815274 [details] [diff] [review]: ----------------------------------------------------------------- The code in general looks good. I'll do a quick second pass once you address the comments by Gijs ::: browser/extensions/webcompat/bootstrap.js @@ +21,5 @@ > + overrider = new UAOverrider(UAOverrides); > + overrider.init(); > + } else if (!isEnabled && overrider) { > + overrider.uninit(); > + overrider = false; set this to null instead of false ::: browser/extensions/webcompat/content/lib/ua_overrider.js @@ +60,5 @@ > + getUAForURI(uri) { > + let bareUri = uri.specIgnoringRef; > + if (this._overrideForURICache.has(bareUri)) { > + let cachedUA = this._overrideForURICache.get(bareUri); > + return (cachedUA ? cachedUA : DefaultUA); if .has() returned true, there's no reason cachedUI wouldn't exist, right?

Sorry for the delay, I don't seem to have CC'd myself when I gave initial feedback so I didn't see this until now. (In reply to Dennis Schubert [:denschub] from comment #7) > Thank you *very much* for your feedback, Gijs! > > > Do we anticipate we'll ever be able to implement this fully within > > webextensions? > > We do not. We need an observer on `http-on-useragent-request` to change the > User Agent header before the request gets send and I highly doubt there will > be an API for that anytime soon. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Intercept_HTTP_requests seems to suggest you can already do this. > > You shouldn't need this if you're using the relevant mozilla eslint plugin > > I've included the `eslint-plugin-mozilla` and included `mozilla` in the > plugins array in eslintrc. However, eslint still complains that these > objects are undefined. The docs inside gecko.readthedocs.io at [0] don't > talk much about setting it up - do you have more information on that? As said on IRC, I don't, but you can try asking the SHIELD folks (see bug 1308656) to see what they're doing. > > ::: browser/extensions/webcompat/content/data/ua_overrides.js > > @@ +37,5 @@ > > > + * } > > > + * ``` > > > + */ > > > + > > > +const UAOverrides = [ // eslint-disable-line > > > > ?? > > Do you mind explaining what's your point here? If the eslint comment bothers > you: Since, without the eslint plugin, it does not recognize the > EXPORTED_SYMBOLS line, so the `UAOverrides` constant would be unused in > eslints perspective. I assume this is fixed if we get the eslint plugin > running though. You're disabling eslint for the entire line. It would make more sense to use the "exported" comment notation instead, which makes explicit what's happening and would also shut up the warning, rather than disabling ALL eslint functionality for the entire line... but if you're going to do this as a full webextension, I expect you won't be reusing the jsm functionality anyway.

Comment on attachment 8820861 [details] [diff] [review] Import sources of the WebCompat Go Faster add-on V1 (In reply to :Gijs Kruitbosch from comment #5) > You shouldn't need this if you're using the relevant mozilla eslint plugin I was able to figure out the issue we've been discussing on IRC. Basically, the latest version of he plugin package that has been pushed to npm has an issue, downgrading was the solution. > No point lazily importing Services.jsm - it will have been loaded here > already, and you call into it as soon as startup() is called anyway. It's, in fact, not always loaded when the startup() method gets called. But you are right, lazy loading is pointless, so I included it non-lazy. > But in terms of the yucky-ness, perhaps it would make more sense if there > was a clear way to turn off the web-compat stuff for "current process > lifetime only", effectively? I've rewritten the code to clear to the default preference on startup. We've discussed about implementing an actual interface for disabling the overrides and we've decided against it for the first version. However, we will add a bunch more override types (including JS and CSS) in future versions, and I am fairly confident that we will have a proper UI then. Thanks for the note! > Nit: missing trailing comma Hm! The github.com/mozilla/example-addon-repo suggests an eslint config that actually prohibits trailing commas. I'm not going to block if you feel strong about these, however. > This is a strange instruction. Recursive overrides in chrome.manifest files > didn't use to work. Did that change? What's this trying to accomplish? You are right - this was a leftover from previous implementations that slipped through. Thanks! (In reply to :Felipe Gomes (needinfo me!) from comment #8) > if .has() returned true, there's no reason cachedUI wouldn't exist, right? Actually, no. We also cache negative cache lookups for performance reaons. We might have cases where the base domain matches but the uriMatcher function returns false, so no override will get applied - in that case, we return `false` instead of an User Agent. I've addressed all other points, including the remarks added by Felipe. Thanks again for the feedback.

Comment on attachment 8820861 [details] [diff] [review] Import sources of the WebCompat Go Faster add-on V1 Review of attachment 8820861 [details] [diff] [review]: ----------------------------------------------------------------- > (In reply to :Felipe Gomes (needinfo me!) from comment #8) > > if .has() returned true, there's no reason cachedUI wouldn't exist, right? > > Actually, no. We also cache negative cache lookups for performance reaons. > We might have cases where the base domain matches but the uriMatcher > function returns false, so no override will get applied - in that case, we > return `false` instead of an User Agent. Ok. Please add a comment there saying that `false` is a value that is cached too, otherwise someone will inevitably stumble on this code and think the ternary is unnecessary.

Comment on attachment 8824475 [details] [diff] [review] Import sources of the WebCompat Go Faster add-on V1 Added comment, and minor issue discovered by QA. Carrying over the r+ from felipe.

Alright. If we missed anything in the GoFaster process, feel free to point that out! :) QA signed off the version attached to this bug today, the mail was sent to release-drivers@ as well. We've decided against uplifting this to aurora/beta since we do not have any urgent fixes at the moment, so getting this into Nightly is perfect.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3c50d608b69 Import sources of the WebCompat Go Faster add-on V1. r=felipe

Backed out for various test failures (useragent string and leak related): https://hg.mozilla.org/integration/mozilla-inbound/rev/1b31d3903fa02c7a370294218010b0019f77e75e Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1a9af299bd523c977310a25397655f9c0cd1dd1d Please fix the issues and verify that they are fixed with a Try run.

Hi :denschub I opened Bug 1330076 with attached a patch that rewrite the mochitest toolkit/components/extensions/test/mochitest/test_chrome_ext_shutdown_cleanup.html into an xpcshell test (and I've submitted it to the review) It should fix the issue that we briefly discussed on IRC today. Let me know if you give it a try.

Will do another try push with some fixes as soon as bug 1330076 landed. Thank you very much for your support, Luca!

We had to remove the console logging because of consecutive failures in different locations. We will target this for version 2. Felipe, I just sent you a review request for the version without site console logging, instead, we log into the browser console for now. Would be nice if you can have another look.

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac387d8b7f3b6472b684657e5765df015f712822 and it looks good.

Comment on attachment 8830517 [details] Bug 1308271 - Import sources of the WebCompat Go Faster add-on V1. https://reviewboard.mozilla.org/r/107260/#review108994

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a4fe5a905e41 Import sources of the WebCompat Go Faster add-on V1. r=Felipe