Show if an extension has set the default search engine

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bsilverberg, Assigned: mstriemer)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 ?, firefox58 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.

Updated

2 years ago
Priority: -- → P2

Updated

2 years ago
Comment hidden (mozreview-request)
Assignee

Comment 2

2 years ago
Posted image search-newtab-home-controlled.mov.gif (obsolete) —
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.
Assignee

Comment 3

2 years ago
Here's a screenshot of the placement of the message. Can you confirm this is what you'd like, Markus?
Flags: needinfo?(mjaritz)
Comment hidden (mozreview-request)
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 `/>`
Attachment #8909977 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
Markus is out so I got confirmation from Emanuela over Slack that these screenshots are good.
Flags: needinfo?(mjaritz)
Keywords: checkin-needed
Assignee

Comment 8

2 years ago
Removing checkin-needed for now since bug 1373853 needs to land first.
Keywords: checkin-needed

Comment 9

2 years ago
Kev, will ping you on this one.
Flags: needinfo?(kev)

Comment 10

2 years ago
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.
Flags: needinfo?(kev)
Comment hidden (mozreview-request)
Assignee

Comment 12

2 years ago
Posted image default-search.mov.gif (obsolete) —
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.
Attachment #8909979 - Attachment is obsolete: true
Flags: needinfo?(mjaritz)
(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!
Flags: needinfo?(mjaritz) → needinfo?(mstriemer)

Comment 14

2 years ago
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".
Assignee

Comment 15

2 years ago
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.
Flags: needinfo?(mstriemer)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8909977 - Attachment is obsolete: true
Assignee

Comment 17

2 years ago
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.
Attachment #8912931 - Attachment is obsolete: true
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);
Attachment #8916149 - Flags: review?(jaws) → review+
Assignee

Comment 19

2 years ago
mozreview-review
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.
Assignee

Comment 20

2 years ago
> 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.
Reporter

Comment 21

2 years ago
mozreview-review
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?
Attachment #8916149 - Flags: review?(bob.silverberg) → review-
Comment hidden (mozreview-request)
Assignee

Comment 23

2 years ago
mozreview-review-reply
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.
Reporter

Comment 24

2 years ago
mozreview-review
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.
Attachment #8916149 - Flags: review?(bob.silverberg) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 25

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ba4a39241953
Tell users that the default search engine was set by an extension r=bsilverberg,jaws
Keywords: checkin-needed
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(mstriemer)
Keywords: checkin-needed

Comment 28

2 years ago
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 hidden (mozreview-request)

Comment 30

2 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Comment 32

2 years ago
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 hidden (mozreview-request)

Comment 34

2 years ago
mozreview-review
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
Attachment #8916149 - Flags: review?(aswan) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 35

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/607922c730a1
Tell users that the default search engine was set by an extension r=aswan,bsilverberg,jaws
Keywords: checkin-needed
Reporter

Comment 37

2 years ago
Mark, did you see that this was backed out?
Flags: needinfo?(mstriemer)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(mstriemer)
Keywords: checkin-needed

Comment 39

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1253b19c7200
Tell users that the default search engine was set by an extension r=aswan,bsilverberg,jaws
Keywords: checkin-needed

Comment 40

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1253b19c7200
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.