Closed Bug 1399673 Opened 4 years ago Closed 4 years ago

Update the debugger frontend (9-13)

Categories

(DevTools :: Debugger, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 6 obsolete files)

No description provided.
Attached patch 9-13.patch (obsolete) — Splinter Review
Attachment #8907868 - Flags: review?(jdescottes)
Attached patch 9-13-2.patch (obsolete) — Splinter Review
Attachment #8907868 - Attachment is obsolete: true
Attachment #8907868 - Flags: review?(jdescottes)
Attachment #8907870 - Flags: review?(jdescottes)
Attached patch 9-13-3.patch (obsolete) — Splinter Review
Attachment #8907870 - Attachment is obsolete: true
Attachment #8907870 - Flags: review?(jdescottes)
Attachment #8907881 - Flags: review?(jdescottes)
Attached patch 9-13-4.patch (obsolete) — Splinter Review
Attachment #8907881 - Attachment is obsolete: true
Attachment #8907881 - Flags: review?(jdescottes)
Attachment #8907915 - Flags: review?(jdescottes)
Failing mochitests: 

- devtools/client/framework/test/browser_source_map-01.js
- devtools/client/framework/test/browser_source_map-absolute.js

Stack trace:

at chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-01.js:45 - TypeError: newLoc is null
Stack trace:
    checkLoc1@chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-01.js:45:3
    @chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-01.js:30:3
    Tester_execTest@chrome://mochikit/content/browser-test.js:794:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:694:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:794:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:694:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
I guess we are missing a sourcemap bundle update in m-c mentioned here:

https://github.com/devtools-html/devtools-core/pull/672
Comment on attachment 8907915 [details] [diff] [review]
9-13-4.patch

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

Looks good to me, didn't spot new regressions while testing. Quickly tested function copy, seems to work fine too.

As discussed we need to update some sourcemap tests in order to get this to land. 
I'll let you update the patch and flag Tom for a review of the sourcemap related changes.
Attachment #8907915 - Flags: review?(jdescottes) → review+
Attached patch 9-13-5.patch (obsolete) — Splinter Review
Attachment #8907915 - Attachment is obsolete: true
Attachment #8908168 - Flags: review?(jdescottes)
Comment on attachment 8908168 [details] [diff] [review]
9-13-5.patch

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

Two files left over, please remove them.
Still one test failure, I'm confident you can come up with a fix. Forwarding the r? to Tom.

Tom: I reviewed the debugger related change. Have a look at the sourcemap changes (mostly test fixes in browser_source_map-absolute.js & browser_source_map-01.js)

::: devtools/client/debugger/test/mochitest/browser_dbg_source-maps-01.js.orig
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Remove this file

::: devtools/client/shared/source-map/worker.js.orig
@@ +1,1 @@
> +(function webpackUniversalModuleDefinition(root, factory) {

Remove this file.
Attachment #8908168 - Flags: review?(jdescottes) → review?(ttromey)
Comment on attachment 8908168 [details] [diff] [review]
9-13-5.patch

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

Thank you.  Splinter review, old school!

Julian, I only looked at the two files you asked me to.  Jason and I discussed these.  The lack of a column in these tests was incidental, and doesn't affect what the tests are intended to exercise.  So this change is ok.
Attachment #8908168 - Flags: review?(ttromey) → review+
Attached patch 9-13-6.patch (obsolete) — Splinter Review
Attachment #8908168 - Attachment is obsolete: true
Attachment #8908269 - Flags: review?(jdescottes)
Comment on attachment 8908269 [details] [diff] [review]
9-13-6.patch

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

R+ with green try.

Could you update the commit message to mention that this also updates source-maps to v0.13.0 ?
Attachment #8908269 - Flags: review?(jdescottes) → review+
Attached patch 9-13-7.patchSplinter Review
Attachment #8908269 - Attachment is obsolete: true
Whiteboard: checkin-needed
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8939119c17
Update Debugger Frontend and upgrade Source Map Worker to v0.13.0. r=jdescottes
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ee8939119c17
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
Assignee: nobody → jlaster
You need to log in before you can comment on or make changes to this bug.