Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P2
Created attachment 8855924 [details] [diff] [review] bundle-4-7.patch
Created attachment 8855928 [details] [diff] [review] bundle-4-7-1.patch
looks like there's a pretty innocent bug with console linking. I'll fix this and re-share.
Created attachment 8856062 [details] [diff] [review] bundle-4-7-2.patch
Created attachment 8856063 [details] [diff] [review] bundle-4-7-2.patch
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?
> 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`.
Created attachment 8858013 [details] [diff] [review] bundle-4-7-3.patch
Created attachment 8858015 [details] [diff] [review] bundle-4-7-4.patch changed a locale key
Created attachment 8858049 [details] [diff] [review] bundle-4-7-5.patch rebased to get the uptodate content.
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+
Created attachment 8858131 [details] [diff] [review] bundle-4-7-6.patch small changes
Attachment #8858049 - Attachment is obsolete: true
This has conflicts with inbound and the bug number in the commit message isn't this bug.
Created attachment 8859072 [details] [diff] [review] bug1354672.amended.patch 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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/26825d7a225d update debugger frontend 04/18/17;r=jdescottes
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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?
> 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 ?
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.
OK I'll ask for a backout and will wait for Jason to discuss out the proper spelling for blackboxing
backed out in https://hg.mozilla.org/mozilla-central/rev/f6be27f5457ffd75b3763db09a139e24bc2155dc on request
Status: RESOLVED → REOPENED
status-firefox55: fixed → affected
Resolution: FIXED → ---
Created attachment 8860012 [details] [diff] [review] bug1354672.l10n.fixes.patch 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.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddbf12eaee0 update debugger frontend 04/18/17;r=jdescottes
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago → 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
(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?
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.
You need to log in before you can comment on or make changes to this bug.