Web Inspector: ResourceContentView.js incorrectly contains call to WebInspector.UIString with a variable parameter

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

UIString only takes in String literals. This is generating an error with update-webkit-localizable-strings.

[Radar WebKit Bug Importer]

<rdar://problem/19417623>

[Jonathan Wells]

Created attachment 244295 [details]
[PATCH] fix.

[Joseph Pecoraro]

Comment on attachment 244295 [details]
[PATCH] fix.

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

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:102
> +        this.element.appendChild(WebInspector.createMessageTextView(error.message, true));

While this is correct, I think there may be an actual bug here.

This is called from:

    // Request content last so the spinner will always be removed in case the content is immediately available.
    resource.requestContent().then(this._contentAvailable.bind(this)).catch(this._contentError.bind(this));

So it looks like the "error.message" here comes from a requestContent rejected promise:

    requestContent: function()
    {
        if (this._finished)
            return WebInspector.SourceCode.prototype.requestContent.call(this);

        if (this._failed)
            return Promise.reject(new Error("An error occurred trying to load the resource."));
        ...
    }

So, shouldn't that Error message be localized for this view to have a localized string?

    Promise.reject(new Error(WebInspector.UIString("An error occurred trying to load the resource.")));

If that is the case, are there other error strings that may need to be localized?

[Brian Burg]

Comment on attachment 244295 [details]
[PATCH] fix.

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:102
>> +        this.element.appendChild(WebInspector.createMessageTextView(error.message, true));
> 
> While this is correct, I think there may be an actual bug here.
> 
> This is called from:
> 
>     // Request content last so the spinner will always be removed in case the content is immediately available.
>     resource.requestContent().then(this._contentAvailable.bind(this)).catch(this._contentError.bind(this));
> 
> So it looks like the "error.message" here comes from a requestContent rejected promise:
> 
>     requestContent: function()
>     {
>         if (this._finished)
>             return WebInspector.SourceCode.prototype.requestContent.call(this);
> 
>         if (this._failed)
>             return Promise.reject(new Error("An error occurred trying to load the resource."));
>         ...
>     }
> 
> So, shouldn't that Error message be localized for this view to have a localized string?
> 
>     Promise.reject(new Error(WebInspector.UIString("An error occurred trying to load the resource.")));
> 
> If that is the case, are there other error strings that may need to be localized?

I would agree with Joe. We should localize any string passed to the Error constructor if it is thrown and possibly displayed to the user.

However, I'm not sure that these exceptions would be shown to the user. Won't they instead appear in Inspector^2? If we don't plan to localize those strings (which exist mainly for our benefit and for testing) then this patch is good.
In either case, the explanation should be added to the changelog and the WebInspectorUI style guide for future reference.

[Jonathan Wells]

Comment on attachment 244295 [details]
[PATCH] fix.

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

>>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:102
>>> +        this.element.appendChild(WebInspector.createMessageTextView(error.message, true));
>> 
>> While this is correct, I think there may be an actual bug here.
>> 
>> This is called from:
>> 
>>     // Request content last so the spinner will always be removed in case the content is immediately available.
>>     resource.requestContent().then(this._contentAvailable.bind(this)).catch(this._contentError.bind(this));
>> 
>> So it looks like the "error.message" here comes from a requestContent rejected promise:
>> 
>>     requestContent: function()
>>     {
>>         if (this._finished)
>>             return WebInspector.SourceCode.prototype.requestContent.call(this);
>> 
>>         if (this._failed)
>>             return Promise.reject(new Error("An error occurred trying to load the resource."));
>>         ...
>>     }
>> 
>> So, shouldn't that Error message be localized for this view to have a localized string?
>> 
>>     Promise.reject(new Error(WebInspector.UIString("An error occurred trying to load the resource.")));
>> 
>> If that is the case, are there other error strings that may need to be localized?
> 
> I would agree with Joe. We should localize any string passed to the Error constructor if it is thrown and possibly displayed to the user.
> 
> However, I'm not sure that these exceptions would be shown to the user. Won't they instead appear in Inspector^2? If we don't plan to localize those strings (which exist mainly for our benefit and for testing) then this patch is good.
> In either case, the explanation should be added to the changelog and the WebInspectorUI style guide for future reference.

These are shown to the user if you select a resource that had an error loading. For images, in English, this reads:

"Failed to load the resource: The requested URL was not found on this server."

For scripts, this is localized as this internal error is not available. Resource.js:682:

return Promise.reject(new Error("An error occurred trying to load the resource."));

It is shown to the user in bright red text in both cases when the resource is selected. I can make them both have the localized message "An error occurred trying to load the resource." for consistency... though I do wish that this message was more specific about the nature of the issue.

[Timothy Hatcher]

Comment on attachment 244295 [details]
[PATCH] fix.

I think it is odd to use the Error class for a string that gets displayed. It should be something else, if not just a plain string.

[Timothy Hatcher]

Comment on attachment 244295 [details]
[PATCH] fix.

We likely should not reject for cases when the page failed to load a Resource. Those are page errors, not Inspector errors. Page errors likely should fulfill the Promise, not reject it.

[Jonathan Wells]

Created attachment 244389 [details]
[PATCH] fix.

Fix based on comments. Note that the localized strings files have not been updated. I haven't been able to properly configure my machine to diff those files despite many efforts.

[Timothy Hatcher]

Comment on attachment 244389 [details]
[PATCH] fix.

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

You don't need to post a readable diff for localizedString.js, webkit-patch or git format-patch will output a binary patch just fine. The trick to use iconv is just helpful for seeing the diff with git diff.

> Source/WebInspectorUI/UserInterface/Models/Script.js:119
> +            return Promise.reject(new Error(WebInspector.UIString("There is no identifier to request content with.")));

No need to localize since this error should not be shown to the user.

> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:186
> +        var errorMessage = WebInspector.UIString("Needs to be implemented by a subclass.");
> +        console.error(errorMessage);
> +        return Promise.reject(new Error(errorMessage));

No need to be localized. Revert.

[Jonathan Wells]

Created attachment 244477 [details]
[PATCH] fix 2, with attempted localized strings addition.

[WebKit Commit Bot]

Attachment 244477 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 244477 [details]
[PATCH] fix 2, with attempted localized strings addition.

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

> Source/WebCore/ChangeLog:10
> +        * English.lproj/Localizable.strings:

You should split this script sorting out and land separately.

[Jonathan Wells]

Created attachment 245179 [details]
[PATCH] separated out localized strings, reviewed patch.

I'll submit the updated localized strings separately.

[Joseph Pecoraro]

(In reply to comment #13)
> Created attachment 245179 [details]
> [PATCH] separated out localized strings, reviewed patch.
> 
> I'll submit the updated localized strings separately.

Why? Can you just land manually?

[Jonathan Wells]

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 245179 [details]
> > [PATCH] separated out localized strings, reviewed patch.
> > 
> > I'll submit the updated localized strings separately.
> 
> Why? Can you just land manually?

I suppose so.

[Jonathan Wells]

Committed r178970: <http://trac.webkit.org/changeset/178970>

[Joseph Pecoraro]

Comment on attachment 245179 [details]
[PATCH] separated out localized strings, reviewed patch.

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

> Source/WebInspectorUI/UserInterface/Models/Resource.js:683
> +            return Promise.resolve({ error: WebInspector.UIString("An error occurred trying to load the resource.") });

Style: Our style for object literals is {name: property}.

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:93
> +            this._contentError({ message: parameters.error });

Style: ditto.