Respect the charset specified in PAC file responses. ProxyScriptFetcher is no...

Respect the charset specified in PAC file responses.

I have updated the documentation of ProxyResolver and ProxyScriptFetcher to indicate that the response must always be given as UTF8. So ProxyScriptFetcher is responsible for any charset conversions internally.

This CL also adds a unit-test to make sure that content-encodings are respected (like gzip). This was not previously broken, but it is a related area (and wasn't being tested.)

BUG=http://crbug.com/22310


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26790

[eroman, 2009-09-19 02:39:58]

wtc: please review this change in its entirety.
jshin: comment for you below :)

http://codereview.chromium.org/210028/diff/1003/1006
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/210028/diff/1003/1006#newcode48
Line 48: void ConvertResponseToUTF8(std::string* bytes, const std::string&
charset) {
Jungshik: Can you review this function?

I couldn't find an existing function that does this in chrome (presumably this
is done in webkit?), but I suspect there is one.

In particular, I am shaky on what the default codepage should be. I suspect it
would be more compatible to use the system's default codepage rather than
ISO-8859-1 as I do here, please advise.

[wtc, 2009-09-21 18:28:16]

LGTM.

http://codereview.chromium.org/210028/diff/1003/1005
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/210028/diff/1003/1005#newcode32
Line 32: // Convert a std::string (UTF8) to a V8 string.
Does this method not work if the std::string 's' is not UTF-8?

http://codereview.chromium.org/210028/diff/1003/1006
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/210028/diff/1003/1006#newcode46
Line 46: // Convert |bytes| (which is encoded by |charset|) to UTF8.
Add "in place".

http://codereview.chromium.org/210028/diff/1003/1006#newcode60
Line 60: // outside of |charset| (i.e. invalid), then substite them with
Typo: substite => "substitute", or actually, "replace"

With "substitute", you should say
  substitute U+FFFD for them

http://codereview.chromium.org/210028/diff/1003/1006#newcode301
Line 301: if (result_code_ != OK) {
Nit: change the test to
  result_code_ == OK
and reverse the two blocks of code.

http://codereview.chromium.org/210028/diff/1003/1004
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/210028/diff/1003/1004#newcode293
Line 293: // (like gzip, bzip, etc.), and apply any charsets to yield
Nit: charsets => charset conversions

[eroman, 2009-09-21 20:06:30]

http://codereview.chromium.org/210028/diff/1003/1005
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/210028/diff/1003/1005#newcode32
Line 32: // Convert a std::string (UTF8) to a V8 string.
On 2009/09/21 18:28:16, wtc wrote:
> Does this method not work if the std::string 's' is not UTF-8?

It will work in the sense that V8 will see a bunch of bytes and assume they are
UTF8 encoded.

If the data is not in fact UTF8 encoded, V8 may see gibberish (i.e. if the bytes
were part of a UTF16 script, the script is going to fail parsing).

http://codereview.chromium.org/210028/diff/1003/1006
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/210028/diff/1003/1006#newcode46
Line 46: // Convert |bytes| (which is encoded by |charset|) to UTF8.
On 2009/09/21 18:28:16, wtc wrote:
> Add "in place".

Done.

http://codereview.chromium.org/210028/diff/1003/1006#newcode301
Line 301: if (result_code_ != OK) {
On 2009/09/21 18:28:16, wtc wrote:
> Nit: change the test to
>   result_code_ == OK
> and reverse the two blocks of code.

Done.

http://codereview.chromium.org/210028/diff/1003/1004
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/210028/diff/1003/1004#newcode293
Line 293: // (like gzip, bzip, etc.), and apply any charsets to yield
On 2009/09/21 18:28:16, wtc wrote:
> Nit: charsets => charset conversions

Done.

[wtc, 2009-09-22 01:15:58]

http://codereview.chromium.org/210028/diff/1003/1005
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/210028/diff/1003/1005#newcode32
Line 32: // Convert a std::string (UTF8) to a V8 string.
On 2009/09/21 20:06:30, eroman wrote:
>
> It will work in the sense that V8 will see a bunch of bytes and assume they
are
> UTF8 encoded.

Are you saying that V8 strings are all UTF-8?

[eroman, 2009-09-22 01:35:24]

http://codereview.chromium.org/210028/diff/1003/1005
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/210028/diff/1003/1005#newcode32
Line 32: // Convert a std::string (UTF8) to a V8 string.
On 2009/09/22 01:15:58, wtc wrote:
> On 2009/09/21 20:06:30, eroman wrote:
> >
> > It will work in the sense that V8 will see a bunch of bytes and assume they
> are
> > UTF8 encoded.
> 
> Are you saying that V8 strings are all UTF-8?

From the documentation of String::New() in v8.h:

  /**
   * Allocates a new string from either utf-8 encoded or ascii data.
   * The second parameter 'length' gives the buffer length.
   * If the data is utf-8 encoded, the caller must
   * be careful to supply the length parameter.
   * If it is not given, the function calls
   * 'strlen' to determine the buffer length, it might be
   * wrong if 'data' contains a null character.
   */
  static Local<String> New(const char* data, int length = -1);


So the expectation is that the byte data you give it, is UTF8. (or ASCII, but
that is just a subset of UTF8).

Internally I am not sure how V8 stores the strings (probably as UTF8), but that
doesn't really matter; the only bit I really care about is what data format
String::New(const char* data, int len) is expecting.

[darin (slow to review), 2009-09-22 03:59:54]

On Mon, Sep 21, 2009 at 6:35 PM, <eroman@chromium.org> wrote:

>
> http://codereview.chromium.org/210028/diff/1003/1005
> File net/proxy/proxy_resolver_v8.cc (right):
>
> http://codereview.chromium.org/210028/diff/1003/1005#newcode32
> Line 32: // Convert a std::string (UTF8) to a V8 string.
> On 2009/09/22 01:15:58, wtc wrote:
>
>> On 2009/09/21 20:06:30, eroman wrote:
>> >
>> > It will work in the sense that V8 will see a bunch of bytes and
>>
> assume they
>
>> are
>> > UTF8 encoded.
>>
>
>  Are you saying that V8 strings are all UTF-8?
>>
>
> From the documentation of String::New() in v8.h:
>
>  /**
>   * Allocates a new string from either utf-8 encoded or ascii data.
>   * The second parameter 'length' gives the buffer length.
>   * If the data is utf-8 encoded, the caller must
>   * be careful to supply the length parameter.
>   * If it is not given, the function calls
>   * 'strlen' to determine the buffer length, it might be
>   * wrong if 'data' contains a null character.
>   */
>  static Local<String> New(const char* data, int length = -1);
>
>
> So the expectation is that the byte data you give it, is UTF8. (or
> ASCII, but that is just a subset of UTF8).
>
> Internally I am not sure how V8 stores the strings (probably as UTF8),
> but that doesn't really matter; the only bit I really care about is what
> data format String::New(const char* data, int len) is expecting.
>
>
You should be good passing UTF-8 data to this function.  (V8 does prefer
UTF-8 internally.)

-Darin



>
> http://codereview.chromium.org/210028
>