Closed Bug 1331654 Opened 3 years ago Closed 3 years ago

Update Debugger frontend (1/17/2017)

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jlast, Assigned: jdescottes)

References

Details

Attachments

(2 files, 10 obsolete files)

No description provided.
Attached patch update-dbg.patch (obsolete) — Splinter Review
The most important thing to review is:
* the preferences.js file
* properties file
* update-tools script, package.json
Attachment #8827492 - Flags: review?(jdescottes)
Attached patch update-dbg2.patch (obsolete) — Splinter Review
Attachment #8827509 - Flags: review?(jdescottes)
Attached patch update-dbg4.patch (obsolete) — Splinter Review
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)
Attached patch update-dbg5.patch (obsolete) — Splinter Review
Attachment #8827526 - Attachment is obsolete: true
Attachment #8827526 - Flags: review?(jdescottes)
Attachment #8827531 - Flags: review?(jdescottes)
Attached patch update-dbg7.patch (obsolete) — Splinter Review
Attachment #8827544 - Flags: review?(jdescottes)
Attached patch update-dbg7.patch (obsolete) — Splinter Review
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)
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+
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+
Attached patch update-dbg8.patch (obsolete) — Splinter Review
Attachment #8827545 - Attachment is obsolete: true
Attached patch update-dbg9.patch (obsolete) — Splinter Review
Attachment #8828066 - Attachment is obsolete: true
Attached patch update-dbg10.patch (obsolete) — Splinter Review
Attachment #8828128 - Attachment is obsolete: true
Attached patch bug1331654.licenses.patch (obsolete) — Splinter Review
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: nobody → jdescottes
Priority: -- → P2
Attachment #8828517 - Attachment is obsolete: true
Attachment #8828889 - Flags: review+
Keywords: leave-open
All green on linux opt and debug. Adding checkin-needed for attachment 8828889 [details] [diff] [review]
Keywords: checkin-needed
Attachment #8828889 - Attachment is patch: true
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.
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.
(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 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-
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 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+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d7137f59c6
add licenses for new debugger dependencies;r=gerv
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1333602
Depends on: 1316413
Depends on: 1340971
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.