Closed Bug 1423158 Opened 6 years ago Closed 6 years ago

Update Debugger Frontend (12-5)

Categories

(DevTools :: Debugger, enhancement, P3)

58 Branch
enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: yulia, Assigned: yulia)

References

Details

Attachments

(3 files, 10 obsolete files)

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
Attached patch Update Debugger Frontend 12-5-17 (obsolete) — Splinter Review
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)
:jdescottes, :jlast There are two items to review but they are identical. something happened with splinter review.

try builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ac8e67dcefb84b0ac2091390f489a97bfc0043
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ff611d6b3fc7022493b9dfe3f2e381f2f17251
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 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 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 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+
Attached patch fix intermittent (obsolete) — Splinter Review
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 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 on attachment 8934533 [details]
Bug 1423158 - Update Debugger Frontend (12-5)

https://reviewboard.mozilla.org/r/205440/#review211864
Attachment #8934533 - Flags: review?(jdescottes) → review+
Attachment #8935363 - Attachment is obsolete: true
Need to synchronize the change from Bug 1408949 which updated browser_dbg-toggling-tools.js.
try run looks good!
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7b279271a15
Update Debugger Frontend (12-5) r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/b7b279271a15
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1428586
Attachment #8935333 - Attachment is obsolete: false
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: