1408472 - Read-only browserSettings should return false on write attempts

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P3)

Description

[Will Bamberg [:wbamberg]]

The homepageOverride[1] and newTabPageOverride[2] browser settings are read-only.

Currently, if you try to set them, the promise resolves with undefined. As per the docs for BrowserSetting.set[3], it should probably resolve with false.

Also, if you call get(), you get:

Object { levelOfControl: "controllable_by_this_extension", value: "about:newtab" }

I think levelOfControl should be "not_controllable" as per [4]

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/homepageOverride
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/newTabPageOverride
[3] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting/set#Return_value
[4] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting/get#Return_value

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1408472 - Read-only browserSettings should return false on write attempts,

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

Comment 2

[Bob Silverberg [:bsilverberg]]

I'm working on a patch for this, and I agree that both set and clear should return false. I also agree that get should sometimes, but not always, return a levelOfControl of "not_controllable". In the case where either the current extension is controlling the setting, or another extension has control, I think we should still return "controlled_by_this_extension" and "controlled_by_other_extensions", respectively. In the case where we would normally return "controllable_by_this_extension", I agree that we should return "not_controllable" instead.

Do you agree, Will?

Comment 3

[Will Bamberg [:wbamberg]]

I might be confused here, but I thought an extension could only get "control" of a setting by changing it. So if it's read-only, they can't, so "controlled_by_this_extension" and "controlled_by_other_extensions" should not be possible.

Comment 4

[Bob Silverberg [:bsilverberg]]

(In reply to Will Bamberg [:wbamberg] from comment #3)
> I might be confused here, but I thought an extension could only get
> "control" of a setting by changing it. So if it's read-only, they can't, so
> "controlled_by_this_extension" and "controlled_by_other_extensions" should
> not be possible.

I know it _is_ a bit confusing. The "read-only" browserSettings are actually controllable, but only via manifest keys, as opposed to dynamically controllable via the browserSettings API. Both new tab page and home page can be controlled by an extension, but the value can not be changed via browserSettings - it can only be changed via an update which makes changes to the manifest keys.

Does that clear it up at all?

Comment 5

[Will Bamberg [:wbamberg]]

Oh right, yes, that makes sense. I had forgotten that extensions could control this using manifest keys. I'll update the docs to mention this :).

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8932550 [details]
Bug 1408472 - Read-only browserSettings should return false on write attempts,

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932550 [details]
Bug 1408472 - Read-only browserSettings should return false on write attempts,

https://reviewboard.mozilla.org/r/203600/#review209496

::: toolkit/components/extensions/ext-browserSettings.js:39
(Diff revision 2)
> +      levelOfControl =
> +        (readOnly && levelOfControl === "controllable_by_this_extension") ?
> +          "not_controllable" :
> +          levelOfControl;
> +      return {
> +        levelOfControl,

I wish this had just been readOnly: bool and owner: bool.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01627b4d5c0c
Read-only browserSettings should return false on write attempts, r=mixedpuppy

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/01627b4d5c0c

Comment 10

[Victor Carciu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!