1386018 - Show if an extension has set the default search engine

Status: RESOLVED FIXED

(Toolkit :: Preferences, enhancement, P2)

Description

[Bob Silverberg [:bsilverberg]]

If an extension has changed the default search engine page then the about:preferences page will show the default search engine as the new value, but it will not be clear to a user that it was set by an extension.

For some other values, such as the home page and new tab page, we are proposing to prevent the user from changing the value if an extension has control, and forcing them to remove or disable the extension in order to regain control. I doubt we want to take that approach with the default search engine, be we do need to discuss how to expose this to users.

Bug 1378882 proposes an API to allow the default search engine to be changed. If that bug lands we should decide how to deal with exposing that information to the user.

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1386018 - Show the extension controlling the default search engine

Review commit: https://reviewboard.mozilla.org/r/181464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181464/

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: search-newtab-home-controlled.mov.gif

This is based off of the changes from bug 1373853. It moves the helper functions into preferences.js so they can be used outside of main.js and then hooks them up for the default search engine.

Attached is a gif showing the default search engine, new tab and homepage all controlled by the same extension.

Comment 3

[Mark Striemer [:mstriemer]]

Attachment: default-search-controlled.png

Here's a screenshot of the placement of the message. Can you confirm this is what you'd like, Markus?

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8909977 [details]
Bug 1386018 - Show the extension controlling the default search engine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181464/diff/1-2/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8909977 [details]
Bug 1386018 - Show the extension controlling the default search engine

https://reviewboard.mozilla.org/r/181464/#review186810

::: browser/components/preferences/in-content/search.js:87
(Diff revision 2)
>      // This is called each time something affects the list of engines.
>      let list = document.getElementById("defaultEngine");
>      // Set selection to the current default engine.
>      let currentEngine = Services.search.currentEngine.name;
>  
> +

nit, undo this extra line.

::: browser/components/preferences/in-content/search.xul:49
(Diff revision 2)
>      <groupbox id="defaultEngineGroup" data-category="paneSearch">
>        <caption><label>&defaultSearchEngine.label;</label></caption>
>        <description>&chooseYourDefaultSearchEngine2.label;</description>
> +
> +      <hbox id="browserDefaultSearchExtensionContent" align="center" hidden="true">
> +        <description control="disableDefaultSearchExtension" flex="1" />

nit, no blank space before the `/>`

::: browser/components/preferences/in-content/search.xul:52
(Diff revision 2)
> +
> +      <hbox id="browserDefaultSearchExtensionContent" align="center" hidden="true">
> +        <description control="disableDefaultSearchExtension" flex="1" />
> +        <button id="disableDefaultSearchExtension"
> +                class="extension-controlled-button accessory-button"
> +                label="&disableExtension.label;" />

nit, no blank space before the `/>`

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8909977 [details]
Bug 1386018 - Show the extension controlling the default search engine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181464/diff/2-3/

Comment 7

[Mark Striemer [:mstriemer]]

Markus is out so I got confirmation from Emanuela over Slack that these screenshots are good.

Comment 8

[Mark Striemer [:mstriemer]]

Removing checkin-needed for now since bug 1373853 needs to land first.

Comment 9

[Andy McKay]

Kev, will ping you on this one.

Comment 10

[Andy McKay]

We'd like to make this optional, not required. In other words, even you've installed an add-on that changed the default, you can still change it about:preferences as well. If that means it can't land in 57, let's target it at 58 instead.

Comment 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8909977 [details]
Bug 1386018 - Show the extension controlling the default search engine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181464/diff/3-4/

Comment 12

[Mark Striemer [:mstriemer]]

Attachment: default-search.mov.gif

Here's a new gif of how this works. Disable Extension button is gone and the user can instead change their selection without disabling the extension.

I think Markus still wants to review this interaction so I'll ni? him.

Comment 13

[Markus Jaritz [:markus] (UX)]

(In reply to Mark Striemer [:mstriemer] from comment #12)
> Here's a new gif of how this works. Disable Extension button is gone and the
> user can instead change their selection without disabling the extension.
Can we be certain that the extension will not change the search engine back right after the user changed it?
If not, disabling the extension is the only way to stop am endless loop of changes for the user.

> I think Markus still wants to review this interaction so I'll ni? him.
Thanks.

If we can ensure that the extension can not change the search engine back, after the user has changed it, the message should read:
"An extension, <icon> <extension name>, set your default search engine."

If we can NOT ensure the use can take control back. Meaning the extension can change back the search-engine whenever they like, the text should read:
"An extension, <icon> <extension name>, controls your default search engine."
And then we need a disable button to help the user get back control!

Comment 14

[Stephan Sokolow]

I was under the impression that the only way to set a search engine as default with WebExtensions was using the is_default manifest key.

If that's the case, why not simply apply it once, the first time it's encountered, and expose it to the user as if it's a permission:

1. The extension would lack the power to request a re-apply

2. Users are familiar with the idea of having an install process make a one-time change to settings

3. You could mention that it's going to happen in the permissions prompt, either on initial user-triggered install or as a "new permission requested by a version upgrade".

Comment 15

[Mark Striemer [:mstriemer]]

There is code in place to ensure that the change only happens at install time (or upgrade time, if the manifest changed). Because of this we should be able to just let the user change the value rather than disabling the extension.

Stephan, we are revising the flow for the adding default search engines targeted at Firefox 58. What we're looking at doing currently is very similar to your suggestion.

Comment 16

[Mark Striemer [:mstriemer]]

Attachment: Bug 1386018 - Tell users that the default search engine was set by an extension

Review commit: https://reviewboard.mozilla.org/r/187396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187396/

Comment 17

[Mark Striemer [:mstriemer]]

Attachment: default-search-setByUser.mov.gif

jaws: Looks like adding bsilverberg as a reviewer put you back to r?. I don't think the changes here affect what you looked at much but you're welcome to have another look. I updated setDefaultEngine() to call ExtensionSettingsStore.setByUser(), added a Services.obs listener (using "unload" to remove it) and removed the disable extension button.

bsilverberg: I updated the ExtensionSettingsStore to have a setByUser method which will disable all of the settings for a type/key. This prevents the message from re-appearing if you set the value away from then back to what the extension had set it to. Adding new extensions or enabling previous extensions affects the setting as expected.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review192456

I got flagged for review because the mozreview-commit-id in the commit message changed. Maybe you made a new commit then rolled up the old one into the new one?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:373
(Diff revision 1)
> +    for (let item of precedenceList) {
> +      item.enabled = false;
> +    }

precendenceList.map(i => i.enabled = false);

Comment 19

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review193192

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:373
(Diff revision 1)
> +    for (let item of precedenceList) {
> +      item.enabled = false;
> +    }

This file has a lot of for loops in it. I originally used a `forEach` but went with this for consistency within the file.

Comment 20

[Mark Striemer [:mstriemer]]

> I got flagged for review because the mozreview-commit-id in the commit message changed. Maybe you made a new commit then rolled up the old one into the new one?

Woops, yeah. I accidentally got this mixed up with some other changes and split it into two commits. I'll keep that in mind next time I do some history rewriting, thanks.

Comment 21

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review193592

This looks good, Mark, but I think we need another test. We don't have any tests for the case you and I discussed, i.e., a user manually sets the search engine back and then an extension updates itself. I assume you tested that manually, but I think we should also include an automated test for that, as it's one of the reasons the code for setByUser was added.

::: browser/components/preferences/in-content/main.js:782
(Diff revision 1)
>        useCurrent.label = useCurrent.getAttribute("label2");
>      else
>        useCurrent.label = useCurrent.getAttribute("label1");
>  
>      // If the homepage is controlled by an extension then you can't use this.
> -    if (await getControllingExtensionId("prefs", "homepage_override")) {
> +    if (await getControllingExtensionInfo("prefs", "homepage_override")) {

You use "prefs" and "homepage_override" twice in this file. Is it worth converting them to consts?

::: browser/components/preferences/in-content/preferences.js:440
(Diff revision 1)
> +  getControllingExtensionEl(settingName).hidden = true;
> +}
> +
> +
> +function makeDisableControllingExtension(type, settingName) {
> +  return async function disableExtension() {

Why are you returning an async function rather than just making `makeDisableControllingExtension` async?

Comment 22

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187396/diff/1-2/

Comment 23

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review193592

> Why are you returning an async function rather than just making `makeDisableControllingExtension` async?

This creates a function that will disable the specified add-on, rather than creating a function for each type/setting combination. The async function it creates gets passed to the click handler.

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review197662

This looks good, thanks Mark.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba4a39241953
Tell users that the default search engine was set by an extension r=bsilverberg,jaws

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for xpcshell failures, e.g. toolkit/mozapps/extensions/test/xpcshell/test_signed_install.js on Linux debug:

https://hg.mozilla.org/integration/autoland/rev/9c1bf5ec6b26d19db70ff38fa892c11097b9d5aa

Please check the XPCshell failures (and also other failures) on that push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ba4a39241953d5079dba48dddac6093f161745aa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 27

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187396/diff/2-3/

Comment 28

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f2e9af41c240 -d b3dc668bebb9: rebasing 430277:f2e9af41c240 "Bug 1386018 - Tell users that the default search engine was set by an extension r=bsilverberg,jaws" (tip)
merging browser/components/preferences/in-content/main.js
merging browser/components/preferences/in-content/tests/browser_extension_controlled.js
merging toolkit/components/extensions/ExtensionSettingsStore.jsm
warning: conflicts while merging browser/components/preferences/in-content/main.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_extension_controlled.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 29

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187396/diff/3-4/

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review199146

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:376
(Diff revision 4)
> +  // Set the engine to the initial one and verify an upgrade doesn't change it.
> +  setEngine(initialEngine);
> +  await waitForMessageHidden(controlledContent.id);
> +
> +  // Update the extension and wait for "ready".
> +  let update = await promiseFindAddonUpdates(addon);

I don't think you need to actually go through the addons manager update process here, just installing an extension with the same id and a newer version should have the same effect and be simpler.

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1174
(Diff revision 4)
>     * @param {object} addon The add-on to find updates for.
>     * @param {integer} reason The type of update to find.
>     * @return {Promise<object>} an object containing information about the update.
>     */
>    promiseFindAddonUpdates(addon, reason = AddonManager.UPDATE_WHEN_PERIODIC_UPDATE) {
> -    let equal = this.testScope.equal;
> +    let equal = this.testScope.equal || this.testScope.is;

In mochitest, this should be available as `testScope.Assert.equal`, can you use that instead?

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1326
(Diff revision 4)
> +        });
> +        response.write(JSON.stringify(responseData));
> +      });
> +
> +      // The test extension uses an insecure update url.
> +      Services.prefs.setBoolPref(PREF_EM_CHECK_UPDATE_SECURITY, false);

This needs to be reverted when the test is done.  In mochitests, this should use `SpecialPowers.pushPrefEnv()` but AddonTestUtils is actually used primarily from xpchsell.

Having something reusable for updates sounds nice, but I don't think we actually need to do full-blown updates from mochitests at all (see separate comment above)

Comment 31

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187396/diff/4-5/

Comment 32

[Mark Striemer [:mstriemer]]

I switched this to install a new version of the extension rather than update it and removed the changes to extract the UpdateServer. This addressed the other comments as well.

Comment 33

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187396/diff/5-6/

Comment 34

[Andrew Swan [:aswan]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

https://reviewboard.mozilla.org/r/187396/#review200104

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/607922c730a1
Tell users that the default search engine was set by an extension r=aswan,bsilverberg,jaws

Comment 36

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/9889fd6d9a5c for eslint bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=141190701&repo=autoland

Comment 37

[Bob Silverberg [:bsilverberg]]

Mark, did you see that this was backed out?

Comment 38

[Mark Striemer [:mstriemer]]

Comment on attachment 8916149 [details]
Bug 1386018 - Tell users that the default search engine was set by an extension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187396/diff/6-7/

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1253b19c7200
Tell users that the default search engine was set by an extension r=aswan,bsilverberg,jaws

Comment 40

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1253b19c7200