[webkit-reviews] review denied: [Bug 195132] Web Inspector: Provide UIString descriptions to improve localizations : [Attachment 367267] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 195132: Web Inspector: Provide UIString descriptions to improve
localizations
https://bugs.webkit.org/show_bug.cgi?id=195132

Attachment 367267: Patch

https://bugs.webkit.org/attachment.cgi?id=367267&action=review




--- Comment #23 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 367267
  --> https://bugs.webkit.org/attachment.cgi?id=367267
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367267&action=review

r-, many of the comments could use more explanation and I think we can
structure the logic for creating/using a repeated UI string better.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:103
> +/* A submenu of 'Break on' */

This could use a better explanation, like "A submenu item of 'Break on' that
breaks (pauses) before all network requests".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:132
> +/* Break when console.assert() fails */

Please be consistent, so use "Break (pause)".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:139
> +/* A submenu of 'Break On' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:203
> +/* Capture Screenshot of the selected DOM node */

NIT: the "S" shouldn't be capitalized.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:217
> +/* A submenu of 'Add' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:252
> +/* Composite phase records, where graphic layers are combined */

"records" is ambiguous in this case.  Perhaps "timeline records"?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:492
> +/* Layout phase records that were imperative (forced) */

Ditto (>252).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:494
> +/* A context menu to force (override) element's pseudo-classes */

What does "element" mean in this context (devils' advocate).  How about "A
context menu item to force (override) a DOM node's pseudo-classes"?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:585
> +/* A section of CSS rules inherited from another CSS rule */

Technically, they're inherited from an ancestor DOM node.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:615
> +/* Layout phase records */

Ditto (>252).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:634
> +/* Log DOM element to Console */

For someone unfamiliar with the concept of a "Log", should we add another word
in parenthesis (like above)?  Perhaps "Log (print)" or "Log (record)"?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:683
> +/* A submenu of 'Add' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:724
> +/* A submenu of 'Break On' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:764
> +/* Paint (render) phase records */

Ditto (>252).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:795
> +/* A submenu of 'Add' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:931
> +/* Selected DOM element */

This comment doesn't really provide much more information, such as in what
context the string is used.  How about "When logging (printing) the selected
DOM node to the console, this is logged (printed) before to identify it as
such" or something like that that's less wordy?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:936
> +/* Selected DOM node */

Ditto (>931).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1050
> +/* A submenu of 'Break On' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1057
> +/* A submenu of 'Edit' */

Ditto (>103).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1135
> +/* Break (pause) on every uncaught (unhandled) exceptions */

Grammar: either remove the "every" or make "exceptions" singular.

> Source/WebInspectorUI/UserInterface/Base/LoadLocalizedStrings.js:50
> +    if (key && comment === undefined) {

I'd just check `arguments` directly so that a caller can't manually specify
`comment` as `undefined`.
```
    if (arguments.length === 2)
```

> Source/WebInspectorUI/UserInterface/Base/LoadLocalizedStrings.js:74
> +WI.repeatingUIString = function(key) {

This is a really odd way of doing repeated UI strings.	The way WebKit normally
does it (see WebCore/platform/LocalizedStrings.h) is to have a separate
function for each UI string that contains this information, that way you'd only
need to know the function name (not the exact key for it).  Additionally, this
makes modifications to the string MUCH cleaner as you wouldn't have to modify
any callsites (since they're just calling a function).

```
    WI.repeatedUIString = {};

    WI.repeatedUIString.timelineRecordNameLayout = function() {
	return WI.UIString("Layout", "Layout @ Timeline Record", "Layout phase
records");
    };

    ...
```