1428306 - Investigate removing browser.storageManager.enabled pref

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

:johannh hinted that we might be able to remove this pref at this point.

To be clear I'm not extremely aware of the background behind this pref. I'm assuming that it was used to control rollout of a new feature, and that feature is now mature, and we can remove the pref and replace all usages with code that would have been executed if the pref was enabled, and remove code that would have been executed if it was disabled. I'm happy to write a patch to this effect but that's all I know.

:fischer, could you comment on whether we can go ahead and do this? Here are all the usages of the pref: https://searchfox.org/mozilla-central/search?q=browser.storageManager.enabled

There is a web-platform test usage of this pref, and I'm not sure exactly how this is related to dom.storageManager.enabled. Filing this bug to start a real conversation about this.

Comment 1

[Fischer [:Fischer]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> :johannh hinted that we might be able to remove this pref at this point.
> 
> To be clear I'm not extremely aware of the background behind this pref. I'm
> assuming that it was used to control rollout of a new feature, and that
> feature is now mature, and we can remove the pref and replace all usages
> with code that would have been executed if the pref was enabled, and remove
> code that would have been executed if it was disabled. I'm happy to write a
> patch to this effect but that's all I know.
> 
> :fischer, could you comment on whether we can go ahead and do this? Here are
> all the usages of the pref:
> https://searchfox.org/mozilla-central/search?q=browser.storageManager.enabled
> 
> There is a web-platform test usage of this pref, and I'm not sure exactly
> how this is related to dom.storageManager.enabled. Filing this bug to start
> a real conversation about this.
Hi Nihanth,

`browser.storageManager.enabled` and `dom.storageManager.enabled` are both the prefs to on/off the functions of the web persistent storage standard[1].
The persistent storage is an new and living standard.
`browser.storageManager.enabled` is for the UI part and `dom.storageManager.enabled` is for the DOM part.

For the UI part, since this persistent storage has been on by default from 57(works fine till now) and afaik there are even ongoing plans to modify about:preferences, permission around it. Probably ok to remove `browser.storageManager.enabled`.

But I would say probably we should consider `browser.storageManager.enabled` and `dom.storageManager.enabled` together.
Because the UI part and the DOM part have to do with each other, we may want to be able to on/off the UI and the DOM side together.

NI the DOM side to see their plan for `dom.storageManager.enabled`

[1] https://storage.spec.whatwg.org/

------------------------------------------

Hi Shawn,

As described above, there is a plan to remove `browser.storageManager.enabled` pref. What is your plan to remove `dom.storageManager.enabled`?

Thank you

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Fischer [:Fischer] from comment #1)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> > :johannh hinted that we might be able to remove this pref at this point.
> > 
> > To be clear I'm not extremely aware of the background behind this pref. I'm
> > assuming that it was used to control rollout of a new feature, and that
> > feature is now mature, and we can remove the pref and replace all usages
> > with code that would have been executed if the pref was enabled, and remove
> > code that would have been executed if it was disabled. I'm happy to write a
> > patch to this effect but that's all I know.
> > 
> > :fischer, could you comment on whether we can go ahead and do this? Here are
> > all the usages of the pref:
> > https://searchfox.org/mozilla-central/search?q=browser.storageManager.enabled
> > 
> > There is a web-platform test usage of this pref, and I'm not sure exactly
> > how this is related to dom.storageManager.enabled. Filing this bug to start
> > a real conversation about this.
> Hi Nihanth,
> 
> `browser.storageManager.enabled` and `dom.storageManager.enabled` are both
> the prefs to on/off the functions of the web persistent storage standard[1].
> The persistent storage is an new and living standard.
> `browser.storageManager.enabled` is for the UI part and
> `dom.storageManager.enabled` is for the DOM part.
> 
> For the UI part, since this persistent storage has been on by default from
> 57(works fine till now) and afaik there are even ongoing plans to modify
> about:preferences, permission around it. Probably ok to remove
> `browser.storageManager.enabled`.
> 
> But I would say probably we should consider `browser.storageManager.enabled`
> and `dom.storageManager.enabled` together.
> Because the UI part and the DOM part have to do with each other, we may want
> to be able to on/off the UI and the DOM side together.
> 
> NI the DOM side to see their plan for `dom.storageManager.enabled`
> 
> [1] https://storage.spec.whatwg.org/
> 
> ------------------------------------------
> 
> Hi Shawn,
> 
> As described above, there is a plan to remove
> `browser.storageManager.enabled` pref. What is your plan to remove
> `dom.storageManager.enabled`?
> 
> Thank you
Yes, there is no strong reason to keep this pref anymore now.
I think I will remove the pref.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Well, maybe we still need this pref. Giving the reason that we don't support storage feature for non-desktop platforms.
I wondered how we are going to support this feature for fennec.

If we remove dom.storageManager.enabled, the DOM interface will be exposed, but there is no UI for site data management, right?

Comment 4

[Fischer [:Fischer]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> Well, maybe we still need this pref. Giving the reason that we don't support storage feature for non-desktop platforms.
> I wondered how we are going to support this feature for fennec.
> 
> If we remove dom.storageManager.enabled, the DOM interface will be exposed, but there is no UI for site data management, right?
Yes, the mobile UI and the desktop UI are separated from each other.

Comment 5

[Johann Hofmann [:johannh]]

So, let me try to summarize:

- browser.storageManager.enabled only controls the desktop browser UI for storage management
- dom.storageManager.enabled controls the DOM interface for both desktop and mobile (where it's off by default)
- The UI storage management seems to work fine for me even with dom.storageManager.enabled
- There are no plans to reconsider the features that were added behind the browser.storageManager.enabled flag

This sounds like we can just go ahead and remove browser.storageManager.enabled while keeping dom.storageManager.enabled around, unless there's some dependency between the two that I'm not aware of.

One more thing: browser.storageManager.enabled seems to be included in some webplatform tests:

https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/testing/web-platform/meta/storage/storagemanager-persist.https.html.ini#3

Can we just remove it from Gecko or do we need to upstream this somehow?

Comment 6

[Fischer [:Fischer]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> So, let me try to summarize:
> 
> - browser.storageManager.enabled only controls the desktop browser UI for
> storage management
> - dom.storageManager.enabled controls the DOM interface for both desktop and
> mobile (where it's off by default)
> - The UI storage management seems to work fine for me even with
> dom.storageManager.enabled
> - There are no plans to reconsider the features that were added behind the
> browser.storageManager.enabled flag
> 
> This sounds like we can just go ahead and remove browser.storageManager.enabled while keeping dom.storageManager.enabled around, unless there's some dependency between the two that I'm not aware of.
If "dom.storageManager.enabled" were turned off, the Site Data and related features would not work.
So if, unfortunately, "dom.storageManager.enabled" were going to be off somehow, because our about:preferences has been(and continuing being) modified around this persistent storage feature:
  Case A: "browser.storageManager.enabled" still there. We have to disable(hide) or remove features around it and revamp about:preferences and permission UI etc.
  Case B: "browser.storageManager.enabled" not there. We have to remove or hide features around it and revamp about:preferences and permission UI etc.

In either case, we all have to revisit and revamp our UI parts. From this aspect, with/without "browser.storageManager.enabled" both are much the same.

The usages with "browser.storageManager.enabled" I could think of are:
  - Turn on/off the Dom side and the front-end side altogether
  - Hide the features around it faster
If these usages were not relevant any more here, then it would be fine without "browser.storageManager.enabled".

> One more thing: browser.storageManager.enabled seems to be included in some webplatform tests:
> 
> https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/testing/web-platform/meta/storage/storagemanager-persist.https.html.ini#3
> 
Some tests are expecting the features behind "browser.storageManager.enabled" are on so removing "browser.storageManager.enabled" should be fine because we are making these features always on.

Found 2 another tests expect "browser.storageManager.enabled" as false, which are browser_search_within_preferences_1.js[1] and browser_search_within_preferences_command[2].
Both tests are testing about:preferences search function.
Before the persistent storage got released, "browser.storageManager.enabled" were off to hide the features from users.
As a result, the 2 tests set "browser.storageManager.enabled" as false to prevent it appear in the search results.
At this stage, it is fine to get rid of that as well because now users can search "Site Data" in the about:preferences.

[1] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js#220
[2] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/browser/components/preferences/in-content/tests/browser_search_within_preferences_command.js#9

> Can we just remove it from Gecko or do we need to upstream this somehow?
Depends on still want to reserve its usage in the future.

Comment 7

[Johann Hofmann [:johannh]]

> If "dom.storageManager.enabled" were turned off, the Site Data and related features would not work.

I can't reproduce this. I set dom.storageManager.enabled to false in about:config, restarted my browser, and the storage UI in about:preferences continues to work (the Quota Manager Service still works, just the public DOM StorageManager API is turned off). This makes sense to me since that pref should, IMO, toggle the external API rather than our internal APIs.

> Some tests are expecting the features behind "browser.storageManager.enabled" are on so removing "browser.storageManager.enabled" should be fine because we are making these features always on.

I meant that one webplatform test specifically (see the link) :)

Comment 8

[Fischer [:Fischer]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> > If "dom.storageManager.enabled" were turned off, the Site Data and related features would not work.
> 
> I can't reproduce this. I set dom.storageManager.enabled to false in
> about:config, restarted my browser, and the storage UI in about:preferences
> continues to work (the Quota Manager Service still works, just the public
> DOM StorageManager API is turned off). This makes sense to me since that
> pref should, IMO, toggle the external API rather than our internal APIs.
> 
Yeah, that's true. In about:preferences the Site Data section functions without the external API.
I think I too focused on its product concept.

Another place to think about is the permission UI. The persistent permission represents the permission for those external API.
Maybe that part we want to hide.

> > Some tests are expecting the features behind "browser.storageManager.enabled" are on so removing "browser.storageManager.enabled" should be fine because we are making these features always on.
> 
> I meant that one webplatform test specifically (see the link) :)
Yes, that one is safe to remove as well.

Comment 9

[Michael Kohler [:mkohler]]

Attachment: Bug 1428306 - Remove browser.storageManager.enabled pref

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

Comment 10

[Michael Kohler [:mkohler]]

Too late here to run all tests locally, I've pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9997ce415f266a85c583f9b17e7021f04806ce

Will check back tomorrow.

Do the changes to mozscreenshots get synced to the github repo or should I do them there? Also, is there anybody else who would need to review this patch as there are other modules than browser touched as well?

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

https://reviewboard.mozilla.org/r/218244/#review225348

This actually looks good to me but I'd prefer to take another look at this after you made the change below :)

Thank you!

::: browser/components/preferences/in-content/privacy.js:388
(Diff revision 1)
> -      setEventListener("siteDataSettings", "command",
> +    setEventListener("siteDataSettings", "command",
> -        gPrivacyPane.showSiteDataSettings);
> +      gPrivacyPane.showSiteDataSettings);
> -      let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "storage-permissions";
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "storage-permissions";
> -      document.getElementById("siteDataLearnMoreLink").setAttribute("href", url);
> +    document.getElementById("siteDataLearnMoreLink").setAttribute("href", url);
> -      let siteDataGroup = document.getElementById("siteDataGroup");
> +    let siteDataGroup = document.getElementById("siteDataGroup");
> -      siteDataGroup.removeAttribute("data-hidden-from-search");
> +    siteDataGroup.removeAttribute("data-hidden-from-search");

Please remove the two lines here as well as the "hidden" and "data-hidden-from-search" attributes here: https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/browser/components/preferences/in-content/privacy.xul#292

Comment 12

[Johann Hofmann [:johannh]]

Smaug, can you help us out with the web-platform test here? Can we just delete this code?

Thanks!

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

I know next to nothing about this code.

Comment 14

[Johann Hofmann [:johannh]]

Baku, can you review the web-platform test change?

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

https://reviewboard.mozilla.org/r/218244/#review226786

Comment 16

[Jan Varga [:janv]]

I'm not sure if it was mentioned here, but we need a way to disable whole feature if there's a security problem or something similar.
So we have to keep dom.storageManager.enabled for sure, but the other one can go away.

Comment 17

[Michael Kohler [:mkohler]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

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

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

https://reviewboard.mozilla.org/r/218244/#review228550

Nice, thank you!

::: commit-message-0790e:1
(Diff revision 2)
> +Bug 1428306 - Remove browser.storageManager.enabled pref r=johannh

You could amend your commit message with r=johannh,baku to reflect the actual review. :)

Comment 19

[Michael Kohler [:mkohler]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

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

Comment 20

[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 8d8241305eb3febfb364ba25c8369df473fc6478 -d 7f5e205a73ed: rebasing 448606:8d8241305eb3 "Bug 1428306 - Remove browser.storageManager.enabled pref r=baku,johannh" (tip)
merging browser/base/content/browser.js
merging browser/base/content/test/general/browser_storagePressure_notification.js
merging browser/components/nsBrowserGlue.js
merging browser/components/preferences/in-content/privacy.js
merging browser/components/preferences/in-content/privacy.xul
merging browser/components/preferences/in-content/tests/browser_bug795764_cachedisabled.js
merging browser/components/preferences/in-content/tests/browser_siteData.js
merging browser/components/preferences/in-content/tests/browser_siteData2.js
merging browser/components/preferences/in-content/tests/browser_siteData3.js
merging browser/modules/SitePermissions.jsm
merging browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm
merging modules/libpref/init/all.js
warning: conflicts while merging browser/components/preferences/in-content/privacy.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/privacy.xul! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_bug795764_cachedisabled.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_siteData2.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_siteData3.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 21

[Michael Kohler [:mkohler]]

Comment on attachment 8948858 [details]
Bug 1428306 - Remove browser.storageManager.enabled pref

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

Comment 22

[Johann Hofmann [:johannh]]

Would you like me to land this or does it still need rebasing?

Comment 23

[Michael Kohler [:mkohler]]

(In reply to Johann Hofmann [:johannh] from comment #22)
> Would you like me to land this or does it still need rebasing?

With the latest diff all the rebasing needs should have been taken care of. Can't test if it applies right now, but I'll guess we'll see if there is another conflict from the last 4 days ;)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19591cc39b9a
Remove browser.storageManager.enabled pref r=baku,johannh

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/19591cc39b9a