Update Debugger frontend (8/16/2017).

RESOLVED FIXED in Firefox 57

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jlast, Assigned: jlast)

Tracking

unspecified
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(6 attachments, 15 obsolete attachments)

545.58 KB, patch
gerv
: review+
Details | Diff | Splinter Review
14.16 KB, image/png
Details
81.66 KB, image/png
Details
2.89 MB, image/gif
Details
5.50 MB, patch
jdescottes
: review+
Details | Diff | Splinter Review
45.58 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
Assignee

Description

2 years ago
No description provided.
Assignee

Comment 1

2 years ago
Posted patch 8-16.patch (obsolete) — Splinter Review
Attachment #8898935 - Flags: review?(jdescottes)
Assignee

Comment 2

2 years ago
Posted patch 8-16-2.patch (obsolete) — Splinter Review
Attachment #8898935 - Attachment is obsolete: true
Attachment #8898935 - Flags: review?(jdescottes)
Attachment #8898936 - Flags: review?(jdescottes)
Assignee: nobody → jlaster
Priority: -- → P3
Status: NEW → ASSIGNED
Patch doesn't apply cleanly on latest central:

> applying https://bug1391719.bmoattachments.org/attachment.cgi?id=8898936
> patching file devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-reloading.js
> Hunk #1 FAILED at 18
> 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-> reloading.js.rej
> abort: patch failed to apply

Also it looks like splinter review can't handle it because it's too big. I'll try mozreview.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8899031 [details]
Bug 1391719 - add lodash library

https://reviewboard.mozilla.org/r/170350/#review175574

Could you resubmit this by splitting your patches the same way I did?
For this particular patch (lodash) it would be nice to have a descriptive commit message explaining why this new library is needed.

::: devtools/client/shared/vendor/lodash.js:1
(Diff revision 2)
> +debugger;

Is this debugger statement really part of the lib?

::: devtools/client/shared/vendor/lodash.js:3
(Diff revision 2)
> +debugger;
> +/**
> + * @license

This will need to be added to license.html (Bug 1296600 for an example)
Assignee

Comment 11

2 years ago
Posted patch 816-lod.patchSplinter Review
Attachment #8898936 - Attachment is obsolete: true
Attachment #8899031 - Attachment is obsolete: true
Attachment #8898936 - Flags: review?(jdescottes)
Attachment #8899175 - Flags: review?(gerv)
Assignee

Comment 12

2 years ago
Posted patch 816-files.patch (obsolete) — Splinter Review
Attachment #8899029 - Attachment is obsolete: true
Attachment #8899029 - Flags: review?(jdescottes)
Attachment #8899176 - Flags: review?(jdescottes)
Assignee

Comment 13

2 years ago
Posted patch 816-bundles.patch (obsolete) — Splinter Review
Attachment #8899033 - Attachment is obsolete: true
Attachment #8899177 - Flags: review?(jdescottes)
Assignee

Comment 15

2 years ago
Posted patch 816-files2.patch (obsolete) — Splinter Review
Attachment #8899176 - Attachment is obsolete: true
Attachment #8899176 - Flags: review?(jdescottes)
Attachment #8899178 - Flags: review?(jdescottes)
(In reply to Jason Laster [:jlast] from comment #15)
> Created attachment 8899178 [details] [diff] [review]
> 816-files2.patch

This patch still doesn't apply cleanly on the latest mozilla-central.
Comment on attachment 8899178 [details] [diff] [review]
816-files2.patch

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

Looks good to me thanks!

::: devtools/client/debugger/new/test/mochitest/browser_dbg-pretty-print-paused.js
@@ +18,3 @@
>  
> +  // this doesnt work yet
> +  // assertPausedLocation(dbg);

just highlighting this commented out code, in case you want to clean it up upstream.

::: devtools/client/debugger/new/test/mochitest/browser_dbg-pretty-print.js
@@ +15,4 @@
>    ok(ppSrc, "Pretty-printed source exists");
>  
> +  // this is not implemented yet
> +  // assertHighlightLocation(dbg, "math.min.js:formatted", 18);

same comment
Attachment #8899178 - Flags: review?(jdescottes) → review+
Comment on attachment 8899177 [details] [diff] [review]
816-bundles.patch

r+ since I didn't see any major issue when testing. Some minor UI nits I noted:
- the popup showing hidden source tabs seems to have overflow: visible
- when switching from horizontal to vertical layout, the breakpoint "icons" are shifted to the left
- outline view should have a message when no method is available in the current source

Will attach screenshots to illustrate. 

I found project search to behave a bit weirdly when querying big projects, for queries that return a lot of results. On a personal project, it takes ~30 seconds for the results to stabilize. If I go from a query on str1 to str2, results from both queries will overlap for a long time before only str2 results are displayed. But for regular queries it seems to work really well!
Attachment #8899177 - Flags: review?(jdescottes) → review+
Sorry about the blinking in the GIF! As you can see when switching layouts the breakpoints seem to shrink. After scrolling/waiting for a while they come back to a normal state.
Attachment #8899175 - Flags: review?(gerv) → review+
Assignee

Comment 22

2 years ago
Posted patch 8162-files.patch (obsolete) — Splinter Review
Attachment #8899178 - Attachment is obsolete: true
Attachment #8899476 - Flags: review?(jdescottes)
Assignee

Comment 23

2 years ago
Attachment #8899177 - Attachment is obsolete: true
Attachment #8899477 - Flags: review?(jdescottes)
Assignee

Comment 24

2 years ago
Posted patch 8162-files.patch (obsolete) — Splinter Review
Attachment #8899476 - Attachment is obsolete: true
Attachment #8899476 - Flags: review?(jdescottes)
Attachment #8899477 - Flags: review?(jdescottes) → review+
8162-files.patch still doesn't apply cleanly, we need a rebased patch before landing.
Assignee

Comment 27

2 years ago
Posted patch 8162-files2.patch (obsolete) — Splinter Review
Attachment #8899481 - Attachment is obsolete: true
Assignee

Comment 28

2 years ago
Posted patch 8162-files3.patch (obsolete) — Splinter Review
Attachment #8899518 - Attachment is obsolete: true
Comment on attachment 8899522 [details] [diff] [review]
8162-files3.patch

This one is good :)
Attachment #8899522 - Flags: review+
Assignee

Comment 32

2 years ago
Posted patch 8162-files4.patch (obsolete) — Splinter Review
Attachment #8899522 - Attachment is obsolete: true
Assignee

Comment 34

2 years ago
Posted patch 8162-files5.patch (obsolete) — Splinter Review
Attachment #8899889 - Attachment is obsolete: true
Assignee

Comment 35

2 years ago
Posted patch 8162-files7.patch (obsolete) — Splinter Review
Attachment #8899946 - Attachment is obsolete: true
Depends on: 1393121

Comment 38

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46e8123e8f52
add lodash (devtools).r=gerv
https://hg.mozilla.org/integration/mozilla-inbound/rev/42be1ed6623d
update debugger frontend (8/16/2017) v=0.12 - webpack bundle files. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97a8d7abdd6
update debugger frontend (8/16/2017) v=0.12 - tests, locales and preferences. r=jdescottes
Jason: I slightly modified the browser.ini to add a reference to the bug I just logged about fixing the tests: Bug 1393121.

Can you reapply this change on GitHub?
Flags: needinfo?(jlaster)
Attachment #8900356 - Flags: review+
Attachment #8899962 - Attachment is obsolete: true
Duplicate of this bug: 1388825

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/46e8123e8f52
https://hg.mozilla.org/mozilla-central/rev/42be1ed6623d
https://hg.mozilla.org/mozilla-central/rev/d97a8d7abdd6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Next time you touch debugger.properties, please put the access keys near the label they belong to ;-)

Also note that localization comments are not mandatory, devtools are heavily over commented at this point.
Assignee

Updated

Last year
Flags: needinfo?(jlaster)

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.