1346902 - Enable new debugger UI for Browser Toolbox

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Back in bug 1300820, we had disabled the new debugger UI for the case of Browser Toolbox because it was failing to load.

These issues seems to be resolved now, so it seems logical to turn it back on.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

It appears the new debugger UI is working for the Browser Toolbox case, so let's
turn it on to match our approach with web content.

Review commit: https://reviewboard.mozilla.org/r/119778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119778/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Lots of reviews for this.  In addition to :jlast as the debugger lead, I'm hoping Gijs can verify the new UI is sufficient for his browser debugging, and similarly I'd like :rpl to verify it's sufficient for add-on debugging.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(It seemed to work for both of these use cases in my own testing, but I only checked very quickly.)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8846778 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

https://reviewboard.mozilla.org/r/119778/#review122134

I'm assuming you've checked this works with e.g. XBL?

It does seem to fix bug 876655, fwiw...

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8846778 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

https://reviewboard.mozilla.org/r/119778/#review122134

XBL debugging appears to work, I was able to break in `addTab` and step through.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As far as known features that would be lost with this change (since they currently don't exist in new debugger UI), there is at least:

* Blackboxing to hide minified files (I don't think this has much relevance for Browser Toolbox today, but perhaps add-ons...?)
* Project search / searching across multiple files (I typically open a specific file and search within that file when using Browser Toolbox, but others may use a different workflow)

We do want to bring these back in the future, but for the moment these features would be lost assuming we proceed here.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

I tried the new debugger UI in the Addons Debugger and it seems to work great (actually it also seems to fix some issues of the previous debugger UI, e.g. the listed of the addon sources is not emptied when the toolbox is switched between frames, which is great!)

The only issue that I've experienced so far is related to a breakpoint set into the javascript sources from a popup window (and I didn't tried yet, but I'm pretty sure that this issue can be also reproduced from a the Browser Toolbox).

STR:
- Set a breakpoint in a source file related to a popup window
- Open the popup window (without setting the noautohide toggle button from the developer toolbox, and so the popup window is going to auto hide when it will lost the focus)
- The breakpoint is fired and the debugger is paused on the breakpoint
- Click on the debugger to interact with the debugger UI, the popup window loses its focus and it is closed automatically
- The debugger is stuck in the "paused on a breakpoint" mode and I wasn't able to unpause it anymore
- The browser is also stuck and the UI do not react to the events (probably because it is still paused in the debugger)
- closing the toolbox window is the only way to unstuck the browser UI (and if you open the toolbox window again everything seems to be back to normal)

My guess is that this issue could probably happen often enough to addon developers that are trying to debug a WebExtension that has a popup window in its UI (e.g. a pageAction / browserAction with a popup window).

Besides the above issue, the new Debugger UI looks to work great in the Addon Debugger, and I would be happy to see it enabled by default for the Addon Debugger (it is also pretty strange for the user to get two different debugger UIs in the Addon Debugger and the regular tab Developer Toolbox).

I agree that the "Blackboxing" feature is probably going to be helpful for the addon developers, but I'm not sure how much it is actually used and how much it would impact the addon developers if missing (my guess is that the Inspector panel and the WebConsole are far the two more used features of the Addon Debugger).

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8846778 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

https://reviewboard.mozilla.org/r/119778/#review122538

r+ with the comments in Comment 7

Comment 9

[Jason Laster [:jlast]]

Thanks :rpl for the feedback. I've created a new issue for tracking the popup bug https://bugzilla.mozilla.org/show_bug.cgi?id=1349956.

Could you provide a test case in the bug that we can use to hunt it down? I'm not sure if this bug needs to block the browser toolbox if the toolbox is not handling many popups. That's not to say, it is not a priority though.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

I'm looking into how to put together a minimal test case for the popup issue (that I'm then going to attach in Bug 1349956 where we are going to track the popup bug), in the meantime I've checked what was the behavior of the old debugger in this scenario and it has exactly the same bug, it is only less visible because of the different UI.

So long story short, I confirm that Bug 1349956 is not a blocker for this bug from a WebExtension addon debugging point of view.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:jlast, since :rpl has said the add-on issue shouldn't be a blocker, do you think we're ready to proceed here?

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

AFAIK, :jlaster does not have objections here, so let's proceed.  We can always revert if needed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33be5bce0905
Re-enable new debugger UI for Browser Toolbox. r=Gijs,rpl

Comment 14

[Iris Hsiao [:ihsiao]]

sorry had to back this out for devtool test failing in browser_browser_toolbox_debugger.js like https://treeherder.mozilla.org/logviewer.html#?job_id=89053059&repo=autoland&lineNumber=2213

https://hg.mozilla.org/integration/autoland/rev/173691d9857c

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1346902 - WIP: Update BT tests and integration for new debugger

Review commit: https://reviewboard.mozilla.org/r/127406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127406/

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8846778 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119778/diff/1-2/

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I had forgotten there's a somewhat custom test to verify the debugger UI with Browser Toolbox.

I got partway through converting the test, but I am out of time to work on this.

Can someone finish this up, or else decide to disable the test?  Perhaps :jlast or :ochameau are interested?

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

We should get this settled before we resolve bug 1294139

Comment 19

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f08521a60f648e4427908d03bcd292de27e4adc9

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

It appears the new debugger UI is working for the Browser Toolbox case, so let's
turn it on to match our approach with web content.

Review commit: https://reviewboard.mozilla.org/r/133936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133936/

Comment 21

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1346902 - Update browser toolbox integration test for new debugger.

Review commit: https://reviewboard.mozilla.org/r/133938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133938/

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8861921 [details]
Bug 1346902 - Update browser toolbox integration test for new debugger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133938/diff/1-2/

Comment 23

[Alexandre Poirot [:ochameau]]

Jason, this test is quite hairy because of the Browser Toolbox and mochitests.
We have to run code from the browser toolbox, which lives in another process.
Unfortunately, this process doesn't have access to mochitests URL/resources, like test scripts and head files.
So this test stringify a bunch of javascript (New debugger head.js, some glue and a minimal debugger test script) in order to hand it over the browser toolbox via an env variable.

I had to patch s.get("url") as sometimes for resources displayed from the browsere toolbo s.get("url") is null.
Also, I don't know why dbg.win.cm is undefined or why this function works for other mochitests??

Comment 24

[Jason Laster [:jlast]]

Hi alex, thanks for making progress on this. The test is ugly, but it makes sense.

- dbg.win.cm comes from the Editor saving cm in the global context in development and testing (links below).


https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor/index.js#L168-L169
https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/src/utils/debug.js#L3-L8

Comment 25

[Jason Laster [:jlast]]

Comment on attachment 8861921 [details]
Bug 1346902 - Update browser toolbox integration test for new debugger.

https://reviewboard.mozilla.org/r/133938/#review139178

Thanks. This looks good.

::: devtools/client/debugger/new/test/mochitest/head.js:216
(Diff revision 2)
>    const location = getPause(getState()).getIn(["frame", "location"]);
>    is(location.get("sourceId"), source.id);
>    is(location.get("line"), line);
>  
>    // Check the debug line
> +  let cm = dbg.win.document.querySelector(".CodeMirror").CodeMirror;

nice idea

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8861920 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133936/diff/1-2/

Comment 27

[Alexandre Poirot [:ochameau]]

Comment on attachment 8861921 [details]
Bug 1346902 - Update browser toolbox integration test for new debugger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133938/diff/2-3/

Comment 28

[Alexandre Poirot [:ochameau]]

Comment on attachment 8861920 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

https://reviewboard.mozilla.org/r/133936/#review139310

Carrying over the review.

Comment 29

[Alexandre Poirot [:ochameau]]

Comment on attachment 8861920 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

https://reviewboard.mozilla.org/r/133936/#review139312

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8861920 [details]
Bug 1346902 - Re-enable new debugger UI for Browser Toolbox.

https://reviewboard.mozilla.org/r/133936/#review139314

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80992422994d
Re-enable new debugger UI for Browser Toolbox. r=jryans
https://hg.mozilla.org/integration/autoland/rev/291b406b10b7
Update browser toolbox integration test for new debugger. r=jlast

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80992422994d
https://hg.mozilla.org/mozilla-central/rev/291b406b10b7

Comment 33

[Bryan Clark (DevTools PM) [:clarkbw]]

If someone needed to revert to the old debugger for the toolbox, is there a pref or system for doing that?

Comment 34

[Alexandre Poirot [:ochameau]]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #33)
> If someone needed to revert to the old debugger for the toolbox, is there a
> pref or system for doing that?

No pref and I can't think of any way to control that except patching this code:
  https://hg.mozilla.org/mozilla-central/rev/80992422994d

But we can introduce a new pref, just for that.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Alexandre Poirot [:ochameau] from comment #34)
> But we can introduce a new pref, just for that.

Ok, not necessary, here's the work around if you need the old UI in the browser toolbox:

* Start the Browser Toolbox
* Before going to the Debugger Tab
* Go to the DevTools Settings (gear icon)
* Uncheck the new debugger frontend pref