Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Update Debugger frontend (4/7/2017)

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jlast, Assigned: jlast)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

3 months ago
Regular update.
(Assignee)

Updated

3 months ago
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 1

3 months ago
Created attachment 8855924 [details] [diff] [review]
bundle-4-7.patch
Attachment #8855924 - Flags: review?(jdescottes)
(Assignee)

Comment 2

3 months ago
Created attachment 8855928 [details] [diff] [review]
bundle-4-7-1.patch
Attachment #8855924 - Attachment is obsolete: true
Attachment #8855924 - Flags: review?(jdescottes)
Attachment #8855928 - Flags: review?(jdescottes)
(Assignee)

Comment 3

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11433c282f886f167b68e2929f8935c344f320f7
(Assignee)

Comment 4

3 months ago
looks like there's a pretty innocent bug with console linking. I'll fix this and re-share.
(Assignee)

Comment 5

3 months ago
Created attachment 8856062 [details] [diff] [review]
bundle-4-7-2.patch
Attachment #8855928 - Attachment is obsolete: true
Attachment #8855928 - Flags: review?(jdescottes)
Attachment #8856062 - Flags: review?(jdescottes)
(Assignee)

Comment 6

3 months ago
Created attachment 8856063 [details] [diff] [review]
bundle-4-7-2.patch
Attachment #8856062 - Attachment is obsolete: true
Attachment #8856062 - Flags: review?(jdescottes)
Attachment #8856063 - Flags: review?(jdescottes)
(Assignee)

Comment 7

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=458c7a2ed88f10e06983b77f084e24d2350a63df
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

3 months 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

3 months ago
Created attachment 8858013 [details] [diff] [review]
bundle-4-7-3.patch
Attachment #8856063 - Attachment is obsolete: true
Attachment #8858013 - Flags: review?(jdescottes)
(Assignee)

Comment 11

3 months ago
Created attachment 8858015 [details] [diff] [review]
bundle-4-7-4.patch

changed a locale key
Attachment #8858013 - Attachment is obsolete: true
Attachment #8858013 - Flags: review?(jdescottes)
Attachment #8858015 - Flags: review?(jdescottes)
(Assignee)

Comment 12

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=703fb54aa0346b005fad87c92d634e92c3f208c7
(Assignee)

Comment 13

3 months ago
Created attachment 8858049 [details] [diff] [review]
bundle-4-7-5.patch

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+
(Assignee)

Comment 15

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0d3ab4ccb43fc2d3397dc0c611166f7dca8a82&selectedJob=91389486
(Assignee)

Comment 16

3 months ago
Created attachment 8858131 [details] [diff] [review]
bundle-4-7-6.patch

small changes
Attachment #8858049 - Attachment is obsolete: true
(Assignee)

Comment 17

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=036a35af227606b743aae9c8358286424240d3d8
(Assignee)

Updated

3 months ago
Keywords: checkin-needed
This has conflicts with inbound and the bug number in the commit message isn't this bug.
Keywords: checkin-needed
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
Flags: needinfo?(jlaster)

Comment 20

3 months 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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26825d7a225d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months 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?
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)

Comment 24

3 months 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)
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.
Attachment #8859072 - Attachment is obsolete: true
Attachment #8860012 - Flags: review+

Comment 28

3 months 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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ddbf12eaee0
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Screenshot changes to review: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=e9a5d4f62461ee0db07a41f59b73163ec106bc3e&newProject=mozilla-central&newRev=20dff607fb88ee69135a280bbb7f32df75a86237
(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
You need to log in before you can comment on or make changes to this bug.