Right now the browser toolbox defaults to the debugger panel, which can lead to a slow startup. We could open it to the console panel instead.

I would be fine with this personally. The main reason it opens debugger first is so it resembles it's precursor the Browser Debugger. However, there's also the command line use case (starting Firefox with -jsdebugger). Do you think we should go console tab for every case? Or console if started from a menu, but debugger if started from the command line?

From what I understand, it would be best to stick with debugger from the command line and console when started from the menu. I haven't used the flag much myself but it makes sense that from a test you may want to hit a debugger statement or set a breakpoint, and starting on the debugger would be more convenient.

It would be weird to have two menu items that give you a browser console though. Also, if people are actually interested in the debugger when opening the Browser Toolbox, then we will make it even slower for them. Do we have any data (even anecdotal) that people open the Browser Toolbox instead of the Browser Console to interact with the console? Opening the inspector might make more sense, but only if people use that more often and we don't fully support inspecting anonymous content, so they might not.

Would it be possible to remember the last opened tool, like we do for the toolbox? This would indeed be better than us picking one or the other.

Yes, that sounds doable.

Ryan, what changes would it take to make the browser toolbox open on the last used panel? I could work on this with a bit of guidance

We should first wait for bug 1060464 I think, since this reworks the profile used by the Browser Toolbox (it should hopefully re-land soon). I think the following approach would work: 1. Change toolbox-process-window[1] to pass a null tool on toolbox open, so we rely on the selectedTool pref to pick the one to use. This implements the basic behavior you're after. 2. To handle the -jsdebugger case, the CLI handler can pass a new option[2] to BrowserToolboxProcess with the tool ID to open. 3. BrowserToolboxProcess would then set the selectedTool pref to this value when it sets up the profile[3] (this is the part bug 1060464 reworks a bit). Let me know if more help is needed! [1]: http://hg.mozilla.org/mozilla-central/annotate/e58842c764dd/browser/devtools/framework/toolbox-process-window.js#l78 [2]: http://hg.mozilla.org/mozilla-central/annotate/e58842c764dd/browser/devtools/devtools-clhandler.js#l56 [3]: http://hg.mozilla.org/mozilla-central/annotate/e58842c764dd/browser/devtools/framework/ToolboxProcess.jsm#l154

Ryan, when I apply this patch it is a bit odd - it actually uses the "devtools.toolbox.selectedTool" pref from my main profile. Like, if I open up the console tab in my normal toolbox then reopen BT then BT opens on console tab. But if I open the tab in the BT and reopen it will not keep that last used tab. Are all of the prefs being copied over from the main profile on BT startup?

(In reply to Brian Grinstead [:bgrins] from comment #8) > Created attachment 8499590 [details] [diff] [review] > last-used-tab-browsertoolbox.patch > > Ryan, when I apply this patch it is a bit odd - it actually uses the > "devtools.toolbox.selectedTool" pref from my main profile. Like, if I open > up the console tab in my normal toolbox then reopen BT then BT opens on > console tab. But if I open the tab in the BT and reopen it will not keep > that last used tab. Are all of the prefs being copied over from the main > profile on BT startup? Yes, this is a recent change... I believe it also came up a few weeks back when you asked about "pause on exceptions" happening in BT. Bug 1060464 made a change[1] to copy all prefs. However, this is seemingly more and more like a poor idea. It was done to support mach run with "--jsdebugger" (see the bug). There are really only a few prefs that would need to get copied to address that bug's issue. A sensible way to resolve the problem you are seeing would be to only copy the prefs we actually need to resolve the issue from bug 1060464. Gijs may also have better ideas. [1]: http://hg.mozilla.org/mozilla-central/annotate/b85c260821ab/browser/devtools/framework/ToolboxProcess.jsm#l171

(In reply to J. Ryan Stinnett [:jryans] from comment #9) > (In reply to Brian Grinstead [:bgrins] from comment #8) > > Created attachment 8499590 [details] [diff] [review] > > last-used-tab-browsertoolbox.patch > > > > Ryan, when I apply this patch it is a bit odd - it actually uses the > > "devtools.toolbox.selectedTool" pref from my main profile. Like, if I open > > up the console tab in my normal toolbox then reopen BT then BT opens on > > console tab. But if I open the tab in the BT and reopen it will not keep > > that last used tab. Are all of the prefs being copied over from the main > > profile on BT startup? > > Yes, this is a recent change... I believe it also came up a few weeks back > when you asked about "pause on exceptions" happening in BT. > > Bug 1060464 made a change[1] to copy all prefs. However, this is seemingly > more and more like a poor idea. It was done to support mach run with > "--jsdebugger" (see the bug). There are really only a few prefs that would > need to get copied to address that bug's issue. > > A sensible way to resolve the problem you are seeing would be to only copy > the prefs we actually need to resolve the issue from bug 1060464. Gijs may > also have better ideas. > > [1]: > http://hg.mozilla.org/mozilla-central/annotate/b85c260821ab/browser/devtools/ > framework/ToolboxProcess.jsm#l171 Gijs, there are some prefs that we do not want to copy over when opening the Browser Toolbox (like "devtools.toolbox.selectedTool"). Any thoughts on how to best exclude certain prefs (or whitelist only a select set that matter)?

(In reply to Brian Grinstead [:bgrins] from comment #10) > (In reply to J. Ryan Stinnett [:jryans] from comment #9) > > (In reply to Brian Grinstead [:bgrins] from comment #8) > > > Created attachment 8499590 [details] [diff] [review] > > > last-used-tab-browsertoolbox.patch > > > > > > Ryan, when I apply this patch it is a bit odd - it actually uses the > > > "devtools.toolbox.selectedTool" pref from my main profile. Like, if I open > > > up the console tab in my normal toolbox then reopen BT then BT opens on > > > console tab. But if I open the tab in the BT and reopen it will not keep > > > that last used tab. Are all of the prefs being copied over from the main > > > profile on BT startup? > > > > Yes, this is a recent change... I believe it also came up a few weeks back > > when you asked about "pause on exceptions" happening in BT. > > > > Bug 1060464 made a change[1] to copy all prefs. However, this is seemingly > > more and more like a poor idea. It was done to support mach run with > > "--jsdebugger" (see the bug). There are really only a few prefs that would > > need to get copied to address that bug's issue. > > > > A sensible way to resolve the problem you are seeing would be to only copy > > the prefs we actually need to resolve the issue from bug 1060464. Gijs may > > also have better ideas. > > > > [1]: > > http://hg.mozilla.org/mozilla-central/annotate/b85c260821ab/browser/devtools/ > > framework/ToolboxProcess.jsm#l171 > > Gijs, there are some prefs that we do not want to copy over when opening the > Browser Toolbox (like "devtools.toolbox.selectedTool"). Any thoughts on how > to best exclude certain prefs (or whitelist only a select set that matter)? Not really, short of writing your own pref serializer. However, the pref copying happens only once, and the browser toolbox is now stored in a sub-profile inside the parent profile, and so it can determine it's a "browser toolbox profile", and could then nuke a bunch of prefs that we know interfere on first startup? (we could track first browser-toolbox-profile startup by... another pref... :-) )

I also wonder if this explains why I've been having issues with prefs not persisting in the BT though... it looks like the devtools pref pane doesn't use <preference> elements and never manually saves prefs. Ideally, an unload handler in the browser toolbox (or similar) should be doing that manually (switching to <preference> elements would be a lot of work, I guess, for limited benefit).

(In reply to :Gijs Kruitbosch from comment #11) > However, the pref copying happens only once, and the browser toolbox is now > stored in a sub-profile inside the parent profile, and so it can determine > it's a "browser toolbox profile", and could then nuke a bunch of prefs that > we know interfere on first startup? (we could track first > browser-toolbox-profile startup by... another pref... :-) ) Actually, it is copying over the prefs every time. I'm not sure if this is intentional, but if I early return in the case of debuggingProfileDir already existing things work how I expect: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ToolboxProcess.jsm?from=toolboxprocess.jsm#162.

(In reply to Brian Grinstead [:bgrins] from comment #14) > (In reply to :Gijs Kruitbosch from comment #11) > > However, the pref copying happens only once, and the browser toolbox is now > > stored in a sub-profile inside the parent profile, and so it can determine > > it's a "browser toolbox profile", and could then nuke a bunch of prefs that > > we know interfere on first startup? (we could track first > > browser-toolbox-profile startup by... another pref... :-) ) > > Actually, it is copying over the prefs every time. I'm not sure if this is > intentional, but if I early return in the case of debuggingProfileDir > already existing things work how I expect: > http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ > ToolboxProcess.jsm?from=toolboxprocess.jsm#162. That's not intentional. I wrote that code, I didn't intend it to copy prefs all the time. Oversight on my part. r=me to just move the return out of the if block - AIUI that should fix that part, at least?

(In reply to :Gijs Kruitbosch from comment #15) > (In reply to Brian Grinstead [:bgrins] from comment #14) > > (In reply to :Gijs Kruitbosch from comment #11) > > > However, the pref copying happens only once, and the browser toolbox is now > > > stored in a sub-profile inside the parent profile, and so it can determine > > > it's a "browser toolbox profile", and could then nuke a bunch of prefs that > > > we know interfere on first startup? (we could track first > > > browser-toolbox-profile startup by... another pref... :-) ) > > > > Actually, it is copying over the prefs every time. I'm not sure if this is > > intentional, but if I early return in the case of debuggingProfileDir > > already existing things work how I expect: > > http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ > > ToolboxProcess.jsm?from=toolboxprocess.jsm#162. > > That's not intentional. I wrote that code, I didn't intend it to copy prefs > all the time. Oversight on my part. r=me to just move the return out of the > if block - AIUI that should fix that part, at least? I think we'll also need to set this._dbgProfilePath, but I could do something like: catch (ex) { if (ex.result === Cr.NS_ERROR_FILE_ALREADY_EXISTS) { this._dbgProfilePath = debuggingProfileDir.path; } else { dumpn("Error trying to create a profile directory, failing."); dumpn("Error: " + (ex.message || ex)); } return; }

Gijs, can you double check the change to toolboxprocess.jsm? Ryan, can you look at the change to toolbox-process-window.js?

Comment on attachment 8527958 [details] [diff] [review] last-used-tab-bt.patch Review of attachment 8527958 [details] [diff] [review]: ----------------------------------------------------------------- This does break the rule of always showing the debugger when "--jsdebugger" is used, but only in the case that the Browser Toolbox has been opened before, and the last tool was some non-debugger tool. I think that seems okay. At the very least, it allows us to better profile the DevTools without requiring the debugger tool to start first. We'll see if anyone gets mad, and we can be more clever if so.

(In reply to J. Ryan Stinnett [:jryans] from comment #18) > Comment on attachment 8527958 [details] [diff] [review] > last-used-tab-bt.patch > > Review of attachment 8527958 [details] [diff] [review]: > ----------------------------------------------------------------- > > This does break the rule of always showing the debugger when "--jsdebugger" > is used, but only in the case that the Browser Toolbox has been opened > before, and the last tool was some non-debugger tool. Yeah, good point. Probably the biggest place this would be a problem is during a test (especially until bug 956087 is fixed since it could skip a debugger; statement until then. It appears that for tests it may be using the scratch_user profile from ./mach run, so that could end up being an issue if you were using the Inspector for content pages then ran a mochitest with the --jsdebugger flag. That seems like it could be annoying, is there a way to tell if we are being running for a test?

(In reply to Brian Grinstead [:bgrins] from comment #19) > (In reply to J. Ryan Stinnett [:jryans] from comment #18) > > Comment on attachment 8527958 [details] [diff] [review] > > last-used-tab-bt.patch > > > > Review of attachment 8527958 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This does break the rule of always showing the debugger when "--jsdebugger" > > is used, but only in the case that the Browser Toolbox has been opened > > before, and the last tool was some non-debugger tool. > > Yeah, good point. Probably the biggest place this would be a problem is > during a test (especially until bug 956087 is fixed since it could skip a > debugger; statement until then. > > It appears that for tests it may be using the scratch_user profile from > ./mach run, so that could end up being an issue if you were using the > Inspector for content pages then ran a mochitest with the --jsdebugger flag. > That seems like it could be annoying, is there a way to tell if we are being > running for a test? Hmm, don't tests create a new profile for each run? When I run mochitest-devtools, I see a new sub-profile created at "/var/folders/j6/jhbppx_x1050lwg8bmjxkt240000gp/T/tmp7nv_ub.mozrunner/chrome_debugger_profile" and this is a new path for each run. So, it may not be a big deal, since each new run would reset these prefs and start from debugger by default.

(In reply to J. Ryan Stinnett [:jryans] from comment #20) > (In reply to Brian Grinstead [:bgrins] from comment #19) > > (In reply to J. Ryan Stinnett [:jryans] from comment #18) > > > Comment on attachment 8527958 [details] [diff] [review] > > > last-used-tab-bt.patch > > > > > > Review of attachment 8527958 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > This does break the rule of always showing the debugger when "--jsdebugger" > > > is used, but only in the case that the Browser Toolbox has been opened > > > before, and the last tool was some non-debugger tool. > > > > Yeah, good point. Probably the biggest place this would be a problem is > > during a test (especially until bug 956087 is fixed since it could skip a > > debugger; statement until then. > > > > It appears that for tests it may be using the scratch_user profile from > > ./mach run, so that could end up being an issue if you were using the > > Inspector for content pages then ran a mochitest with the --jsdebugger flag. > > That seems like it could be annoying, is there a way to tell if we are being > > running for a test? > > Hmm, don't tests create a new profile for each run? When I run > mochitest-devtools, I see a new sub-profile created at > "/var/folders/j6/jhbppx_x1050lwg8bmjxkt240000gp/T/tmp7nv_ub.mozrunner/ > chrome_debugger_profile" and this is a new path for each run. So, it may > not be a big deal, since each new run would reset these prefs and start from > debugger by default. Oh yeah, you are right. I'm not sure what configuration I was using - I just retested and it seems to open the debugger panel regardless of the last used panel by ./mach run.

> > Hmm, don't tests create a new profile for each run? When I run > > mochitest-devtools, I see a new sub-profile created at > > "/var/folders/j6/jhbppx_x1050lwg8bmjxkt240000gp/T/tmp7nv_ub.mozrunner/ > > chrome_debugger_profile" and this is a new path for each run. So, it may > > not be a big deal, since each new run would reset these prefs and start from > > debugger by default. > > Oh yeah, you are right. I'm not sure what configuration I was using - I > just retested and it seems to open the debugger panel regardless of the last > used panel by ./mach run. OK, did some more testing. And it *is* copying over the profile as you expected for the new test profile. However, that is actually what is causing problems in this case (it is copying over and using that profile's last used devtools tab). And worse, the default value for this is 'webconsole' so it will always start with this if you haven't changed anything with the scratch_user. So, I see two possible ways to handle this: 1. If there are there any prefs or anything else in this newly created test-specific profile that could tell us that it's running in a test, then we could always open up on the debugger panel regardless of the last used pref. 2. We could force toolboxprocess.jsm to copy this particular pref over as "jsdebugger" - something like: let orig = Services.prefs.getCharPref("devtools.toolbox.selectedTool"); Services.prefs.setCharPref("devtools.toolbox.selectedTool", "jsdebugger"); Services.prefs.savePrefFile(prefsFile); Services.prefs.setCharPref("devtools.toolbox.selectedTool", orig);

Using the suggestion to put the pref default in testing/profiles/prefs_general.js to prevent any nasty workarounds to guarantee that the debugger panel opens by default in tests.

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c1890cc0359d, mochitest-chrome says https://treeherder.mozilla.org/logviewer.html#?job_id=1280340&repo=fx-team

So... setting the test default to jsdebugger caused some failures in app-manager and webide. I started trying to look into the actual failures but didn't get too far. Since putting the prefs back for these tests just restores the old behavior I think it should be fine. What do you think? Try push: https://tbpl.mozilla.org/?tree=Try&rev=627e114cef83

Comment on attachment 8529293 [details] [diff] [review] last-used-tab-bt.patch Review of attachment 8529293 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, I guess I should have considered that... :/ Now I'm worried it could make other tests intermittently slower too, since they would now start on debugger unless specific otherwise. How about this: in the --jsdebugger CLI handler[1], set the pref (with a comment about why you'd ever do such a thing). Then, ToolboxProcess will copy it to the sub-profile (at least for tests, since we are in a temporary profile). That way we aren't affecting other tests anymore. [1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/devtools-clhandler.js#55

OK, another attempt at this as we discussed on IRC. Setting a new pref for testing only - "devtools.browsertoolbox.panel" - and looking for this in toolbox-process-window.js. This ensures that any test run with --jsdebugger will open on the debugger panel, and should not affect other toolboxes that are opened during tests (which is what was causing the webide failures). Try push: https://tbpl.mozilla.org/?tree=Try&rev=7b583c825634