Bug 52295 – Web Inspector: Add check-inspector-strings script, fix bugs found

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mikhail Naganov]

Incidentally I discovered that localizedStrings.js contains a number of obsolete strings. Then I wrote a simple script for diffing strings, and also found that several UI strings are absent in localizedStrings.js.

The script detects 3 kinds of problematic strings:

  - new strings: strings that are used in WebInspector.UIString, but absent in localizedStrings.js

  - unused strings: strings that present in localizedStrings.js, but can't be found as string literals in Inspector scripts (not only in WebInspector.UIString calls!)

  - suspicious strings: strings that present in localizedStrings.js, but present as string literals. This may mean that either someone forgot to use WebInspector.UIString, OR this string is used in more complex scenario (like returned from a function and then passed to WebInspector.UIString) -- in this case the string must be prefixed with /*@LS*/ comment.

After fixing all the bugs found, the only unused string is "Drag" from BreakPointsSidebarPane.js:272, which may be used in future.

[Mikhail Naganov]

Created attachment 78688 [details]
patch

Strings removed from localizedStrings.js:

"%s (%s transferred)"                                                           
"Any Request"                                                                   
"Enable resource tracking"                                                      
"Enabling resource tracking will reload the page and make page loading slower." 
"GRAPHS"                                                                        
"Media"                                                                         
"Prototype"                                                                     
"RESOURCES"                                                                     
"Resource tracking disabled. Click to enable."                                  
"Resource tracking enabled. Click to disable."                                  
"Set XHR Breakpoint"                                                            
"Sort by Duration"                                                              
"Sort by End Time"                                                              
"Sort by Latency"                                                               
"Sort by Response Time"                                                         
"Sort by Size"                                                                  
"Sort by Start Time"                                                            
"Sort by Transfer Size"                                                         
"Stop on Attributes Modifications"                                              
"Stop on Node Removal"                                                          
"Stop on Subtree Modifications"                                                 
"Storage"                                                                       
"TIMELINES"                                                                     
"Timing."                                                                       
"You need to enable resource tracking to use this panel."                       
"inline stylesheet"                                                             

Strings added to localizedStrings.js:

"(from cache)"                             
"Any XHR"                                  
"Database not found."                      
"File System is disabled."                 
"File System"                              
"Recording"                                
"Request Cookies"                          
"Response Cookies"                         
"Reveal in Elements Panel"                 
"This request has no detailed timing info."
"Timing"

[Mikhail Naganov]

Forgot that I also added these 3 lines discovered after I prefixed strings from Resource.Type.toString with /*@LR*/:

"media"
"websocket"
"xhr"

[Mikhail Naganov]

    2011-01-12  Mikhail Naganov  <[e-mail]>
    
            Reviewed by Pavel Feldman.
    
            Add check-inspector-strings script.
            Fix inconsistencies in Inspector strings found by the script.
    
            https://bugs.webkit.org/show_bug.cgi?id=52295
    
            * English.lproj/localizedStrings.js:
            * inspector/front-end/BreakpointsSidebarPane.js:
            (WebInspector.EventListenerBreakpointsSidebarPane):
            * inspector/front-end/ProfilesPanel.js:
            (WebInspector.ProfilesPanel.prototype.setRecordingProfile):
            * inspector/front-end/Resource.js:
            (WebInspector.Resource.Type.toString):
            * inspector/front-end/ResourcesPanel.js:
            (WebInspector.ResourceRevisionTreeElement):
            * inspector/front-end/utilities.js:
            ():
            * Scripts/check-inspector-strings: Added.

[Joseph Pecoraro]

Mikhail, thanks for doing this. It is more useful, when landing, to provide a link
to the trac / provide the SVN revision number of when this landed then providing
the ChangeLog entry which can be found either at that link or in an attached patch.

Here you could link to:
http://trac.webkit.org/changeset/75613

This looks great! Thanks for doing this. I just recently opened the following, which
I will now move to a duplicate of this:
<http://webkit.org/b/52196> Web Inspector: Some Localized Strings are Outdated

It looks like you most of what I noticed, but I still had at least one difference:

  There is "Assertion failed: " in localizedStrings.js and "Assertion failed:" without
  the trailing space in code. Did your script catch this, or maybe I am looking at
  an older tree.

[Joseph Pecoraro]

*** Bug 52196 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Comment on attachment 78688 [details]
patch

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

> Source/WebCore/ChangeLog:8
> [phone]  Mikhail Naganov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix inconsistencies in Inspector strings found by the new check-inspector-strings script.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=52295
> +

It would be very helpful to point out here that the /*@LS*/ "sigil" which annotates a String
that could be localized but is being used outside of known localized functions.

For instance, if I started working on inspector code, I would be very confused about this
comment and I'd do a git annotate to see this ChangeLog / commit message. A comment
here would be helpful.

[Joseph Pecoraro]

> "sigil" which annotates a String

Should just be: annotates a String

[Mikhail Naganov]

Sorry, there was a "mid-air collision" while I was submitting my comment, and for the second time I forgot to provide a Trac link. Thanks for adding it!

Yes, "Assertion failed:" is OK now, it was fixed by Yury in http://trac.webkit.org/changeset/73607

[Joseph Pecoraro]

(In reply to comment #8)
> Sorry, there was a "mid-air collision" while I was submitting my comment,
> and for the second time I forgot to provide a Trac link. Thanks for adding it!
> 
> Yes, "Assertion failed:" is OK now, it was fixed by Yury in http://trac.webkit.org/changeset/73607

Excellent, thanks!!

[Mikhail Naganov]

Comment on attachment 78688 [details]
patch

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

>> Source/WebCore/ChangeLog:8
>> +
> 
> It would be very helpful to point out here that the /*@LS*/ "sigil" which annotates a String
> that could be localized but is being used outside of known localized functions.
> 
> For instance, if I started working on inspector code, I would be very confused about this
> comment and I'd do a git annotate to see this ChangeLog / commit message. A comment
> here would be helpful.

I think, the l10n topic deserves its own wiki page. A newbie needs also to know about WebInspector.UIString, about localizedStrings.js, etc...

[Timothy Hatcher]

I don't think the /*@LS*/ technique is a good approch. Instead it should be illegal to pass a non string literal to WebInspector.UIString.

The /*@LS*/ makes the code ugly and as Joe mentioned is confusing.

If the length of WebInspector.UIString is at issue we can have a shorter global function for localized strings that would be prettier, shorter and less confusing than /*@LS*/.

[Timothy Hatcher]

Comment on attachment 78688 [details]
patch

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

> Source/WebCore/inspector/front-end/Resource.js:84
> -                return "document";
> +                return /*@LS*/"document";
>              case this.Stylesheet:
> -                return "stylesheet";
> +                return /*@LS*/"stylesheet";
>              case this.Image:
> -                return "image";
> +                return /*@LS*/"image";
> -                return "font";
>              case this.Script:
> -                return "script";
> +                return /*@LS*/"script";
>              case this.XHR:
> -                return "xhr";
> +                return /*@LS*/"xhr";
>              case this.Media:
> -                return "media";
> +                return /*@LS*/"media";
>              case this.WebSocket:
> -                return "websocket";
> +                return /*@LS*/"websocket";
>              case this.Other:
>              default:
> -                return "other";
> +                return /*@LS*/"other";

These should not be localized. Instead the toUIString should have a similar switch that returns UIStrings. Because these are all lowercase they are not good UIStrings. "xhr" should be "XHR" in the UI. "websocket" should be "WebSocket" or "Web Socket". And the others likely should have Title case. So you are masking a "bug" by doing this.

[Mikhail Naganov]

Comment on attachment 78688 [details]
patch

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

>> Source/WebCore/inspector/front-end/Resource.js:84
>> +                return /*@LS*/"other";
> 
> These should not be localized. Instead the toUIString should have a similar switch that returns UIStrings. Because these are all lowercase they are not good UIStrings. "xhr" should be "XHR" in the UI. "websocket" should be "WebSocket" or "Web Socket". And the others likely should have Title case. So you are masking a "bug" by doing this.

I agree with you about this place, but what about Number.bytesToString? Formatter is passed as a parameter, and there are several uses of bytesToString, where the parameter isn't WebInspector.UIString

[Timothy Hatcher]

Not passing a formatter to bytesToString should be considered a bug. The fact that is falls back to sprintf leads to unlocalized strings. We should change bytesToString (and friends) to always use UIString, even though utilities.js dosen't currently have a dependancy on WebInspector i think it is fine to prevent unlocalized strings.