Jason Laster [:jlast] Reporter
	Description • 8 years ago

      No description provided.

	Jason Laster [:jlast] Reporter
	Comment 1 • 8 years ago

The most important thing to review is:
* the preferences.js file
* properties file
* update-tools script, package.json

	Jason Laster [:jlast] Reporter
	Comment 2 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aba651712757dee22337b13f6eb93303bf4a5c0

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

	Julian Descottes [:jdescottes] 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+

	Jason Laster [:jlast] Reporter
	Comment 11 • 8 years ago

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

	Jason Laster [:jlast] Reporter
	Comment 14 • 8 years ago

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

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

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

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

	Julian Descottes [:jdescottes] Assignee
	Comment 19 • 8 years ago

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b2f0962a43d4b22e873f61e383ef069adb9995

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

	Julian Descottes [:jdescottes] Assignee
	Comment 21 • 8 years ago

All green on linux opt and debug. Adding checkin-needed for attachment 8828889 [details] [diff] [review]

	Pulsebot
	Comment 22 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2301f25d1595
Bump Debugger. r=jdescottes

	Phil Ringnalda (:philor)
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2301f25d1595

	Francesco Lodolo [:flod]
	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.

	Jason Laster [:jlast] 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.

	Francesco Lodolo [:flod]
	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.

	Gervase Markham [:gerv]
	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

	Julian Descottes [:jdescottes] Assignee
	Comment 28 • 8 years ago

Thanks for the review! Updated the description, what you proposed is what I meant :)

	Gervase Markham [: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

	Pulsebot
	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

	Carsten Book [:Tomcat]
	Comment 31 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/15d7137f59c6