1403736 - Update debugger frontend

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Jason Laster [:jlast]]

I'd like to experiment w/ an open bug that we can continue to add patches too. Hopefully, this will make it easier to do daily releases.

Comment 1

[Jason Laster [:jlast]]

Attachment: 9-27.patch

Comment 2

[Jason Laster [:jlast]]

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: debugger_ui_nits.png

Try looks a bit orange for now :) Some UI nits I saw while quickly testing:
- missing left padding in source tree
- vertical separators are not aligned between main area and left sidebar

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8912922 [details] [diff] [review]
9-27.patch

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

Let's see another bundle:
- mochitest/browser_dbg-search-project.js should be updated with the new pref
- some ui nits
- try busted

::: devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-cond.js
@@ +38,5 @@
>  add_task(async function() {
>    const dbg = await initDebugger("doc-scripts.html");
>    await selectSource(dbg, "simple2");
>  
> +  dump('Adding a conditional Breakpoint\n')

why dump and not info?

::: devtools/client/preferences/debugger.js
@@ +41,5 @@
>  pref("devtools.debugger.file-search-case-sensitive", false);
>  pref("devtools.debugger.file-search-whole-word", false);
>  pref("devtools.debugger.file-search-regex-match", false);
>  pref("devtools.debugger.features.async-stepping", true);
> +pref("devtools.debugger.features.project-text-search", true);

The old preference is still used in devtools/client/debugger/new/test/mochitest/browser_dbg-search-project.js

Comment 5

[Jason Laster [:jlast]]

Attachment: 9-27-2.patch

Comment 6

[Jason Laster [:jlast]]

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

Comment 7

[Jason Laster [:jlast]]

Attachment: 9-27-3.patch

Comment 8

[Jason Laster [:jlast]]

Attachment: 9-27-4.patch

Comment 9

[Jason Laster [:jlast]]

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

Comment 10

[Jason Laster [:jlast]]

Attachment: 9-27-5.patch

i forgot if 4 is up to date

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8913546 [details] [diff] [review]
9-27-5.patch

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

devtools/client/framework/test/browser_browser_toolbox_debugger.js is still failing, on try and locally.

Comment 12

[Julian Descottes [:jdescottes]]

Found the root cause of the failure. The nodes in the tree now include some white space in their text content, making one of the checks in the test fail. Workaround below fixes the issue. Some intermediary checks should be added in this test (check that we could find "mozilla.org" etc etc)

--- a/devtools/client/framework/test/test_browser_toolbox_debugger.js
+++ b/devtools/client/framework/test/test_browser_toolbox_debugger.js
@@ -15,17 +15,17 @@ Task.spawn(function* () {
   let document = window.document;
 
   yield waitForSources(dbg, testUrl);
 //  yield waitForSourceCount(dbg, 6);
 
   info("Loaded, selecting the test script to debug");
   // First expand the domain
   let domain = [...document.querySelectorAll(".tree-node")].find(node => {
-    return node.textContent == "mozilla.org";
+    return node.textContent.trim() == "mozilla.org";
   });
   let arrow = domain.querySelector(".arrow");
   arrow.click();
   let script = [...document.querySelectorAll(".tree-node")].find(node => {
     return node.textContent.includes("browser-toolbox-test.js");
   });
   script = script.querySelector(".node");
   script.click();

Comment 13

[Jason Laster [:jlast]]

Attachment: 9-27-6.patch

Comment 14

[Jason Laster [:jlast]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c7fbe3f779
Update debugger frontend (9/27/2017). r=jdescottes

Comment 16

[Jason Laster [:jlast]]

Attachment: 9-29-1.patch

Comment 17

[Jason Laster [:jlast]]

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

Comment 18

[Jason Laster [:jlast]]

Here's the release diff: https://github.com/devtools-html/debugger.html/compare/9fe3c952d5d4c2f106b...2b18094c
And the release PR: https://github.com/devtools-html/debugger.html/pull/4214

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8913808 [details] [diff] [review]
9-29-1.patch

Moved to Bug 1404837

Comment 21

[Stefan Plewako [:stef]]

https://hg.mozilla.org/mozilla-central/diff/f4c7fbe3f779/devtools/client/locales/en-US/debugger.properties

> +# LOCALIZATION NOTE (anonymous): The text that is displayed when the
> +# display name is null.
> +anonymous=(anonymous)

What name?
Would it be possible to stop these code dumps and instead import patches?

Comment 22

[Julian Descottes [:jdescottes]]

(In reply to Stefan Plewako [:stef] from comment #21)
> https://hg.mozilla.org/mozilla-central/diff/f4c7fbe3f779/devtools/client/
> locales/en-US/debugger.properties
> 
> > +# LOCALIZATION NOTE (anonymous): The text that is displayed when the
> > +# display name is null.
> > +anonymous=(anonymous)
> 
> What name?

I think this refers to a method name.

> Would it be possible to stop these code dumps and instead import patches?

We are not really happy with the bundle approach either, but this is a complex topic. Short term we want to increase the frequency of synchronization so that it feels more incremental on m-c side. l10n file changes are usually reviewed by :flod on GitHub as well, but we missed this one.

Comment 23

[Stefan Plewako [:stef]]

(In reply to Julian Descottes [:jdescottes] from comment #22)
> (In reply to Stefan Plewako [:stef] from comment #21)
> > https://hg.mozilla.org/mozilla-central/diff/f4c7fbe3f779/devtools/client/
> > locales/en-US/debugger.properties
> > 
> > > +# LOCALIZATION NOTE (anonymous): The text that is displayed when the
> > > +# display name is null.
> > > +anonymous=(anonymous)
> > 
> > What name?
> 
> I think this refers to a method name.

What method? Where? In what form?

> > Would it be possible to stop these code dumps and instead import patches?
> 
> We are not really happy with the bundle approach either, but this is a
> complex topic. Short term we want to increase the frequency of
> synchronization so that it feels more incremental on m-c side. l10n file
> changes are usually reviewed by :flod on GitHub as well, but we missed this
> one.

More frequent code dumps do not make changes easier to understand. Dumping series of patches instead one big doesn't sound like complex topic but rather minor technical change.

On the other hand if there are reviews somewhere on Github for the changes that create code dumps then it might be enough to link those reviews in commit message.

Comment 24

[Julian Descottes [:jdescottes]]

(In reply to Stefan Plewako [:stef] from comment #23)
> (In reply to Julian Descottes [:jdescottes] from comment #22)
> > (In reply to Stefan Plewako [:stef] from comment #21)
> > > https://hg.mozilla.org/mozilla-central/diff/f4c7fbe3f779/devtools/client/
> > > locales/en-US/debugger.properties
> > > 
> > > > +# LOCALIZATION NOTE (anonymous): The text that is displayed when the
> > > > +# display name is null.
> > > > +anonymous=(anonymous)
> > > 
> > > What name?
> > 
> > I think this refers to a method name.
> 
> What method? Where? In what form?

I think it's in the call stack? ni? jlast

> 
> > > Would it be possible to stop these code dumps and instead import patches?
> > 
> > We are not really happy with the bundle approach either, but this is a
> > complex topic. Short term we want to increase the frequency of
> > synchronization so that it feels more incremental on m-c side. l10n file
> > changes are usually reviewed by :flod on GitHub as well, but we missed this
> > one.
> 
> More frequent code dumps do not make changes easier to understand. Dumping
> series of patches instead one big doesn't sound like complex topic but
> rather minor technical change.
> 

The source code of the debugger is not in mozilla-central. Except some files such as debugger.properties which are copied between the two repos, the debugger sources all live in GitHub, and only a built artifact gets delivered to mozilla-central. We can't simply apply the GH patches on m-c. And we don't want to add a build step to our m-c setup. 

As I said, we're trying to increase the frequency so that at some point we can have nightlies, maybe at some point one synchronization per PR.

If you have ideas on how to improve this, can you open an issue on the debugger.html repo?

> On the other hand if there are reviews somewhere on Github for the changes
> that create code dumps then it might be enough to link those reviews in
> commit message.

I agree that we should do a better job at this. We now have the upstream commit reference in devtools/client/debugger/new/README.mozilla. We could easily add it to the commit message. Even better would be to have a link to the GitHub push log since the last release. I'll see if we can change it to include this.

Comment 25

[Jason Laster [:jlast]]

The property is used in the scopes pane

https://github.com/loganfsmyth/debugger.html/blob/map-expressions/src/utils/pause/scopes/getScope.js#L33-L37

Here is a list of our releases to mc - http://github.com/devtools-html/debugger.html/releases/
And you can see the commits for a release by looking at the comparisons: https://github.com/devtools-html/debugger.html/compare/release-18...release-19