Bug 110095

Summary:

Web Inspector: Specify return type of WebInspector.UIString

Product:

WebKit

Reporter:

Eugene Klyuchnikov <eustas>

Component:

Web Inspector (Deprecated)

Assignee:

Eugene Klyuchnikov <eustas>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	pfeldman: review+
Patch	none

Eugene Klyuchnikov

Reported 2013-02-18 01:54:58 PST

1) Extract "fragment builder" from AuditsPanel. This method builds DOM fragment by format and substitutions (and optional formatters). In case of standard string formatters we can use fact that "%s" means literal substitution. 2) Extract "localize" from WebInspector.UIString. Fragment builder should be supplied with localized formats. This was impossible because all localization logic was baked in WebInspector.UIString.

Attachments
Patch (5.23 KB, patch) 2013-02-18 01:57 PST, Eugene Klyuchnikov	no flags	Details Formatted Diff Diff
Patch (5.07 KB, patch) 2013-02-18 04:28 PST, Eugene Klyuchnikov	no flags	Details Formatted Diff Diff
Patch (4.05 KB, patch) 2013-02-21 03:51 PST, Eugene Klyuchnikov	pfeldman: review+	Details Formatted Diff Diff
Patch (3.22 KB, patch) 2013-02-26 08:09 PST, Eugene Klyuchnikov	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Eugene Klyuchnikov

Comment 1 2013-02-18 01:57:52 PST

Created attachment 188825 [details] Patch

Pavel Feldman

Comment 2 2013-02-18 02:24:26 PST

Comment on attachment 188825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188825&action=review > Source/WebCore/inspector/front-end/UIString.js:36 > +WebInspector.localize = function(string) { Should be private. > Source/WebCore/inspector/front-end/UIUtils.js:1106 > +WebInspector.formatFragment = function(format, substitutions, formatters) { { on the next line. If that is extracted, where is the updated client?

Eugene Klyuchnikov

Comment 3 2013-02-18 04:21:25 PST

With extracted _localize method typed code ration increases on 0.4%

Eugene Klyuchnikov

Comment 4 2013-02-18 04:22:52 PST

Comment on attachment 188825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188825&action=review >> Source/WebCore/inspector/front-end/UIString.js:36 >> +WebInspector.localize = function(string) { > > Should be private. Done >> Source/WebCore/inspector/front-end/UIUtils.js:1106 >> +WebInspector.formatFragment = function(format, substitutions, formatters) { > > { on the next line. > > If that is extracted, where is the updated client? Take a look at AuditRuleResult.addFormatted

Eugene Klyuchnikov

Comment 5 2013-02-18 04:28:10 PST

Created attachment 188846 [details] Patch

Pavel Feldman

Comment 6 2013-02-18 05:00:48 PST

Comment on attachment 188846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188846&action=review > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:554 > + console.error(WebInspector.UIString("Unknown scope type: \"%s\"", scope.type)); Why is this a part of the change? > Source/WebCore/inspector/front-end/UIString.js:59 > + return String.vsprintf(WebInspector._localize(string), Array.prototype.slice.call(arguments, 1)); Why do you extract it?

Eugene Klyuchnikov

Comment 7 2013-02-18 09:18:05 PST

Comment on attachment 188846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188846&action=review >> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:554 >> + console.error(WebInspector.UIString("Unknown scope type: \"%s\"", scope.type)); > > Why is this a part of the change? Otherwise js compiler will report type mismatch warning >> Source/WebCore/inspector/front-end/UIString.js:59 >> + return String.vsprintf(WebInspector._localize(string), Array.prototype.slice.call(arguments, 1)); > > Why do you extract it? Localize could be used in cases when we need to use power of String.format but still be localized.

Eugene Klyuchnikov

Comment 8 2013-02-21 03:48:29 PST

Specify return type of WebInspector.UIString. Fix new js-compiler warnings. This change will increase type coverage by 0.4% (to 81%)

Eugene Klyuchnikov

Comment 9 2013-02-21 03:51:55 PST

Created attachment 189504 [details] Patch

Pavel Feldman

Comment 10 2013-02-22 07:30:21 PST

Comment on attachment 189504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189504&action=review > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:554 > + console.error(WebInspector.UIString("Unknown scope type: \"%s\"", scope.type)); There is no need to localize it. Otherwise looks good.

Eugene Klyuchnikov

Comment 11 2013-02-26 08:09:08 PST

Created attachment 190286 [details] Patch

Eugene Klyuchnikov

Comment 12 2013-02-26 08:11:16 PST

Comment on attachment 189504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189504&action=review >> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:554 >> + console.error(WebInspector.UIString("Unknown scope type: \"%s\"", scope.type)); > > There is no need to localize it. Otherwise looks good. Fixed.

Eugene Klyuchnikov

Comment 13 2013-02-26 08:16:12 PST

Committed r144061: <http://trac.webkit.org/changeset/144061>

Note You need to log in before you can comment on or make changes to this bug.