Closed Bug 1403736 Opened 3 years ago Closed 3 years ago

Update debugger frontend

Categories

(DevTools :: Debugger, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch 9-27.patch (obsolete) — Splinter Review
Attachment #8912922 - Flags: review?(jdescottes)
Assignee: nobody → jlaster
Attached image debugger_ui_nits.png (obsolete) —
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 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
Attachment #8912922 - Flags: review?(jdescottes) → review-
Attached patch 9-27-2.patch (obsolete) — Splinter Review
Attachment #8912922 - Attachment is obsolete: true
Attachment #8913209 - Attachment is obsolete: true
Attachment #8913377 - Flags: review?(jdescottes)
Attached patch 9-27-3.patch (obsolete) — Splinter Review
Attachment #8913377 - Attachment is obsolete: true
Attachment #8913377 - Flags: review?(jdescottes)
Attachment #8913437 - Flags: review?(jdescottes)
Attached patch 9-27-4.patch (obsolete) — Splinter Review
Attachment #8913437 - Attachment is obsolete: true
Attachment #8913437 - Flags: review?(jdescottes)
Attachment #8913470 - Flags: review?(jdescottes)
Attached patch 9-27-5.patch (obsolete) — Splinter Review
i forgot if 4 is up to date
Attachment #8913470 - Attachment is obsolete: true
Attachment #8913470 - Flags: review?(jdescottes)
Attachment #8913546 - Flags: review?(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.
Attachment #8913546 - Flags: review?(jdescottes) → review-
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();
Attached patch 9-27-6.patch (obsolete) — Splinter Review
Attachment #8913546 - Attachment is obsolete: true
Attachment #8913674 - Flags: review?(jdescottes)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c7fbe3f779
Update debugger frontend (9/27/2017). r=jdescottes
Attachment #8913674 - Flags: review?(jdescottes) → review+
Attached patch 9-29-1.patchSplinter Review
Attachment #8913674 - Attachment is obsolete: true
Attachment #8913808 - Flags: review?(jdescottes)
https://hg.mozilla.org/mozilla-central/rev/f4c7fbe3f779
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8913808 [details] [diff] [review]
9-29-1.patch

Moved to Bug 1404837
Attachment #8913808 - Flags: review?(jdescottes)
Depends on: 1405140
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?
(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.
(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.
(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.
Flags: needinfo?(jlaster)
Blocks: 1400308
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
Flags: needinfo?(jlaster)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.