F5 and Shift-F5 should reload the Target tab when using a detached Toolbox window.

also Cmd/Ctrl(+Shift)-R.

Just came to report this bug... I noticed that it's currently possible with Chrome DevTools as well.

Will be a good-first-bug once bug 864098 lands.

Can you assign me this bug? I would like to work on it.

Optimizer, can you mentor this bug? Akshay, sorry but Alex is already looking at this.

Sure

(In reply to Girish Sharma [:Optimizer] from comment #6) > Sure Hello, Optimizer. I try fix this bug by using <key.../> xul-shortcut. Is it right direction? Do you have any ideas?

Optimizer, Alex looks like he needs a bit of help and you forgot to answer.

We talked over IRC, I gave him a general direction of the bug and wanted to know if he is okay with it. He has not replied yet.

Girlish, can you repeat your advices to me, please. I have not received it.

If I am not wrong, you are hexman on IRC. Since you want to continue with this bug, Here is what you will need to do: 1) Add keys in the toolbox-options.xul for reload and shift+reload 2) In toolbox-options.js , add methods like `_disableCacheClicked` which sends message back to the server telling to reload or shift-reload 3) In toolkit\devtools\server\actors\webbrowser.js, change the `onReconfigure` method to understand the new object that you are sending to it. 4) Then similar to `_toggleJsOrCache` method, create a method to reload the page. One thing to note is that you cannot simply refresh the page on the oncommand of the <key> because the frontend devtools might not have access to the webpage at all. Ping me on IRC if you are stuck. Also, call me Optimizer, if my name is hard to spell :)

Alex, are you still planning to work on this bug ?

I've gotten feedback that this gap feels like a bug, would like to see it get fixed. If Alex isn't up to it, maybe we can take it next week?

(In reply to Jeff Griffiths (:canuckistani) from comment #14) > I've gotten feedback that this gap feels like a bug, would like to see it > get fixed. If Alex isn't up to it, maybe we can take it next week? Sorry for my long reply. On the weekend I have time for fixing this bug.

Sorry, Optimizer, can you dismiss me from this bug, please? I'm so sorry, but I haven't time for it now!

Sure thing.

Optimizer, I believe since no-one is currently working on this bug, could you assign me on this? I would like to solve this.

Sure, comment 11 has the details of approach on how to solve this bug, https://wiki.mozilla.org/DevTools/Hacking has the details of how to code and build Firefox and devtools

Here's a patch that lets you reload from the devtools window. (I see someone else is assigned—I started looking into it before my original bug got marked as a duplicate).

There has been no activity from the previous assignee. Given that your patch is almost correct, I am assigning this to you! :)

Cool. Just let me know what I need to do to take it from "almost" to "completely". :-)

Comment on attachment 8418908 [details] [diff] [review] reload-from-devtools.patch Review of attachment 8418908 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Thanks for the patch David. The patch looks all good apart from some NITs. Please add a test for this behavior. You will need to test that both the shortcuts work from both the docked and undocked toolboxes. ::: browser/devtools/framework/toolbox.js @@ +297,5 @@ > }, > > + _addReloadKeys: function() { > + let reloadKey = this.doc.getElementById("toolbox-reload-key"); > + reloadKey.addEventListener("command", (event) => { this.reloadTarget(false) }, true); You might wanna add a preventDefault and/or stopPropagation to prevent the page from loading due to this shortcut key when the toolbox is undocked. @@ +300,5 @@ > + let reloadKey = this.doc.getElementById("toolbox-reload-key"); > + reloadKey.addEventListener("command", (event) => { this.reloadTarget(false) }, true); > + > + let shiftReloadKey = this.doc.getElementById("toolbox-shift-reload-key"); > + shiftReloadKey.addEventListener("command", (event) => { this.reloadTarget(true) }, true); ditto @@ +916,5 @@ > + * Tells the our target tab to reload. > + */ > + reloadTarget: function(disableCache) { > + this.target.activeTab.reconfigure({ performReload: true, > + cacheEnabled: !disableCache }); NIT: A bit different styling from the rest of the file. Should be something like: this.target.activeTab.reconfigure({ performReload: true, cacheEnabled: !disableCache })

(In reply to Girish Sharma [:Optimizer] from comment #24) > Please add a test for this behavior. You will need to test that both the > shortcuts work from both the docked and undocked toolboxes. Will do. > ::: browser/devtools/framework/toolbox.js > @@ +297,5 @@ >> }, >> >> + _addReloadKeys: function() { >> + let reloadKey = this.doc.getElementById("toolbox-reload-key"); >> + reloadKey.addEventListener("command", (event) => { this.reloadTarget(false) }, true); > > You might wanna add a preventDefault and/or stopPropagation to prevent the page > from loading due to this shortcut key when the toolbox is undocked. This didn't seem to be happening—I tested with it undocked and didn't see the devtools window itself reload. Is that what you meant? > NIT: A bit different styling from the rest of the file. Should be something > like: > > this.target.activeTab.reconfigure({ > performReload: true, > cacheEnabled: !disableCache > }) Will do. Thanks!

(In reply to David Caldwell from comment #25) > > > > You might wanna add a preventDefault and/or stopPropagation to prevent the page > > from loading due to this shortcut key when the toolbox is undocked. > > This didn't seem to be happening—I tested with it undocked and didn't see > the devtools window itself reload. Is that what you meant? err. I meant in docked mode. (If it was reloading in undocked mode itself, this bug would be invalid :) )

(In reply to Girish Sharma [:Optimizer] from comment #26) > (In reply to David Caldwell from comment #25) > > > > > > You might wanna add a preventDefault and/or stopPropagation to prevent the page > > > from loading due to this shortcut key when the toolbox is undocked. > > > > This didn't seem to be happening—I tested with it undocked and didn't see > > the devtools window itself reload. Is that what you meant? > > err. I meant in docked mode. > > (If it was reloading in undocked mode itself, this bug would be invalid :) ) Gotcha. That makes sense. So I'm looking into testing it. What's you opinion on how detailed the test should be? Should I piggyback on the browser_toolbox_options_disable_cache.js tests so that I can verify that the force reload happens? Or should I just send the key and see if the remote tab gets a new load event, trusting that the disable cache part is taken care of by another test?

Yeah, disabled cache part should be already taken care of. What we should test here is that pressing the keys work.

Here's a new version of the patch. The 2 major differences: 1. The F5 key variants are there now—the last patch only supported Ctrl-R variants. 2. Instead of going through the "reconfigure" packet/message, it now goes through "reload". This seems to be more correct—The previous patch was disabling the cache just like the "disable cache" checkbox in the toolbox options window does, which seems much too harsh. With an a newly added option to the "reload" packet/message it can now do a real force reload, just like Ctrl-Shift-R in a normal browser window does. A side effect is that the reload that reconfigure does in certain cases now uses the webNavigator.reload() instead of window.location.reload(). I don't think this should matter. The new patch doesn't attempt to stopPropagation() or preventDefault()—my testing shows that it doesn't double up the reload when the devtools window is docked. I suspect that it's because the <key> element in toolbox.xul is implicitly doing it for me. I'm still working on the testing. I'm having trouble because I wanted the test to prove that there's only one reload message happening in the docked case, but for some reason I get 2 "load" events to the frame. As a sanity check I put printf()s in docshell/base/nsDocShell.cpp's Reload() and InternalLoad() functions and verified that I was only causing one real reload, but I still reliably get 2 "load" events (even in the undocked case!). This seems like a bug to me. Is there another way to detect that the reload is happening?

(In reply to David Caldwell from comment #29) > Created attachment 8420472 [details] [diff] [review] > reload-from-devtools-2.patch > > Here's a new version of the patch. The 2 major differences: > > 1. The F5 key variants are there now—the last patch only supported Ctrl-R > variants. > > 2. Instead of going through the "reconfigure" packet/message, it now goes > through "reload". This seems to be more correct—The previous patch was > disabling the cache just like the "disable cache" checkbox in the toolbox > options window does, which seems much too harsh. With an a newly added > option to the "reload" packet/message it can now do a real force reload, > just like Ctrl-Shift-R in a normal browser window does. > > A side effect is that the reload that reconfigure does in certain cases > now uses the webNavigator.reload() instead of window.location.reload(). I > don't think this should matter. > I don't think so either. > The new patch doesn't attempt to stopPropagation() or preventDefault()—my > testing shows that it doesn't double up the reload when the devtools window > is docked. I suspect that it's because the <key> element in toolbox.xul is > implicitly doing it for me. > > > I'm still working on the testing. I'm having trouble because I wanted the > test to prove that there's only one reload message happening in the docked > case, but for some reason I get 2 "load" events to the frame. As a sanity > check I put printf()s in docshell/base/nsDocShell.cpp's Reload() and > InternalLoad() functions and verified that I was only causing one real > reload, but I still reliably get 2 "load" events (even in the undocked > case!). This seems like a bug to me. Is there another way to detect that the > reload is happening? the first load might probably an about:blank load. You can check the url of the loaded document to be sure.

(In reply to Girish Sharma [:Optimizer] from comment #30) > (In reply to David Caldwell from comment #29) > > The new patch doesn't attempt to stopPropagation() or preventDefault()—my > > testing shows that it doesn't double up the reload when the devtools window > > is docked. I suspect that it's because the <key> element in toolbox.xul is > > implicitly doing it for me. > > > > > > I'm still working on the testing. I'm having trouble because I wanted the > > test to prove that there's only one reload message happening in the docked > > case, but for some reason I get 2 "load" events to the frame. As a sanity > > check I put printf()s in docshell/base/nsDocShell.cpp's Reload() and > > InternalLoad() functions and verified that I was only causing one real > > reload, but I still reliably get 2 "load" events (even in the undocked > > case!). This seems like a bug to me. Is there another way to detect that the > > reload is happening? > > the first load might probably an about:blank load. You can check the url of > the loaded document to be sure. It doesn't appear to be. When I check the URL I get the "data:" url of my test in both cases. I've attached a snippet of the test output. The salient lines: @ 0:16.54 is an info() in my test right before I send the synthesized key event. @ 0:16.66 are my debug printf()s in docshell/base/nsDocShell.cpp (nsDocShell::Reload and nsDocShell::InternalLoad respectively) @ 0:16.70 the first 'load' event callback @ 0:16.71 the second 'load' event callback I've noticed that occasionally I *don't* get the double load event callback, which makes me think it's some sort of race.

Here's a test that doesn't try to look for the reload happening twice. It just checks for it happening at all. Should this be combined into the current patch or should the test be separate?

Comment on attachment 8420472 [details] [diff] [review] reload-from-devtools-2.patch Review of attachment 8420472 [details] [diff] [review]: ----------------------------------------------------------------- Code looks fine, I am gonna give it a try with the tests to see how it works and to see why you were getting two reload events ... One comment below. ::: browser/devtools/framework/toolbox.js @@ +917,5 @@ > } > }, > > /** > + * Tells the our target tab to reload. grammatically incorrect line :)

Comment on attachment 8425140 [details] [diff] [review] reload-from-devtools-test.patch Review of attachment 8425140 [details] [diff] [review]: ----------------------------------------------------------------- test looks nice. I will play around with it over the weekend to try to make sense out of two reloads. i have some comments below. ::: browser/devtools/framework/test/browser.ini @@ +25,5 @@ > [browser_toolbox_ready.js] > [browser_toolbox_select_event.js] > [browser_toolbox_sidebar.js] > [browser_toolbox_tabsswitch_shortcuts.js] > +[browser_toolbox_reload_target.js] it should be "browser_toolbox_window_reload_target.js" as the below tests which are undocked only window features... ::: browser/devtools/framework/test/browser_toolbox_reload_target.js @@ +34,5 @@ > +} > + > +function startUndockedReloadTest() { > + toolbox.switchHost(Toolbox.HostType.WINDOW).then(() => { > + toolbox.selectTool(toolbox.currentToolId); // make sure we're focused after the switch (bug?) if this is really required, you can simply focus using window.focus(), (getting hold of window first). But really, you can make your key synthesizer emit on the window so focus is not really required. @@ +66,5 @@ > + synthesizeKeyForToolbox(key); > +} > + > +function tidyUp() { > + toolbox.destroy().then(function() { Should also dock back the toolbox first such that subsequent tests are not affected

Comment on attachment 8425140 [details] [diff] [review] reload-from-devtools-test.patch Review of attachment 8425140 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I ran the test and found out the cause of multiple reloads you were getting. Can you fix that (as per the below comments), add back the check for a single refresh, test on all the tools (instead of the first one) and finally, fold the two patches. Once again, thanks for contributing! ::: browser/devtools/framework/test/browser_toolbox_reload_target.js @@ +17,5 @@ > + let toolIDs = gDevTools.getToolDefinitionArray() > + .filter(def => def.isTargetSupported(target)) > + .map(def => def.id); > + gDevTools.showToolbox(target, toolIDs[1], Toolbox.HostType.BOTTOM) > + .then(startDockedReloadTest); Should probably iterate on all the tools. @@ +35,5 @@ > + > +function startUndockedReloadTest() { > + toolbox.switchHost(Toolbox.HostType.WINDOW).then(() => { > + toolbox.selectTool(toolbox.currentToolId); // make sure we're focused after the switch (bug?) > + I was right, doing toolbox.doc.defaultView.focus() works @@ +54,5 @@ > +} > + > +function testReload(key, docked, callback) { > + loadEvent = function() { > + ok(true, 1, "Reloaded when pressing #"+key+" in "+docked+" devtools"); ok() only takes two argument, the second is the message. @@ +59,5 @@ > + gBrowser.selectedTab.removeEventListener('load', loadEvent); > + executeSoon(callback); > + } > + > + gBrowser.selectedTab.addEventListener('load', loadEvent); selectedTab actually gives the reference to the XUL node of the tab that you see at the top of the screen. Thus it gets multiple reloads from inner elements. You should instead do gBrowser.selectedBrowser.addEventListener('load', method, true); to get reload of the page. Remember to put the third argument as true. This fixes the multiple load events that you were getting.

Comment on attachment 8420472 [details] [diff] [review] reload-from-devtools-2.patch Review of attachment 8420472 [details] [diff] [review]: ----------------------------------------------------------------- See my above comment. I will review once again once you update the patches. I will also push it to try if you dont have try access yet.

(In reply to Girish Sharma [:Optimizer] from comment #33) > ::: browser/devtools/framework/toolbox.js > @@ +917,5 @@ > > } > > }, > > > > /** > > + * Tells the our target tab to reload. > > grammatically incorrect line :) Whoops. :-) Fixed.

(In reply to Girish Sharma [:Optimizer] from comment #34) > it should be "browser_toolbox_window_reload_target.js" as the below tests which are undocked only window features... Ok. Renamed. > Should also dock back the toolbox first such that subsequent tests are not affected Done. (In reply to Girish Sharma [:Optimizer] from comment #35) > finally, fold the two patches. Done (though I only noticed your comment *after* I'd uploaded patch-3). > Should probably iterate on all the tools. Ok. The test now checks for each tool. > @@ +35,5 @@ > > + > > +function startUndockedReloadTest() { > > + toolbox.switchHost(Toolbox.HostType.WINDOW).then(() => { > > + toolbox.selectTool(toolbox.currentToolId); // make sure we're focused after the switch (bug?) > > + > > I was right, doing toolbox.doc.defaultView.focus() works Ok. I switched to that technique. > @@ +59,5 @@ > > + gBrowser.selectedTab.removeEventListener('load', loadEvent); > > + executeSoon(callback); > > + } > > + > > + gBrowser.selectedTab.addEventListener('load', loadEvent); > > selectedTab actually gives the reference to the XUL node of the tab that you > see at the top of the screen. Thus it gets multiple reloads from inner > elements. > > You should instead do > > gBrowser.selectedBrowser.addEventListener('load', method, true); > > to get reload of the page. Remember to put the third argument as true. > This fixes the multiple load events that you were getting. Yes, it does. Thanks! I added tests for the F5 and Shift F5 keys as well. The whole test now takes about 35 seconds to run (8 tools * 4 keys * (docked+undocked)). Now that it's back to counting the reloads it needs a timeout which slows it down a bit. It currently waits 100ms after the last "load" event before deciding there will be no further events--that number can be tweaked and I'm not sure what the appropriate value is (I tested with it set to 1000ms until I trusted the test, then dialed it back to speed things up). > I will review once again once you update the patches. Thanks. I set the review bit again on this patch. I'm not exactly sure what the convention is, so let me know if that's wrong. > I will also push it to try if you dont have try access yet. I do not have any commit access, so push away. :-)

Comment on attachment 8428355 [details] [diff] [review] reload-from-devtools-test-4.patch Review of attachment 8428355 [details] [diff] [review]: ----------------------------------------------------------------- r+ for the code. As for the tests, this is bound to give timeouts on all platforms, specially debug builds. Let me suggest a different approach for checking the load events. - Have a single load event listener on the browser. Remove it at the end of the test. - Maintain total numbers of reloads till now. - Maintain total numbers of keys pressed till now. - Compare them in the load listener everytime. For ex. let keys = 0; // this gets incremented in the synthesizeKeyForToolbox method let reloads = 0; // this gets incremented in the load handler Then in the load handler itself is (keys, reloads, "Reloads match meys");

(In reply to Girish Sharma [:Optimizer] from comment #39) > Let me suggest a different approach for checking the load events. > > - Have a single load event listener on the browser. Remove it at the end of > the test. > - Maintain total numbers of reloads till now. > - Maintain total numbers of keys pressed till now. > - Compare them in the load listener everytime. Ok, I've modified the test to work this way. It now takes 30 seconds on my computer instead of 35 and there are no arbitrary timeouts.

Comment on attachment 8430264 [details] [diff] [review] reload-from-devtools-5.patch Review of attachment 8430264 [details] [diff] [review]: ----------------------------------------------------------------- Overall its good to go. There is one bug/race condition below, so you should fix that. Please ask :bgrins for the next final review as I am not a devtools peer. The code looks good to go otherwise. ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js @@ +53,5 @@ > +function testAllTheTools(docked, callback, toolNum=0) { > + if (toolNum >= toolIDs.length) { > + return callback(); > + } > + toolbox.selectTool(toolIDs[toolNum]); selectTool returns a promise. So you should wrap the next few statements in this method in a .then call so that the tool is properly selected before the tests are performed

(In reply to Girish Sharma [:Optimizer] from comment #41) > ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js > @@ +53,5 @@ > > +function testAllTheTools(docked, callback, toolNum=0) { > > + if (toolNum >= toolIDs.length) { > > + return callback(); > > + } > > + toolbox.selectTool(toolIDs[toolNum]); > > selectTool returns a promise. So you should wrap the next few statements in > this method in a .then call so that the tool is properly selected before the > tests are performed Oops! Fixed. > Please ask :bgrins for the next final review as I am not a devtools peer. > The code looks good to go otherwise. Will do. Thanks for all your help.

Comment on attachment 8434378 [details] [diff] [review] reload-from-devtools-6.patch Review of attachment 8434378 [details] [diff] [review]: ----------------------------------------------------------------- Can you rebase the patch? I'm getting errors applying to latest fx-team: 1 out of 3 hunks FAILED -- saving rejects to file browser/devtools/framework/toolbox.js.rej 1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/framework/toolbox.xul.rej ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js @@ +88,5 @@ > + synthesizeKeyForToolbox(key); > + reloadsSent++; > +} > + > +function tidyUp() { Nit: as a convention, most tests use finishUp() for this function name

(In reply to Brian Grinstead [:bgrins] from comment #44) > Comment on attachment 8434378 [details] [diff] [review] > reload-from-devtools-6.patch > > Review of attachment 8434378 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you rebase the patch? I'm getting errors applying to latest fx-team: Oops! Fixed. > ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js > @@ +88,5 @@ > > + synthesizeKeyForToolbox(key); > > + reloadsSent++; > > +} > > + > > +function tidyUp() { > > Nit: as a convention, most tests use finishUp() for this function name Really? I copied and pasted that from another test… $ grep 'function finishUp' browser/devtools/framework/test/*.js | wc -l 6 $ grep 'function tidyUp' browser/devtools/framework/test/*.js | wc -l 8 :-) Regardless, fixed.

> Really? I copied and pasted that from another test… Maybe it isn't as much of a convention as I thought :). Was just basing that off of all the other tests I've worked with in the codebase. Obviously not a big deal either way

Comment on attachment 8435139 [details] [diff] [review] reload-from-devtools-7.patch Review of attachment 8435139 [details] [diff] [review]: ----------------------------------------------------------------- The code changes look good to me. Just add r=bgrins at the end of the commit message and reupload Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f32cc59cff00

One thing I want to note here is that the test time is huge. 30s. It definitely is a likely client for timeouts and stuff (at least on debug builds). Maybe request longer timeout or skip some tools ?

(In reply to Girish Sharma [:Optimizer] from comment #48) > One thing I want to note here is that the test time is huge. 30s. It > definitely is a likely client for timeouts and stuff (at least on debug > builds). > > Maybe request longer timeout or skip some tools ? I actually like that it runs on all tools since that will catch a case of preventing default on any of these keys from an individual tool. That said, it probably doesn't need to run all tools in both host types. Could run all the tools while docked and then pass toolIDs.length-1 as the original toolnum for testAllTheTools the second time.

Comment on attachment 8435184 [details] [diff] [review] reload-from-devtools-7r.patch Review of attachment 8435184 [details] [diff] [review]: ----------------------------------------------------------------- Let's wait for the try run to finish before requesting checkin. Also, as pointed out in Comments 48 and 49 we should update the test to run a few less checks

Comment on attachment 8435184 [details] [diff] [review] reload-from-devtools-7r.patch Review of attachment 8435184 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js @@ +44,5 @@ > + toolbox.switchHost(origHostType).then(() => { > + gBrowser.selectedBrowser.removeEventListener('load', reloadCounter, true); > + finishUp(); > + }); > + }); We should probably be preemptive here and speed up the test before we run into intermittents (which will probably happen as it takes a while to go through all the reloads and the test boxes are pretty slow). So if you pass toolIDs.length - 1 in as the toolNum parameter to testAllTheTools for the second run I don't think we are losing any test coverage. If we still run into issues with it running too slowly we could look into just doing a requestLongerTimeout

(In reply to Brian Grinstead [:bgrins] from comment #49) > (In reply to Girish Sharma [:Optimizer] from comment #48) > > Maybe request longer timeout or skip some tools ? > > I actually like that it runs on all tools since that will catch a case of > preventing default on any of these keys from an individual tool. That said, > it probably doesn't need to run all tools in both host types. Could run all > the tools while docked and then pass toolIDs.length-1 as the original > toolnum for testAllTheTools the second time. I just tried that out and it reduces the test time on my machine from 36 seconds to 30 seconds. That's a complete ./mach run of only that test, so there's lots of setup and teardown included in that measurement. Would you like me to make that change? (Does it need the review bit again?) Actually the change I made was to make it test all the tools on the undocked window, and only test one tool on the docked (mostly because the main bug report was all about undocked windows).

(In reply to David Caldwell from comment #53) > (In reply to Brian Grinstead [:bgrins] from comment #49) > > (In reply to Girish Sharma [:Optimizer] from comment #48) > > > Maybe request longer timeout or skip some tools ? > > > > I actually like that it runs on all tools since that will catch a case of > > preventing default on any of these keys from an individual tool. That said, > > it probably doesn't need to run all tools in both host types. Could run all > > the tools while docked and then pass toolIDs.length-1 as the original > > toolnum for testAllTheTools the second time. > > I just tried that out and it reduces the test time on my machine from 36 > seconds to 30 seconds. That's a complete ./mach run of only that test, so > there's lots of setup and teardown included in that measurement. Would you > like me to make that change? (Does it need the review bit again?) > > Actually the change I made was to make it test all the tools on the undocked > window, and only test one tool on the docked (mostly because the main bug > report was all about undocked windows). I'm not sure how the test times are tallied (if it includes setup/teardown). I'd say go ahead and make the change then reupload the patch and we can push to try again. Then we will wait and see if there are any timeouts after a few retriggers of the test and if so we can request a longer timeout.

This patch now tests all tools in undocked mode and just one tool in docked mode.

(In reply to David Caldwell from comment #55) > Created attachment 8435211 [details] [diff] [review] > reload-from-devtools-8.patch > > This patch now tests all tools in undocked mode and just one tool in docked > mode. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=762478469cf0

Last try push didn't have patch applied - here is a new one: https://tbpl.mozilla.org/?tree=Try&rev=0576d8c6207c

Looks like this is causing test errors in web audio editor / canvas debugger / shader editor, all of which use the reload method. It looks like this new options argument needs to be optional. One such error is on browser/devtools/canvasdebugger/test/browser_canvas-frontend-call-highlight.js: 0:09.13 Handler function DebuggerClient.requester threw an exception: Error: Bad index into params: 0 0:09.13 Stack: DebuggerClient.Argument.prototype.getArgument@resource://gre/modules/devtools/dbg-client.jsm:369:5 0:09.13 DebuggerClient.requester/<@resource://gre/modules/devtools/dbg-client.jsm:325:9 0:09.13 makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7 0:09.13 reload/<@chrome://mochitests/content/browser/browser/devtools/canvasdebugger/test/head.js:167:21 0:09.13 testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:781:9

OK, so the trick is that we need to change the 'reload' function to accept an optional parameter and call the DebuggerClient.requester separately. For example Instead of this: /** * Reload the page in this tab. */ reload: DebuggerClient.requester({ type: "reload" }, { telemetry: "RELOAD" }), We want to do this: /** * Reload the page in this tab. * * @param [optional] bool force * Whether or not this reload should skip the cache. */ reload: function(force = false) { this._doReload({force: force}); }, /** * Reload the page in this tab. * * @param object options * An object with a `force` property indicating whether or not * this reload should skip the cache */ _doReload: DebuggerClient.requester({ type: "reload", options: args(0) }, { telemetry: "RELOAD" }),

Comment on attachment 8435211 [details] [diff] [review] reload-from-devtools-8.patch Review of attachment 8435211 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js @@ +40,5 @@ > + let origHostType = toolbox.hostType; > + toolbox.switchHost(Toolbox.HostType.WINDOW).then(() => { > + toolbox.doc.defaultView.focus(); > + testAllTheTools("undocked", () => { > + toolbox.switchHost(origHostType).then(() => { Also, the test is throwing 3 promise resolution errors at the end, which are going to start causing the test to fail in the future (Bug 1018184). Two of these seem to fixed by just not doing this last switchHost(), which is an easy change since that isn't actually adding anything to the test. The remaining one I think has to do with the test finishing before the page has fully loaded. 0:22.43 ************************* 0:22.43 A coding exception was thrown in a Promise resolution callback. 0:22.43 Full message: TypeError: this._target is null 0:22.43 See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise 0:22.43 Full stack: InspectorPanel.prototype.selectionCssSelector@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:365:1 0:22.43 InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:212:7 0:22.43 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:863:11 0:22.43 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:7 0:22.43 0:22.43 *************************

(In reply to Brian Grinstead [:bgrins] from comment #59) > OK, so the trick is that we need to change the 'reload' function to accept > an optional parameter and call the DebuggerClient.requester separately. I came up with something similar last night (though I just now stole your documentation because it was better): /** * Reload the page in this tab. * * @param [optional] object options * An object with a `force` property indicating whether or not * this reload should skip the cache */ reload: function(options = { force: false }) { return this._reload(options); }, _reload: DebuggerClient.requester({ type: "reload", options: args(0) }, { telemetry: "RELOAD" }), (Is it necessary to document the internal function? I notice you did, but it seems overkill to me). (In reply to Brian Grinstead [:bgrins] from comment #60) > Comment on attachment 8435211 [details] [diff] [review] > reload-from-devtools-8.patch > > Review of attachment 8435211 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js > @@ +40,5 @@ > > + let origHostType = toolbox.hostType; > > + toolbox.switchHost(Toolbox.HostType.WINDOW).then(() => { > > + toolbox.doc.defaultView.focus(); > > + testAllTheTools("undocked", () => { > > + toolbox.switchHost(origHostType).then(() => { > > Also, the test is throwing 3 promise resolution errors at the end, which are > going to start causing the test to fail in the future (Bug 1018184). Two of > these seem to fixed by just not doing this last switchHost(), which is an > easy change since that isn't actually adding anything to the test. The > remaining one I think has to do with the test finishing before the page has > fully loaded. So, I've been playing around here and *all* of them go away if I do setTimeout(finishUp, 2000); instead of calling it directly. You are right, it appears to be a problem of something not completely loading before it gets torn down. The question is, what (looks like the DOM inspector to me)? Just delaying is the wrong thing to do, so maybe the test should be waiting for some other event… Any ideas on what event? On the other hand, if some part of devtools is throwing exceptions when it's torn down too quickly, isn't that a different bug that this test just happens to be tickling?

(In reply to David Caldwell from comment #61) > (In reply to Brian Grinstead [:bgrins] from comment #59) > > OK, so the trick is that we need to change the 'reload' function to accept > > an optional parameter and call the DebuggerClient.requester separately. > > I came up with something similar last night (though I just now stole your > documentation because it was better): > > /** > * Reload the page in this tab. > * > * @param [optional] object options > * An object with a `force` property indicating whether or not > * this reload should skip the cache > */ > reload: function(options = { force: false }) { > return this._reload(options); > }, > _reload: DebuggerClient.requester({ > type: "reload", > options: args(0) > }, { > telemetry: "RELOAD" > }), > > (Is it necessary to document the internal function? I notice you did, but it > seems overkill to me). I was just copying what was happening with resume/_doResume. I don't think it hurts to add the docs. > > (In reply to Brian Grinstead [:bgrins] from comment #60) > > Comment on attachment 8435211 [details] [diff] [review] > > reload-from-devtools-8.patch > > > > Review of attachment 8435211 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/devtools/framework/test/browser_toolbox_window_reload_target.js > > @@ +40,5 @@ > > > + let origHostType = toolbox.hostType; > > > + toolbox.switchHost(Toolbox.HostType.WINDOW).then(() => { > > > + toolbox.doc.defaultView.focus(); > > > + testAllTheTools("undocked", () => { > > > + toolbox.switchHost(origHostType).then(() => { > > > > Also, the test is throwing 3 promise resolution errors at the end, which are > > going to start causing the test to fail in the future (Bug 1018184). Two of > > these seem to fixed by just not doing this last switchHost(), which is an > > easy change since that isn't actually adding anything to the test. The > > remaining one I think has to do with the test finishing before the page has > > fully loaded. > > So, I've been playing around here and *all* of them go away if I do > setTimeout(finishUp, 2000); > instead of calling it directly. You are right, it appears to be a problem of > something not completely loading before it gets torn down. The question is, > what (looks like the DOM inspector to me)? Just delaying is the wrong thing > to do, so maybe the test should be waiting for some other event… Any ideas > on what event? You could try listening for the inspector new-root event - something like toolbox.getPanel("inspector").once("new-root"). That might be what we need to wait for. > On the other hand, if some part of devtools is throwing exceptions when it's > torn down too quickly, isn't that a different bug that this test just > happens to be tickling? Yes, sounds like it

(In reply to Brian Grinstead [:bgrins] from comment #62) > (In reply to David Caldwell from comment #61) > > (In reply to Brian Grinstead [:bgrins] from comment #59) > > > OK, so the trick is that we need to change the 'reload' function to accept > > > an optional parameter and call the DebuggerClient.requester separately. > > > > (Is it necessary to document the internal function? I notice you did, but it > > seems overkill to me). > > I was just copying what was happening with resume/_doResume. I don't think > it hurts to add the docs. I see. resume()/_doResume() seems like a different scenario since there are a bunch of front end functions that all use the _doResume() workhorse, but in the reload() case it's really just a work-around for DebuggerClient.requester not handling optional arguments. Resume could be technically done without exposing _doResume (but it doesn't seem worth the gymnastics). > > (In reply to Brian Grinstead [:bgrins] from comment #60) > > > Also, the test is throwing 3 promise resolution errors at the end, which are > > > going to start causing the test to fail in the future (Bug 1018184). Two of > > > these seem to fixed by just not doing this last switchHost(), which is an > > > easy change since that isn't actually adding anything to the test. The > > > remaining one I think has to do with the test finishing before the page has > > > fully loaded. > > > > So, I've been playing around here and *all* of them go away if I do > > setTimeout(finishUp, 2000); > > instead of calling it directly. You are right, it appears to be a problem of > > something not completely loading before it gets torn down. The question is, > > what (looks like the DOM inspector to me)? Just delaying is the wrong thing > > to do, so maybe the test should be waiting for some other event… Any ideas > > on what event? > > You could try listening for the inspector new-root event - something like > toolbox.getPanel("inspector").once("new-root"). That might be what we need > to wait for. That did the trick. Thanks! > > On the other hand, if some part of devtools is throwing exceptions when it's > > torn down too quickly, isn't that a different bug that this test just > > happens to be tickling? > > Yes, sounds like it Should I file a bug report about this? If it's a bug should I even try to work around it with the new-root event handler (since that's really hiding a real issue)? Or maybe just add a comment to the new-root handler mentioning the issue so that it could be taken out later? At any rate, this new patch waits for new-root and makes the reload() parameter optional. If you like it, could you push it to try?

(In reply to David Caldwell from comment #63) > Created attachment 8436027 [details] [diff] [review] > reload-from-devtools-9.patch > > (In reply to Brian Grinstead [:bgrins] from comment #62) > > (In reply to David Caldwell from comment #61) > > > (In reply to Brian Grinstead [:bgrins] from comment #59) > > > > OK, so the trick is that we need to change the 'reload' function to accept > > > > an optional parameter and call the DebuggerClient.requester separately. > > > > > > (Is it necessary to document the internal function? I notice you did, but it > > > seems overkill to me). > > > > I was just copying what was happening with resume/_doResume. I don't think > > it hurts to add the docs. > > I see. resume()/_doResume() seems like a different scenario since there are > a bunch of front end functions that all use the _doResume() workhorse, but > in the reload() case it's really just a work-around for > DebuggerClient.requester not handling optional arguments. Resume could be > technically done without exposing _doResume (but it doesn't seem worth the > gymnastics). > > > > (In reply to Brian Grinstead [:bgrins] from comment #60) > > > > Also, the test is throwing 3 promise resolution errors at the end, which are > > > > going to start causing the test to fail in the future (Bug 1018184). Two of > > > > these seem to fixed by just not doing this last switchHost(), which is an > > > > easy change since that isn't actually adding anything to the test. The > > > > remaining one I think has to do with the test finishing before the page has > > > > fully loaded. > > > > > > So, I've been playing around here and *all* of them go away if I do > > > setTimeout(finishUp, 2000); > > > instead of calling it directly. You are right, it appears to be a problem of > > > something not completely loading before it gets torn down. The question is, > > > what (looks like the DOM inspector to me)? Just delaying is the wrong thing > > > to do, so maybe the test should be waiting for some other event… Any ideas > > > on what event? > > > > You could try listening for the inspector new-root event - something like > > toolbox.getPanel("inspector").once("new-root"). That might be what we need > > to wait for. > > That did the trick. Thanks! > > > > On the other hand, if some part of devtools is throwing exceptions when it's > > > torn down too quickly, isn't that a different bug that this test just > > > happens to be tickling? > > > > Yes, sounds like it > > Should I file a bug report about this? If it's a bug should I even try to > work around it with the new-root event handler (since that's really hiding a > real issue)? Or maybe just add a comment to the new-root handler mentioning > the issue so that it could be taken out later? > > At any rate, this new patch waits for new-root and makes the reload() > parameter optional. If you like it, could you push it to try? I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1e40720ee5c6. I'm OK with landing the patch with the additional listener and handling the promise failure separately. If you'd file a follow up bug for this, that would be great.

Comment on attachment 8436027 [details] [diff] [review] reload-from-devtools-9.patch Review of attachment 8436027 [details] [diff] [review]: ----------------------------------------------------------------- Test runs look good

Thanks for fixing my initial request: https://twitter.com/weibelt/status/369848412537389056! Verified it today as well. Works great!