After much discussion, we're going to allow is_default with opt-in.

A few notes on this. 1. Test removal is temporary. I need to know how to have my browser tests interact with the notifications (click them) and then I'll put back. 2. Text is obviously not final. Also need to decide Yes/No or Accept/Deny. 3. Browser.css changes should probably be in toolkit/themes/shared/popupnotification.inc.css on further thought. The reason they don't need to be platform specific is because they are changing things back to the generic behavior in that file. 4. Changes to ext-chrome-settings-overrides.js are primarily movement of code around to make calling it easier/cleaner. Just a small change for the actual permissions stuff.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 I'm having a hard time evaluating this since I don't really understand the division of resonsibilities between the code here and the search service. I have some questions right now but it would be good to have somebody familiar with the search service (:flo?) review this as well before landing. When code in this patch adds a search engine and/or sets the currentEngine, is that something that just lasts for the current browser session or is the result of that action persisted somewhere? When the equivalents of those actions are done from the UI, where is that information persisted? Separately, what's the desired extensions behavior if the user installs an extension that sets the default search engine but then goes on to 1. change the default to something else? 2. delete the search engine entry added by the extension?

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 This code is reusing the existing is_default code which handles those cases. So we already properly do the right thing when the user changes the engine (preserve that change forever) or installs another extension. The search service code in here specifically is really just a moving of code that is already checked in. All this patch adds in is the "add engine" dialog that then invokes the existing code which was moved into its own function. So the main thing for review here is the permission code and the dialog.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 Just to be more clear, the primary thing for review here are the changes to ExtensionUI, the dialog and the permissions addition in the extension file. The search engine stuff was all just movement of things into functions.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. Need feedback specifically on the text of the prompt and based on that text whether to use Allow/Deny or Yes/No (or something else).

Text as-is seems good to me, and I'll ask Michelle to give a quick review as well. Not sure if we want a (Learn More) pointing at a Sumo article talking about how it can be changed, but may be overkill. On the answers side, I'd prefer "Yes/No" (in that order). We're asking the user directly if it's ok to do something, and Yes or No is an unambiguous response set that directly answers the question.

mkaply: line 103 of browser/locales/en-US/chrome/browser/browser.properties has a typo ("from" twice), needs a revision pending mheubusch's comments on "%1$S would like to change your default search engine from %2$S to %3$S. Is that OK?"

My recommended text: Will you allow %1$S to change your default search engine from %2$S to %3$S? Allow | Don't Allow If you do opt to link to SUMO, please style the link as Learn more (cap L, lowercase m, no ellipsis)

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 I'm not sure what "the permissions addition in the extension file" means. Does it mean the new "defaultSearch" permission? I'd like to understand why that is necessary but its not clear to me just from reading the patch. Which means that if/when somebody other than you comes along and needs to change or fix something here it's going to be unnecessarily difficult, I'm hoping we can address that before landing this. Which brings me back to the questions I originally asked.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 Sorry, I'll answer directly: > I'm having a hard time evaluating this since I don't really understand the division of resonsibilities between the code here and the search service. I have some questions right now but it would be good to have somebody familiar with the search service (:flo?) review this as well before landing. When code in this patch adds a search engine and/or sets the currentEngine, is that something that just lasts for the current browser session or is the result of that action persisted somewhere? When the equivalents of those actions are done from the UI, where is that information persisted? All of the search engine info is persisted using the Extensions Settings Manager. It was checked in as part of this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1378882. So the default engine responsibility (maintaining when disabling, enabling, user changes, etc.) has all been checked in and tested as part of that bug. This patch contains no new code related to the search service, it just moves it into a function. > Separately, what's the desired extensions behavior if the user installs an extension that sets the default search engine but then goes on to > 1. change the default to something else? The extension loses the default engine and never takes it back > delete the search engine entry added by the extension? That's a good question. I will test that. It should stay gone if the user chooses to delete it. That should have been handled by the original patch that adds a search engine. The intent is that anything the add-on does only persists if the user hasn't made a choice. Once the user changes anything, it stays gone and the add-on can't have it back. What this patch specifically does is add a new permission (searchDefault) and a new popup when the add-on is installed to ask the user to set that permission. If the user selected "yes/allow", it uses the code that was already checked in for 1378882 for all search interactions.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 Sorry for the slow reply. If the idea is that setting the default search engine is a one-time thing that happens at the moment the extension is installed, then why do we need the "defaultSearch" permission at all? I'm still unclear on how the search service works and how this code interacts with it. It looks like this code runs every time the extension starts up, which implies that changes made here only last for the duration of the current session and need to be re-applied every time the browser starts? But it also looks like the code to generate the prompt runs every time an extension that tries to set the default starts? That doesn't seem right (if I say no, I keep getting prompted?). Regardless, if we need to keep track of the results of the prompt somewhere, why does it need to be as a permission, can't it be in the settings store?

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review183486 You are totally correct. I was overthinking the permission save. We don't need any of that. It only needs to be checked at install once. New patch coming.

Is this targeted for 57? Asking for add-ons that would be looking for it...

sorry for being vague, asking so Scott can let Ecosia know that they are unblocked. https://addons.mozilla.org/en-US/firefox/addon/ecosia-the-green-search/ "The search engines that plants trees".

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review186704 A few nits, I think this could also benefit from a comment explaining that the search service maintains the mapping between extensions and search engines but that the default setting is maintained in the extension settings store. But my biggest concern is about how we get the browser for displaying the notification. But doing that properly would put this at serious risk for making 57. Lets get a second opinion from somebody with deeper front-end knowledge such as :flo ::: browser/components/extensions/ext-chrome-settings-overrides.js:78 (Diff revision 2) > - if (searchProvider.is_default) { > - let engineName = searchProvider.name.trim(); > + let engineName = searchProvider.name.trim(); > + if (searchProvider.is_default) { > let engine = Services.search.getEngineByName(engineName); > if (engine && Services.search.getDefaultEngines().includes(engine)) { > + await this.setDefault(engineName); Why is this case treated specially? More specifically, why don't we prompt here? ::: browser/components/extensions/ext-chrome-settings-overrides.js:85 (Diff revision 2) > + return; > + } > + } > + this.addSearchEngine(searchProvider); > + if (searchProvider.is_default) { > + if (extension.startupReason === "ADDON_INSTALL") { Is this meant to handle the case where version N of an extension does not set is_default but version N+1 does? In that case, startupReason will be ADDON_UPGRADE but you won't process is_default... ::: browser/components/extensions/ext-chrome-settings-overrides.js:89 (Diff revision 2) > + if (searchProvider.is_default) { > + if (extension.startupReason === "ADDON_INSTALL") { > + let allow = await new Promise(resolve => { > + let subject = { > + wrappedJSObject: { > + browser: windowTracker.topWindow.gBrowser.selectedBrowser, This makes me uneasy, ideally we would be getting the browser from the actual install. Using this technique, if I start an install and then switch windows while the extension is downloading, this notification will pop up in an unexpected place. Or, it might be unlikely but it is possible that on a platform like Mac the last window could be closed at which point `topWindow` will be null. ::: browser/components/extensions/ext-chrome-settings-overrides.js:93 (Diff revision 2) > + wrappedJSObject: { > + browser: windowTracker.topWindow.gBrowser.selectedBrowser, > + name: this.extension.name, > + icon: this.extension.iconURL, > + currentEngine: Services.search.currentEngine.name, > + newEngine: engineName, This is an arbitrary name supplied by the extension? I don't have a solution for this but it seems like extensions that are trying to hijack search can easily put an innocent-sounding name here (ie some trivial variant of Google or something) and trick users into saying yes. ::: browser/components/extensions/ext-chrome-settings-overrides.js:99 (Diff revision 2) > + resolve, > + }, > + }; > + Services.obs.notifyObservers(subject, "webextension-defaultsearch-prompt"); > + }); > + Components.utils.reportError(allow); please remove this line ::: browser/components/extensions/ext-chrome-settings-overrides.js:121 (Diff revision 2) > + } > + } > + } > + } > + > + async onShutdown(reason) { Using both this and `extension.callOnClose()` (which is used in `setDefault()` below) is confusing, can you please consolidate into one? Preferrably `extension.callOnClose()` ::: browser/modules/ExtensionsUI.jsm:369 (Diff revision 2) > + resolve(false); > + } > + } > + }; > + > + // These are reversed because we want accept on the left (Yes/No) I don't understand this comment ::: browser/modules/ExtensionsUI.jsm:396 (Diff revision 2) > + }, > + ]; > + > + let win = browser.ownerGlobal; > + win.PopupNotifications.show(browser, "addon-webext-defaultsearch", "", > + // eslint-disable-next-line no-unsanitized/property what is this line for?

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review186704 > Is this meant to handle the case where version N of an extension does not set is_default but version N+1 does? In that case, startupReason will be ADDON_UPGRADE but you won't process is_default... You also get ADDON_UPGRADE when you install a disabled extension through about:debugging. I'm not sure if the same thing happens through other install paths, but I suspect it might. I think ADDON_UPGRADE should be handled here.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review186704 > Why is this case treated specially? More specifically, why don't we prompt here? This is bug 1378882. If the add-on is switching to a built-in engine (partner engine), we don't prompt the user. > You also get ADDON_UPGRADE when you install a disabled extension through about:debugging. I'm not sure if the same thing happens through other install paths, but I suspect it might. I think ADDON_UPGRADE should be handled here. The original plan for this was not to handle upgrade at all, but I see where you're coming from and we should handle ADDON_UPGRADE. The main goal is to try to prevent addons from figuring out a way to keep harassing the user, which they might do by alternating releases with and without is_default. > This makes me uneasy, ideally we would be getting the browser from the actual install. Using this technique, if I start an install and then switch windows while the extension is downloading, this notification will pop up in an unexpected place. Or, it might be unlikely but it is possible that on a platform like Mac the last window could be closed at which point `topWindow` will be null. I totally understand. I couldn't find a good way to get the browser in this case. > This is an arbitrary name supplied by the extension? I don't have a solution for this but it seems like extensions that are trying to hijack search can easily put an innocent-sounding name here (ie some trivial variant of Google or something) and trick users into saying yes. Yes it is which is why we are showing what the old engine is and the new one. We don't currently do any vetting of that name. We currently at least don't allow "Google " which we allowed before. > I don't understand this comment Sorry, that was left from an older version. I'll remove. > what is this line for? I had copied some code that needed it. I'll remove.

(In reply to :shell escalante from comment #18) > sorry for being vague, asking so Scott can let Ecosia know that they are > unblocked. > https://addons.mozilla.org/en-US/firefox/addon/ecosia-the-green-search/ "The > search engines that plants trees". Thanks for keeping an eye on this for us (Ecosia), it's much appreciated. This solution sounds really great. I sadly don't understand enough of the code to properly help with the review, but we will happily test this once it lands in a nightly. Please let me know if there is any other way I can support you.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review187238 I'm still pretty unhappy about the browser handling. I wish we had had a chance to talk this over before this patch was written, I would have approached this in a different way. But if product management really wants this for 57, I think landing it as-is is the only option. Also a few more comments below, I think data-review for the new telemetry you're trying to add is probably the thing that puts this at greatest risk for 57. ::: browser/components/extensions/ext-chrome-settings-overrides.js:102 (Diff revision 3) > + if (searchProvider.is_default) { > + let engine = Services.search.getEngineByName(engineName); > + if (engine && Services.search.getDefaultEngines().includes(engine)) { > + await this.setDefault(engineName); > + // For built in search engines, we don't do anything further > + return; > + } > + } Shouldn't this clause only happen if the startupReason is INSTALL (or maybe UPGRADE)? Otherwise, if I try to change my default, this will overwrite it every time the browser restarts. ::: browser/components/extensions/ext-chrome-settings-overrides.js:120 (Diff revision 3) > + let engine = Services.search.getEngineByName(engineName); > + if (Services.search.currentEngine != engine) { > + let allow = await new Promise(resolve => { > + let subject = { > + wrappedJSObject: { > + browser: windowTracker.topWindow.gBrowser.selectedBrowser, Please add a comment here to alert readers of this code that this is a hack and has potential issues. A follow-up bug would be nice, though I guess we can wait and see if we get any actual reports of problems from QA or from users. ::: browser/components/extensions/ext-chrome-settings-overrides.js:135 (Diff revision 3) > + if (!allow) { > + return; > + } > + } > + } > + await this.setDefault(engineName); shouldn't this be inside the if body right above? ::: browser/components/extensions/ext-chrome-settings-overrides.js:137 (Diff revision 3) > + // If the setting exists for the extension, but is missing from the manifest, > + // remove it. This can happen if the extension removes is_default. > + // There's really no good place to put this, because the entire search section > + // could be removed. > + // We'll never get here in the normal case because we always return early > + // if we have an is_default value that we use. I'm having a hard time following this comment. But the remark about the whole search section getting removed does seem pertinent, I think that isn't handled correctly right now? ::: browser/modules/ExtensionsUI.jsm:350 (Diff revision 3) > "addons-notification-icon", > action, secondaryActions, popupOptions); > }); > }, > > + showDefaultSearchPrompt(browser, strings, icon, histkey) { nit: this is only called from one place with a fixed argument for `histkey`, you could just make it a local const inside this function. ::: browser/modules/ExtensionsUI.jsm:374 (Diff revision 3) > + label: strings.acceptText, > + accessKey: strings.acceptKey, > + disableHighlight: true, > + callback: () => { > + if (histkey) { > + this.histogram.add(histkey + "Accepted"); I'm pretty sure you need to update Histograms.json for this to have an effect. And I think you may need a data review for that :/ ::: browser/modules/ExtensionsUI.jsm:394 (Diff revision 3) > + }, > + ]; > + > + let win = browser.ownerGlobal; > + win.PopupNotifications.show(browser, "addon-webext-defaultsearch", "", > + // eslint-disable-next-line no-unsanitized/property please remove this line

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review187238 > Shouldn't this clause only happen if the startupReason is INSTALL (or maybe UPGRADE)? Otherwise, if I try to change my default, this will overwrite it every time the browser restarts. The setDefault function itself handles this, doing the change only for install or enable. setDefault always honors what has been done with ExtensionSettings, so if you change your search engine, it does the right thing. It never just explicit sets the search engine (even on enabling/disabling) > shouldn't this be inside the if body right above? We also have to handle the enable case. the setDefault function itself makes sure it only happens during install or enable. > I'm having a hard time following this comment. But the remark about the whole search section getting removed does seem pertinent, I think that isn't handled correctly right now? I simplified this comment, and you are correct, this isn't handled correctly right now. This is a catch-22 in the way manifests are processed. If a section is completely removed, the code would never be called. > I'm pretty sure you need to update Histograms.json for this to have an effect. And I think you may need a data review for that :/ I'll remove the telemetry stuff for now. > please remove this line I also removed it from showPermissionsPrompt

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. https://reviewboard.mozilla.org/r/177534/#review186704 > Yes it is which is why we are showing what the old engine is and the new one. We don't currently do any vetting of that name. We currently at least don't allow "Google " which we allowed before. And note that these add-ons still need to be reviewed, so if this becomes a problem, we will address. > I had copied some code that needed it. I'll remove. Actually, the code it's copied from is here: http://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#330 And uses that there as well...

(In reply to Mike Kaply [:mkaply] from comment #27) > Comment on attachment 8905729 [details] > Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. > > https://reviewboard.mozilla.org/r/177534/#review186704 > > > Yes it is which is why we are showing what the old engine is and the new one. We don't currently do any vetting of that name. We currently at least don't allow "Google " which we allowed before. > > And note that these add-ons still need to be reviewed, so if this becomes a > problem, we will address. Please don't make that assumption, unlisted add-ons still don't get manually reviewed and the AMO team is working very hard to eliminate any reliance on manual reviews. > > I had copied some code that needed it. I'll remove. > > Actually, the code it's copied from is here: > > http://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI. > jsm#330 > > And uses that there as well... Yuck, somebody screwed up a rebase at some point, thanks for cleaning it up.

Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/7e8e47c972f4 Show opt-in dialog for is_default with non built-in engines. r=aswan

Backed out for eslint failure at browser/components/extensions/ext-chrome-settings-overrides.js:124: windowTracker is not defined: https://hg.mozilla.org/integration/autoland/rev/84188868fc418b5d79c218a6abc33296f57a5ad6 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7e8e47c972f41335e9109c51597efdc3bca56910&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132560102&repo=autoland > TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/extensions/ext-chrome-settings-overrides.js:124:28 | 'windowTracker' is not defined. (no-undef)

I got sidetracked on the actual code during review, and overlooked the fact that this has no automated tests which is not good if we're going to keep supporting this. Can you please add some? In a follow-up bug is okay if you still want to crash-land this for 57.

> I got sidetracked on the actual code during review, and overlooked the fact that this has no automated tests which is not good if we're going to keep supporting this. Can you please add some? In a follow-up bug is okay if you still want to crash-land this for 57. Yes I was planning to. Need to know how to interact with the popup in a test.

Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/9f5bf3e0af82 Show opt-in dialog for is_default with non built-in engines. r=aswan

(In reply to Mike Kaply [:mkaply] from comment #32) > Yes I was planning to. Need to know how to interact with the popup in a test. Tests of the existing prompts are in browser/base/content/test/webextensions, you can probably get a good start by looking there.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. Approval Request Comment [Feature/Bug causing the regression]: Allow WebExtensions to set default engine with optin [User impact if declined]: None specifically. Potential for WebExtensions to try other mechanisms to install engines. [Is this code covered by automated tests?]: Partially (default search). Working on new tests. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No, but steps are below. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: Only impacts is_default in WebExtensions, no other code. [String changes made/needed]: Yes. Search confirm message, Yes/No in browser.properties. I realize this is late. We were trying everything we could to get this in before merge. This change is important to help some of our extensions get to Firefox 57. To test: Create a manifest.json like this: { "manifest_version": 2, "name": "Test Engine", "version": "0.1", "chrome_settings_overrides": { "search_provider": { "name": "NEW ENGINE NAME", "search_url": "https://duckduckgo.com/?q={searchTerms}", "is_default": true } } } Add the extension via about:Debugging.

Flod, it had new string, I need your sign off in case we want to take it in beta. Mike, Andy considered that as a p3 and set the flag fix-optional. I would rather let it ride the train as we have already a tone of the thing to do with 57.

It's OK to uplift these strings in Beta if needed.

(In reply to Sylvestre Ledru [:sylvestre] from comment #38) > Flod, it had new string, I need your sign off in case we want to take it in > beta. > > Mike, Andy considered that as a p3 and set the flag fix-optional. I would > rather let it ride the train as we have already a tone of the thing to do > with 57. I totally understand how you feel. I would have rather got this in earlier :) This is something that is important to our partners and a number of extension developers and it's extremely isolated from other functionality.

Comment on attachment 8905729 [details] Bug 1397975 - Show opt-in dialog for is_default with non built-in engines. ok, let's take it then. Should be in 57b3

This feature is verified on Firefox 58.0a1 (20170926100259) under Windows 10 64-bit and Mac OS X 10.12.3. The popup functionality works as expected. Please see the attached video. During testing I have found a small UI issue https://bugzilla.mozilla.org/show_bug.cgi?id=1403184 .

Hey Guys, This is Harsha with Symantec. I tried to use this API in FF 57b3 and it doesn't seem like the commit made. Curious to know if there'd be another beta we could test on? Thanks, Harsha

Nevermind. I take my comment back. I was able to set it up fine in Firefox 57b3. Thanks, Harsha

I just tested it in 57b3 too, it's working really well, thanks a lot! We encountered one issue though: Until now we open a new tab on install to do some on-boarding and explain the user what they just did and how they can now search with Ecosia. Doing this now closes the new dialog and assumes the user clicked no. I guess this one of the reasons why this dialog happens on the first search in chrome instead of on install. Not sure this is something you can/want to address right now, but I thought I'd let you know (I guess it would be nice to at least document this behavior for other developers). Apart from this it worked really well in our initial testing, giving the user an easy way to change his default search, while still giving them a chance to understand what is happening, thanks for this great compromise. We will test it more thoroughly now and will report any potential bugs we find here.

I was able to reproduce the bug mentioned in the comment above and I have logged https://bugzilla.mozilla.org/show_bug.cgi?id=1403568 .

Verified on Firefox 57.0b3 (20170925150345) under Windows 10 64-bit and Mac OS X 10.12.3.