J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Description • 11 years ago

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).

	Girish Sharma [:Optimizer]
	Comment 1 • 11 years ago

(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 .

	Joe Walker [:jwalker]
	Comment 2 • 11 years ago

(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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 3 • 11 years ago

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.

	Girish Sharma [:Optimizer]
	Comment 4 • 11 years ago

(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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 5 • 11 years ago

(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.

	Joe Walker [:jwalker]
	Comment 6 • 11 years ago

(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.

	Girish Sharma [:Optimizer]
	Comment 7 • 11 years ago

Moving to "Developer Tools: Framework" Component

	Sebastian Zartner [:sebo]
	Comment 8 • 11 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 10 • 11 years ago

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.

	Girish Sharma [:Optimizer]
	Comment 11 • 11 years ago

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.

	Rob Campbell [:rc] (:robcee)
	Comment 12 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 13 • 11 years ago

(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.

	Rob Campbell [:rc] (:robcee)
	Comment 14 • 11 years ago

I mean, adding it to the Devtools Toolbox somewhere visible. Say, beside the gear icon for the options panel.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 15 • 11 years ago

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"?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 16 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 18 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 19 • 11 years ago

A good cache test page:
http://www.refreshyourcache.com/en/cache-test/

	Paul Rouget [:paul]
	Comment 20 • 11 years ago

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.

	Paul Rouget [:paul]
	Comment 21 • 11 years ago

Looking at the code of `disableJSClicked`, it's the same story.

We need to build some sort of docshell actor.

	Paul Rouget [:paul]
	Comment 22 • 11 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 23 • 11 years ago

We have split the cache test off into bug 943277

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 25 • 11 years ago

Oh, good pages for testing disable JS and disable cache:
http://www.refreshyourcache.com/en/cache-test/
http://javatester.org/javascript.html

	Paul Rouget [:paul]
	Comment 26 • 11 years ago

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?

	Paul Rouget [:paul]
	Comment 27 • 11 years ago

(In reply to Paul Rouget [:paul] from comment #26)
> - contentViewer can be null

Hmmm, I think I was wrong about that.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 28 • 11 years ago

Comment on attachment 8338420 [details] [diff] [review]
Remoted version

I will address Paul's comments then ask optimizer for feedback.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 29 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 30 • 11 years ago

Rebased

	Girish Sharma [:Optimizer]
	Comment 31 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 32 • 11 years ago

(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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 33 • 11 years ago

Try:
https://tbpl.mozilla.org/?tree=Try&rev=cb88a1cfd3b1

	Girish Sharma [:Optimizer]
	Comment 34 • 11 years ago

(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

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 35 • 11 years ago

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.

	Girish Sharma [:Optimizer]
	Comment 36 • 11 years ago

TIL.

PS: this should probably be added to the protocol.js doc gist.

	Paul Rouget [:paul]
	Comment 37 • 11 years ago

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 }}.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 38 • 11 years ago

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

	Paul Rouget [:paul]
	Comment 39 • 11 years ago

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?

	Paul Rouget [:paul]
	Comment 40 • 11 years ago

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).

	Joe Walker [:jwalker]
	Comment 41 • 11 years ago

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?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 42 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 43 • 11 years ago

(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.

	Joe Walker [:jwalker]
	Comment 44 • 11 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 45 • 11 years ago

(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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 46 • 11 years ago

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.">

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 47 • 11 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 48 • 11 years ago

Removing checkin-needed due to comment 47

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 49 • 11 years ago

(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

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 50 • 11 years ago

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 :-)

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 51 • 11 years ago

(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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 53 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 54 • 11 years ago

(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

	Carsten Book [:Tomcat]
	Comment 55 • 11 years ago

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

	Francesco Lodolo [:flod]
	Comment 56 • 11 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 57 • 11 years ago

Dammit, I thought about this, but apparently forgot to mention it in my first round of feedback. Thanks Francesco.

	Andrei Vaida [:avaida]
	Comment 58 • 10 years ago

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