Bug 197597 - Web Inspector: replace UIString key with comment
Summary: Web Inspector: replace UIString key with comment
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on: 195132
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-04 22:47 PDT by Devin Rousso
Modified: 2022-03-26 04:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (50.24 KB, patch)
2019-05-04 23:09 PDT, Devin Rousso
bburg: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-05-04 22:47:37 PDT
<https://bugs.webkit.org/show_bug.cgi?id=195132#c28> :P
Comment 1 Devin Rousso 2019-05-04 23:09:37 PDT
Created attachment 369089 [details]
Patch
Comment 2 Devin Rousso 2019-05-04 23:10:35 PDT
Comment on attachment 369089 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:81
> +localizedStrings["A context menu item to force (override) a DOM node's pseudo-classes"] = "Forced Pseudo-Classes";

The only negative I can see of this approach is that it "breaks" the alphabetization of UI strings.  This could be solved by ordering based on the format string rather than the key, but I don't think it's that big of an issue either way.
Comment 3 Timothy Hatcher 2019-05-09 16:44:25 PDT
Have we talked to the localization folks about this? They might need to update tooling to support this.
Comment 4 Devin Rousso 2019-08-23 10:21:38 PDT
(In reply to Timothy Hatcher from comment #3)
> Have we talked to the localization folks about this? They might need to update tooling to support this.
What would need to be updated?  AFAIU, they just inject the localizedStrings.js file and replace the text inside the ` = "...";`.

Previously, we'd put any "comment" as an actual JS comment on the line above, and often the key for the UIString would be almost the same string (if not identical).  This patch "unifies" the comment and key into the same value.

For example:
```
    /* Settings tab checkbox label for whether searches should be case sensitive. */
    localizedStrings["Case Sensitive @ Settings"] = "Case Sensitive";
```
becomes
```
    localizedStrings["Settings tab checkbox label for whether searches should be case sensitive"] = "Case Sensitive";
```
Comment 5 BJ Burg 2019-10-01 08:25:43 PDT
Comment on attachment 369089 [details]
Patch

While callsites are a bit shorter, I don't think this is a good change. Whenever a comment string changes or is added, localizers would need to re-localize the key, because the comment string becomes the key.

Unrelated, but I find the @ [Location] formatting to be confusing. Most localization files I've seen on macOS use the format "Search: (Settings tab)" rather than "Search: @ Settings"
Comment 6 Matt Baker 2019-10-01 12:25:51 PDT
(In reply to Brian Burg from comment #5)
> Comment on attachment 369089 [details]
> Patch
> 
> While callsites are a bit shorter, I don't think this is a good change.
> Whenever a comment string changes or is added, localizers would need to
> re-localize the key, because the comment string becomes the key.
> 
> Unrelated, but I find the @ [Location] formatting to be confusing. Most
> localization files I've seen on macOS use the format "Search: (Settings
> tab)" rather than "Search: @ Settings"

Agree.