[webkit-reviews] review denied: [Bug 24150] Add virtual ScriptExecutionContext::encoding() : [Attachment 27999] Updated patch.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 24150: Add virtual ScriptExecutionContext::encoding()
https://bugs.webkit.org/show_bug.cgi?id=24150

Attachment 27999: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=27999&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +    String inputEncoding() const { return encoding(); }
> +    String charset() const { return encoding(); }
> +    String characterSet() const { return encoding(); }

You need to use "return Document::encoding();" syntax to avoid virtual
dispatch. Please also consider making encoding() private (that would improve
safety, but decrease clarity somewhat, as the functions won't be near each
other any more).

> +    // FIXME: nested workers need loading support. Consider adopting
ThreadableLoader here.

Capital "N" is needed in "nested".

> -    // FIXME: does this need to provide a charset, like
Document::completeURL does?
> -    return KURL(m_location->url(), url);
> +
> +    if (m_encoding.isNull())
> +	   return KURL(m_location->url(), url);
> +
> +    return KURL(m_location->url(), url, TextEncoding(m_encoding));
>  }

Have you tested that this is what Firefox does? I wouldn't be surprised if it
always used UTF-8, and didn't inherit the encoding from the document at all.

This change needs an automated test.

> +	   String m_encoding;

Why is it better to store it as String, not as TextEncoding?

> -	   WorkerThread(const KURL&, const String& userAgent, const String&
sourceCode, WorkerObjectProxy*);
> +	   WorkerThread(const KURL&, const String& userAgent,  const String&
encoding, const String& sourceCode, WorkerObjectProxy*);

Two spaces here.

r-, because this needs an automated test (XMLHttpRequest URL completion for
now, but it's worth testing how Firefox decodes nested worker scripts and
importScripts() content).