Web Inspector: Provide UIString descriptions to improve localizations

[Nikita Vasilyev]

I've noticed many wrongly translated strings in Russian localization. I imagine a lot of these issues are present in many other localizations.

Common issues:
- Verb vs noun. In many languages, a verb and a noun for "paint" are different words. Same for "a log" and "to log".
- Lack of context. Localizers didn't pick accurate terms because they didn't have any context.

[Nikita Vasilyev]

Created attachment 363159 [details]
WIP

[Nikita Vasilyev]

Created attachment 363160 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/48457817>

[Nikita Vasilyev]

Comment on attachment 363160 [details]
WIP

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-189
> -localizedStrings["Capture Screenshot"] = "Capture Screenshot";

extract-localizable-js-strings removed strings that had keys matching the strings.
Do we really want to enforce IDs different from the strings?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:190
>  localizedStrings["Case Sensitive @ Context Menu"] = "Case Sensitive";

I've noticed that's the format for the key we used a few times:

"${exactString} @ Some Context"

While I'm not against it, I wonder what influenced it?

[Devin Rousso]

Comment on attachment 363160 [details]
WIP

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-189
>> -localizedStrings["Capture Screenshot"] = "Capture Screenshot";
> 
> extract-localizable-js-strings removed strings that had keys matching the strings.
> Do we really want to enforce IDs different from the strings?

The script uses the key if it is provided.  Passing "" as the argument isn't a good idea.  If a string has a specific comment, I think it deserves a unique key.  Worst case, you can reuse the format string as the key.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:190
>>  localizedStrings["Case Sensitive @ Context Menu"] = "Case Sensitive";
> 
> I've noticed that's the format for the key we used a few times:
> 
> "${exactString} @ Some Context"
> 
> While I'm not against it, I wonder what influenced it?

I just decided on it 😛

I think it's easy to understand, given that "@" is unlikely to appear in a string displayed in the UI.  I also think it makes some logical sense, as "@" is a way of indicating location.

[Greg Marriott]

Comment on attachment 363160 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Models/LayoutTimelineRecord.js:56
> +            return WI.UIString("Layout", "", "Layout action (verb)");

Doesn't "action" imply "(verb)"?

> Source/WebInspectorUI/UserInterface/Models/RenderingFrameTimelineRecord.js:51
> +            return WI.UIString("Paint", "", "Paint action (verb)");

This is inconsistent with the description in LayoutTimelineRecord.js for the same string.

[Nikita Vasilyev]

Comment on attachment 363160 [details]
WIP

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

>> Source/WebInspectorUI/UserInterface/Models/LayoutTimelineRecord.js:56
>> +            return WI.UIString("Layout", "", "Layout action (verb)");
> 
> Doesn't "action" imply "(verb)"?

Yes, I think "(verb)" is unnecessary here.

>> Source/WebInspectorUI/UserInterface/Models/RenderingFrameTimelineRecord.js:51
>> +            return WI.UIString("Paint", "", "Paint action (verb)");
> 
> This is inconsistent with the description in LayoutTimelineRecord.js for the same string.

Yes, they should match.

[Nikita Vasilyev]

Created attachment 363278 [details]
Patch

[Devin Rousso]

Comment on attachment 363278 [details]
Patch

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

I think you could be a lot more specific or clearer in many of these identifiers/comments.  My original "style" of using the "@" was sort of a way of providing a "location" for the UIString.  Something like "@ Edit" is not clear/specific at all, whereas "@ DOM - Context Menu - Edit" is.

If we want to give a UIString an identifier, it should be such that it's unique.  If we want a reusable UIString, then you shouldn't use an identifier.  The more specific/clear the identifier is, the less likely we are to have a collision in the future and the more likely someone is to understand it's context/purpose at first glance (e.g. without having to read surrounding code).

Additionally, the comments are a way for us to provide additional description about the UIString's context so that you wouldn't have to read the code (e.g. for a localizer to look at).  Saying something like "Open Elements tab and select this node in DOM tree" is great for describing what the UIString's action is supposed to convey, but it tells me nothing about where or what form that action will appear (e.g. context menu, in the DOM, as a tooltip, etc.).  Something like "Context menu item for showing this DOM node in the Elements tab" is a bit more descriptive of both what it is expected to do and where it is expected to appear.

I think that it's awesome that you've gone through and done so many of these, but I personally think we can do better.

I'll leave this as r? so others will take a look and maybe we can start a discussion.  Feel free to continue to iterate if you agree or argue with me if you don't :)

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:499
> +                options.title = WI.UIString("All Requests", "All Requests @ Break on", "A submenu of 'Break on'");

I wouldn't describe this as "Break on".  I think it's more consistent to say "@ Debugger" like the rest of them, but I think something like "@ Debugger - add breakpoint" may be the most specific.

[Nikita Vasilyev]

I may have not explained well some of the problems I'm trying to solve.

I added a description for nested menus so localizers would choose correct cases[1] (word endings, basically) for the leaf items.

    Break on... -> Subtree Modified

English doesn't have cases, so "Subtree Modified" looks the same regardless where it's used in the sentence. It isn't the same for many other languages.

In UIString descriptions we should provide surrounding words or sentence fragments, when they exist.

It doesn't really matter whether a UIString is in a tooltip, DOM tree, or context menu. I don't believe this information is helpful for localizers. In fact, it's important that different parts of the UI use the same terms. Console should be always Console, and not Console in one place and Journal in another (this is the real problem in Russian localization which I haven't addressed yet).

I would prefer to have the key match the string exactly in a lot of cases, but extract-localizable-js-strings didn't allow me to do that :/

[1]: https://en.wikibooks.org/wiki/Russian/Grammar/Cases

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #9)
> If we want to give a UIString an identifier, it should be such that it's
> unique.  If we want a reusable UIString, then you shouldn't use an
> identifier.

For several cases, I want to give a UIString description without giving it an identifier.
extract-localizable-js-strings didn't allow me to do that.

[Nikita Vasilyev]

I introduced WI.repeatingUIString. When a UIString with a comment is used in several places, we have to keep the comments up to date everywhere. WI.repeatingUIString was added to address this problem.

(In reply to Nikita Vasilyev from comment #11)
> (In reply to Devin Rousso from comment #9)
> > If we want to give a UIString an identifier, it should be such that it's
> > unique.  If we want a reusable UIString, then you shouldn't use an
> > identifier.
> 
> For several cases, I want to give a UIString description without giving it
> an identifier.
> extract-localizable-js-strings didn't allow me to do that.

I fixed extract-localizable-js-strings. Now most strings with comments don't have an identifier (meaning their identifier matches the string itself). This eliminates a lot of redundancy.

[Nikita Vasilyev]

Created attachment 367154 [details]
Patch

[Nikita Vasilyev]

Created attachment 367155 [details]
Patch

Rebaselined.

[Devin Rousso]

Comment on attachment 367155 [details]
Patch

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

> Tools/Scripts/extract-localizable-js-strings:77
> +        HandleUIString($1, $2 || $1, $3 || "", $file, $.) while s/WI\.UIString\("([^"]+)"(?:,\s*"([^"]*)"(?:,\s*"([^"]+)")?)?\)//;

Rather than allow the `key` to be "", why don't we just allow it to be omitted altogether.  e.g., you'd have three "versions" of `UIString`:
1. [format]
2. [format, comment]
3. [format, key, comment]

I think it's a reasonable thing to say that if a `UIString` has a special `key` (e.g. one that isn't just the `format`), it deserves a comment.

You'd also need to update `WI.UIString` to accommodate this, probably using some `arguments.length` checks :)

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #15)
> Comment on attachment 367155 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367155&action=review
> 
> > Tools/Scripts/extract-localizable-js-strings:77
> > +        HandleUIString($1, $2 || $1, $3 || "", $file, $.) while s/WI\.UIString\("([^"]+)"(?:,\s*"([^"]*)"(?:,\s*"([^"]+)")?)?\)//;
> 
> Rather than allow the `key` to be "", why don't we just allow it to be
> omitted altogether.  e.g., you'd have three "versions" of `UIString`:
> 1. [format]
> 2. [format, comment]
> 3. [format, key, comment]
> 
> I think it's a reasonable thing to say that if a `UIString` has a special
> `key` (e.g. one that isn't just the `format`), it deserves a comment.
> 
> You'd also need to update `WI.UIString` to accommodate this, probably using
> some `arguments.length` checks :)

We also have this case:

4. [format, key] 
WI.UIString("%d Passed", "%d Passed (singular)")
WI.UIString("%d Passed", "%d Passed (plural)")

We would have to add an empty comment here as the 3rd argument to make it work.

Do you still think it's worth doing?

[Devin Rousso]

Comment on attachment 367155 [details]
Patch

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

>>> Tools/Scripts/extract-localizable-js-strings:77
>>> +        HandleUIString($1, $2 || $1, $3 || "", $file, $.) while s/WI\.UIString\("([^"]+)"(?:,\s*"([^"]*)"(?:,\s*"([^"]+)")?)?\)//;
>> 
>> Rather than allow the `key` to be "", why don't we just allow it to be omitted altogether.  e.g., you'd have three "versions" of `UIString`:
>> 1. [format]
>> 2. [format, comment]
>> 3. [format, key, comment]
>> 
>> I think it's a reasonable thing to say that if a `UIString` has a special `key` (e.g. one that isn't just the `format`), it deserves a comment.
>> 
>> You'd also need to update `WI.UIString` to accommodate this, probably using some `arguments.length` checks :)
> 
> We also have this case:
> 
> 4. [format, key] 
> WI.UIString("%d Passed", "%d Passed (singular)")
> WI.UIString("%d Passed", "%d Passed (plural)")
> 
> We would have to add an empty comment here as the 3rd argument to make it work.
> 
> Do you still think it's worth doing?

Hmmm.  Good point.  Maybe we can be smart about that and say that if the `comment` starts with `format`, it should be treated as a `key` instead?  It's an odd "quirk", but it would give us nice flexible functionality.  Regardless, I think the empty `key` case is a much worse experience than requiring a `comment` if `key` is set.  What do you think?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #17)
> Comment on attachment 367155 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367155&action=review
> 
> >>> Tools/Scripts/extract-localizable-js-strings:77
> >>> +        HandleUIString($1, $2 || $1, $3 || "", $file, $.) while s/WI\.UIString\("([^"]+)"(?:,\s*"([^"]*)"(?:,\s*"([^"]+)")?)?\)//;
> >> 
> >> Rather than allow the `key` to be "", why don't we just allow it to be omitted altogether.  e.g., you'd have three "versions" of `UIString`:
> >> 1. [format]
> >> 2. [format, comment]
> >> 3. [format, key, comment]
> >> 
> >> I think it's a reasonable thing to say that if a `UIString` has a special `key` (e.g. one that isn't just the `format`), it deserves a comment.
> >> 
> >> You'd also need to update `WI.UIString` to accommodate this, probably using some `arguments.length` checks :)
> > 
> > We also have this case:
> > 
> > 4. [format, key] 
> > WI.UIString("%d Passed", "%d Passed (singular)")
> > WI.UIString("%d Passed", "%d Passed (plural)")
> > 
> > We would have to add an empty comment here as the 3rd argument to make it work.
> > 
> > Do you still think it's worth doing?
> 
> Hmmm.  Good point.  Maybe we can be smart about that and say that if the
> `comment` starts with `format`, it should be treated as a `key` instead? 
> It's an odd "quirk", but it would give us nice flexible functionality.

I think this is overly complex and unpredictable.

> Regardless, I think the empty `key` case is a much worse experience than
> requiring a `comment` if `key` is set.  What do you think?

With this patch, there's going to be more cases with [format, comment] than [format, key], so I agree.

[Nikita Vasilyev]

Created attachment 367263 [details]
Patch

[Devin Rousso]

Comment on attachment 367263 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/LoadLocalizedStrings.js:52
> +        key = undefined;
> +        comment = key;

This is backwards.  You're invalidating `key` before it's had a chance to be copied/moved.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #20)
> Comment on attachment 367263 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367263&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/LoadLocalizedStrings.js:52
> > +        key = undefined;
> > +        comment = key;
> 
> This is backwards.  You're invalidating `key` before it's had a chance to be
> copied/moved.

Whoops 🤦‍♂️

[Nikita Vasilyev]

Created attachment 367267 [details]
Patch

[Devin Rousso]

Comment on attachment 367267 [details]
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");
    };

    ...
```

[Nikita Vasilyev]

Comment on attachment 367267 [details]
Patch

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

Thanks for reviewing!

>> 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)
> ```

This may cause deoptimization. https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments - this is about V8 but it used to (and may still) be the same for JSC.

>> 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");
>     };
> 
>     ...
> ```

I'm not convinced.

> ... that way you'd only need to know the function name (not the exact key for it).

Why knowing function name is better than the key? You'd need to know the exact function name the same way you'd need to know the key.
Passing keys makes it clear what text shows in the UI just by looking at the callsite.

> 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).

If you change a string you'd likely have to change the function name as well.

[Devin Rousso]

Comment on attachment 367267 [details]
Patch

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

>>> 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)
>> ```
> 
> This may cause deoptimization. https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments - this is about V8 but it used to (and may still) be the same for JSC.

In that link, `arguments.length` is specifically called out as being safe.  That's the only usage I'm suggesting, as it avoids the case where `WI.UIString` is called with `comment` as `undefined` (which we shouldn't do, but that would be somewhat unexpected behavior).

>>> 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");
>>     };
>> 
>>     ...
>> ```
> 
> I'm not convinced.

A function name is much more "unique" than a string (especially when dealing with IDEs).  You're right that either way you'd need to remember something "special", but I'd much rather remember a function name than a string.  Also, if I got "special" name wrong, I'd rather that an error be thrown in the UI (e.g. function not found) than a "LOCALIZED STRING NOT FOUND" be shown.

Part of my objection to this approach is that we don't have anything like this anywhere else in Web Inspector (or other parts of WebKit).  When we need to uniquely identify things, we create enums (e.g. any of our `WI.*.Event` objects) or (in the case of C++ localization) we create a unique function per "thing".  We should be matching that style.  Frankly, if the function name doesn't somewhat indicate what will end up being displayed in the UI, we need a better function name :|

[Nikita Vasilyev]

Comment on attachment 367267 [details]
Patch

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

>>>> 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)
>>> ```
>> 
>> This may cause deoptimization. https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments - this is about V8 but it used to (and may still) be the same for JSC.
> 
> In that link, `arguments.length` is specifically called out as being safe.  That's the only usage I'm suggesting, as it avoids the case where `WI.UIString` is called with `comment` as `undefined` (which we shouldn't do, but that would be somewhat unexpected behavior).

The 1st example there is exactly my case.

    3.1. Reassigning a defined parameter while also mentioning arguments in the body (in sloppy mode only). Typical example:

    function defaultArgsReassign(a, b) {
         if (arguments.length < 2) b = 5;
    }

Anyway, I do like `arguments.length === 2` more. I think it's also more readable. I'll add "use strict".

>>>> 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");
>>>     };
>>> 
>>>     ...
>>> ```
>> 
>> I'm not convinced.
> 
> A function name is much more "unique" than a string (especially when dealing with IDEs).  You're right that either way you'd need to remember something "special", but I'd much rather remember a function name than a string.  Also, if I got "special" name wrong, I'd rather that an error be thrown in the UI (e.g. function not found) than a "LOCALIZED STRING NOT FOUND" be shown.
> 
> Part of my objection to this approach is that we don't have anything like this anywhere else in Web Inspector (or other parts of WebKit).  When we need to uniquely identify things, we create enums (e.g. any of our `WI.*.Event` objects) or (in the case of C++ localization) we create a unique function per "thing".  We should be matching that style.  Frankly, if the function name doesn't somewhat indicate what will end up being displayed in the UI, we need a better function name :|

OK, this makes sense.

[Nikita Vasilyev]

Created attachment 368218 [details]
Patch

[Devin Rousso]

Comment on attachment 368218 [details]
Patch

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

r=me, nice work!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1121
> +/* Amount of data sent over the network */

Maybe add "for a single resource" at the end?

> Tools/Scripts/extract-localizable-js-strings:78
> +        # Allow: WI.UIString(string, comment)
> +        #        WI.UIString(string, key, comment)

So I just realized something.  There's no real need for a version that has `WI.UIString(string, key, comment)` because the `comment` can BE the `key`.  Both `comment` and `key` are basic strings, so why don't we just always have the `comment` be the `key` and completely drop support for `key`.

```
    WI.UIString("layout", "Layout", "Layout @ Timeline record", "Layout phase timeline records")
```
is really no different than
```
    WI.UIString("layout", "Layout phase timeline records")
```

It's fine if you don't want to make that change in this patch, but I think it's a bit of a 🤦‍♂️ on my part.

[Nikita Vasilyev]

Created attachment 369078 [details]
Patch

[Nikita Vasilyev]

Making comments keys may work but I'll explore it in a follow-up.

[WebKit Commit Bot]

Comment on attachment 369078 [details]
Patch

Clearing flags on attachment: 369078

Committed r244952: <https://trac.webkit.org/changeset/244952>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r244952 for reason:

Caused inspector to appear blank.

Committed r245060: <https://trac.webkit.org/changeset/245060>

[Nikita Vasilyev]

Created attachment 370452 [details]
Patch

The same patch, rebaselined.

[WebKit Commit Bot]

Comment on attachment 370452 [details]
Patch

Clearing flags on attachment: 370452

Committed r245665: <https://trac.webkit.org/changeset/245665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 198203

[Devin Rousso]

Comment on attachment 370452 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/LoadLocalizedStrings.js:99
> +    return WI.UIString("Assertion Failures", "Break (pause) when console.assert() fails");

The reason this patch isn't working is because we strip all `console.assert` lines when creating a production build.  Setting `my $shouldCombineMain = 1;` in WebInspectorUI/Scripts/copy-user-interface-resources.pl will reproduce.

<https://webkit.org/b/198207>

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #38)
> Comment on attachment 370452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370452&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/LoadLocalizedStrings.js:99
> > +    return WI.UIString("Assertion Failures", "Break (pause) when console.assert() fails");
> 
> The reason this patch isn't working is because we strip all `console.assert`
> lines when creating a production build.  Setting `my $shouldCombineMain =
> 1;` in WebInspectorUI/Scripts/copy-user-interface-resources.pl will
> reproduce.
> 
> <https://webkit.org/b/198207>

Thank you for resolving this!

[Nikita Vasilyev]

Created attachment 370779 [details]
Patch

I was able to confirm that setting `my $shouldCombineMain = 1;` with my patch broke Web Inspector and that bug 198207 fixed it.

Devin, is it safe to land my patch now?

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #40)
> Created attachment 370779 [details]
> Patch
> 
> I was able to confirm that setting `my $shouldCombineMain = 1;` with my
> patch broke Web Inspector and that bug 198207 fixed it.
> 
> Devin, is it safe to land my patch now?

If you've already confirmed it should work then ya!

[WebKit Commit Bot]

Comment on attachment 370779 [details]
Patch

Clearing flags on attachment: 370779

Committed r245827: <https://trac.webkit.org/changeset/245827>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.