WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 195132
Web Inspector: Provide UIString descriptions to improve localizations
https://bugs.webkit.org/show_bug.cgi?id=195132
Summary
Web Inspector: Provide UIString descriptions to improve localizations
Nikita Vasilyev
Reported
2019-02-27 16:51:07 PST
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.
Attachments
WIP
(1.79 KB, patch)
2019-02-27 17:09 PST
,
Nikita Vasilyev
nvasilyev
: commit-queue-
Details
Formatted Diff
Diff
WIP
(25.36 KB, patch)
2019-02-27 17:09 PST
,
Nikita Vasilyev
nvasilyev
: commit-queue-
Details
Formatted Diff
Diff
Patch
(40.50 KB, patch)
2019-02-28 16:58 PST
,
Nikita Vasilyev
nvasilyev
: review-
nvasilyev
: commit-queue-
Details
Formatted Diff
Diff
Patch
(40.91 KB, patch)
2019-04-10 13:21 PDT
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Patch
(39.81 KB, patch)
2019-04-10 13:30 PDT
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Patch
(42.30 KB, patch)
2019-04-11 16:31 PDT
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Patch
(42.30 KB, patch)
2019-04-11 17:21 PDT
,
Nikita Vasilyev
hi
: review-
Details
Formatted Diff
Diff
Patch
(43.78 KB, patch)
2019-04-25 00:32 PDT
,
Nikita Vasilyev
hi
: review+
Details
Formatted Diff
Diff
Patch
(43.82 KB, patch)
2019-05-04 17:10 PDT
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Patch
(43.34 KB, patch)
2019-05-22 15:26 PDT
,
Nikita Vasilyev
hi
: review-
Details
Formatted Diff
Diff
Patch
(43.37 KB, patch)
2019-05-28 13:26 PDT
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Nikita Vasilyev
Comment 1
2019-02-27 17:09:20 PST
Created
attachment 363159
[details]
WIP
Nikita Vasilyev
Comment 2
2019-02-27 17:09:55 PST
Created
attachment 363160
[details]
WIP
Radar WebKit Bug Importer
Comment 3
2019-02-27 17:19:24 PST
<
rdar://problem/48457817
>
Nikita Vasilyev
Comment 4
2019-02-27 17:19:37 PST
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 5
2019-02-27 17:24:41 PST
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 6
2019-02-27 23:39:10 PST
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 7
2019-02-28 00:13:58 PST
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
Comment 8
2019-02-28 16:58:27 PST
Created
attachment 363278
[details]
Patch
Devin Rousso
Comment 9
2019-03-01 16:04:51 PST
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
Comment 10
2019-03-01 16:58:16 PST
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
Comment 11
2019-03-01 17:07:21 PST
(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
Comment 12
2019-04-10 13:21:01 PDT
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
Comment 13
2019-04-10 13:21:42 PDT
Created
attachment 367154
[details]
Patch
Nikita Vasilyev
Comment 14
2019-04-10 13:30:29 PDT
Created
attachment 367155
[details]
Patch Rebaselined.
Devin Rousso
Comment 15
2019-04-10 15:15:27 PDT
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
Comment 16
2019-04-10 16:52:58 PDT
(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 17
2019-04-10 17:02:19 PDT
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
Comment 18
2019-04-10 17:12:29 PDT
(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
Comment 19
2019-04-11 16:31:29 PDT
Created
attachment 367263
[details]
Patch
Devin Rousso
Comment 20
2019-04-11 16:37:30 PDT
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
Comment 21
2019-04-11 17:21:10 PDT
(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
Comment 22
2019-04-11 17:21:32 PDT
Created
attachment 367267
[details]
Patch
Devin Rousso
Comment 23
2019-04-22 10:14:21 PDT
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 24
2019-04-23 18:40:51 PDT
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 25
2019-04-24 11:06:57 PDT
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 26
2019-04-24 13:25:20 PDT
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
Comment 27
2019-04-25 00:32:52 PDT
Created
attachment 368218
[details]
Patch
Devin Rousso
Comment 28
2019-05-03 23:13:17 PDT
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
Comment 29
2019-05-04 17:10:29 PDT
Created
attachment 369078
[details]
Patch
Nikita Vasilyev
Comment 30
2019-05-04 17:11:52 PDT
Making comments keys may work but I'll explore it in a follow-up.
WebKit Commit Bot
Comment 31
2019-05-04 18:31:16 PDT
Comment on
attachment 369078
[details]
Patch Clearing flags on attachment: 369078 Committed
r244952
: <
https://trac.webkit.org/changeset/244952
>
WebKit Commit Bot
Comment 32
2019-05-04 18:31:18 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 33
2019-05-08 10:44:53 PDT
Reverted
r244952
for reason: Caused inspector to appear blank. Committed
r245060
: <
https://trac.webkit.org/changeset/245060
>
Nikita Vasilyev
Comment 34
2019-05-22 15:26:48 PDT
Created
attachment 370452
[details]
Patch The same patch, rebaselined.
WebKit Commit Bot
Comment 35
2019-05-22 17:44:49 PDT
Comment on
attachment 370452
[details]
Patch Clearing flags on attachment: 370452 Committed
r245665
: <
https://trac.webkit.org/changeset/245665
>
WebKit Commit Bot
Comment 36
2019-05-22 17:44:51 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 37
2019-05-23 18:19:43 PDT
Re-opened since this is blocked by
bug 198203
Devin Rousso
Comment 38
2019-05-23 19:18:14 PDT
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
Comment 39
2019-05-23 20:42:47 PDT
(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
Comment 40
2019-05-28 13:26:02 PDT
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
Comment 41
2019-05-28 13:59:53 PDT
(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 42
2019-05-28 15:22:27 PDT
Comment on
attachment 370779
[details]
Patch Clearing flags on attachment: 370779 Committed
r245827
: <
https://trac.webkit.org/changeset/245827
>
WebKit Commit Bot
Comment 43
2019-05-28 15:22:29 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug