Closed
Bug 1391719
Opened 7 years ago
Closed 7 years ago
Update Debugger frontend (8/16/2017).
Categories
(DevTools :: Debugger, enhancement, P3)
DevTools
Debugger
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jlast, Assigned: jlast)
References
Details
Attachments
(6 files, 15 obsolete files)
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 |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8898935 -
Flags: review?(jdescottes)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8898935 -
Attachment is obsolete: true
Attachment #8898935 -
Flags: review?(jdescottes)
Attachment #8898936 -
Flags: review?(jdescottes)
Updated•7 years ago
|
Assignee: nobody → jlaster
Priority: -- → P3
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 3•7 years ago
|
||
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•7 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•7 years ago
|
||
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•7 years ago
|
||
Attachment #8899029 -
Attachment is obsolete: true
Attachment #8899029 -
Flags: review?(jdescottes)
Attachment #8899176 -
Flags: review?(jdescottes)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8899033 -
Attachment is obsolete: true
Attachment #8899177 -
Flags: review?(jdescottes)
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3b587bd1ebd309d6fe11a42d74ae524b06af71
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8899176 -
Attachment is obsolete: true
Attachment #8899176 -
Flags: review?(jdescottes)
Attachment #8899178 -
Flags: review?(jdescottes)
Comment 16•7 years ago
|
||
(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•7 years ago
|
||
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 18•7 years ago
|
||
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+
Comment 19•7 years ago
|
||
Comment 20•7 years ago
|
||
Comment 21•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8899175 -
Flags: review?(gerv) → review+
Assignee | ||
Comment 22•7 years ago
|
||
Attachment #8899178 -
Attachment is obsolete: true
Attachment #8899476 -
Flags: review?(jdescottes)
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8899177 -
Attachment is obsolete: true
Attachment #8899477 -
Flags: review?(jdescottes)
Assignee | ||
Comment 24•7 years ago
|
||
Attachment #8899476 -
Attachment is obsolete: true
Attachment #8899476 -
Flags: review?(jdescottes)
Assignee | ||
Comment 25•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b1f0dadb56121220bdda057154fbf5cde33990
Updated•7 years ago
|
Attachment #8899481 -
Flags: review+
Updated•7 years ago
|
Attachment #8899477 -
Flags: review?(jdescottes) → review+
Comment 26•7 years ago
|
||
8162-files.patch still doesn't apply cleanly, we need a rebased patch before landing.
Assignee | ||
Comment 27•7 years ago
|
||
Attachment #8899481 -
Attachment is obsolete: true
Assignee | ||
Comment 28•7 years ago
|
||
Attachment #8899518 -
Attachment is obsolete: true
Comment 29•7 years ago
|
||
Comment on attachment 8899522 [details] [diff] [review] 8162-files3.patch This one is good :)
Attachment #8899522 -
Flags: review+
Assignee | ||
Comment 30•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08ab0874008b4f3661c143af964be5bfcf938633
Assignee | ||
Comment 31•7 years ago
|
||
a new try run with a fix for an intermittent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b823000ffaf9a58269617fb9a9ac798b9716e1b6
Assignee | ||
Comment 32•7 years ago
|
||
Attachment #8899522 -
Attachment is obsolete: true
Assignee | ||
Comment 33•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bc54636cfc164a18360df252f98f84de64c363e
Assignee | ||
Comment 34•7 years ago
|
||
Attachment #8899889 -
Attachment is obsolete: true
Assignee | ||
Comment 35•7 years ago
|
||
Attachment #8899946 -
Attachment is obsolete: true
Assignee | ||
Comment 36•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b09a85573b46be7bbeb268355e0ffb4313117757
Assignee | ||
Comment 37•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e766660031e2ae41d24765bcaeb07bea1682bd5 ^ has the windows skips
Comment 38•7 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
Comment 39•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8899962 -
Attachment is obsolete: true
Comment 41•7 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: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 42•7 years ago
|
||
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•6 years ago
|
Flags: needinfo?(jlaster)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•