1391719 - Update Debugger frontend (8/16/2017).

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Comment 1

[Jason Laster [:jlast]]

Attachment: 8-16.patch

Comment 2

[Jason Laster [:jlast]]

Attachment: 8-16-2.patch

Comment 3

[Julian Descottes [:jdescottes]]

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 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1391719 - update debugger frontend (8/16/2017) v=0.12.

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

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1391719 - add lodash library

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8899029 [details]
Bug 1391719 - update debugger frontend (8/16/2017) v=0.12.

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

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1391719 - update debugger bundles

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8899029 [details]
Bug 1391719 - update debugger frontend (8/16/2017) v=0.12.

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

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8899031 [details]
Bug 1391719 - add lodash library

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

Comment 10

[Julian Descottes [:jdescottes]]

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)

Comment 11

[Jason Laster [:jlast]]

Attachment: 816-lod.patch

Comment 12

[Jason Laster [:jlast]]

Attachment: 816-files.patch

Comment 13

[Jason Laster [:jlast]]

Attachment: 816-bundles.patch

Comment 14

[Jason Laster [:jlast]]

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

Comment 15

[Jason Laster [:jlast]]

Attachment: 816-files2.patch

Comment 16

[Julian Descottes [: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 17

[Julian Descottes [:jdescottes]]

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

Comment 18

[Julian Descottes [:jdescottes]]

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!

Comment 19

[Julian Descottes [:jdescottes]]

Attachment: all-source-tabs-overflow-visible.png

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: empty-outline-view.png

Comment 21

[Julian Descottes [:jdescottes]]

Attachment: breakpoints-ui-glitch.gif

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.

Comment 22

[Jason Laster [:jlast]]

Attachment: 8162-files.patch

Comment 23

[Jason Laster [:jlast]]

Attachment: 8162-bundles.patch

Comment 24

[Jason Laster [:jlast]]

Attachment: 8162-files.patch

Comment 25

[Jason Laster [:jlast]]

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

Comment 26

[Julian Descottes [:jdescottes]]

8162-files.patch still doesn't apply cleanly, we need a rebased patch before landing.

Comment 27

[Jason Laster [:jlast]]

Attachment: 8162-files2.patch

Comment 28

[Jason Laster [:jlast]]

Attachment: 8162-files3.patch

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8899522 [details] [diff] [review]
8162-files3.patch

This one is good :)

Comment 30

[Jason Laster [:jlast]]

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

Comment 31

[Jason Laster [:jlast]]

a new try run with a fix for an intermittent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b823000ffaf9a58269617fb9a9ac798b9716e1b6

Comment 32

[Jason Laster [:jlast]]

Attachment: 8162-files4.patch

Comment 33

[Jason Laster [:jlast]]

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

Comment 34

[Jason Laster [:jlast]]

Attachment: 8162-files5.patch

Comment 35

[Jason Laster [:jlast]]

Attachment: 8162-files7.patch

Comment 36

[Jason Laster [:jlast]]

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

Comment 37

[Jason Laster [:jlast]]

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

^ has the windows skips

Comment 38

[Pulsebot]

Pushed by [e-mail]:
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

Comment 39

[Julian Descottes [:jdescottes]]

Attachment: 8162-files8.patch

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?

Comment 41

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

https://hg.mozilla.org/mozilla-central/rev/46e8123e8f52
https://hg.mozilla.org/mozilla-central/rev/42be1ed6623d
https://hg.mozilla.org/mozilla-central/rev/d97a8d7abdd6

Comment 42

[Francesco Lodolo [:flod]]

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.