Ignore UTF-8's BOM when parsing userscript's metadata.

Ignore UTF-8's BOM when parsing userscript's metadata.

BUG=27333
TEST=UserScriptTest and check the issue does not happen against userscipt encoded in UTF-8 with BOM..

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

[hayato, 2009-11-20 10:00:13]

Hi Aaron,

Could you review this?

[Aaron Boodman, 2009-11-20 22:46:15]

+jsin

Jungshik, do you know if we have any code in Chromium already that can be used
to detect/skip the BOM in a string?

http://codereview.chromium.org/420001/diff/1/2
File chrome/common/extensions/user_script.cc (right):

http://codereview.chromium.org/420001/diff/1/2#newcode69
Line 69: content->starts_with(kUtf8ByteOrderMark))
It seems like we should have some common code somewhere that knows how to do
this properly. Adding someone to the review who probably knows.

http://codereview.chromium.org/420001/diff/1/3
File chrome/common/extensions/user_script.h (right):

http://codereview.chromium.org/420001/diff/1/3#newcode101
Line 101: base::StringPiece content_;
This can't be changed to StringPiece: This member is used when the
UserScript::File class owns the string memory.

The rest of the system assumes that user scripts are UTF-8 encoded. We should
get rid of the BOM as early as possible when receiving it from an external
source. 

The best place would be UserScriptMaster::ScriptReloader::ParseMetadataHeader.

[jungshik at Google, 2009-11-20 23:58:14]

http://codereview.chromium.org/420001/diff/1/2
File chrome/common/extensions/user_script.cc (right):

http://codereview.chromium.org/420001/diff/1/2#newcode69
Line 69: content->starts_with(kUtf8ByteOrderMark))
On 2009/11/20 22:46:15, Aaron Boodman wrote:
> It seems like we should have some common code somewhere that knows how to do
> this properly. Adding someone to the review who probably knows.

We don't have a function like this anywhere because there hasn't been a need.
However, it might be a good idea to put this in base/string_util (even though
there's a single caller at the moment).

[hayato, 2009-11-24 09:08:07]

Thank you for the review.

I rewrote this change completely as you suggested.
Now, I check UTF-8's BOM in
UserScriptMaster::ScriptREloader::ParseMetadataHeader.
It would be best place because ConvertUserScriptToExtension call that function
directly without using UserScript::File.

http://codereview.chromium.org/420001/diff/1/2
File chrome/common/extensions/user_script.cc (right):

http://codereview.chromium.org/420001/diff/1/2#newcode69
chrome/common/extensions/user_script.cc:69:
content->starts_with(kUtf8ByteOrderMark))
I added 'const char[] kUtf8ByteOrderMark' to base/string_util.

On 2009/11/20 23:58:15, Jungshik Shin wrote:
> On 2009/11/20 22:46:15, Aaron Boodman wrote:
> > It seems like we should have some common code somewhere that knows how to do
> > this properly. Adding someone to the review who probably knows.
> 
> We don't have a function like this anywhere because there hasn't been a need.
> However, it might be a good idea to put this in base/string_util (even though
> there's a single caller at the moment).  
> 
>

http://codereview.chromium.org/420001/diff/1/3
File chrome/common/extensions/user_script.h (right):

http://codereview.chromium.org/420001/diff/1/3#newcode101
chrome/common/extensions/user_script.h:101: base::StringPiece content_;
Thank you. I reverted it.

On 2009/11/20 22:46:15, Aaron Boodman wrote:
> This can't be changed to StringPiece: This member is used when the
> UserScript::File class owns the string memory.
> 
> The rest of the system assumes that user scripts are UTF-8 encoded. We should
> get rid of the BOM as early as possible when receiving it from an external
> source. 
> 
> The best place would be UserScriptMaster::ScriptReloader::ParseMetadataHeader.

[Aaron Boodman, 2009-11-24 20:33:39]

LGTM with two nits.

http://codereview.chromium.org/420001/diff/6001/6004
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/420001/diff/6001/6004#newcode49
chrome/browser/extensions/user_script_master.cc:49: // Skip UTF-8's BOM.
Nit: can you add newlines before and after this new chunk of logic?

http://codereview.chromium.org/420001/diff/6001/6004#newcode52
chrome/browser/extensions/user_script_master.cc:52: size_t line_end = 0;
Nit: it would be more clear to initialize this to line_start.

[hayato, 2009-11-25 03:31:19]

Thank you for the review. I'll land this.

http://codereview.chromium.org/420001/diff/6001/6004
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/420001/diff/6001/6004#newcode49
chrome/browser/extensions/user_script_master.cc:49: // Skip UTF-8's BOM.
On 2009/11/24 20:33:39, Aaron Boodman wrote:
> Nit: can you add newlines before and after this new chunk of logic?

Done.

http://codereview.chromium.org/420001/diff/6001/6004#newcode52
chrome/browser/extensions/user_script_master.cc:52: size_t line_end = 0;
On 2009/11/24 20:33:39, Aaron Boodman wrote:
> Nit: it would be more clear to initialize this to line_start.

Done.