1354672 - Update Debugger frontend (4/7/2017)

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Jason Laster [:jlast]]

Regular update.

Comment 1

[Jason Laster [:jlast]]

Attachment: bundle-4-7.patch

Comment 2

[Jason Laster [:jlast]]

Attachment: bundle-4-7-1.patch

Comment 3

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11433c282f886f167b68e2929f8935c344f320f7

Comment 4

[Jason Laster [:jlast]]

looks like there's a pretty innocent bug with console linking. I'll fix this and re-share.

Comment 5

[Jason Laster [:jlast]]

Attachment: bundle-4-7-2.patch

Comment 6

[Jason Laster [:jlast]]

Attachment: bundle-4-7-2.patch

Comment 7

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=458c7a2ed88f10e06983b77f084e24d2350a63df

Comment 8

[Julian Descottes [: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?

Comment 9

[Jason Laster [:jlast]]

> 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`.

Comment 10

[Jason Laster [:jlast]]

Attachment: bundle-4-7-3.patch

Comment 11

[Jason Laster [:jlast]]

Attachment: bundle-4-7-4.patch

changed a locale key

Comment 12

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=703fb54aa0346b005fad87c92d634e92c3f208c7

Comment 13

[Jason Laster [:jlast]]

Attachment: bundle-4-7-5.patch

rebased to get the uptodate content.

Comment 14

[Julian Descottes [: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.

Comment 15

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0d3ab4ccb43fc2d3397dc0c611166f7dca8a82&selectedJob=91389486

Comment 16

[Jason Laster [:jlast]]

Attachment: bundle-4-7-6.patch

small changes

Comment 17

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=036a35af227606b743aae9c8358286424240d3d8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This has conflicts with inbound and the bug number in the commit message isn't this bug.

Comment 19

[Julian Descottes [:jdescottes]]

Attachment: 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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26825d7a225d
update debugger frontend 04/18/17;r=jdescottes

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/26825d7a225d

Comment 22

[Francesco Lodolo [:flod]]

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?

Comment 23

[Julian Descottes [: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 ?

Comment 24

[Axel Hecht [:Pike]]

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.

Comment 25

[Julian Descottes [:jdescottes]]

OK I'll ask for a backout and will wait for Jason to discuss out the proper spelling for blackboxing

Comment 26

[Carsten Book [:Tomcat]]

backed out in  https://hg.mozilla.org/mozilla-central/rev/f6be27f5457ffd75b3763db09a139e24bc2155dc on request

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: 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.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddbf12eaee0
update debugger frontend 04/18/17;r=jdescottes

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ddbf12eaee0

Comment 30

[Matthew N. [:MattN]]

Screenshot changes to review: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=e9a5d4f62461ee0db07a41f59b73163ec106bc3e&newProject=mozilla-central&newRev=20dff607fb88ee69135a280bbb7f32df75a86237

Comment 31

[Julian Descottes [:jdescottes]]

(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?

Comment 32

[Matthew N. [:MattN]]

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.