Closed Bug 864098 Opened 7 years ago Closed 7 years ago

Add "Disable Cache" to options panel

Categories

(DevTools :: Framework, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 29

People

(Reporter: jryans, Assigned: miker)

References

Details

Attachments

(1 file, 10 obsolete files)

29.03 KB, patch
Details | Diff | Splinter Review
It would be nice to have a "Disable Cache" option exposed in the new Developer Tools options panel (from bug 851546).

Yes, people could toggle the about:config pref, but it's a frequent web developer need for testing, so putting in the options UI would be helpful.

For comparison to the other browsers, I believe Chrome's "Disable Cache" is slightly more complex than toggling a pref because even if it is enabled, it is only actually in effect while the Developer Tools are opened, which is nice for preserving full performance on most pages where you aren't using the tools.  It might be nice to do something similar here (assuming having an option in the tools at all is agreed to be a good idea).
(In reply to J. Ryan Stinnett [:jryans] from comment #0)
> For comparison to the other browsers, I believe Chrome's "Disable Cache" is
> slightly more complex than toggling a pref because even if it is enabled, it
> is only actually in effect while the Developer Tools are opened, which is

This can be done. (not that complex). But should that be done.

@Joe - I am assuming that a lot of such requests will start coming to be put in options panel, like this one (even though this one is a good and useful one). But most of the ones would be just people's complaints of the result of bug 851698 .
(In reply to Girish Sharma [:Optimizer] from comment #1)
> @Joe - I am assuming that a lot of such requests will start coming to be put
> in options panel, like this one (even though this one is a good and useful
> one). But most of the ones would be just people's complaints of the result
> of bug 851698 .

I think adding a 'disable JS' makes sense for us (and I raised bug 864249 for this), but I'm yet to see a good case for anything else.
I have been spending a lot of time on a side project where cached scripts have become an immensely frustrating issue. I think it would be a huge mistake not to include this in the options panel.

At the moment this is the pattern that people need to use to handle changes to cached scripts:
1. Open the web page that references the script in tab 1.
2. Open the script file itself in tab 2.
3. Shift refresh tab 2 in order for the cached script to be updated.
4. Refresh tab 1.

For every script change redo steps 2, 3 & 4.

It is also useful for performance tuning.

I also agree that this setting should only be applied when devtools are active.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> I have been spending a lot of time on a side project where cached scripts
> have become an immensely frustrating issue. I think it would be a huge
> mistake not to include this in the options panel.
> 
> At the moment this is the pattern that people need to use to handle changes
> to cached scripts:
> 1. Open the web page that references the script in tab 1.
> 2. Open the script file itself in tab 2.
> 3. Shift refresh tab 2 in order for the cached script to be updated.
> 4. Refresh tab 1.
> 
> For every script change redo steps 2, 3 & 4.
> 
> It is also useful for performance tuning.
> 
> I also agree that this setting should only be applied when devtools are
> active.

Just curious - why will they not use "Ctrl+Shift+Del" and then clear cache, instead of following the long step you mentioned.


A general thought - using the current about:config capabilities, the cache will be disabled for *all* of the firefox when the setting is activated and toolbox is open. I am sure there can be methods to disable the cache only for the tab on which toolbox is open, but those will be extremely hacky (unless Firefox supports per tab cache settings from the backend Necko layer).
But I think this should be fine though.
(In reply to Girish Sharma [:Optimizer] from comment #4)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> > I have been spending a lot of time on a side project where cached scripts
> > have become an immensely frustrating issue. I think it would be a huge
> > mistake not to include this in the options panel.
> > 
> > At the moment this is the pattern that people need to use to handle changes
> > to cached scripts:
> > 1. Open the web page that references the script in tab 1.
> > 2. Open the script file itself in tab 2.
> > 3. Shift refresh tab 2 in order for the cached script to be updated.
> > 4. Refresh tab 1.
> > 
> > For every script change redo steps 2, 3 & 4.
> > 
> > It is also useful for performance tuning.
> > 
> > I also agree that this setting should only be applied when devtools are
> > active.
> 
> Just curious - why will they not use "Ctrl+Shift+Del" and then clear cache,
> instead of following the long step you mentioned.
> 

I don't want to clear the whole of the browser cache, just the entries related to the current page. Also, using the hotkey there are still more steps per change vs. having the option in a settings panel.
(In reply to Joe Walker [:jwalker] from comment #2)
> I think adding a 'disable JS' makes sense for us (and I raised bug 864249
> for this), but I'm yet to see a good case for anything else.

This wasn't clear, sotty. I mean that I think we should have both 'disable JS' and 'disable cache' - I was thinking of stuff we might need in addition to this bug.
Moving to "Developer Tools: Framework" Component
Component: Developer Tools → Developer Tools: Framework
FYI, the Firebug Working Group is also discussing the behavior of it's "Disable Browser Cache" option[1].

Sebastian

[1] http://code.google.com/p/fbug/issues/detail?id=5224
Duplicate of this bug: 897703
Because the toolbox is a per tab item we really need a way to disable the cache for a particular tab in order to implement this.
I was talking to some people regarding this. Seems like we sure can refresh the page forcing it to load all resources again. But it was not concluded whether any further XHR calls will have cache disabled or not.
see also bug 651888.

Maybe the solution is to provide an explicit Shift+Reload button somewhere in the devtools rather than twiddling a global pref.
Blocks: 651888
(In reply to Rob Campbell [:rc] (:robcee) from comment #12)
> see also bug 651888.
> 
> Maybe the solution is to provide an explicit Shift+Reload button somewhere
> in the devtools rather than twiddling a global pref.

My only issues with doing that are discoverability and the fact that it is unbelievably annoying to work on a site with decent caching capabilities and having to shift refresh every time. Very few devs would know about this and even less would realize that AJAX responses after shift refresh are not cached.

Ideally we would simply use docShell.allowCache = false to disable the cache for the current tab (similar to disable JS) but that does not seem to exist at the moment.
I mean, adding it to the Devtools Toolbox somewhere visible. Say, beside the gear icon for the options panel.
I would still worry about the discoverability myself, like Mike mentioned...  Most web developers I've worked with at past companies are not interested in having the cache be enabled at any time for the page they are working on (unless you are debugging the cache headers, but that is quite rare).

At the same time, it is very difficult for people to change their muscle memory away from regular refresh, even though they want a complete re-download of all content, so I worry that a special button in the Dev Tools could be basically the same as just telling people to shift refresh today, which they are unlikely to do consistently during development.

I would continue to advocate for a "Disable Cache" that I can leave on at all times but only affects a page if the toolbox is open.  Do we know how difficult the work would be to add something like "docShell.allowCache = false"?
Now that bug 909218 has landed it will now be easy to disable the cache for a tab. Something like:
let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIWebNavigation)
                     .QueryInterface(Ci.nsIDocShell);
docShell.defaultLoadFlags = Ci.nsIRequest.LOAD_BYPASS_CACHE |
                            Ci.nsIRequest.INHIBIT_CACHING;
window.location.reload(true);

This would be consistent with disable JS and would disable the cache for a toolbox's tab. Of course, if the toolbox is closed we would need to remove the flags.
Attached patch toggle-cache-864098.patch (obsolete) — Splinter Review
I thought I would quickly hack this together as it is often requested.

It works fine to disable the cache for the current tab. The one thing that I don't like is that if the cache is disabled and the toolbox is closed then we have to refresh the page in order for the cache to be enabled again.
Attachment #830858 - Flags: review?(jwalker)
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Comment on attachment 830858 [details] [diff] [review]
toggle-cache-864098.patch

> +    let linkedBrowser = this.toolbox._host.hostTab.linkedBrowser;
> +    let docShell = linkedBrowser.docShell;

This won't work if the toolbox is undocked.

But more generally, this won't work with remote targets.

You need to use an actor for that (I can help you to implement that if you want).

Also, we need tests.
Attachment #830858 - Flags: review?(jwalker) → review-
Looking at the code of `disableJSClicked`, it's the same story.

We need to build some sort of docshell actor.
As expected, trying to disable JS with a undocked toolbox throws:

TypeError: this._host.hostTab is undefined: Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:892
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:110
TabTarget.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/target.js:435
TabTarget.prototype.handleEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/target.js:396
We have split the cache test off into bug 943277
Attached patch Remoted version (obsolete) — Splinter Review
Attachment #830858 - Attachment is obsolete: true
Attachment #8338420 - Flags: review?(paul)
Comment on attachment 8338420 [details] [diff] [review]
Remoted version

A couple of things:

- contentViewer can be null
- reload is not a docshell method. I think you can use the target to do that.
- Ci.nsIRequest.* values are local. You can't be sure that the server values are the same. I don't know what's the best strategy is here though.

Maybe you can merge the test bug in this one. It will make the reviews easier.

I haven't looked, but do you first check that JS is enabled when you init the checkbox?

I won't have time to look at this today.

Optimizer, can you do a first pass?
Attachment #8338420 - Flags: feedback?(scrapmachines)
(In reply to Paul Rouget [:paul] from comment #26)
> - contentViewer can be null

Hmmm, I think I was wrong about that.
Comment on attachment 8338420 [details] [diff] [review]
Remoted version

I will address Paul's comments then ask optimizer for feedback.
Attachment #8338420 - Flags: review?(paul)
Attachment #8338420 - Flags: feedback?(scrapmachines)
Changes:
- I decided to create an options actor as a docShell actor doesn't make a whole amount of sense.
- Fixed a leak in the disableJS test's finishUp().
- Added disableCache test that uses server side JS (.sjs). It turns out that caching in tests only works for .sjs files.
- Fixed font color of disable JavaScript citation.
- Moved revert disable JS code from toolbox into options actor.
- Danced a little jig.
Attachment #8338420 - Attachment is obsolete: true
Attachment #8339942 - Flags: feedback?(scrapmachines)
Attached patch toggle-cache-864098.patch (obsolete) — Splinter Review
Rebased
Attachment #8339942 - Attachment is obsolete: true
Attachment #8339942 - Flags: feedback?(scrapmachines)
Attachment #8339947 - Flags: feedback?(scrapmachines)
Comment on attachment 8339947 [details] [diff] [review]
toggle-cache-864098.patch

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

Code looks good to me, although when I run the framework tests locally, I get these two exceptions twice in the two tests which eventually then timeout.

Exception : 

 0:19.44 [Exception... "'[JavaScript Error: "form is undefined" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/options.js" line: 80}]' wh
en calling method: [nsIRunnable::run]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
 0:19.44 ************************************************************
 0:19.44 TEST-INFO | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_tabsswitch_shortcuts.js | Console message: [JavaScript Error: "form is undefined" {file: "re
source://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/options.js" line: 80}]


the two tests in which I get this and which time out are :

 1:52.38 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_tabsswitch_shortcuts.js | Test timed out
 1:52.38 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_tool_ready.js | Test timed out

::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.js
@@ +57,5 @@
> +  }, true);
> +
> +  doc.location.reload(false);
> +}
> +

Isn't this the same check as above, for the same situation ?

@@ +98,5 @@
> +  }, true);
> +
> +  doc.location.reload(false);
> +}
> +

here too.

::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.sjs
@@ +17,5 @@
> +    response.setHeader("Content-Type", "text/html", false);
> +    response.setHeader("Content-Length", page.length + "", false);
> +    response.write(page);
> +  }
> +}

This is cool stuff. Never knew we support server side js like this.

::: browser/devtools/framework/toolbox-options.js
@@ +40,5 @@
>    this.panelDoc = iframeWindow.document;
>    this.panelWin = iframeWindow;
>    this.toolbox = toolbox;
>    this.isReady = false;
> +  this._optionsFront = new OptionsFront(this.target.client, this.target.form);

you dont actually have to do new here. Not sure if doing it makes any bad impact though.
Attachment #8339947 - Flags: feedback?(scrapmachines) → feedback+
Attached patch toggle-cache-864098.patch (obsolete) — Splinter Review
(In reply to Girish Sharma [:Optimizer] from comment #31)
> Comment on attachment 8339947 [details] [diff] [review]
> toggle-cache-864098.patch
> 
> Review of attachment 8339947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code looks good to me, although when I run the framework tests locally, I
> get these two exceptions twice in the two tests which eventually then
> timeout.
> 
> Exception : 
> 
>  0:19.44 [Exception... "'[JavaScript Error: "form is undefined" {file:
> "resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/options.js" line: 80}]' wh
> en calling method: [nsIRunnable::run]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame ::
> <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
>  0:19.44 ************************************************************
>  0:19.44 TEST-INFO |
> chrome://mochitests/content/browser/browser/devtools/framework/test/
> browser_toolbox_tabsswitch_shortcuts.js | Console message: [JavaScript
> Error: "form is undefined" {file: "re
> source://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/options.js" line: 80}]
> 
> 
> the two tests in which I get this and which time out are :
> 
>  1:52.38 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/framework/test/
> browser_toolbox_tabsswitch_shortcuts.js | Test timed out
>  1:52.38 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/framework/test/
> browser_toolbox_tool_ready.js | Test timed out
> 

Fixed

> ::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.js
> @@ +57,5 @@
> > +  }, true);
> > +
> > +  doc.location.reload(false);
> > +}
> > +
> 
> Isn't this the same check as above, for the same situation ?
> 

The situation is different. We are checking that it is still disabled after reloading the page for a second time.

> @@ +98,5 @@
> > +  }, true);
> > +
> > +  doc.location.reload(false);
> > +}
> > +
> 
> here too.
> 

The situation is different. We are checking that it is still disabled after reloading the page for a second time.

> ::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.sjs
> @@ +17,5 @@
> > +    response.setHeader("Content-Type", "text/html", false);
> > +    response.setHeader("Content-Length", page.length + "", false);
> > +    response.write(page);
> > +  }
> > +}
> 
> This is cool stuff. Never knew we support server side js like this.
> 

It turned out that it doesn't matter which headers you add to a local file, it will never be cached. Using our own HttpRequestChannel fails because we need to test using the current tab's docShell.

It turns out that .sjs files are cached by default but adding the eTag etc. guarantees that the file is explicitely cached.

> ::: browser/devtools/framework/toolbox-options.js
> @@ +40,5 @@
> >    this.panelDoc = iframeWindow.document;
> >    this.panelWin = iframeWindow;
> >    this.toolbox = toolbox;
> >    this.isReady = false;
> > +  this._optionsFront = new OptionsFront(this.target.client, this.target.form);
> 
> you dont actually have to do new here. Not sure if doing it makes any bad
> impact though.

It starts with a capital letter so we probably should... besides, that is how we do it almost everywhere else.
Attachment #8339947 - Attachment is obsolete: true
Attachment #8342473 - Flags: review?(paul)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #32)

> > ::: browser/devtools/framework/toolbox-options.js
> > @@ +40,5 @@
> > >    this.panelDoc = iframeWindow.document;
> > >    this.panelWin = iframeWindow;
> > >    this.toolbox = toolbox;
> > >    this.isReady = false;
> > > +  this._optionsFront = new OptionsFront(this.target.client, this.target.form);
> > 
> > you dont actually have to do new here. Not sure if doing it makes any bad
> > impact though.
> 
> It starts with a capital letter so we probably should... besides, that is
> how we do it almost everywhere else.

Not here [0] (search for "let hello = HelloFront(this.client, { actorID: });").
or here [inspector-panel.js:93]
Basically, if the method is not meant to be called like "new Foo()" calling it like that will make a totally different result. That being said, the device actors do call the fronts like "new DeviceFront" and it seems to work.

Not sure how the internals of the protocol.FrontClass are handling both the cases. Maybe ask Dave ?



[0] https://gist.github.com/campd/5460401
It's a common pattern to make a constructor new-agnostic. In this case the add-on SDK's Class utility function does it for us. Effective JavaScript has a section describing this in detail.
TIL.

PS: this should probably be added to the protocol.js doc gist.
Blocks: 907310
Comment on attachment 8342473 [details] [diff] [review]
toggle-cache-864098.patch

As recommended by Panos, we should use {type: "reconfigure", "options": { "caching": true, "js": true }}.
Attachment #8342473 - Flags: review?(paul)
Attached patch toggle-cache-864098.patch (obsolete) — Splinter Review
Finally moved it into BrowserTabActor using the same setup as ThreadActor.

Tests pass fine locally so I wouldn't expect any issues on try: 
https://tbpl.mozilla.org/?tree=Try&rev=de9247ab77da
Attachment #8342473 - Attachment is obsolete: true
Attachment #8345955 - Flags: review?(paul)
Comment on attachment 8345955 [details] [diff] [review]
toggle-cache-864098.patch

Sorry, I won't have time to review this. Can you ask Joe or Panos?
Attachment #8345955 - Flags: review?(paul) → review?(jwalker)
Quick comment though: get rid of getAllow*, and put these info in the listTabs response (next to title and url, somewhere in the form() function iirc).
Comment on attachment 8345955 [details] [diff] [review]
toggle-cache-864098.patch

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

Thanks, looks good. I'd like another check over when these changes are done.

::: browser/devtools/framework/toolbox-options.js
@@ +264,5 @@
> +        to: this.target.client.activeTab._actor,
> +        type: "getAllowJavaScript"
> +      }, (result) => {
> +        this._origAllowJavaScript = result.allowjs;
> +      });

We're just lobbing this request over the fence and hoping, which could bring 2 problems: 1. It fails and we don't notice and 2, it's slow and something else uses this._origAllowJavaScript before it's set. Perhaps we should disable the button until the request is completed?

@@ +306,5 @@
> +
> +  _cleanupAsyncOptions: function() {
> +    let deferred = promise.defer();
> +
> +    // If the cache of JavaScript are disabled we need to revert them to their

Did you mean?
// If the JavaScript cache is disabled ...

@@ +323,5 @@
> +
> +    return deferred.promise;
> +  },
> +
> +  destroy: function() {

If destroy is called twice, then we'll get 2 activities trying to do cleanup. Lets do:

if (this.destroyPromise != null) return this.destroyPromise;

var deferred = promise.defer();
this.destroyPromise = deferred.promise;
//...

Do we need _cleanupAsyncOptions to be a separate function?

::: browser/devtools/framework/toolbox-options.xul
@@ +73,5 @@
> +            <checkbox id="devtools-disable-cache"
> +                      label="&options.disableCache.label;"
> +                      tooltiptext="&options.disableCache.tooltip;"/>
> +            <label class="options-citation-label"
> +                   value="(&options.context.triggersPageRefresh2;)"/>

This is going to leave us with the "(Current session only, reload page)" message twice, one directly above the other, which is going to look messy IMHO.
Maybe we should have it as a footnote?
Attachment #8345955 - Flags: review?(jwalker)
So hopefully this is the last iteration. So far we have had:
- None remotable
- Remoted through some actor but I can't remember what
- Remoted through OptionsActor using protocol.js
- Remoted through TabActor
- Remoted through TabActor using onReconfigure (similar to ThreadActor)
- Remoted through TabActor using onReconfigure (similar to ThreadActor) and returning cacheEnabled and jsEnabled through the TabList.
- Remoted through TabActor using onReconfigure (similar to ThreadActor) and returning cacheEnabled and jsEnabled through the tabAttached response.

We really need a better way to know with which actor things should be placed.

I will Panos for review as this latest iteration was his suggestion.
Attachment #8345955 - Attachment is obsolete: true
Attachment #8346585 - Flags: review?(past)
Attached patch Addressed Joe's comments (obsolete) — Splinter Review
(In reply to Joe Walker [:jwalker] from comment #41)
> Comment on attachment 8345955 [details] [diff] [review]
> toggle-cache-864098.patch
> 
> Review of attachment 8345955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, looks good. I'd like another check over when these changes are done.
> 
> ::: browser/devtools/framework/toolbox-options.js
> @@ +264,5 @@
> > +        to: this.target.client.activeTab._actor,
> > +        type: "getAllowJavaScript"
> > +      }, (result) => {
> > +        this._origAllowJavaScript = result.allowjs;
> > +      });
> 
> We're just lobbing this request over the fence and hoping, which could bring
> 2 problems: 1. It fails and we don't notice and 2, it's slow and something
> else uses this._origAllowJavaScript before it's set. Perhaps we should
> disable the button until the request is completed?
> 

I no longer use getAllowJavaScript() due to Panos suggested using the onAttach response to get the original values and then tracking using the checkbox.

> @@ +306,5 @@
> > +
> > +  _cleanupAsyncOptions: function() {
> > +    let deferred = promise.defer();
> > +
> > +    // If the cache of JavaScript are disabled we need to revert them to their
> 
> Did you mean?
> // If the JavaScript cache is disabled ...
> 

Or even "the cache or JavaScript is disabled"

> @@ +323,5 @@
> > +
> > +    return deferred.promise;
> > +  },
> > +
> > +  destroy: function() {
> 
> If destroy is called twice, then we'll get 2 activities trying to do
> cleanup. Lets do:
> 
> if (this.destroyPromise != null) return this.destroyPromise;
> 
> var deferred = promise.defer();
> this.destroyPromise = deferred.promise;
> //...
> 

Done, I like that.

> Do we need _cleanupAsyncOptions to be a separate function?
> 
> ::: browser/devtools/framework/toolbox-options.xul
> @@ +73,5 @@
> > +            <checkbox id="devtools-disable-cache"
> > +                      label="&options.disableCache.label;"
> > +                      tooltiptext="&options.disableCache.tooltip;"/>
> > +            <label class="options-citation-label"
> > +                   value="(&options.context.triggersPageRefresh2;)"/>
> 
> This is going to leave us with the "(Current session only, reload page)"
> message twice, one directly above the other, which is going to look messy
> IMHO.
> Maybe we should have it as a footnote?

Agreed, done.
Attachment #8346585 - Attachment is obsolete: true
Attachment #8346585 - Flags: review?(past)
Attachment #8346625 - Flags: review?(jwalker)
Comment on attachment 8346625 [details] [diff] [review]
Addressed Joe's comments

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

The r+ is conditional on the comparing of timestamps not being an intermittent orange waiting to happen. Can you be sure the timestamps will be the same?

::: browser/devtools/framework/test/browser_toolbox_options_disable_cache.js
@@ +8,5 @@
> +
> +let doc;
> +let toolbox;
> +
> +function test() {

I strongly recommend looking into Task.jsm, it would half the length of this test file and make it easier to read. But I'm not going to ask you to do that for this patch.

For an example
  https://hg.mozilla.org/integration/fx-team/file/1f570fe4ee88/browser/devtools/debugger/test/browser_dbg_cmd-dbg.js

I think you could compact everything to something like this:

  function test() {
    return Task.spawn(spawnTest).then(finish, ok.bind(null, false);
  }

  function spawnTest() {
    waitForExplicitFinish();
    // tests, yielding a promise wherever necessary
    finish();
  }

@@ +26,5 @@
> +
> +function testSelectTool(aToolbox) {
> +  toolbox = aToolbox;
> +  toolbox.once("options-selected", testCacheEnabled);
> +  toolbox.selectTool("options");

Another time, selectTool() returns a promise, so you could save yourself the event, which means if you were using Task.jsm you could do:

  yield toolbox.selectTool("options");
  let prevTimestamp = getTimestamp();

etc...

@@ +32,5 @@
> +
> +function testCacheEnabled() {
> +  let prevTimestamp = getTimestamp();
> +
> +  gBrowser.selectedBrowser.addEventListener("load", function onLoad(evt) {

And when you really need a one-off event you could use a trivial wrapper to turn an event into a promise:

    let ev = yield listenOnce(gBrowser.selectedBrowser, "load");
    is(ev.source, mysource, "Got the right source");

Where listenOnce is like:

    function listenOnce(eventTarget, event, useCapture=false) {
      let deferred = promise.defer();
      var callback = function(ev) {
        eventTarget.removeEventListener("load", callback, useCapture);
        deferred.resolve(ev);
      };
      eventTarget.addEventListener("load", callback, useCapture);
      return deferred.promise;
    }

@@ +35,5 @@
> +
> +  gBrowser.selectedBrowser.addEventListener("load", function onLoad(evt) {
> +    gBrowser.selectedBrowser.removeEventListener(evt.type, onLoad, true);
> +    doc = content.document;
> +    is(prevTimestamp, getTimestamp(), "timestamp has not changed (page is cached)");

Isn't comparing timestamps just an intermittent orange waiting to happen?

@@ +77,5 @@
> +
> +  cbx.scrollIntoView();
> +
> +  // After uising scrollIntoView() we need to use executeSoon() to wait for the
> +  // browser to scroll.

Thanks for this explanation

::: browser/devtools/framework/test/browser_toolbox_tool_ready.js
@@ +19,5 @@
>          ok(panel.isReady, toolId + " panel should be ready");
>  
>          let nextIndex = index + 1;
>          if (nextIndex >= toolIds.length) {
> +          toolbox.destroy().then(function() {

Not important to fix, but it might be worth getting into the fat arrow habit for inline functions.
Attachment #8346625 - Flags: review?(jwalker) → review+
Attached patch Switched to GUIDs (obsolete) — Splinter Review
(In reply to Joe Walker [:jwalker] from comment #44)
> Comment on attachment 8346625 [details] [diff] [review]
> Addressed Joe's comments
> 
> Review of attachment 8346625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The r+ is conditional on the comparing of timestamps not being an
> intermittent orange waiting to happen. Can you be sure the timestamps will
> be the same?
> 

We are reading the timestamp from the node in the HTML document and if the document is cached the value will be the same because there is no recalculation. If the value is different we want the test to fail as the cache was not successfully disabled.

Anyhow, I have switched to using GUIDs for your peace of mind.
Attachment #8346625 - Attachment is obsolete: true
Comment on attachment 8347201 [details] [diff] [review]
Switched to GUIDs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 864249
User impact if declined: The disable JS option in the DevTools options panel will throw an error when attempting to disable JS for remote targets e.g. FirefoxOS
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Very low
String or IDL/UUID changes made by this patch:

-<!ENTITY options.context.triggersPageRefresh2  "Current session only, reloads the page">
+<!ENTITY options.context.triggersPageRefresh  "* Current session only, reloads the page">
  
-<!ENTITY options.disableJavaScript.label2    "Disable JavaScript">
+<!ENTITY options.disableJavaScript.label     "Disable JavaScript *">

+<!ENTITY options.disableCache.label     "Disable Cache *">
+<!ENTITY options.disableCache.tooltip   "Turning this option on will disable the cache for the current tab. If the tab or the toolbox is closed then this setting will be forgotten.">
Attachment #8347201 - Flags: approval-mozilla-aurora?
Comment on attachment 8347201 [details] [diff] [review]
Switched to GUIDs

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

I read the patch and I have a few comments FWIW:

- we generally hide protocol implementation details, such as packet structure, from client code, and add it instead to the dbg-client.jsm library (e.g. DebuggerClient.reconfigureThread)
- these options seem transient, so closing and reopening the toolbox would reset them, but don't we want to let users debug with the cache always disabled (e.g. in their debug profile)?
- if the options are meant to be transient, then why do we need to return their values in the tabAttached response, since they are guaranteed to always be false?
- if the options are transient, we don't have to reset them asynchronously from the client on toolbox teardown, but could simply reset them in the tab actor's _detach method, which is always called when the protocol connection is terminated
- _toggleJsOrCache could be simplified by calling onReload instead of reimplementing it
- it's confusing to have different option names in the "reconfigure" request and the "tabAttached" response packets
Attachment #8347201 - Flags: approval-mozilla-aurora?
Removing checkin-needed due to comment 47
Keywords: checkin-needed
Attached patch Addressed Panos's comments (obsolete) — Splinter Review
(In reply to Panos Astithas [:past] from comment #47)
> Comment on attachment 8347201 [details] [diff] [review]
> Switched to GUIDs
> 
> Review of attachment 8347201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I read the patch and I have a few comments FWIW:
> 
> - we generally hide protocol implementation details, such as packet
> structure, from client code, and add it instead to the dbg-client.jsm
> library (e.g. DebuggerClient.reconfigureThread)

Nice, I have added DebuggerClient.reconfigureTab() and made use of it on the client side.

> - these options seem transient, so closing and reopening the toolbox would
> reset them, but don't we want to let users debug with the cache always
> disabled (e.g. in their debug profile)?

These are tab specific settings so a persistent setting doesn't really make sense although having a global version whenever the toolbox is opened may be a reasonable option.

> - if the options are meant to be transient, then why do we need to return
> their values in the tabAttached response, since they are guaranteed to
> always be false?

Extensions etc. also have access to these settings so the least we can do is reset them to their original values on exit.

> - if the options are transient, we don't have to reset them asynchronously
> from the client on toolbox teardown, but could simply reset them in the tab
> actor's _detach method, which is always called when the protocol connection
> is terminated

In order for the settings to be changed the tab has to be reloaded, which makes it a bad candidate for adding to _detach().

> - _toggleJsOrCache could be simplified by calling onReload instead of
> reimplementing it

Agreed, done.

> - it's confusing to have different option names in the "reconfigure" request
> and the "tabAttached" response packets

Fixed
Attachment #8347201 - Attachment is obsolete: true
Attachment #8348020 - Flags: review?(past)
Comment on attachment 8348020 [details] [diff] [review]
Addressed Panos's comments

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

Thanks! Could you also add the new changes to the protocol doc please? The public location is on wikimo (https://wiki.mozilla.org/Remote_Debugging_Protocol), but Jim keeps the source on GitHub (https://github.com/jimblandy/DebuggerDocs). Update the "protocol" file, send a PR to him and he will update the wiki after merging. Jim is very strict about updating it every time the code changes :-)
Attachment #8348020 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #50)
> Comment on attachment 8348020 [details] [diff] [review]
> Addressed Panos's comments
> 
> Review of attachment 8348020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! Could you also add the new changes to the protocol doc please? The
> public location is on wikimo
> (https://wiki.mozilla.org/Remote_Debugging_Protocol), but Jim keeps the
> source on GitHub (https://github.com/jimblandy/DebuggerDocs). Update the
> "protocol" file, send a PR to him and he will update the wiki after merging.
> Jim is very strict about updating it every time the code changes :-)

Done.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=6b4311623b51
Attached patch RebaseSplinter Review
Attachment #8348020 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #53)
> https://hg.mozilla.org/integration/fx-team/rev/812c5f165aca

Hi, sorry had to back this changeout since it was causing XPC Test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=32083451&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ae5d8b396299
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
For the record, going backwards with number on entities is not a great idea.

-<!ENTITY options.disableJavaScript.label2    "Disable JavaScript">
+<!ENTITY options.disableJavaScript.label     "Disable JavaScript *">

If there is a "label2", it means at some point we had a "label". The reason for this name change is to invalidate the existing string in all localizations. 

Going back from "label2" to "label" will consider valid an unmaintained localization from previous cycles.

No need to fix this specific case, but please keep it in mind when reviewing patches with strings.
Dammit, I thought about this, but apparently forgot to mention it in my first round of feedback. Thanks Francesco.
Keywords: verifyme
I was able to confirm the addition and proper functioning of Disable Cache and Disable JavaScript options on the latest Beta (Build ID: 20140318013849), using:
- Windows 7 64-bit [1],
- Ubuntu 13.10 64-bit [2],
- Mac OS X 10.9.1 [3],
with the help of Comment 19 (thanks Michael) and a sample page containing JavaScript.


1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
2. Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
3. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.