Closed Bug 994732 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: General, defect)

31 Branch
x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: ryan.anklam, Assigned: miker)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
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.
Blocks: 651888
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools
Ever confirmed: true
I will take this as it is a good first step to bug 651888.
Assignee: nobody → mratcliffe
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.
(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.
So the setting should be "Disable cache when toolbox open."
Summary: Disable Cache in Devtools should persist. → Make "Disable Cache" in Devtools persist (only when the toolbox is open)
(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.
Attached patch Patch 1: Make cache persistent (obsolete) — Splinter Review
I will add tests in a separate patch.
Attachment #8447251 - Flags: review?(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();
}

?
Attachment #8447251 - Flags: review?(jwalker) → review+
Attached patch Patch 1: Make cache persistent (obsolete) — Splinter Review
Addressed comments...still need to create test.
Attachment #8447251 - Attachment is obsolete: true
Attachment #8447911 - Flags: review+
Fixed network test issue.
Attachment #8447911 - Attachment is obsolete: true
Attachment #8456825 - Flags: review+
Fairly simple test with a little task stuff added to head.js.
Attachment #8456830 - Flags: review?(pbrosset)
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.
Attachment #8456830 - Flags: review?(pbrosset) → review+
(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.
Attachment #8456830 - Attachment is obsolete: true
Attachment #8456884 - Flags: review+
Folded patches together.

Green try:
https://tbpl.mozilla.org/?tree=Try&rev=89f196b99880
Attachment #8456825 - Attachment is obsolete: true
Attachment #8456884 - Attachment is obsolete: true
Attachment #8457878 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ff1c060f95e9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.