Closed Bug 1354672 Opened 4 years ago Closed 4 years ago

Update Debugger frontend (4/7/2017)

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 9 obsolete files)

Regular update.
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch bundle-4-7.patch (obsolete) — Splinter Review
Attachment #8855924 - Flags: review?(jdescottes)
Attached patch bundle-4-7-1.patch (obsolete) — Splinter Review
Attachment #8855924 - Attachment is obsolete: true
Attachment #8855924 - Flags: review?(jdescottes)
Attachment #8855928 - Flags: review?(jdescottes)
looks like there's a pretty innocent bug with console linking. I'll fix this and re-share.
Attached patch bundle-4-7-2.patch (obsolete) — Splinter Review
Attachment #8855928 - Attachment is obsolete: true
Attachment #8855928 - Flags: review?(jdescottes)
Attachment #8856062 - Flags: review?(jdescottes)
Attached patch bundle-4-7-2.patch (obsolete) — Splinter Review
Attachment #8856062 - Attachment is obsolete: true
Attachment #8856062 - Flags: review?(jdescottes)
Attachment #8856063 - Flags: review?(jdescottes)
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)
> 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`.
Attached patch bundle-4-7-3.patch (obsolete) — Splinter Review
Attachment #8856063 - Attachment is obsolete: true
Attachment #8858013 - Flags: review?(jdescottes)
Attached patch bundle-4-7-4.patch (obsolete) — Splinter Review
changed a locale key
Attachment #8858013 - Attachment is obsolete: true
Attachment #8858013 - Flags: review?(jdescottes)
Attachment #8858015 - Flags: review?(jdescottes)
Attached patch bundle-4-7-5.patch (obsolete) — Splinter Review
rebased to get the uptodate content.
Attachment #8858015 - Attachment is obsolete: true
Attachment #8858015 - Flags: review?(jdescottes)
Attachment #8858049 - Flags: review?(jdescottes)
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+
Attached patch bundle-4-7-6.patch (obsolete) — Splinter Review
small changes
Attachment #8858049 - Attachment is obsolete: true
Keywords: checkin-needed
This has conflicts with inbound and the bug number in the commit message isn't this bug.
Keywords: checkin-needed
Attached patch bug1354672.amended.patch (obsolete) — Splinter Review
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)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26825d7a225d
update debugger frontend 04/18/17;r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/26825d7a225d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
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?
Flags: needinfo?(jdescottes)
> 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)
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)
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
Resolution: FIXED → ---
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+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddbf12eaee0
update debugger frontend 04/18/17;r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/2ddbf12eaee0
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
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?
Flags: needinfo?(MattN+bmo)
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)
Depends on: 1364837
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.