Closed Bug 1378882 Opened 7 years ago Closed 7 years ago

Introduce an API for setting the default engine to a built in engine only

Categories

(WebExtensions :: General, enhancement, P1)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bug 1378882 - Support is_default for built-in engines only.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
As a stopgap measure as we discuss bug 1374312, I'd like to introduce a simple chrome setting API that allows the default search engine to be changes to an engine that is built in to Firefox. It will not allow the engine to be changed to user added engines or extension added engine. The primary purpose of this API is for our partners that change the search engine to our built in engines (like DuckDuckGo).
I’m not clear why that is necessary, cant we just set the default via pref, perhaps in the distribution.ini file, if it’s a builtin?
Flags: needinfo?(mozilla)
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. https://reviewboard.mozilla.org/r/154960/#review160016 Pardon the drive-by, but looking at this, and my patch at https://reviewboard.mozilla.org/r/153440/, I notice *a lot* of code duplication. It seems like we should find a way to de-dupe this. I wonder if, rather than having both `ext-chrome-settings-overrides.js` and `ext-url-overrides.js` as implementations we could combine these in a file and still serve the two different API namespaces? If that's not possible or practical, maybe we can extract out some of this logic into a supporting module which each can then import?
bsilverberg: Yeah, I thought that as I was writing it. The same thing happened when I did the prefs stuff for search. We ended up putting everything in the prefs files.
@Shane just a quick explanation on how we are currently using the legacy addons to set the search engine with different tracking parameters for different partners (by partner, it means that other companies are helping us distribute the extension, and we need to be able to identify them when users are using the search engine. If a user installs our extension with this link: https://addons.mozilla.org/en-US/firefox/addon/{addon_name}/?src=external-partner1 The extension installs itself and identifies the parameter "src=external-partner1" in the tab URL. Let's say that it is going to set the search engine to Yahoo with the following parameter "&fr=partner1" That way, we are able to know how many "partner1" users are using Yahoo search. We currently have about a dozen of partners, for which we need to set different search engine parameters in order to identify which users came from which partner. The Chrome implementation of hardcoding the search URL in the manifest.json is not a good solution for us, because we won't be able to dynamically append the parameter &fr=partner{n} in the URL. Chrome extensions policy allows us to create as many extensions as we want, so for each partner we are able to build 1 individual extension instance in the Chrome store. But Mozilla has a "single extension" policy, meaning that we can only distribute 1 extension of a kind on addons.mozilla.org. So I am afraid that with the manifest.json URL restriction + single extension policy, we have no good solution to identify different partnerships in our search URL. (Note that there are also installers installing the extension, in which case the installer is writing a JSON file with the partner name in the profD folder, which we are able to read from within the extension during install time. And webExtensions don't seem to have this possibility of reading files in the profD folder) // Francois
Flags: needinfo?(mixedpuppy)
Francois: Can you give me a specific extension example please? What company is producing these extensions?
Flags: needinfo?(mozilla)
Hello Mike, I work for Yahoo, we've had a chat over Hangout a few months ago regarding webExtensions. https://addons.mozilla.org/en-US/firefox/addon/search-and-new-tab-by-yahoo/ has the search set logic that I described above Francois Devatine - devatine@yahoo-inc.com (In reply to Mike Kaply [:mkaply] from comment #6) > Francois: > > Can you give me a specific extension example please? What company is > producing these extensions?
Sorry, Francois, didn't realize it was you. Was looking at the gmail address account. We have not come to a final conclusion how we are going to handle search in WebExtensions (particularly default search).
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. mkaply doing more research, putting on hold.
Flags: needinfo?(mixedpuppy)
Attachment #8884009 - Flags: review?(mixedpuppy)
Priority: -- → P1
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. This new patch uses is_default so we don't add a new parameter. Bob, would love to get your feedback regards to the duplication on somethings with bug 1330494
Attachment #8884009 - Flags: feedback?(bob.silverberg)
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. https://reviewboard.mozilla.org/r/154960/#review163658 ::: browser/components/extensions/ext-chrome-settings-overrides.js:96 (Diff revision 2) > + await ExtensionSettingsStore.addSetting( > + extension, LOCAL_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => { > + return Services.search.currentEngine.name; > + }); > + setDefault = true; > + } Feels like we should dump an warning if is_default is not used. ::: browser/components/extensions/schemas/chrome_settings_overrides.json (Diff revision 2) > "deprecated": "Unsupported on Firefox." > }, > "is_default": { > "type": "boolean", > - "optional": true, > + "optional": true > - "deprecated": "Unsupported on Firefox at this time." add documentation that this is not generally supported for new search engines.
Attachment #8884009 - Flags: review?(mixedpuppy) → review+
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/c49af70f1a94 Support is_default for built-in engines only. r=mixedpuppy
Beyond the eslint failures, looks like this caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=115457597&repo=autoland on at least linux asan builds.
I ran eslint locally. Odd. I'll take a look.
Flags: needinfo?(mozilla)
One more pass at this for bsilverberg to look at when he gets a chance. I've reworked the code so we only set the default engine on initial install. Disable/enable has no effect other than to make it NOT the default engine if it was, but it doesn't put it back on reenable. Removing/disabling only sets the engine back to what it was on before install if we're the engine. shutdown code is moved to callOnClose. I know this bug is getting long in the teeth. Hopefully we can stick a fork in it soon. (seeing how many metaphors I can mix)
(In reply to Mike Kaply [:mkaply] from comment #21) > > I've reworked the code so we only set the default engine on initial install. > Disable/enable has no effect other than to make it NOT the default engine if > it was, but it doesn't put it back on reenable. > Why? Wouldn't a user expect for an extension to "start working again" when they re-enable it?
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. https://reviewboard.mozilla.org/r/154960/#review165136 Looks pretty good, Mike, but I think there are still a few issues with the use of the SettingsStore in this patch. ::: browser/components/extensions/ext-chrome-settings-overrides.js:42 (Diff revision 5) > + let {manifest} = extension; > + let item = ExtensionSettingsStore[action](extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME); > + if (item) { > + let searchProvider = manifest.chrome_settings_overrides.search_provider; > + let engineName = searchProvider.name.trim(); > + if (Services.search.currentEngine.name != engineName) { This doesn't seem right. If another extension is currently in control then `Services.search.currentEngine.name` will not be the same as `engineName`, but that doesn't mean the user changed the engine. I think you need to compare `Services.search.currentEngine.name` to `item.value || item.initialValue` to see whether the current engine is the same as what the ExtensionSettingsStore expects it to be. ::: browser/components/extensions/ext-chrome-settings-overrides.js:75 (Diff revision 5) > + let engineName = searchProvider.name.trim(); > + let engine = Services.search.getEngineByName(engineName); > + if (engine && Services.search.getDefaultEngines().includes(engine)) { > + // Only add onclose handlers if we would definitely > + // be setting the default engine. > + extension.callOnClose({ I know this was also discussed in the bug, but it seems like you're missing some cases in your code for shutdown and startup, namely enable/disable and upgrade/downgrade. ::: browser/components/extensions/ext-chrome-settings-overrides.js:93 (Diff revision 5) > + if (extension.startupReason === "ADDON_INSTALL") { > + await ExtensionSettingsStore.addSetting( > + extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => { > + return Services.search.currentEngine.name; > + }); > + Services.search.currentEngine = engine; This will always set *this* extension's engine as the current engine, ignoring all of the precedence rules from the SettingsStore. I think you need to use the item returned from the call to `ExtensionSettingsStore.addSetting` to determine if the engine needs to be set. ::: browser/components/extensions/ext-chrome-settings-overrides.js:102 (Diff revision 5) > + return; > + } > + Components.utils.reportError("is_default can only be used for built-in engines."); > + // If the setting exists for the extension, but is missing from the manifest, > + // remove it. This can happen if the extension removes is_default. > + } else if (ExtensionSettingsStore.hasSetting( If the extension was updated and the `chrome_settings_overrides` key was removed from the manifest, will we ever reach this code?
Attachment #8884009 - Flags: review?(bob.silverberg) → review-
(In reply to Bob Silverberg [:bsilverberg] from comment #22) > (In reply to Mike Kaply [:mkaply] from comment #21) > > > > I've reworked the code so we only set the default engine on initial install. > > Disable/enable has no effect other than to make it NOT the default engine if > > it was, but it doesn't put it back on reenable. > > > > Why? Wouldn't a user expect for an extension to "start working again" when > they re-enable it? The problem is an issue of state. If a user disables the add-on, then changes the search engine, then reenables the add-on, what should I do in that case? How do I even maintain the state in that case?
(In reply to Bob Silverberg [:bsilverberg] from comment #23) > Comment on attachment 8884009 [details] > Bug 1378882 - Support is_default for built-in engines only. > > https://reviewboard.mozilla.org/r/154960/#review165136 > > Looks pretty good, Mike, but I think there are still a few issues with the > use of the SettingsStore in this patch. > > ::: browser/components/extensions/ext-chrome-settings-overrides.js:42 > (Diff revision 5) > > + let {manifest} = extension; > > + let item = ExtensionSettingsStore[action](extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME); > > + if (item) { > > + let searchProvider = manifest.chrome_settings_overrides.search_provider; > > + let engineName = searchProvider.name.trim(); > > + if (Services.search.currentEngine.name != engineName) { > > This doesn't seem right. If another extension is currently in control then > `Services.search.currentEngine.name` will not be the same as `engineName`, > but that doesn't mean the user changed the engine. I think you need to > compare `Services.search.currentEngine.name` to `item.value || > item.initialValue` to see whether the current engine is the same as what the > ExtensionSettingsStore expects it to be. But it does mean that we shouldn't do anything to the engine. Basically what this code is saying is "If the current Engine isn't the engine we set it to, don't do anything" The only case where item.value and item.initialValue matter is where we are setting the engine back to what it was before we were installed. > ::: browser/components/extensions/ext-chrome-settings-overrides.js:93 > (Diff revision 5) > > + if (extension.startupReason === "ADDON_INSTALL") { > > + await ExtensionSettingsStore.addSetting( > > + extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => { > > + return Services.search.currentEngine.name; > > + }); > > + Services.search.currentEngine = engine; > > This will always set *this* extension's engine as the current engine, > ignoring all of the precedence rules from the SettingsStore. I think you > need to use the item returned from the call to > `ExtensionSettingsStore.addSetting` to determine if the engine needs to be > set. But if we just added the setting, can't we be guaranteed that the engine we would get back from addSetting is the one we just added? Or is that not how the precedence works? > > ::: browser/components/extensions/ext-chrome-settings-overrides.js:102 > (Diff revision 5) > > + return; > > + } > > + Components.utils.reportError("is_default can only be used for built-in engines."); > > + // If the setting exists for the extension, but is missing from the manifest, > > + // remove it. This can happen if the extension removes is_default. > > + } else if (ExtensionSettingsStore.hasSetting( > > If the extension was updated and the `chrome_settings_overrides` key was > removed from the manifest, will we ever reach this code? Nope. I'm not sure the right way to handle that. So many edge cases.
Here are some edge cases I'm wondering about in terms of taking back the search engine. Install add-on that takes default search, disable add-on, reenable add-on. Install add-on that takes default search, disable add-on, change search engine, reenable add-on. Install add-on that takes default search, disable add-on, install another add-on that takes over default search, reenable add-on. Install add-on that takes default search, disable add-on, install another add-on that takes over default search, change search engine, reenable add-on. Install add-on that takes default search, change search engine, disable add-on, reenable add-on. In Chrome, reenabling would always take back the search engine (keeping in mind that in Chrome, when an add-on has the search engine, no one else can have it). I'm starting to lean towards "reenabling should just take back default"
(In reply to Mike Kaply [:mkaply] from comment #24) > (In reply to Bob Silverberg [:bsilverberg] from comment #22) > > (In reply to Mike Kaply [:mkaply] from comment #21) > > > > > > I've reworked the code so we only set the default engine on initial install. > > > Disable/enable has no effect other than to make it NOT the default engine if > > > it was, but it doesn't put it back on reenable. > > > > > > > Why? Wouldn't a user expect for an extension to "start working again" when > > they re-enable it? > > The problem is an issue of state. If a user disables the add-on, then > changes the search engine, then reenables the add-on, what should I do in > that case? > > How do I even maintain the state in that case? Sorry, Mike, I missed this question earlier on. I think the answer is that we rely on the ExtensionSettingsStore to maintain state, but if the user manually changes something then all bets are off. We intend to do something via the front-end to prevent users from changing settings that are currently controlled by extensions, but that is a work in progress. As mentioned before, for now what we are doing is not making any changes to settings when the current value of the setting is not what we expect it to be. This was implemented in the ExtensionPreferencesManager in processSetting [1], but, because you are not using preferences for this setting you'll have to implement that logic yourself. I think you have tried to do that, which led to a number of my comments below. I will also try to answer your more recent questions in comment 25 and comment 26. [1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/components/extensions/ExtensionPreferencesManager.jsm#126-141
(In reply to Mike Kaply [:mkaply] from comment #25) > (In reply to Bob Silverberg [:bsilverberg] from comment #23) > > > > ::: browser/components/extensions/ext-chrome-settings-overrides.js:42 > > (Diff revision 5) > > > + let {manifest} = extension; > > > + let item = ExtensionSettingsStore[action](extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME); > > > + if (item) { > > > + let searchProvider = manifest.chrome_settings_overrides.search_provider; > > > + let engineName = searchProvider.name.trim(); > > > + if (Services.search.currentEngine.name != engineName) { > > > > This doesn't seem right. If another extension is currently in control then > > `Services.search.currentEngine.name` will not be the same as `engineName`, > > but that doesn't mean the user changed the engine. I think you need to > > compare `Services.search.currentEngine.name` to `item.value || > > item.initialValue` to see whether the current engine is the same as what the > > ExtensionSettingsStore expects it to be. > > But it does mean that we shouldn't do anything to the engine. Basically what > this code is saying is "If the current Engine isn't the engine we set it to, > don't do anything" Maybe I'm not understanding, but let's take a concrete example. Let's say this extension was in control and then became disabled. This code would be called with the action `disable`. It would disable the setting via the call to ExtensionSettingsStore at line 38, which would return an item. If another extension was previously in control of the setting, that other extension's value would be returned in this item. We therefore would want to change the engine to that other value, which is part of the reason that we're calling processDefaultSearchSetting in the first place, but this line would prevent that from happening. > > The only case where item.value and item.initialValue matter is where we are > setting the engine back to what it was before we were installed. > Exactly, and that's what we want to do, I believe. > > ::: browser/components/extensions/ext-chrome-settings-overrides.js:93 > > (Diff revision 5) > > > + if (extension.startupReason === "ADDON_INSTALL") { > > > + await ExtensionSettingsStore.addSetting( > > > + extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => { > > > + return Services.search.currentEngine.name; > > > + }); > > > + Services.search.currentEngine = engine; > > > > This will always set *this* extension's engine as the current engine, > > ignoring all of the precedence rules from the SettingsStore. I think you > > need to use the item returned from the call to > > `ExtensionSettingsStore.addSetting` to determine if the engine needs to be > > set. > > But if we just added the setting, can't we be guaranteed that the engine we > would get back from addSetting is the one we just added? Or is that not how > the precedence works? Ah, this is only called during install, it's not called every time the extension is started (as happens with the new tab override), so yes, if we've just installed the extension that should mean that it's the one with the highest precedence. I do wonder, however, if we still wouldn't be safer to just make sure that's the case by checking the item. What if we changed the rules for precedence? As unlikely as that is to happen, it's probably best if code like this doesn't assume it understands the precedence rules and instead relies on the settings store to tell it whether an extension has precedence or not. > > > > > ::: browser/components/extensions/ext-chrome-settings-overrides.js:102 > > (Diff revision 5) > > > + return; > > > + } > > > + Components.utils.reportError("is_default can only be used for built-in engines."); > > > + // If the setting exists for the extension, but is missing from the manifest, > > > + // remove it. This can happen if the extension removes is_default. > > > + } else if (ExtensionSettingsStore.hasSetting( > > > > If the extension was updated and the `chrome_settings_overrides` key was > > removed from the manifest, will we ever reach this code? > > Nope. I'm not sure the right way to handle that. So many edge cases. I am handling that for the new tab override in onManifestEntry [1], but that only works because we add and remove the setting each time the extension is started, which you are not doing here, so a different, but similar approach will be needed. [1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/browser/components/extensions/ext-url-overrides.js#26-69
(In reply to Mike Kaply [:mkaply] from comment #26) > Here are some edge cases I'm wondering about in terms of taking back the > search engine. > > Install add-on that takes default search, disable add-on, reenable add-on. Default search should be controlled by the add-on. > > Install add-on that takes default search, disable add-on, change search > engine, reenable add-on. Default search should be unchanged because a user changed the engine manually. Until we have UI to support this our approach to this is, if a user manually changes a setting the precedence queue is broken and therefore do not make any automatic changes to the setting. > > Install add-on that takes default search, disable add-on, install another > add-on that takes over default search, reenable add-on. In this case the second add-on should retain control because the precedence rules say that the most recently *installed* add-on gets control. > > Install add-on that takes default search, disable add-on, install another > add-on that takes over default search, change search engine, reenable add-on. As with case #2 above, the engine should remain as set by the user because they made a manual change which breaks the chain. > > Install add-on that takes default search, change search engine, disable > add-on, reenable add-on. Same as above. Manual change breaks the chain so we no longer make any automatic changes to the setting. > > In Chrome, reenabling would always take back the search engine (keeping in > mind that in Chrome, when an add-on has the search engine, no one else can > have it). > When you say "no one else can have it" do you mean that the user cannot override it, or that a different extension cannot override it, or both? > > I'm starting to lean towards "reenabling should just take back default" I'm not sure what you mean by "take back default", but I think that re-enabling should respect precedence rules, as long as the chain hasn't been broken by a manual change.
>> >> Install add-on that takes default search, disable add-on, change search >> engine, reenable add-on. > Default search should be unchanged because a user changed the engine manually. Until we have UI to support > this our approach to this is, if a user manually changes a setting the precedence queue is broken and > therefore do not make any automatic changes to the setting. This is where it gets really tricky. How can we know that the user changed the search engine while the add-on was disabled? I wonder if I should add code that if the user changes the default engine, it removes the settings from ExtensionSettings?
bob: This covers most of your comments. I don't know of any clever way to handle the "engine switch while we are disabled" so I think we'll have to punt on that. I've written extensive tests to cover all known scenarios (including obscure stuff) and everything is working great for me.
(In reply to Mike Kaply [:mkaply] from comment #30) > >> > >> Install add-on that takes default search, disable add-on, change search > >> engine, reenable add-on. > > Default search should be unchanged because a user changed the engine manually. Until we have UI to support > > this our approach to this is, if a user manually changes a setting the precedence queue is broken and > > therefore do not make any automatic changes to the setting. > > This is where it gets really tricky. How can we know that the user changed > the search engine while the add-on was disabled? Well, even if the add-on is disabled, its setting still remains in the settings store (only in a disabled state), and the settings store will also have recorded what the initial value of the engine was. So when the extension is re-enabled, and your code checks to make sure that the current value for the engine is the same as what the settings store expects, it should find that not to be true, and therefore will not automatically switch the engine. Does that make sense? > > I wonder if I should add code that if the user changes the default engine, > it removes the settings from ExtensionSettings? I had thought about taking that approach too, but I think we're sticking with the check mentioned above, and in the long run will deal with it via changes to the UI.
> So when the extension is re-enabled, and your code checks to make sure that the current value for the engine is the same as what the settings store expects, it should find that not to be true, and therefore will not automatically switch the engine. Does that make sense? That won't work if another add-on has the engine when I was disabled, right? Because when I come back, the settings store will match the correct engine, but I should not take the engine. I did end up adding code to remove the setting if the user changes the search engine. Once the user changes the search engine, the settings code should not be relied on to change the engine. My new patch is up and you can take a look at the tests to see all the things I'm covering. I'll continue to look to see if I can cover the "user change while disabled" case. The problem I see there is what I really need is what the value was set to after I was disabled so I can see if that's still the value when I come back. I don't have that info (unless I store it in a pref or something). Maybe the settings store needs to have "LastValue" that is stored when something is disabled?
(In reply to Mike Kaply [:mkaply] from comment #34) > > So when the extension is re-enabled, and your code checks to make sure that the current value for the engine is the same as what the settings store expects, it should find that not to be true, and therefore will not automatically switch the engine. Does that make sense? > > That won't work if another add-on has the engine when I was disabled, right? > Because when I come back, the settings store will match the correct engine, > but I should not take the engine. Maybe I'm missing something, but there are two things incorrect about the above. First, if another extension had control over the extension when you are disabled, then you shouldn't regain control when you are enabled, because control is based on most recently installed extension, not most recently enabled extension. But even ignoring that, whether another add-on has the engine when you are disabled or not doesn't matter. Let's say another extension does have the engine when you are disabled - at that point the current engine will match what the settings store expects (which is the one specified by the "other" add-on). However, as soon as a user manually changes the engine, the current engine will no longer match what the settings store expects (which is the one specified by the "other" add-on), because the current engine will be whatever the user set it to. At that point the chain is broken and you will know that by comparing the current engine to what the settings store expects. This should work any time a user changes the engine manually, unless, by coincidence, they change it to the exact same value that the settings store expects, and we admittedly do not cover that use case. So I don't think your use case above causes any problems for an implementation that just relies on comparing the current value to what the settings store expects. Does that make sense? > > > I did end up adding code to remove the setting if the user changes the > search engine. Once the user changes the search engine, the settings code > should not be relied on to change the engine. While your last statement is true, I think just adding the check for the expected value of the engine should suffice, and you shouldn't need to manually remove any settings. > > My new patch is up and you can take a look at the tests to see all the > things I'm covering. I'll continue to look to see if I can cover the "user > change while disabled" case. > > The problem I see there is what I really need is what the value was set to > after I was disabled so I can see if that's still the value when I come > back. I don't have that info (unless I store it in a pref or something). > > Maybe the settings store needs to have "LastValue" that is stored when > something is disabled?
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. https://reviewboard.mozilla.org/r/154960/#review168336 Thanks for the updates, Mike. The tests do look pretty comprehensive, thanks for that as well. It looks like maybe some of them could be combined into a single test, but I guess they're easier to follow the way you have them. I've added a few more comments. ::: browser/components/extensions/ext-chrome-settings-overrides.js:117 (Diff revision 6) > + if (extension.startupReason === "ADDON_INSTALL") { > + await ExtensionSettingsStore.addSetting( > + extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => { > + return Services.search.currentEngine.name; > + }); > + let item = ExtensionSettingsStore.getSetting(DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME); This shouldn't be required. `ExtensionSettingsStore.addSetting` will return an item if the setting just added should be the new setting, so you should be able to just do `let item = await ExtensionSettingsStore.addSetting(...)` and then just check `if (item)` ::: browser/components/extensions/ext-chrome-settings-overrides.js:121 (Diff revision 6) > + if (ExtensionSettingsStore.hasSetting( > + extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME)) { > + let item = ExtensionSettingsStore.enable(extension, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME); > + if (item && item.value) { > + let engine = Services.search.getEngineByName(item.value); > + Services.search.currentEngine = engine; > + Services.obs.addObserver(this, "browser-search-engine-modified"); > + } > + } I feel like this should be able to re-use `processDefaultSearchSetting()`. ::: browser/components/extensions/ext-chrome-settings-overrides.js:168 (Diff revision 6) > + // 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. Yeah, this still has the problem I mentioned earlier which is that it won't work if the `chrome_settings_overrides` key is removed entirely. I do think we need to worry about that though, and cannot simply ignore it. The only solution I can see is to put something on `onStartup`, but that has its own problems. We should probably ask aswan what we can do about this, but let's iron out everything else first. ::: browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js:37 (Diff revision 6) > + > + let ext1 = ExtensionTestUtils.loadExtension({ > + manifest: { > + "chrome_settings_overrides": { > + "search_provider": { > + "name": "DuckDuckGo", Is it safe to hardcode `DuckDuckGo` here, or should we be getting the name of a valid default engine from the search service? ::: browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js:73 (Diff revision 6) > + useAddonManager: "temporary", > + }); > + > + await ext1.startup(); > + > + is(Services.search.currentEngine.name, defaultEngineName, "Default engine should be " + defaultEngineName); Should we be checking for the error message that is expected when trying to set an invalid engine to the default?
Attachment #8884009 - Flags: review?(bob.silverberg) → review-
I had to remove all of the disabling tests because they don't work when run in combination with other browser tests. I believe it's because a lot of the things I'm doing won't work in browser tests (specifically related to shutdown events in the add-ons). I'm not sure how to proceed. I've verified that all these changes work locally.
(In reply to Mike Kaply [:mkaply] from comment #39) > I had to remove all of the disabling tests because they don't work when run > in combination with other browser tests. > > I believe it's because a lot of the things I'm doing won't work in browser > tests (specifically related to shutdown events in the add-ons). > > I'm not sure how to proceed. I've verified that all these changes work > locally. The issue is that one line in the test is running out of sequence. I'm at a complete loss. I can't produce a testcase until this is checked in.
I've opened up bug 1392634 to try to get to the bottom of the strange test failure.
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. https://reviewboard.mozilla.org/r/154960/#review177066 ::: browser/components/extensions/ext-chrome-settings-overrides.js:44 (Diff revisions 8 - 9) > } > - let item; > - item = ExtensionSettingsStore.getSetting(DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME); > if (Services.search.currentEngine.name != item.value && > Services.search.currentEngine.name != item.initialValue) { > - // Current engine is not what we think it is - do nothing. > + // Current engine is not what we think it is - do nothing and remove our settings What does this mean? Why is it not what we think it is? ::: browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js:356 (Diff revisions 8 - 9) > + await ext1.unload(); > + > + is(Services.search.currentEngine.name, defaultEngineName, `Default engine is ${defaultEngineName}`); > +}); > + > +add_task(async function test_two_addons_with_more_disabling() { better names please. test_disable_non_default_engine might be good here.
Comment on attachment 8884009 [details] Bug 1378882 - Support is_default for built-in engines only. https://reviewboard.mozilla.org/r/154960/#review177076 r+ with: Fix the comments and test names, maybe add a comment on the tests to clarify what the test is specifically doing.
Attachment #8884009 - Flags: review+
Attachment #8884009 - Flags: review?(bob.silverberg)
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/5d5c1a6c3f3a Support is_default for built-in engines only. r=mixedpuppy
Backed out for eslint failures at ext-chrome-settings-overrides.js:37 and ext-chrome-settings-overrides.js:37: https://hg.mozilla.org/integration/autoland/rev/75b01708e389deeec401d5d51bfb0ccc586291b5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5d5c1a6c3f3a0e951a9f479ab71aef22e3ab87e4&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=125576364&repo=autoland TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/extensions/ext-chrome-settings-overrides.js:37:10 | 'manifest' is assigned a value but never used. (no-unused-vars) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js:259:7 | 'defaultEngineName' is assigned a value but never used. (no-unused-vars)
Flags: needinfo?(mozilla)
Just rechecked in. Tx.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/072ae808c7b3 Support is_default for built-in engines only. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/072ae808c7b3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Keywords: dev-doc-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: