Allow WebExtensions to disable the browser cache

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
3 months ago
14 days ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla56
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: triaged[browserSettings])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
This will use browserSetting [1] to manage which extension has control over the pref. This does not really belong in privacy so will go into the browserSettings API.

According to the notes in [2], this would make use of the `browser.cache.disk.enable` and `browser.cache.memory.enable` preferences, which are boolean prefs.

I am assuming from the doc that the desire is for WebExtensions to be able to gloabally disable the cache, so we could use a single setting called browser.browserSettings.cacheEnabled which accepts a boolean and either enables or disables both of the above mentioned prefs.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/BrowserSetting
[2] https://docs.google.com/document/d/1HZiI-dqFATzLpmTtN1Bg6tD8y4N0Ne3D74LEWWgttOk/edit#
(Assignee)

Comment 1

3 months ago
Honza, could you please take a quick look at the API proposed above and let me know whether:

1. Are these the correct prefs to be flipping to obtain the behaviour we seek?
2. Are there any concerns or issues that we need to consider in terms of allowing WebExtensions to enable and disable the browser cache in this manner?
Flags: needinfo?(odvarko)
(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> Honza, could you please take a quick look at the API proposed above and let
> me know whether:
> 
> 1. Are these the correct prefs to be flipping to obtain the behaviour we
> seek?
Yes

> 2. Are there any concerns or issues that we need to consider in terms of
> allowing WebExtensions to enable and disable the browser cache in this
> manner?
I am not aware of any issues.

If I understand correctly, Chrome doesn't allow to disabled cache globally.
Should we do the same (to keep compatibility).

I am yet asking Mike who implemented "Disable HTTP Cache" DevTools options.

@Mike: please see comment #1

Honza
Flags: needinfo?(odvarko) → needinfo?(mratcliffe)
DevTools has an option to disable the HTTP Cache, which sets a flag on the docShell:

```
// Disable cache
this.docShell.defaultLoadFlags = Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING;

// Enable cache
this.docShell.defaultLoadFlags = Ci.nsIRequest.LOAD_NORMAL;
```

The reason we do it this way instead of using the prefs is because we only want to disable the cache for a particular tab.

Looking at the implementation of `browser.cache.disk.enable` and `browser.cache.memory.enable` they should work, even to disable the appcache. I would have no concerns over allowing web extensions to disable these caches.
Flags: needinfo?(mratcliffe)
(Assignee)

Comment 4

2 months ago
Thanks Honza and Mike. I think it is safe for us to proceed as discussed in the bug description.
(Assignee)

Updated

2 months ago
Assignee: nobody → bob.silverberg
(Assignee)

Comment 5

2 months ago
Scott,

This introduces a new API namespace called `browserSettings`, which will be used to control global behaviour of the browser. This is the first setting, which controls the browser cache, but a number of other settings will likley be added. I think this needs a permission string, but it's hard to know what it should be.

I propose "Read and modify browser settings", but I know that's kind of vague. What do you think?
Flags: needinfo?(sdevaney)

Comment 6

2 months ago
For APIs like this with sweeping capabilities ("used to control global behavior of the browser..." "... a number of other settings will likely be added") it's difficult to be specific within a permission bullet point. We (and Chrome) certainly have a precedent to be concise and vague when we can't get verbose. 

So I'm good with "Read and modify browser settings" or even simply "Manage browser settings".
Flags: needinfo?(sdevaney)
Comment hidden (mozreview-request)

Comment 8

2 months ago
This bug describes a what without any why.  What is the use case for this API?
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 9

2 months ago
The requirement came from the document linked in the bug description [1]. It discusses that a popular add-on (Web Developer [2]) is requesting the functionality to port the add-on to WebExtensions. 

The specific use case for why that add-on needs this feature is not discussed in that doc, but I assume it has to do with enhancing the Firefox environment for web developers. I will let Rob, who wrote that doc, provide a better answer, if he has one.

[1] https://docs.google.com/document/d/1HZiI-dqFATzLpmTtN1Bg6tD8y4N0Ne3D74LEWWgttOk/edit#
[2] https://addons.mozilla.org/en-US/firefox/addon/web-developer/
Flags: needinfo?(bob.silverberg) → needinfo?(rob)

Comment 10

2 months ago
During development, web devs want to see the latest version of their code, even if the server has incorrectly configured caching headers. To do so, the user needs to be able to disable the cache.

As an example (besides the Web Developer toolbar), both Firefox and Chromium's devtools have an option to (temporarily) disable the cache for the inspected page:
https://developer.mozilla.org/en-US/docs/Tools/Settings#Advanced_settings
https://developers.google.com/web/tools/chrome-devtools/network-performance/reference#change_loading_behavior
Flags: needinfo?(rob)
(Assignee)

Comment 11

2 months ago
Is this enough justification for you, aswan?
Flags: needinfo?(aswan)

Comment 12

2 months ago
It sounds like what is implemented in this bug is not what is actually desired (the stated use case is for a single page/tab, the attached patch implements a global switch).
Why didn't this run through the community advisory group?
Flags: needinfo?(aswan)
(Assignee)

Comment 13

2 months ago
(In reply to Andrew Swan [:aswan] from comment #12)
> It sounds like what is implemented in this bug is not what is actually
> desired (the stated use case is for a single page/tab, the attached patch
> implements a global switch).

Well, the global switch will also give them what they need. While it may be true that they would often only need to disable the cache for a single page/tab/site, it's also quite common for a developer to be working on a number of different sites at the same time and therefore _would_ want to disable the cache globally, rather than just for one page/tab/site.

> Why didn't this run through the community advisory group?

That's a question for andym. I was given the impression by him that Rob's Google doc [1] contained APIs that have already been accepted as something we want to add. A number of people saw and commented on that doc. I think this decision is partially based on the fact that these are seen as simple APIs that pretty much just flip prefs. I have proceeded with this effort under the assumption that anything in the doc has already been deemed as an API we want to add, not that they are just proposed APIs.

Andy, would you care to comment on this?

[1] https://docs.google.com/document/d/1HZiI-dqFATzLpmTtN1Bg6tD8y4N0Ne3D74LEWWgttOk/edit#
Flags: needinfo?(amckay)

Comment 14

2 months ago
mozreview-review
Comment on attachment 8877253 [details]
Bug 1364936 - Allow WebExtensions to disable the browser cache,

https://reviewboard.mozilla.org/r/148604/#review154604

mostly nits

::: toolkit/components/extensions/ext-browserSettings.js:22
(Diff revision 1)
> +            extension, name),
> +        value: await callback(),
> +      };
> +    },
> +    async set(details) {
> +      return await ExtensionPreferencesManager.setSetting(

no await here and no need for the function to be async

::: toolkit/components/extensions/ext-browserSettings.js:26
(Diff revision 1)
> +    async set(details) {
> +      return await ExtensionPreferencesManager.setSetting(
> +        extension, name, details.value);
> +    },
> +    async clear(details) {
> +      return await ExtensionPreferencesManager.removeSetting(

same as above

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:5
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
> +                                  "resource://gre/modules/AddonManager.jsm");

why is this needed?

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:34
(Diff revision 1)
> +      },
> +    },
> +  };
> +
> +  async function background() {
> +    browser.test.onMessage.addListener(async (msg, ...args) => {

why use rest instead of just naming your args, e.g. `(msg, setting, value)`

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:37
(Diff revision 1)
> +
> +  async function background() {
> +    browser.test.onMessage.addListener(async (msg, ...args) => {
> +      // The first argument is the end of the api name,
> +      // e.g., "webAPINotificationsEnabled".
> +      let apiObj = args[0].split(".").reduce((o, i) => o[i], browser.browserSettings);

This seems unnecessary, why not
`let apiObj = browser.browserSettings[args[0]];`

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:38
(Diff revision 1)
> +      let settingData;
> +      // The data to pass to set is in the second argument.
> +      await apiObj.set(args[1]);
> +      settingData = await apiObj.get({});
> +      browser.test.sendMessage("settingData", settingData);

nit: how about just
```
await apiObject.set(value);
browser.test.sendMessage("settingData", await apiobj.get());
```

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:46
(Diff revision 1)
> +  // Set prefs to our initial values.
> +  for (let setting in SETTINGS) {
> +    for (let pref in SETTINGS[setting].prefs) {
> +      Preferences.set(pref, SETTINGS[setting].prefs[pref]);
> +    }
> +  }
> +
> +  do_register_cleanup(() => {
> +    // Reset the prefs.
> +    for (let setting in SETTINGS) {
> +      for (let pref in SETTINGS[setting].prefs) {
> +        Preferences.reset(pref);
> +      }
> +    }
> +  });

how about just using `SpecialPowers.pushPrefEnv()` here

::: toolkit/components/extensions/test/xpcshell/xpcshell-common.ini:16
(Diff revision 1)
>  [test_ext_background_runtime_connect_params.js]
>  [test_ext_background_sub_windows.js]
>  [test_ext_background_telemetry.js]
>  [test_ext_background_window_properties.js]
>  skip-if = os == "android"
> +[test_ext_browserSettings.js]

you don't actually need this here, this file got left behind in a botched backout, probalby best to just leave it alone.
Attachment #8877253 - Flags: review?(aswan) → review-

Comment 15

2 months ago
Because not everything goes through the community group. Sometimes we look at things and go "hey that's cool let's do it" and we don't need to talk about it more. I think we've been pretty consistent about that message.
Flags: needinfo?(amckay)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 months ago
mozreview-review-reply
Comment on attachment 8877253 [details]
Bug 1364936 - Allow WebExtensions to disable the browser cache,

https://reviewboard.mozilla.org/r/148604/#review154604

> how about just using `SpecialPowers.pushPrefEnv()` here

`SpecialPowers` do not seem to be available to xpcshell tests, and I don't see an equivalent in any xpcshell tests for `pushPrefEnv`.

Comment 18

2 months ago
mozreview-review
Comment on attachment 8877253 [details]
Bug 1364936 - Allow WebExtensions to disable the browser cache,

https://reviewboard.mozilla.org/r/148604/#review154662

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:23
(Diff revision 2)
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
> +
> +add_task(async function test_browser_settings() {
> +  // Create an object to hold the values to which we will initialize the prefs.
> +  const SETTINGS = {
> +    "cacheEnabled": {

nit: why do you need this nested structure instead of just a list of prefs?  all this is used for is to iterate over everything at startup to set initial values and then at shutdown to reset everything.
Attachment #8877253 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)

Comment 20

2 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4383e27b5216
Allow WebExtensions to disable the browser cache, r=aswan
(Assignee)

Updated

2 months ago
Keywords: dev-doc-needed

Comment 21

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4383e27b5216
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I've added:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/cacheEnabled

Let me know if we need anything else!
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 23

15 days ago
(In reply to Will Bamberg [:wbamberg] from comment #22)
> I've added:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/
> browserSettings/cacheEnabled
> 
> Let me know if we need anything else!

Looks great, thanks Will!
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.