Closed Bug 1333602 Opened 7 years ago Closed 7 years ago

Update Debugger frontend (1/24/2017)

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a couple of things:
* a simple bundle update from debugger.html for debugger.js, debugger.css, pretty-print-worker.js, source-map-worker.js, debugger.properties, tests
* deleted devtools/client/debugger/new/images as it's not used
* deleted update-tools.js and package.json as it's going to be replaced with something simpler (https://github.com/devtools-html/devtools-core/issues/122)
* updated codemirror and theme styles (missed this last time)
* added additional codemirror modes (elm, coffee-script, jsx)
Assignee: nobody → jlaster
Priority: -- → P2
Attached patch bundle24.patch (obsolete) — Splinter Review
Attachment #8830085 - Flags: review?(jdescottes)
Comment on attachment 8830085 [details] [diff] [review]
bundle24.patch

Review of attachment 8830085 [details] [diff] [review]:
-----------------------------------------------------------------

I think properties need to be slightly updated again. But r+ with my comments addressed and a green try.

The only thing that was a bit surprising was the behavior change on escape to get to 
the split console, but there's no reason to block the update for this. 

Thanks for doing the follow up!

::: devtools/client/debugger/new/test/mochitest/browser_dbg-console.js
@@ +37,5 @@
>  
>    // close the console
>    clickElement(dbg, "codeMirror");
> +  // First time to focus out of text area
> +  pressKey(dbg, "Escape");

Having to press escape twice to get from the debugger editor to the splitconsole is a different behavior compared with the old console (and Chrome as well).

I don't have a strong opinion on which is best, but it seems that right now having the focus on the textarea brings no value, you can't tab-out of it for instance.

Should we file an issue to fix this?

::: devtools/client/locales/en-US/debugger.properties
@@ +25,3 @@
>  # LOCALIZATION NOTE (pauseButtonTooltip): The tooltip that is displayed for the pause
>  # button when the debugger is in a running state.
> +pauseButtonTooltip=Pause %s

The values for the "legacy" strings are wrong here. I assume the intent is to keep the old strings for the old debugger, so the values should be restored to what they were before last week's debugger update.

Maybe it would have been nice to mention that the entries are duplicated because of the new debugger vs. old debugger issue in the localization notes.

"pauseButtonTooltip" was "Click to pause (%S)"

@@ +33,5 @@
> +# LOCALIZATION NOTE (resumeButtonTooltip1): The label that is displayed on the pause
> +# button when the debugger is in a paused state.
> +resumeButtonTooltip1=Resume %S
> +
> +# LOCALIZATION NOTE (resumeButtonTooltips): The label that is displayed on the pause

mismatch between the loc. note and the key : remove the "s" at the end of resumeButtonTooltips

@@ +38,2 @@
>  # button when the debugger is in a paused state.
> +resumeButtonTooltip=Resume %s

was resumeButtonTooltip=Click to resume (%S)

@@ +44,4 @@
>  
>  # LOCALIZATION NOTE (stepOverTooltip): The label that is displayed on the
>  # button that steps over a function call.
> +stepOverTooltip=Step Over %s

was stepOverTooltip=Step Over (%S)

@@ +52,4 @@
>  
>  # LOCALIZATION NOTE (stepInTooltip): The label that is displayed on the
>  # button that steps into a function call.
> +stepInTooltip=Step In %s

was stepInTooltip=Step In (%S)

@@ +60,4 @@
>  
>  # LOCALIZATION NOTE (stepOutTooltip): The label that is displayed on the
>  # button that steps out of a function call.
> +stepOutTooltip=Step Out %s

was stepOutTooltip=Step Out %S

::: devtools/client/themes/dark-theme.css
@@ +207,5 @@
>    border-left: solid 1px #fff;
>  }
>  
>  .cm-s-mozilla.CodeMirror-focused .CodeMirror-selected { /* selected text (focused) */
> +  background: rgb(77, 86, 103);

great catch! 

The selected text was indeed unreadable in dark theme

::: devtools/client/themes/light-theme.css
@@ +213,5 @@
>    background: rgb(185, 215, 253);
>  }
>  
>  .cm-s-mozilla .CodeMirror-selected { /* selected text (unfocused) */
> +  background: rgb(255, 255, 0);

bright yellow ? :) 
Why not do the same as for the dark theme and use the same color as when CodeMirror is focused ?
Attachment #8830085 - Flags: review?(jdescottes) → review+
Thanks for the feedback, I'll make the updates tomorrow morning. I think you're right about the yellow being a bit too strong. It's true that helen did advocate for garish, but I think we can tone it down a bit and still keep the intent. There might also be a nice tweak we can do with the border.
(In reply to Jason Laster [:jlast] from comment #4)
> Thanks for the feedback, I'll make the updates tomorrow morning. I think
> you're right about the yellow being a bit too strong. It's true that helen
> did advocate for garish, but I think we can tone it down a bit and still
> keep the intent. There might also be a nice tweak we can do with the border.

I have no strong opinion here. It looked a bit random and since dark theme didn't have a different color when focused/not-focused, I thought it was a leftover.
Attached patch bundle24-1.patchSplinter Review
Attachment #8830085 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe88899da4aa
Update new frontend (1/24/2017). r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fe88899da4aa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Unfortunately the fix for l10n made things worse (I don't think I've seen it as part of the issues I was CCed to)
https://hg.mozilla.org/mozilla-central/diff/fe88899da4aa/devtools/client/locales/en-US/debugger.properties

Here's a practical example.

In 52 
pauseButtonTooltip = Click to pause (%S)

This was changed in 53 without a new ID. As explained, this change is not going to trigger a retranslation
pauseButtonTooltip = Pause %S

In 54 we have
pauseButtonTooltip1 = Pause %S --> good, they're going to retranslate this
pauseButtonTooltip = Click to pause (%S) --> crap, you changed again an existing string

You also introduced a duplicated pausePendingButtonTooltip in the file.

I assume you need pauseButtonTooltip & C. for the old version of debugger (still enabled in beta and release).

At this point I'm not sure how we can get out of this mess. One solution is to back-out the l10n changes contained in this landing, only keeping editor.jumpToMappedLocation1 (for the variable format change) and whyPaused.breakpointConditionThrown, and consider the issues landed in 53 as gone for good.
Flags: needinfo?(jlaster)
(In reply to Francesco Lodolo [:flod] from comment #10)
> At this point I'm not sure how we can get out of this mess. One solution is
> to back-out the l10n changes contained in this landing, only keeping
> editor.jumpToMappedLocation1 (for the variable format change) and
> whyPaused.breakpointConditionThrown, and consider the issues landed in 53 as
> gone for good.

Dropping NI per IRC discussion, we'll fix it in a follow-up following this approach.
Flags: needinfo?(jlaster)
Depends on: 1334154
as a note for your information, the installer size on all builds seemed to increase when this landed:
== Change summary for alert #4930 (as of January 26 2017 03:44 UTC) ==

Regressions:

  1%  installer size summary windows8-64 pgo          57326426.83 -> 57661031.08
  1%  installer size summary windows2012-32 pgo       53388776.17 -> 53677572.42
  1%  installer size summary windows2012-32 opt       52164449.67 -> 52445988.25
  1%  installer size summary windowsxp opt            52222395.58 -> 52505346.67
  1%  installer size summary windows2012-64 opt       57090681.25 -> 57378917
  1%  installer size summary windows8-64 opt          57152534.08 -> 57435771.5
  0%  installer size summary windows2012-64 pgo       57269639.67 -> 57548004
  0%  installer size summary windows2012-32 debug     65075445.25 -> 65358721
  0%  installer size summary windowsxp debug          65161190.67 -> 65443873.42
  0%  installer size summary linux32 pgo              61809165.17 -> 62064550.42
  0%  installer size summary linux32 debug            65071757.67 -> 65339867.25
  0%  installer size summary linux32 opt              56768122.17 -> 56995411.17
  0%  installer size summary linux64 debug            64949340.5 -> 65194282.33
  0%  installer size summary windows2012-64 debug     77051836.08 -> 77339556.92
  0%  installer size summary windows8-64 debug        77138876.42 -> 77424867.58
  0%  installer size summary linux64 pgo              61535540.42 -> 61752513.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4930

small adjustments, but worth making notes.
thanks :jmaher that's interesting.
Depends on: 1331654
No longer blocks: 1320188
See Also: → 1320188
Blocks: 1338567
Depends on: 1346698
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: