Closed
Bug 1331654
Opened 8 years ago
Closed 8 years ago
Update Debugger frontend (1/17/2017)
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jlast, Assigned: jdescottes)
References
Details
Attachments
(2 files, 10 obsolete files)
(deleted),
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•8 years ago
|
||
The most important thing to review is: * the preferences.js file * properties file * update-tools script, package.json
Attachment #8827492 -
Flags: review?(jdescottes)
Reporter | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aba651712757dee22337b13f6eb93303bf4a5c0
Reporter | ||
Comment 3•8 years ago
|
||
Attachment #8827509 -
Flags: review?(jdescottes)
Reporter | ||
Comment 4•8 years ago
|
||
Attachment #8827492 -
Attachment is obsolete: true
Attachment #8827509 -
Attachment is obsolete: true
Attachment #8827492 -
Flags: review?(jdescottes)
Attachment #8827509 -
Flags: review?(jdescottes)
Attachment #8827526 -
Flags: review?(jdescottes)
Reporter | ||
Comment 5•8 years ago
|
||
Attachment #8827526 -
Attachment is obsolete: true
Attachment #8827526 -
Flags: review?(jdescottes)
Attachment #8827531 -
Flags: review?(jdescottes)
Reporter | ||
Comment 6•8 years ago
|
||
Attachment #8827544 -
Flags: review?(jdescottes)
Reporter | ||
Comment 7•8 years ago
|
||
Attachment #8827531 -
Attachment is obsolete: true
Attachment #8827544 -
Attachment is obsolete: true
Attachment #8827531 -
Flags: review?(jdescottes)
Attachment #8827544 -
Flags: review?(jdescottes)
Attachment #8827545 -
Flags: review?(jdescottes)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8827545 [details] [diff] [review] update-dbg7.patch Review of attachment 8827545 [details] [diff] [review]: ----------------------------------------------------------------- Bundle works well but we have some compatibility issues to sort out before this lands. Clearing the review flag for now, most important is to: - fix the localized string names - fix codemirror css so that it doesn't impact the old debugger (maybe it impacts other tools for the .error classname too? not sure) ::: devtools/client/locales/en-US/debugger.properties @@ +40,4 @@ > > # LOCALIZATION NOTE (stepOutTooltip): The label that is displayed on the > # button that steps out of a function call. > +stepOutTooltip=Step Out %S The rule is that when you update a l10n string, the name of the string has to be updated so that localizers are notified of the change and can update the localization for this string. Usually if it's just a cosmetic change, we append 2 to the string name. Comment applies to all 7 updated strings in this diff: - pauseButtonTooltip - resumeButtonTooltip - stepOverTooltip - stepInTooltip - stepOutTooltip - welcome.search - sourceSearch.search @@ +226,5 @@ > # LOCALIZATION NOTE(editor.addBreakpoint): Editor gutter context menu item > # for adding a breakpoint on a line. > editor.addBreakpoint=Add Breakpoint > > +# LOCALIZATION NOTE(editor.disableBreakpoint): Editor gutter context menu item nit: there's usually a space between LOCALIZATION_NODE and (name.of.string). Even if some strings don't respect that in the file, let's try to stick to it. (applies to some other strings added here) ::: devtools/client/package.json @@ +3,5 @@ > + "version": "0.0.1", > + "description": "Devtools", > + "scripts": {}, > + "author": "", > + "license": "ISC", Need to double check with bgrins or jryans, but for the inspector we ended up removing the license field. I don't think we should ship with a license different from MPL or Public Domain. ::: devtools/client/sourceeditor/codemirror/mozilla.css @@ +14,5 @@ > + /* --breakpoint-background: url("chrome://devtools/skin/images/breakpoint.svg#dark"); */ > + /* --breakpoint-hover-background: url("chrome://devtools/skin/images/breakpoint.svg#dark-hover"); */ > + --breakpoint-active-color: rgba(0,255,175,.4); > + --breakpoint-active-color-hover: rgba(0,255,175,.7); > + /* --breakpoint-conditional-background: url("chrome://devtools/skin/images/breakpoint.svg#dark-conditional"); */ Commenting-out intentional here? Breakpoints for the old debugger are no longer visible. We need to find a way to isolate the changes for the new debugger. @@ +29,5 @@ > height: 12px; > background-repeat: no-repeat; > background-position: center; > background-size: contain; > + /* background-image: url("chrome://devtools/skin/images/editor-error.png"); */ Same question ::: devtools/client/update-tools.js @@ +1,1 @@ > +"use strict"; This needs a license header. Probably the one we use for tests: /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */
Attachment #8827545 -
Flags: review?(jdescottes) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8827545 [details] [diff] [review] update-dbg7.patch We clarified what needed to be updated for the patch on IRC so feel free to push the update and land! Switching to r+
Attachment #8827545 -
Flags: feedback+ → review+
Reporter | ||
Comment 10•8 years ago
|
||
Attachment #8827545 -
Attachment is obsolete: true
Reporter | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4de3e9450d824ad24fe2c42c76e881302377651
Reporter | ||
Comment 12•8 years ago
|
||
Attachment #8828066 -
Attachment is obsolete: true
Reporter | ||
Comment 13•8 years ago
|
||
Attachment #8828128 -
Attachment is obsolete: true
Reporter | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf8b8e243683004bab04431d984acb49b0c5170
Assignee | ||
Comment 15•8 years ago
|
||
Let's get a license review for : - lodash : https://github.com/lodash/lodash/blob/master/LICENSE - node-md5 : https://github.com/pvorb/node-md5/blob/master/LICENSE - fuzz-aldrin : https://github.com/jeancroy/fuzz-aldrin-plus/blob/master/LICENSE.md
Assignee | ||
Comment 16•8 years ago
|
||
talos run ongoing: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f53bbf10a755270de10eebe145b91eb9339499f3&newProject=try&newRevision=6fc3da32ac0169ed3b4d53b048827017124471f6&framework=1&filter=damp&showOnlyImportant=0
Assignee | ||
Comment 17•8 years ago
|
||
gerv: The new devtools debugger is pulling in three external libraries which are being bundled inside a built file 'debugger.js' which is here being committed inside of mozilla-central. The patch here adds license information for all 3 libraries to license.html.
Attachment #8828842 -
Flags: review?(gerv)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jdescottes
Priority: -- → P2
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8828517 -
Attachment is obsolete: true
Attachment #8828889 -
Flags: review+
Assignee | ||
Comment 19•8 years ago
|
||
New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b2f0962a43d4b22e873f61e383ef069adb9995
Assignee | ||
Comment 20•8 years ago
|
||
New talos run: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=278f573760714f1e92d1205301dd44ddd3336475&newProject=try&newRevision=415b9ba73a7793a38268f8b5b48411fa2c68fd49&framework=1&filter=damp&showOnlyImportant=0
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 21•8 years ago
|
||
All green on linux opt and debug. Adding checkin-needed for attachment 8828889 [details] [diff] [review]
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Attachment #8828889 -
Attachment is patch: true
Comment 22•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2301f25d1595 Bump Debugger. r=jdescottes
Keywords: checkin-needed
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2301f25d1595
Comment 24•8 years ago
|
||
Commenting here, instead of filing a new bug, since the bug is marked as "leave-open". Can we all agree on avoid dumping a ton of code in mozilla-central 24 hours before merge day? Ironically it's impossible to check it in Firefox thanks to bug 827937 (tip: don't click the link in the previous comment). Here's the link to the changes in debugger.properties https://hg.mozilla.org/mozilla-central/diff/2301f25d1595/devtools/client/locales/en-US/debugger.properties 7 strings were changed without updating their IDs, which means almost nobody will notice the new text and update their localizations https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings > sourceSearch.resultsSummary=%d instances of “%S” This requires proper plural forms. https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms I've asked before: do you support proper plural forms in devtools/l10n? I didn't get a clear answer https://bugzilla.mozilla.org/show_bug.cgi?id=1313196#c11 > editor.jumpToMappedLocation=Jump to %s location Is "%s" correct here? I think it is based on https://github.com/juliandescottes/sprintf.js But I would really use %S in the future (they're all but interchangeable in Firefox). Also for consistency, %S is used just a few lines later in sourceTabs.newTabButtonTooltip > # LOCALIZATION NOTE (scopes.block): Scopes right sidebar block subheading > scopes.block=Block Is this a verb or a noun? Given the other scopes.* strings, I would even expect "Blocked". Note that it doesn't really matter that localization is broken due to bug 1320188, once localized nobody will look back at these strings. And this comment is helpful up to a certain point, since it's too late to fix issues before they're exposed to hundreds of people. I've said it before: we have evidence that localization is important for devtools, based on number of users on localized builds. We should try our best to treat these users, and our community of localizers, as best as we can. Given the timeline, it's impossible to fix them before they slip in aurora. At this point a fix should land in m-c and ride the trains with 54.
Reporter | ||
Comment 25•8 years ago
|
||
Thanks reviewing :flod We're holding the new frontend in aurora this cycle. 1332114 I apologize for releasing late on Friday. It took us some time to iron out many of the integration issues. We hope to get on a weekly release cycle soon. And there are several issues for generating small patches and automating the release steps. Re: localization issues. - I could not get to 1320188 this week, but I plan on getting to it soon. - We have an issue filed for supporting pluralization: https://github.com/devtools-html/devtools-core/issues/16 - I filed this issue for renaming the keys - https://github.com/devtools-html/debugger.html/issues/1783 - I filed a %S issue here https://github.com/devtools-html/debugger.html/issues/1784 - Block is a noun. I can update the comment to explain that we're referring to a block of code. I'm sorry again that this patch landed in the way that it did. I hope that we can work out the kinks in our process in a timely manner so that this does not happen again.
Comment 26•8 years ago
|
||
(In reply to Jason Laster [:jlast] from comment #25) > - Block is a noun. I can update the comment to explain that we're referring > to a block of code. Thanks, it would definitely help. > I'm sorry again that this patch landed in the way that it did. I hope that > we can work out the kinks in our process in a timely manner so that this > does not happen again. I've CCed a few people from DevTools (I've talked with them about l10n issues in the past few months), because I expect that any solutions you'll find to this process will be useful for other tools in the near future.
Comment 27•8 years ago
|
||
Comment on attachment 8828842 [details] [diff] [review] bug1331654.licenses.patch Review of attachment 8828842 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/license.html @@ +2906,5 @@ > + > + <h1><a id="fuzz-aldrin"></a>fuzz-aldrin License</h1> > + > + <p>This license applies to dependencies pulled in the devtools debugger.js > + bundle in <span class="path">devtools/client/debugger/new</span>.</p> This is all generally fine, but I don't understand this sentence. Where is the actual code covered by this license? Is it in debugger.js or somewhere else? Either say: This license applies to some of the code in <span class="path">devtools/client/debugger/new/debugger.js</span> or, if that's not what you mean, say something else :-) Gerv
Attachment #8828842 -
Flags: review?(gerv) → review-
Assignee | ||
Comment 28•8 years ago
|
||
Thanks for the review! Updated the description, what you proposed is what I meant :)
Attachment #8828842 -
Attachment is obsolete: true
Attachment #8829368 -
Flags: review?(gerv)
Comment 29•8 years ago
|
||
Comment on attachment 8829368 [details] [diff] [review] bug1331654.licenses.v2.patch Review of attachment 8829368 [details] [diff] [review]: ----------------------------------------------------------------- r=gerv. Gerv
Attachment #8829368 -
Flags: review?(gerv) → review+
Comment 30•8 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/15d7137f59c6 add licenses for new debugger dependencies;r=gerv
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/15d7137f59c6
Assignee | ||
Updated•8 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
•