Closed
Bug 1354672
Opened 7 years ago
Closed 7 years ago
Update Debugger frontend (4/7/2017)
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jlast, Assigned: jlast)
References
Details
Attachments
(1 file, 9 obsolete files)
6.26 MB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
Regular update.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8855924 -
Flags: review?(jdescottes)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8855924 -
Attachment is obsolete: true
Attachment #8855924 -
Flags: review?(jdescottes)
Attachment #8855928 -
Flags: review?(jdescottes)
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11433c282f886f167b68e2929f8935c344f320f7
Assignee | ||
Comment 4•7 years ago
|
||
looks like there's a pretty innocent bug with console linking. I'll fix this and re-share.
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8855928 -
Attachment is obsolete: true
Attachment #8855928 -
Flags: review?(jdescottes)
Attachment #8856062 -
Flags: review?(jdescottes)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8856062 -
Attachment is obsolete: true
Attachment #8856062 -
Flags: review?(jdescottes)
Attachment #8856063 -
Flags: review?(jdescottes)
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=458c7a2ed88f10e06983b77f084e24d2350a63df
Comment 8•7 years ago
|
||
Comment on attachment 8856063 [details] [diff] [review] bundle-4-7-2.patch Review of attachment 8856063 [details] [diff] [review]: ----------------------------------------------------------------- From the comments below: - remove source-map-worker from the source control - revert jumpToMappedLocation to jumpToMappedLocation1 (unless this has been agreed to with someone from l10n?) The rest are mostly nits, up to you to take them into account. Clearing the review flag since we still have a significant number of test failures on try. Can you update the changeset summary with r=jdescottes as well? ::: devtools/client/debugger/new/moz.build @@ -10,2 @@ > 'pretty-print-worker.js', > - 'source-map-worker.js' source-map-worker should be deleted from the source control ::: devtools/client/locales/en-US/debugger.properties @@ +279,2 @@ > # for navigating to a source mapped location > +editor.jumpToMappedLocation=Jump to %S location Why is this string changing again :) ? I feel like every time we get a debugger update, this comes back. @@ +352,5 @@ > +# LOCALIZATION NOTE (sourceFooter.unblackbox): Tooltip text associated > +# with the black box button > +sourceFooter.unblackbox=Unblackbox Source > + > +# LOCALIZATION NOTE (sourceFooter.blackbox.accessKey): Access key to black box We are using accesskey (all lowercase) everywhere in this file. @@ +525,5 @@ > # and its real name (if available). > functionSearchSeparatorLabel=← > + > +# LOCALIZATION NOTE(symbolSearch.search.functionsPlaceholder): The placeholder > +# text displayed when the user searches for funcions in a file typo: funcions -> functions @@ +534,5 @@ > +symbolSearch.search.variablesPlaceholder=Search variables… > + > +# LOCALIZATION NOTE(symbolSearch.search.key): The shortcut (cmd+shift+o) for > +# searching for a function or variable > +symbolSearch.search.key=O Not sure how this has been done so far for the debugger, but most of the other panels have the full shortcut in the properties file (for instance inspector.searchHTML.key=CmdOrCtrl+F) The reason behind this is that when you localize a shortcut "cmd+shift+O" to use another letter, the cmd+shift combo might no longer be appropriate. @@ +616,5 @@ > # listener breakpoint set > whyPaused.other=Debugger paused > + > +# LOCALIZATION NOTE (ctrl): The text that is used > +# keyboard shortcuts that use the control key The localization is probably missing a few words to be understandable?
Attachment #8856063 -
Flags: review?(jdescottes)
Assignee | ||
Comment 9•7 years ago
|
||
> editor.jumpToMappedLocation
This is a new string that's not in the old debugger so I believe it is okay to not have the `1`.
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8856063 -
Attachment is obsolete: true
Attachment #8858013 -
Flags: review?(jdescottes)
Assignee | ||
Comment 11•7 years ago
|
||
changed a locale key
Attachment #8858013 -
Attachment is obsolete: true
Attachment #8858013 -
Flags: review?(jdescottes)
Attachment #8858015 -
Flags: review?(jdescottes)
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=703fb54aa0346b005fad87c92d634e92c3f208c7
Assignee | ||
Comment 13•7 years ago
|
||
rebased to get the uptodate content.
Attachment #8858015 -
Attachment is obsolete: true
Attachment #8858015 -
Flags: review?(jdescottes)
Attachment #8858049 -
Flags: review?(jdescottes)
Comment 14•7 years ago
|
||
Comment on attachment 8858049 [details] [diff] [review] bundle-4-7-5.patch Review of attachment 8858049 [details] [diff] [review]: ----------------------------------------------------------------- R+, try seems good, _please_ update the string key before landing :) Feel free to ping me if you want to discuss it! ::: devtools/client/debugger/new/test/mochitest/browser.ini @@ +60,5 @@ > [browser_dbg-pretty-print-paused.js] > [browser_dbg-searching.js] > skip-if = true > [browser_dbg-sourcemaps.js] > +skip-if = true Just want to draw your attention on this since you mentioned sourcemaps tests being skipped unintentionally: are those skips ok with you? ::: devtools/client/debugger/new/test/mochitest/browser_dbg-expressions.js @@ -1,2 @@ > -/* Any copyright is dedicated to the Public Domain. > - * http://creativecommons.org/publicdomain/zero/1.0/ */ license header? ::: devtools/client/debugger/new/test/mochitest/browser_dbg-pretty-print.js @@ +null,0 @@ Is it an issue to keep the license header? ::: devtools/client/locales/en-US/debugger.properties @@ +286,2 @@ > # for navigating to a source mapped location > +editor.jumpToMappedLocation=Jump to %S location The issue is that this key existed in mozilla-central prior to Bug 1333602 with the value "Jump to %s location" (lowercase s instead of uppercase). This means the string will appear as already translated for localizers who picked it up at that time, and will probably have the issue with %s (which I guess doesn't allow for the the string to be used as a template). We need to revert this key to editor.jumpToMappedLocation1 ::: devtools/client/preferences/debugger.js @@ +null,0 @@ Is it an issue to keep the license header? Maybe since it's part of files shipped as the "debugger library", it's a bit less relevant? I don't think it should block the update, but maybe check with jryans or gerv.
Attachment #8858049 -
Flags: review?(jdescottes) → review+
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0d3ab4ccb43fc2d3397dc0c611166f7dca8a82&selectedJob=91389486
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=036a35af227606b743aae9c8358286424240d3d8
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 18•7 years ago
|
||
This has conflicts with inbound and the bug number in the commit message isn't this bug.
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Jason, this is the previous patch manually amended to : - be compatible with inbound - include changes from Bug 1356569 & Bug 1355161 - have the correct changeset summary (bug number + reviewer) This should be ready to land, but I'm not sure if you want to run with this bundle or with something newer? Feel free to checkin needed or to replace with another patch.
Attachment #8858131 -
Attachment is obsolete: true
Flags: needinfo?(jlaster)
Comment 20•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/26825d7a225d update debugger frontend 04/18/17;r=jdescottes
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/26825d7a225d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 22•7 years ago
|
||
Nothing is badly broken this time, mostly an FYI. I've asked before to ping me before dumping changes to strings in mozilla-central https://hg.mozilla.org/mozilla-central/diff/26825d7a225d/devtools/client/locales/en-US/debugger.properties We have localizers on Nightly now because of Dawn. As harsh as it sounds, I'm going to start asking sheriffs to back out if badly broken strings land, because you're going to block all the rest of the localization for 100 teams. Tools work against a clone of mozilla-central that is a few days behind the actual repo, broken strings would prevent me from updating this clone. -editor.jumpToMappedLocation1=Jump to %S location +editor.jumpToMappedLocation=Jump to %s location Any specific reason for this change, going back to lowercase %s? The spelling of black box is now completely inconsistent in the file: it was spelled with a space before, it's "blackbox" now (but "black box" in comments). Is that known and wanted?
Flags: needinfo?(jdescottes)
Comment 23•7 years ago
|
||
> Any specific reason for this change, going back to lowercase %s?
No there's no good reason for this change. I thought we were clear during the review when I said "We need to revert this key to editor.jumpToMappedLocation1" and I didn't check the next patch in detials.
Can I push a follow up to revert this or do we need to back out ?
Flags: needinfo?(jlaster)
Flags: needinfo?(jdescottes)
Flags: needinfo?(francesco.lodolo)
Comment 24•7 years ago
|
||
jumping in for flod, as he's traveling. The question on backout vs follow-up depends on whether we have an answer for the blackbox yet? If we do, I think landing a follow-up is a good idea, if we don't, I suggest to back out and reland once that's settled.
Flags: needinfo?(francesco.lodolo)
Comment 25•7 years ago
|
||
OK I'll ask for a backout and will wait for Jason to discuss out the proper spelling for blackboxing
Comment 26•7 years ago
|
||
backed out in https://hg.mozilla.org/mozilla-central/rev/f6be27f5457ffd75b3763db09a139e24bc2155dc on request
Comment 27•7 years ago
|
||
For the record, the existing "black box" string (using a space) was used by the old debugger. Even though there was inconsistency in the file, the UI of the new debugger was actually consistently using "blackbox". After discussing with Jason "blackbox" (single word) seems to be the preferred option, so let's go with that. The editor.jumpToMappedLocation1 key and values have been reverted.
Attachment #8859072 -
Attachment is obsolete: true
Attachment #8860012 -
Flags: review+
Comment 28•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddbf12eaee0 update debugger frontend 04/18/17;r=jdescottes
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ddbf12eaee0
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 30•7 years ago
|
||
Screenshot changes to review: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=e9a5d4f62461ee0db07a41f59b73163ec106bc3e&newProject=mozilla-central&newRev=20dff607fb88ee69135a280bbb7f32df75a86237
Comment 31•7 years ago
|
||
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #30) > Screenshot changes to review: > https://screenshots.mattn.ca/compare/?oldProject=mozilla- > central&oldRev=e9a5d4f62461ee0db07a41f59b73163ec106bc3e&newProject=mozilla- > central&newRev=20dff607fb88ee69135a280bbb7f32df75a86237 Thanks for the heads up Matthew, I think both changes are expected: - text "this page has no sources" is displayed when the source pane is empty - default state for pausing on exceptions is disabled (before, the default was on "pause on all exceptions" with a clean profile) I'm not sure if I have to do something to validate the new images as the new correct baseline?
Flags: needinfo?(MattN+bmo)
Comment 32•7 years ago
|
||
Thanks for reviewing. No, we don't work like reftests with reference images or a baseline yet. Every change gets emailed to the https://www.mozilla.org/en-US/about/forums/#dev-ui-alerts list and then I or someone else tries to figure out whether the change was intentional.
Flags: needinfo?(MattN+bmo)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•