Closed Bug 1408472 Opened 2 years ago Closed 2 years ago

Read-only browserSettings should return false on write attempts

Categories

(WebExtensions :: Untriaged, enhancement, P3)

enhancement

Tracking

(firefox57 wontfix, firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: wbamberg, Assigned: bsilverberg)

Details

Attachments

(1 file)

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
Priority: -- → P3
Assignee: nobody → bob.silverberg
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?
Flags: needinfo?(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.
Flags: needinfo?(wbamberg)
(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?
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 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.
Attachment #8932550 - Flags: review?(mixedpuppy) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01627b4d5c0c
Read-only browserSettings should return false on write attempts, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/01627b4d5c0c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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!
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(bob.silverberg) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.