Closed
Bug 1333602
Opened 7 years ago
Closed 7 years ago
Update Debugger frontend (1/24/2017)
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(firefox54 fixed)
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jlast, Assigned: jlast)
References
Details
Attachments
(1 file, 1 obsolete file)
3.37 MB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → jlaster
Priority: -- → P2
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93338fabcba68be035617a71aa67fb3cbb1ab114
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8830085 -
Flags: review?(jdescottes)
Comment 3•7 years ago
|
||
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+
Assignee | ||
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd79890733afebb30b2d91690a02dcf755781e01
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8830085 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
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
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe88899da4aa
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
(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.
Updated•7 years ago
|
Flags: needinfo?(jlaster)
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
thanks :jmaher that's interesting.
Updated•7 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•