1354344 - Show if an extension has set the home page

Status: RESOLVED FIXED

(Toolkit :: Preferences, enhancement, P3)

Description

[Andy McKay]

Attachment: Screenshot 2017-04-06 16.25.51.png

If an extension has changed the home page then the about:preferences will show the home page as the new value. Users can change it there. However its not clear that the change has come from the extension, its just shows the change (as per attachment).

Chatting to kev he thought this would be a good thing to highlight that its been changed by an add-on. Given we've already got the UI here, it should be possible to do something.

Primarily all we have to do is show the value is set by an add-on. I'm not sure if we record the value has been set by an add-on behind the scenes... or perhaps just check if the value matches what an add-on sets it too?

Either way. Markus, any ideas what we could do for UX for this field?

Comment 1

[Markus Jaritz [:designakt] (UX)]

Can I test the current implementation somehow Andy?
What will happen to changes a user makes if an extension has set the value?
Will the extension reset everything the user changes?

And some aspects that need UI:
How can the user stop an extension from setting the homepage?
Will it continue to be possible to have set multiple URLs? By extension and user?

Comment 2

[Andy McKay]

(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #1)
> Can I test the current implementation somehow Andy?

Create a manifest.json file that reads:

{
  "manifest_version": 2,
  "name": "Home page example",
  "version": "0.1",
  "chrome_settings_overrides": {
      "homepage": "http://example.com"
  }
}

And run that in Firefox nightly and you'll see how it works.


> What will happen to changes a user makes if an extension has set the value?

The users preference overrides the add-ons, go to about:preferences to set the new value.

> Will the extension reset everything the user changes?
> 
> And some aspects that need UI:
> How can the user stop an extension from setting the homepage?

You can't, but it only does it at extension install.

> Will it continue to be possible to have set multiple URLs? 

You can do that for the home page?

> By extension and user?

Comment 3

[Markus Jaritz [:designakt] (UX)]

(In reply to Andy McKay [:andym] from comment #2)
> (In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #1)
> > Will it continue to be possible to have set multiple URLs? 
> 
> You can do that for the home page?

using a pipe you can set multiple home pages. e.g.
http://example.com/|http://www.domain.com/|http://example.net/

Comment 4

[Andy McKay]

(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #3)
> (In reply to Andy McKay [:andym] from comment #2)
> > (In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #1)
> > > Will it continue to be possible to have set multiple URLs? 
> > 
> > You can do that for the home page?
> 
> using a pipe you can set multiple home pages. e.g.
> http://example.com/|http://www.domain.com/|http://example.net/

I never knew. Why do we do this sort of thing to ourselves?

Anyways the WebExtension API validates if its a URL and sets that value. It throws an error:

Reading manifest: Error processing chrome_settings_overrides.homepage: TypeError: https://github.com|https://example.com is not a valid URL

Seems fine to me :)

Comment 5

[Markus Jaritz [:designakt] (UX)]

Will people know that an extension will replace their set Home Page before installing?
Will this be listed as a permission, shown to the user in context (as Chrome does) on none?

Will people be able to revert that change if they realize they do not like it?

If one of them is no, we should address those before we consider shipping this feature. Even if we would expose it in about:preferences it would have potential to cause a lot of confusion for some people. 

(adding Tina who is working on preferences UX)

Comment 6

[Andy McKay]

(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #5)
> Will people know that an extension will replace their set Home Page before
> installing?

No, since it currently does not require a permission.

> Will this be listed as a permission, shown to the user in context (as Chrome
> does) on none?
> 
> Will people be able to revert that change if they realize they do not like
> it?

Yes, in about:preferences.

> If one of them is no, we should address those before we consider shipping
> this feature. Even if we would expose it in about:preferences it would have
> potential to cause a lot of confusion for some people. 
> 
> (adding Tina who is working on preferences UX)

Comment 7

[Markus Jaritz [:designakt] (UX)]

Attachment: Surface Preference Changes - Firefox Preference Override - Home Page.png

We should be adopting how Chrome does this:
Override setting the Home Page with what the extension sets.
On disabling the extensions,the previous value is restored. (Either an earlier installed extension, or the user set value/default)

And we should adopt notifying the user about that change when it comes into effect while showing the new Home Page. See here for the current flow Chrome uses:
https://www.lucidchart.com/documents/view/9e862cda-f9f8-4174-bcd6-b1c4bcf3f514/1

I do not know how this notification will interact with an extension opening another tab on install... If that would block seeing the notification, we should either block that other tab, as the extension will already get a tab (the Home Page), or wait with notifying until the user opens a new tab themselves and see the override.

Comment 8

[Andy McKay]

So in your image, I cannot change the home page. If I don't like that an add-on has set the home page, my only option is to install it?

I think if we were going to implement a flow similar to Chrome, it might make sense to have some Firefox mocks, I bet there's some UI differences and interactions that need to be thought through.

Comment 9

[Andy McKay]

Also, currently there is no UI in about:preferences for changing the new tab page. Would you like to consider it as part of this bug, or should we do it separately?

Comment 10

[Markus Jaritz [:designakt] (UX)]

(In reply to Andy McKay [:andym] from comment #8)
> So in your image, I cannot change the home page. If I don't like that an
> add-on has set the home page, my only option is to install it?

I assume you meant disable when you said install. Yes, that is the option.
As this is a big change to Firefox, I expect it to be central to the function of an extension (therefor disable), and not just a thing the extension does also. And it would be odd to call the function an override if it just sets one pref once. (I'll chat with Kev about that, as this touches on what the expected scope of extensions is.)

> I think if we were going to implement a flow similar to Chrome, it might
> make sense to have some Firefox mocks, I bet there's some UI differences and
> interactions that need to be thought through.

Yes, there needs to be some more detailing on
- the notification informing the user about the new Home Page
- considering an undo of disabling the extension (in preferences)

other than that I do not see any dependencies. Please share if you can think of any.

(In reply to Andy McKay [:andym] from comment #9)
> Also, currently there is no UI in about:preferences for changing the new tab
> page. Would you like to consider it as part of this bug, or should we do it
> separately?

I am under the impression that newTab and HomePage will merge with 57. 
So there should not be the need for any additional work.

Comment 11

[Markus Jaritz [:designakt] (UX)]

> (In reply to Andy McKay [:andym] from comment #9)
> > Also, currently there is no UI in about:preferences for changing the new tab
> > page. Would you like to consider it as part of this bug, or should we do it
> > separately?
> 
> I am under the impression that newTab and HomePage will merge with 57. 
> So there should not be the need for any additional work.

I was mistaken, those page will stay separate in Photon, but will both show Activity Stream by default.
So, finding a solution to how to surface if an extension controls new Tab, or new Tab and Home Page is still to be addressed. As new Tab is not visible in preferences, and opening both pages on startup for the user to is not an option, this becomes a bigger issue to resolve. (But would still consider as one issue)

Comment 13

[Mark Striemer [:mstriemer]]

Attachment: Bug 1354344 - Show extension controlling home page in preferences

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

Comment 14

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

r?bsilverberg for the Extensions{Settings,Preferences}Manager changes

r?dao for the preferences changes, it looks like you were the most recent reviewer here, but feel free to redirect if you'd like

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review172688

Nice work, Mark. I've left a couple of comments to be addressed.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:233
(Diff revision 1)
>    initialize() {
>      return initialize();
>    },
>  
> +  isInitialized() {
> +    return _store.dataReady;

I think this is okay, but I am wondering about a possible race condition with this in conjunction with its use in `getControllingExtensionId` above. What if the store has started being initialized (i.e., `_initializePromise` is not undefined), but hasn't finished initializing yet (so `_store.dataReady` does not return true)? If that's happening while `getControllingExtensionId` is called, mightn't it return undefined even though it should actually query the settings store for the id? It might be safer to dispense with `isInitialized` entirely and stick with the pattern in the ExtensionPreferencesManager where we always initialize the store first, and then interact with it.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:461
(Diff revision 1)
>        "controllable_by_this_extension";
>    },
>  
> +  // Return the id of the controlling extension or undefined if no extension is
> +  // controlling this setting.
> +  getTopExtensionId(type, key) {

On further reflection, this method is pretty much exactly the same as `getTopItem`, except that it returns the id. Why not just update `getTopItem` to also return the id? I haven't checked, but I don't think that would mess up any code that currently uses `getTopItem` and even if it does, it should be a pretty simple thing to fix.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:478
(Diff revision 1)
> +        return item.id;
> +      }
> +    }
> +
> +    // Nothing found in the precedenceList, return undefined.
> +    return undefined;

Why return null at line 466, but undefined here? There are already methods in ExtensionSettingsStore which return null, so perhaps it would be better to have this return null, as opposed to undefined, if no controlling extension is found?

Comment 16

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review172688

> I think this is okay, but I am wondering about a possible race condition with this in conjunction with its use in `getControllingExtensionId` above. What if the store has started being initialized (i.e., `_initializePromise` is not undefined), but hasn't finished initializing yet (so `_store.dataReady` does not return true)? If that's happening while `getControllingExtensionId` is called, mightn't it return undefined even though it should actually query the settings store for the id? It might be safer to dispense with `isInitialized` entirely and stick with the pattern in the ExtensionPreferencesManager where we always initialize the store first, and then interact with it.

It would definitely be nice to have the calling functions be async but I'm not sure that would work great with preferences. I could make them part sync and part async but that could get a bit weird.

When I was looking at this code I was thinking that if an extension is controlling something then this code will have been loaded. But maybe that's not the case. From a quick test of installing an extension that controls the home page and closing/opening Firefox with the preferences tab as the open page the controlling extension is shown, as expected.

> On further reflection, this method is pretty much exactly the same as `getTopItem`, except that it returns the id. Why not just update `getTopItem` to also return the id? I haven't checked, but I don't think that would mess up any code that currently uses `getTopItem` and even if it does, it should be a pretty simple thing to fix.

`getTopItem` is outside of the `ExtensionSettingsStore` API so I moved its body to a separate function with the id and `getTopItem` and `getTopExtensionId` both call that.

Comment 17

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review172956

Comment 18

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 19

[Mark Striemer [:mstriemer]]

This is now rebased on top of bug 1390174 which should be landing on inbound soon. It simplified some of the hiding/showing since the content is no longer in a table. I also tried to pull that out into more generic functions that should reduce the noise in syncFromHomePref().

Ready for another look, @bsilverberg and @dao.

Comment 20

[Mark Striemer [:mstriemer]]

Attachment: show-extension.png

Comment 21

[Mark Striemer [:mstriemer]]

Attachment: show-extension.png

Comment 22

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review180484

This looks good, Mark, thanks for the updates. Just a few minor issues, but I am also still concerned about the `isInitialized` thing and race conditions. Can you comment on that issue?

::: browser/components/preferences/in-content-new/main.js:2587
(Diff revision 2)
>    localHandlerApp.executable = aFile;
>  
>    return localHandlerApp;
>  }
>  
> +function getControllingExtensionId(name) {

Nit: `name` seems like a confusing argument name here, as it sounds like it might be the addon name, rather than the setting name. Maybe `setting` or `settingName` would be better?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:181
(Diff revision 2)
> +   * @param {string} prefName The name of the preference.
> +   *
> +   * @returns {string|undefined} The id of the extension, or undefined.
> +   */
> +  getControllingExtensionId(prefName) {
> +    if (ExtensionSettingsStore.isInitialized()) {

I'm not sure that the issue I raised about the fact that the settingsStore may not be initialized at this point has been addressed. Can you comment on this?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:111
(Diff revision 2)
>    if (!_store.data[type]) {
>      _store.data[type] = {};
>    }
>  }
>  
>  // Return an object with properties for key and value|initialValue, or null

This comment does not mention that this function can return `id` and still states that it returns `key`.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:113
(Diff revision 2)
>    }
>  }
>  
>  // Return an object with properties for key and value|initialValue, or null
>  // if no setting has been stored for that key.
> -function getTopItem(type, key) {
> +function getTopItemData(type, key) {

I may be missing something here, but why not just also return `id` from `getTopItem`, rather than introducing a new function for `getTopItemData`? It seems like there's duplication now between `getTopItem` and `getTopItemData`.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:243
(Diff revision 2)
>     */
>    initialize() {
>      return initialize();
>    },
>  
> +  isInitialized() {

As mentioned in my comments about the PreferencesManager, I'm not sure this `isInitialized` is a good idea, and is potentially setting us up for race conditions.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:470
(Diff revision 2)
>      return topItem.installDate > addon.installDate.valueOf() ?
>        "controlled_by_other_extensions" :
>        "controllable_by_this_extension";
>    },
>  
> +  // Return the id of the controlling extension or undefined if no extension is

s/undefined/null

Comment 23

[Jonathan Kingston [:jkt] he/him]

Things I noticed about the patch I noticed as I was looking at it for Bug 1397100:

- showControllingExtension doesn't change it's string based on the pref
  - Probably worth another lookup map/object like you have for: extensionControlledContentIds
- Methods could be async and use await instead of then
- Instead of using style.display, about:preferences usually has hidden=true/false (tests will need to be fixed too)

Comment 24

[Jonathan Kingston [:jkt] he/him]

Also I think the string should be in: browser/locales/en-US/chrome/browser/preferences/preferences.properties not in browser/locales/en-US/chrome/browser/browser.properties the former can be accessed with document.getElementById("bundlePreferences") like the rest of the file is doing.

Comment 25

[Mark Striemer [:mstriemer]]

Thanks, I addressed these comments but need to rebase this patch with inbound still since it looks like in-content-new moved.

Comment 26

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 27

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 28

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review183344

All of the comments should be addressed now, thanks!

Comment 29

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review172688

> It would definitely be nice to have the calling functions be async but I'm not sure that would work great with preferences. I could make them part sync and part async but that could get a bit weird.
> 
> When I was looking at this code I was thinking that if an extension is controlling something then this code will have been loaded. But maybe that's not the case. From a quick test of installing an extension that controls the home page and closing/opening Firefox with the preferences tab as the open page the controlling extension is shown, as expected.

I made them async and it seems a bit odd in `_updateUseCurrentButton` but it works fine. This is probably the better option as you said.

Comment 30

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review183396

Looks good Mark. Thanks for the updates. r+ with my two comments addressed.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:173
(Diff revision 4)
>      return defaultPreferences.get(prefName);
>    },
>  
>    /**
> +   * Gets the id of the extension controlling a preference. If the setting
> +   * store is not initialized or the preference is not controlled by an

I think this comment remains from the old version. This won't return undefined if the store isn't initialized.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:294
(Diff revision 4)
>  
>      _store.saveSoon();
>  
>      // Check whether this is currently the top item.
>      if (keyInfo.precedenceList[0].id == id) {
> -      return {key, value};
> +      return {id, key, value};

Why do we need to return `id` from this function? 

If we do, the js doc comment needs to be updated and we should add a test for it.

Comment 31

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 32

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review183396

> Why do we need to return `id` from this function? 
> 
> If we do, the js doc comment needs to be updated and we should add a test for it.

The tests expect `getSetting` and `addSetting` to have the same return value, so I added this to be consistent.

Comment 33

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 34

[Justin Dolske [:Dolske]]

Driveby: I believe preferences is now consistently referring to "add-ons", not "extensions". And how concerned are we about having extensions with long gross names? e.g. "An extension, Super Add-on™ With Extra Gizmos, Built With Brand Power Do Not Disable!, controls your home page."

Comment 35

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

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review184600

r=me for the preferences changes once you address the issues below

::: browser/components/preferences/in-content/main.js:727
(Diff revision 6)
> +    // If the homepage is controlled by an extension then you can't use this.
> +    if (await getControllingExtensionId("homepage_override")) {
> +      useCurrent.disabled = true;

It looks like this block can be removed, and then `#useCurrent` can be added to the selector list within `syncFromHomePref`. Am I missing something?

::: browser/components/preferences/in-content/main.js:2621
(Diff revision 6)
> +  let stringParts = document
> +    .getElementById("bundlePreferences")
> +    .getString(`extensionControlled.${settingName}`)
> +    .split("%S");

Splitting the string and rebuilding it will not work for all languages, for example if %S ends up appearing at the beginning of the string.

You will need to use getFormattedString() to build the string. You can look at http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/base/content/browser-addons.js#549 for an example.

::: browser/components/preferences/in-content/main.js:2638
(Diff revision 6)
> +  description.appendChild(document.createTextNode(stringParts[0]));
> +  let image = document.createElement("image");
> +  image.setAttribute("src", addon.iconURL || defaultIcon);
> +  image.classList.add("extension-controlled-icon");
> +  description.appendChild(image);
> +  description.appendChild(document.createTextNode(` ${addon.name}`));

The space here should be part of the string in the .properties file. See the above review comment for more info.

::: browser/components/preferences/in-content/main.xul:354
(Diff revision 6)
>    <caption><label>&homepage2.label;</label></caption>
>  
> +  <hbox id="browserHomePageExtensionContent" align="center">
> +    <description control="disableHomePageExtension" flex="1" />
> +    <button id="disableHomePageExtension"
> +            class="extension-controlled-button"

This button should also have a class name of `accessory-button`.

::: browser/components/preferences/in-content/tests/browser_homepages_filter_aboutpreferences.js:66
(Diff revision 6)
> +    observer.observe(target, { attributes: true, attributeFilter: ["hidden"] });
> +  });
> +}
> +
> +function waitForMessageHidden() {
> +  return waitForMessageChange((target) => target.hidden === true);

return waitForMessageChange(target => target.hidden);

::: browser/components/preferences/in-content/tests/browser_homepages_filter_aboutpreferences.js:70
(Diff revision 6)
> +function waitForMessageHidden() {
> +  return waitForMessageChange((target) => target.hidden === true);
> +}
> +
> +function waitForMessageShown() {
> +  return waitForMessageChange((target) => target.hidden === false);

return waitForMessageChange(target => !target.hidden);

::: browser/themes/shared/incontentprefs/preferences.inc.css:169
(Diff revision 6)
> +.extension-controlled-button {
> +  margin-inline-end: 0;
> +}

This can be removed once you switch to using `accessory-button`.

Comment 36

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review184600

> It looks like this block can be removed, and then `#useCurrent` can be added to the selector list within `syncFromHomePref`. Am I missing something?

This function gets called more often than `syncFromHomePref`. Specifically it gets called when the window is focused so that it can update the text and the disabled state. If this is removed then focusing the window will enable the button.

> Splitting the string and rebuilding it will not work for all languages, for example if %S ends up appearing at the beginning of the string.
> 
> You will need to use getFormattedString() to build the string. You can look at http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/base/content/browser-addons.js#549 for an example.

I don't think I can use `getFormattedString` here since there's supposed to be an image in the middle of the string. Using split should work though since you'll get an empty string on either side if the split is on the end.

```
const strings = ["foo %S bar", "%S foo bar", "foo bar %S"];
strings.forEach((str) => console.log(str.split("%S")));
// ["foo ", " bar"]
// ["", " foo bar"]
// ["foo bar ", ""]
```

> The space here should be part of the string in the .properties file. See the above review comment for more info.

This space is between the image and the addon name. I don't think I could do this as suggested above unless I'm misunderstanding how that works.

I could add the space with CSS if you'd prefer.

Comment 37

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 38

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

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

Comment 39

[Mark Striemer [:mstriemer]]

(In reply to Justin Dolske [:Dolske] from comment #34)
> Driveby: I believe preferences is now consistently referring to "add-ons",
> not "extensions". And how concerned are we about having extensions with long
> gross names? e.g. "An extension, Super Add-on™ With Extra Gizmos, Built With
> Brand Power Do Not Disable!, controls your home page."

I believe Markus has considered these but I've added it to our agenda for our meeting tomorrow.

Comment 40

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

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

https://reviewboard.mozilla.org/r/166222/#review184684

::: browser/components/preferences/in-content/tests/browser_homepages_filter_aboutpreferences.js:66
(Diff revisions 6 - 8)
>      observer.observe(target, { attributes: true, attributeFilter: ["hidden"] });
>    });
>  }
>  
>  function waitForMessageHidden() {
> -  return waitForMessageChange((target) => target.hidden === true);
> +  return waitForMessageChange((target) => target.hidden);

you can drop the parens for a single argument fat-arrow function

::: browser/components/preferences/in-content/tests/browser_homepages_filter_aboutpreferences.js:70
(Diff revisions 6 - 8)
>  function waitForMessageHidden() {
> -  return waitForMessageChange((target) => target.hidden === true);
> +  return waitForMessageChange((target) => target.hidden);
>  }
>  
>  function waitForMessageShown() {
> -  return waitForMessageChange((target) => target.hidden === false);
> +  return waitForMessageChange((target) => !target.hidden);

ditto

Comment 41

[Mark Striemer [:mstriemer]]

Comment on attachment 8895484 [details]
Bug 1354344 - Show extension controlling home page in preferences

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166222/diff/8-9/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/228fbca07af3
Show extension controlling home page in preferences r=bsilverberg,jaws

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/228fbca07af3

Comment 44

[Justin Dolske [:Dolske]]

(In reply to Mark Striemer [:mstriemer] from comment #39)
> (In reply to Justin Dolske [:Dolske] from comment #34)
> > Driveby: I believe preferences is now consistently referring to "add-ons",
> > not "extensions". And how concerned are we about having extensions with long
> > gross names? e.g. "An extension, Super Add-on™ With Extra Gizmos, Built With
> > Brand Power Do Not Disable!, controls your home page."
> 
> I believe Markus has considered these but I've added it to our agenda for
> our meeting tomorrow.

Did this get confirmed?

Comment 45

[Markus Jaritz [:designakt] (UX)]

Mark did ask me, here is what I said to him:

(In reply to Justin Dolske [:Dolske] from comment #34)
> Driveby: I believe preferences is now consistently referring to "add-ons",
> not "extensions". 

The guideline for use is: we only use "add-ons" when we refer to multiple types of things to add on to the browser.
Like when referring to extensions and themes together.
And we use more specific terms where we can. 
Like if we only talk about themes, we say "themes" and when we talk about an extension we say "extension".

> And how concerned are we about having extensions with long
> gross names? e.g. "An extension, Super Add-on™ With Extra Gizmos, Built With
> Brand Power Do Not Disable!, controls your home page."

The longest I could find in the top 500 extensions is "Simple YouTube to MP3/MP4 Converter and Downloader" and it results in having the message use 2 lines, instead of one. Which is ok for the cases it happens.

Mark, I noticed the button aligns to the center of the 2 lines, it would be better if the button would align to the top instead.

Comment 46

[mheubusch]

Drive by for Dolske's Drive By:  Markus is correct. When we can specify an extension, we should.  Add-ons is the collective term we use when referring to a user-installed thing that could be a theme or an extension.  If this setting will never govern a theme, then as Markus suggests it is most appropriate to call it an extension.

Comment 47

[Justin Dolske [:Dolske]]

Thanks!

Comment 48

[Mike Kaply [:mkaply]]

This broke locking of homepage preferences.

See bug 1407999