Closed Bug 1407840 Opened 4 years ago Closed 4 years ago

Update Debugger frontend (10/11/2017).

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 10 obsolete files)

No description provided.
Attached patch patch-10-11-3.patch (obsolete) — Splinter Review
This patch includes a fix for the browser content toolbox. It's also a larger patch because of the additional SVGs that are being included in theme/images.
Attachment #8917659 - Flags: review?(jdescottes)
As I mentioned yesterday: you're hit with duplicates issue:

[task 2017-10-12T03:41:27.952Z] 03:41:27     INFO -  ERROR: The following duplicated files are not allowed:
[task 2017-10-12T03:41:27.952Z] 03:41:27     INFO -  browser/chrome/devtools/skin/images/debugger/refresh.svg
[task 2017-10-12T03:41:27.952Z] 03:41:27     INFO -  browser/chrome/devtools/skin/images/reload.svg
[task 2017-10-12T03:41:27.952Z] 03:41:27     INFO -  browser/chrome/devtools/skin/images/debugger/sad-face.svg
[task 2017-10-12T03:41:27.953Z] 03:41:27     INFO -  browser/chrome/devtools/skin/images/sad-face.svg

http://searchfox.org/mozilla-central/source/browser/installer/allowed-dupes.mn

(you can test locally by doing a ./mach package)
Attachment #8917659 - Flags: review?(jdescottes) → review-
Attached patch patch-10-11-4.patch (obsolete) — Splinter Review
Attachment #8917659 - Attachment is obsolete: true
Attachment #8917826 - Flags: review?(jdescottes)
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8917826 [details] [diff] [review]
patch-10-11-4.patch

LGTM, thanks! Browser Content Toolbox is fixed.

Small regression with setting breakpoints on lines that are right before disabled lines, but that's being tracked already.
Attachment #8917826 - Flags: review?(jdescottes) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26fbc62c177
Update Debugger frontend (10-11). r=jdescottes
Assignee: nobody → jlaster
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
Backed out for failing devtools' browser/base/content/test/static/browser_all_files_referenced.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a84dcc0d240ef0537e6f0dcfd562fb49991c11d

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d26fbc62c1777b0f754beb0bd1b599f35fe379de&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=136540105&repo=mozilla-inbound

TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 50, expected 0
TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://devtools/skin/images/debugger/angle-brackets.svg -
Flags: needinfo?(jlaster)
Attached patch patch-10-12-4.patch (obsolete) — Splinter Review
Flags: needinfo?(jlaster)
Attachment #8917948 - Flags: review?(jdescottes)
Attached patch patch-10-12-4-2.patch (obsolete) — Splinter Review
Attachment #8917826 - Attachment is obsolete: true
Attachment #8917948 - Attachment is obsolete: true
Attachment #8917948 - Flags: review?(jdescottes)
Attachment #8917949 - Flags: review?(jdescottes)
Comment on attachment 8917949 [details] [diff] [review]
patch-10-12-4-2.patch

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

Fixes the issue about unreferenced files.

As discussed, when bundle updates come with unusual changes we should have broader try runs, eg: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a46c1dc04bfa5fa920cb458aad5baaadf34309f

try: -b o -p linux64 -u mochitests -t none
Attachment #8917949 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #12)
> Comment on attachment 8917949 [details] [diff] [review]
> patch-10-12-4-2.patch
> 
> Review of attachment 8917949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fixes the issue about unreferenced files.
> 
> As discussed, when bundle updates come with unusual changes we should have
> broader try runs, eg:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6a46c1dc04bfa5fa920cb458aad5baaadf34309f
> 
> try: -b o -p linux64 -u mochitests -t none

Oh and even in the regular case we should use linux64 and not linux!

try: -b do -p linux64 -u mochitest-dt,mochitest-e10s-devtools-chrome,mochitest-o -t none --artifact

The test browser_all_files_referenced.js for instance only runs in linux64. I think several tests are linux64 only, because they can't fail on linux32 if they pass on linux64. So if we use linux64 we'll also get better try runs.

(and actually in this case, just devtools tests would have been enough)
Keywords: checkin-needed
Attached patch patch-10-12-5.patch (obsolete) — Splinter Review
Attachment #8917949 - Attachment is obsolete: true
Attachment #8918078 - Flags: review?(jdescottes)
Attached patch patch-10-12-9.patch (obsolete) — Splinter Review
Attachment #8918132 - Flags: review?(jlaster)
Attachment #8918132 - Attachment is obsolete: true
Attachment #8918132 - Flags: review?(jlaster)
Attached patch patch-10-12-9.patch (obsolete) — Splinter Review
Attachment #8918133 - Flags: review?(jlaster)
Attachment #8918078 - Attachment is obsolete: true
Attachment #8918078 - Flags: review?(jdescottes)
Attached patch patch-10-12-9.patch (obsolete) — Splinter Review
Attachment #8918134 - Flags: review?(jlaster)
Attachment #8918133 - Attachment is obsolete: true
Attachment #8918133 - Flags: review?(jlaster)
Attachment #8918134 - Attachment is obsolete: true
Attachment #8918134 - Flags: review?(jlaster)
Attached patch patch-10-12-9.patch (obsolete) — Splinter Review
Attachment #8918137 - Flags: review?(jlaster)
Attachment #8918137 - Attachment is obsolete: true
Attachment #8918137 - Flags: review?(jlaster)
Attached patch patch-10-12-10.patch (obsolete) — Splinter Review
Attachment #8918140 - Flags: review?(jdescottes)
Comment on attachment 8918140 [details] [diff] [review]
patch-10-12-10.patch

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

LGTM, try is green.

Will amend the commit message: should just be r=jdescottes
Attachment #8918140 - Flags: review?(jdescottes) → review+
fixed reviewer in commit message
Attachment #8918140 - Attachment is obsolete: true
Attachment #8918195 - Flags: review+
(for the record the L10N job is busted on the try run but a quick look at the current state of this job on try makes me think this is a more global issue. Plus, it fails at repack on a browser/ image, can't see why our patch would have any impact there).
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e76eb11f034
Update Debugger frontend (10-12). r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e76eb11f034
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Duplicate of this bug: 1405140
Duplicate of this bug: 1408012
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.