994732 - Make "Disable Cache" in Devtools persist (only when the toolbox is open)

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[ryan.anklam]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140410030200

Steps to reproduce:

Open Devtools -> Check Disable Cache -> Close Devtools -> Reopen Devtools 


Actual results:

Disable cache is no longer selected


Expected results:

I'd expect for that option to remain selected at a minimum for my current page session, however Id really like it to work similar to Chrome where that option remains selected until its unchanged and is only active while devtools are open.

Comment 1

[Paul Silaghi, QA [:pauly]]

It is written right under - "*Current session only, reloads the page".
Indeed, if're thinking about the current session of Firefox and not the current session of devtools, this might sound like a bug.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I will take this as it is a good first step to bug 651888.

Comment 3

[Jon Rimmer]

The current behavior is very annoying and weird. If I'm developing, I almost always want the cache disabled. Why doesn't Firefox trust me to enable and disable it myself, rather than insisting on resetting it every time I start a new session?

It should be consistent with Chrome devtools IMO -- Persist the setting, but only apply it when the devtools are open.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Jon Rimmer from comment #3)
> The current behavior is very annoying and weird. If I'm developing, I almost
> always want the cache disabled. Why doesn't Firefox trust me to enable and
> disable it myself, rather than insisting on resetting it every time I start
> a new session?
> 
> It should be consistent with Chrome devtools IMO -- Persist the setting, but
> only apply it when the devtools are open.

That is the plan. I will be implementing this soon.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So the setting should be "Disable cache when toolbox open."

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> So the setting should be "Disable cache when toolbox open."

Yes, but only on tabs in which the toolbox is opened on.  So if you had:

Tab A (toolbox opened)
Tab B (no toolbox)
Tab C (no toolbox)

Cache would only be disabled on Tab A.  Once toolbox is closed on Tab A, caching behavior is reset to normal.  If the toolbox is then opened on Tab B, then caching becomes disabled on that tab.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 1: Make cache persistent

I will add tests in a separate patch.

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 8447251 [details] [diff] [review]
Patch 1: Make cache persistent

Review of attachment 8447251 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/toolbox-options.js
@@ +87,5 @@
> +    gDevTools.off("pref-changed", this._prefChanged);
> +  },
> +
> +  _prefChanged: function(event, data) {
> +    switch (data.pref) {

See similar comment about switch statements in toolbox.js

::: browser/devtools/framework/toolbox.js
@@ +287,5 @@
> +   *              The new value
> +   *           $1 {Boolean} oldValue
> +   *              The old value
> +   *           $2 {Boolean} pref
> +   *              The value of the firefox preference that has changed

I'm not clear what the $0 is all about.
We need to remember that JSDoc doesn't exist AFAIK. That is to say we're writing these comments 100% for humans and 0% for some documentation tool. Isn't what we care about here that the event data contains a pref property?

@@ +294,5 @@
> +    switch (data.pref) {
> +      case "devtools.cache.disabled":
> +        this._applyCacheSettings();
> +        break;
> +    }

if (data.pref == "devtools.cache.disabled") {
  this._applyCacheSettings();
}

?

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 1: Make cache persistent

Addressed comments...still need to create test.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: make-disable-cache-persistent-994732.patch

Fixed network test issue.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: make-disable-cache-persistent-tests-994732.patch

Fairly simple test with a little task stuff added to head.js.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8456830 [details] [diff] [review]
make-disable-cache-persistent-tests-994732.patch

Review of attachment 8456830 [details] [diff] [review]:
-----------------------------------------------------------------

These test changes all look good. Just a few minor comments down there.
Obviously, since you changed the tests quite a bit, do you have a try build for this patch?
r=me with a green try build.

::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.js
@@ +5,4 @@
>  
>  const TEST_URI = "http://mochi.test:8888/browser/browser/devtools/framework/" +
>                   "test/browser_toolbox_options_disable_cache.sjs";
> +let tab1 = {

We might want to add a few tab test cases in the future, and for the sake of code size, I think you should refactor like:

let tabs = [
  { title: "Tab 1", desc: "Toggles cache on.", startToolbox: true },
  { title: "Tab 2", ...}
  ...
];

This way you can init tabs like:

for (let tab of tabs) {
  yield initTab(tab, tab.startToolbox);
}

And you can refactor checkCacheStateForAllTabs to accept an array instead:

function* checkCacheStateForAllTabs(states) {
  for (let i = 0; i < tabs.length; i ++) {
    let tab = tabs[i];
    yield checkCacheEnabled(tab, states[i]);
  }
}

@@ +146,5 @@
> +function* finishUp() {
> +  yield destroyTab(tab1);
> +  yield destroyTab(tab2);
> +  yield destroyTab(tab3);
> +  // tab4 has no toolbox.

This comment should also say that the tab will be removed automatically by the cleanup function in head.js.

Also:

for (let tab of tabs) {
  if (tab.startToolbox) {
    yield destroyTab(tab);
  }
}

::: browser/devtools/framework/test/head.js
@@ +5,5 @@
>  let TargetFactory = gDevTools.TargetFactory;
>  
> +const { console } = Cu.import("resource://gre/modules/devtools/Console.jsm", {});
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

really really really minor comment :) s/{devtools}/{ devtools }

@@ +12,5 @@
>  
> +// All test are asynchronous
> +waitForExplicitFinish();
> +
> +//Services.prefs.setBoolPref("devtools.dump.emit", true);

nit: A comment before that line would be great, explaining what this pref is about.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: make-disable-cache-persistent-tests-994732.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> Comment on attachment 8456830 [details] [diff] [review]
> make-disable-cache-persistent-tests-994732.patch
> 
> Review of attachment 8456830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These test changes all look good. Just a few minor comments down there.
> Obviously, since you changed the tests quite a bit, do you have a try build
> for this patch?
> r=me with a green try build.
> 
> ::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.js
> @@ +5,4 @@
> >  
> >  const TEST_URI = "http://mochi.test:8888/browser/browser/devtools/framework/" +
> >                   "test/browser_toolbox_options_disable_cache.sjs";
> > +let tab1 = {
> 
> We might want to add a few tab test cases in the future, and for the sake of
> code size, I think you should refactor like:
> 
> let tabs = [
>   { title: "Tab 1", desc: "Toggles cache on.", startToolbox: true },
>   { title: "Tab 2", ...}
>   ...
> ];
> 

Done

> This way you can init tabs like:
> 
> for (let tab of tabs) {
>   yield initTab(tab, tab.startToolbox);
> }
> 

Done

> And you can refactor checkCacheStateForAllTabs to accept an array instead:
> 

Done

> function* checkCacheStateForAllTabs(states) {
>   for (let i = 0; i < tabs.length; i ++) {
>     let tab = tabs[i];
>     yield checkCacheEnabled(tab, states[i]);
>   }
> }
> 

Done

> @@ +146,5 @@
> > +function* finishUp() {
> > +  yield destroyTab(tab1);
> > +  yield destroyTab(tab2);
> > +  yield destroyTab(tab3);
> > +  // tab4 has no toolbox.
> 
> This comment should also say that the tab will be removed automatically by
> the cleanup function in head.js.
> 

Nooo... using the cleanup function is a last resort and will usually leak when running single tests. this is because the cleanup function runs *after* our leak detection.

Anyhow, well spotted... I now clean up that tab too.

> Also:
> 
> for (let tab of tabs) {
>   if (tab.startToolbox) {
>     yield destroyTab(tab);
>   }
> }
> 
> ::: browser/devtools/framework/test/head.js
> @@ +5,5 @@
> >  let TargetFactory = gDevTools.TargetFactory;
> >  
> > +const { console } = Cu.import("resource://gre/modules/devtools/Console.jsm", {});
> > +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> > +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> 
> really really really minor comment :) s/{devtools}/{ devtools }
> 

lol... Fixed.

> @@ +12,5 @@
> >  
> > +// All test are asynchronous
> > +waitForExplicitFinish();
> > +
> > +//Services.prefs.setBoolPref("devtools.dump.emit", true);
> 
> nit: A comment before that line would be great, explaining what this pref is
> about.

Added a comment to all head.js files containing this pref.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: make-disable-cache-persistent-994732.patch

Folded patches together.

Green try:
https://tbpl.mozilla.org/?tree=Try&rev=89f196b99880

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8457878 [details] [diff] [review]
make-disable-cache-persistent-994732.patch

https://hg.mozilla.org/integration/fx-team/rev/ff1c060f95e9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff1c060f95e9