Update Debugger Frontend (12-5)

RESOLVED FIXED in Firefox 59

Status

P3
enhancement
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: yulia, Assigned: yulia)

Tracking

(Blocks: 1 bug)

58 Branch
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(3 attachments, 10 obsolete attachments)

59 bytes, text/x-review-board-request
jdescottes
: review+
Details
1.69 MB, image/gif
Details
22.37 KB, patch
Details | Diff | Splinter Review
Update Debugger Frontend December 5th, 2017
Attachment #8934529 - Flags: review?(jlaster)
Attachment #8934529 - Flags: review?(jdescottes)
Assignee: nobody → ystartsev
Attachment #8934529 - Attachment is obsolete: true
Attachment #8934529 - Flags: review?(jlaster)
Attachment #8934529 - Flags: review?(jdescottes)
Attachment #8934531 - Flags: review?(jlaster)
Attachment #8934531 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
Blocks: 1412334
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #8934531 - Attachment is obsolete: true
Attachment #8934531 - Flags: review?(jlaster)
Attachment #8934531 - Flags: review?(jdescottes)

Comment 6

a year ago
mozreview-review
Comment on attachment 8934533 [details]
Bug 1423158 - Update Debugger Frontend (12-5)

https://reviewboard.mozilla.org/r/205440/#review211046

Code wise this looks OK, but the changeset is missing the new images as well as the new test files.
Attachment #8934533 - Flags: review?(jdescottes)
Attachment #8934533 - Attachment is obsolete: true
Attachment #8934564 - Flags: review?(jlaster)
Attachment #8934564 - Flags: review?(jdescottes)
Try failures worth looking into:

- devtools/client/framework/test/browser_source_map-01.js and devtools/client/framework/test/browser_source_map-absolute.js on OPT builds
- devtools/client/debugger/new/test/mochitest/browser_dbg-tabs.js on DEBUG builds with:
  | A promise chain failed to handle a rejection: _selectors.getSource(...) is undefined
I ran both tests with --verify but wasn't able to get them to fail. What is the best way to investigate this?
Some regressions spotted when doing quick manual tests:

when closing and reopening the debugger:
- source tab is blank
- breakpoints are persisted but not visible in the gutter

See attached GIF
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment on attachment 8935336 [details]
test

The line that makes splinter review fail is the removal of 

https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/devtools/client/debugger/new/pretty-print-worker.js#3130

I guess we have no choice but to go through mozreview here.
Attachment #8935336 - Attachment is obsolete: true
Attachment #8935336 - Attachment is patch: false
Comment hidden (mozreview-request)

Comment 19

a year ago
mozreview-review
Comment on attachment 8935340 [details]
Bug 1423158 - Update Debugger Frontend (12-5).

https://reviewboard.mozilla.org/r/206234/#review211816

Looks good to me, some license header missing but be addressed after the release. 
Found some regressions, commented on bugzilla, feel free to open separate issues to track them (unless already tracked).

Should be ready to land once the sourcemap intermittent is fixed.

For the debugger tests being intermittent in debug, probably should add skip-if = debug for them and log separate issues to investigate them.

::: devtools/client/debugger/new/test/mochitest/examples/doc-sourcemaps3.html:1
(Diff revision 1)
> +<!DOCTYPE html>

license header

::: devtools/client/themes/images/debugger/react.svg:1
(Diff revision 1)
> +<?xml version="1.0" encoding="utf-8"?>

Missing license header
Attachment #8935340 - Flags: review?(jdescottes) → review+
Posted patch fix intermittent (obsolete) — Splinter Review
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
mozreview-review-reply
Comment on attachment 8935340 [details]
Bug 1423158 - Update Debugger Frontend (12-5).

https://reviewboard.mozilla.org/r/206234/#review211816

> license header

do we need a license header on test files? I noticed we don't have it on all of our example test files, should we add it to everything?

> Missing license header

We are missing license headers for a few of our svgs, i will create an issue for all of them
Attachment #8935351 - Attachment is obsolete: true
Attachment #8934564 - Attachment is obsolete: true
Attachment #8934564 - Flags: review?(jlaster)
Attachment #8934564 - Flags: review?(jdescottes)
Attachment #8935340 - Attachment is obsolete: true

Comment 25

a year ago
mozreview-review
Comment on attachment 8935363 [details]
Bug 1423158 - Update Debugger Frontend (12-5) fix intermittent

https://reviewboard.mozilla.org/r/206254/#review211862

LGTM, maybe a slightly more helpful commit message could be nice :)

::: commit-message-f532f:1
(Diff revision 1)
> +Bug 1423158 - Update Debugger Frontend (12-5) fix intermittent r=jdescottes

nit commit message could mention the fix "swallow error for sourcemap failure if toolbox is closed"

::: commit-message-f532f:1
(Diff revision 1)
> +Bug 1423158 - Update Debugger Frontend (12-5) fix intermittent r=jdescottes

nit: commit message could mention the fix "swallow error for sourcemap failure if toolbox is closed"
Attachment #8935363 - Flags: review?(jdescottes) → review+

Comment 26

a year ago
mozreview-review
Comment on attachment 8934533 [details]
Bug 1423158 - Update Debugger Frontend (12-5)

https://reviewboard.mozilla.org/r/205440/#review211864
Attachment #8934533 - Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request)
Attachment #8935363 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Need to synchronize the change from Bug 1408949 which updated browser_dbg-toggling-tools.js.
Comment hidden (mozreview-request)
try run looks good!

Comment 41

a year ago
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7b279271a15
Update Debugger Frontend (12-5) r=jdescottes

Comment 42

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7b279271a15
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1428586
Attachment #8935333 - Attachment is obsolete: false

Updated

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