Encoding API: Add ignoreBOM flag to decoder options

Encoding API: Add ignoreBOM flag to decoder options

An additional option flag was added to the WHATWG Encoding Living
Standard's API to allow callers to disable the minimal BOM handling
the API supports.

BUG=362609
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174918

[jsbell, 2014-04-14 16:30:29]

jshin@ - please take a look?

[jsbell, 2014-04-22 00:06:38]

jshin@ - ping on this one too?

[jsbell, 2014-04-29 21:55:20]

jshin@ - ping?

[jsbell, 2014-05-13 11:08:49]

Actually, since this really just affects the API and not codec output, kbr@ can
you PTAL ?

[jsbell, 2014-05-20 23:59:17]

ping kbr@ or jshin@ ?

[Ken Russell (switch to Gerrit), 2014-05-21 00:18:48]

I'm not really qualified to review this and defer to jshin's review.

[jungshik at Google, 2014-05-27 17:09:11]

https://codereview.chromium.org/232513003/diff/20001/LayoutTests/fast/encodin...
File LayoutTests/fast/encoding/api/textdecoder-ignorebom.html (right):

https://codereview.chromium.org/232513003/diff/20001/LayoutTests/fast/encodin...
LayoutTests/fast/encoding/api/textdecoder-ignorebom.html:34: testCase.encoding +
': Default is to ignore the BOM');
Default is to "eat" the BOM (no BOM in the decoded output), isn't it? 

1. BOM should be absent from decoded string if not ignored'
2. Default is to ignore the BOM

The output is the same for two cases, but #1 and #2 contradict each other in
'English'. 

In both cases above, BOM is not present in the output. Shouldn't #2 read
'ignoreBOM is unset by default'.  Or, am I mistaken somehow? 


Hmm, 'ignoreBOM' is rather confusing as a flag. I read the spec and can
understand the intent, but I guess we should come up with a better name for the
flag. Perhaps, I'll file a bug against the spec.

[jsbell, 2014-05-27 17:40:46]

Thanks for taking a look. Corrected test uploaded.

https://codereview.chromium.org/232513003/diff/20001/LayoutTests/fast/encodin...
File LayoutTests/fast/encoding/api/textdecoder-ignorebom.html (right):

https://codereview.chromium.org/232513003/diff/20001/LayoutTests/fast/encodin...
LayoutTests/fast/encoding/api/textdecoder-ignorebom.html:34: testCase.encoding +
': Default is to ignore the BOM');
On 2014/05/27 17:09:11, Jungshik Shin wrote:
> Default is to "eat" the BOM (no BOM in the decoded output), isn't it? 

Yes.

> 1. BOM should be absent from decoded string if not ignored'
> 2. Default is to ignore the BOM
> 
> The output is the same for two cases, but #1 and #2 contradict each other in
> 'English'. 
> 
> In both cases above, BOM is not present in the output. Shouldn't #2 read
> 'ignoreBOM is unset by default'.  Or, am I mistaken somehow? 

You are correct. I've fixed the assertion description.

> Hmm, 'ignoreBOM' is rather confusing as a flag. I read the spec and can
> understand the intent, but I guess we should come up with a better name for
the
> flag. Perhaps, I'll file a bug against the spec. 

Yes, a spec bug would be good. I don't know if FF has shipped this yet. Also,
note that we prefer flags to default to 'false' since null/undefined are falsy.

Can we land this as-is, but hold off on "Shipping" the feature until the spec
issue is resolved?

[jungshik at Google, 2014-05-27 17:42:03]

On 2014/05/27 17:40:46, jsbell wrote:
> Thanks for taking a look. Corrected test uploaded.
> 
>
https://codereview.chromium.org/232513003/diff/20001/LayoutTests/fast/encodin...
> File LayoutTests/fast/encoding/api/textdecoder-ignorebom.html (right):
> 
>
https://codereview.chromium.org/232513003/diff/20001/LayoutTests/fast/encodin...
> LayoutTests/fast/encoding/api/textdecoder-ignorebom.html:34: testCase.encoding
+
> ': Default is to ignore the BOM');
> On 2014/05/27 17:09:11, Jungshik Shin wrote:
> > Default is to "eat" the BOM (no BOM in the decoded output), isn't it? 
> 
> Yes.
> 
> > 1. BOM should be absent from decoded string if not ignored'
> > 2. Default is to ignore the BOM
> > 
> > The output is the same for two cases, but #1 and #2 contradict each other in
> > 'English'. 
> > 
> > In both cases above, BOM is not present in the output. Shouldn't #2 read
> > 'ignoreBOM is unset by default'.  Or, am I mistaken somehow? 
> 
> You are correct. I've fixed the assertion description.
> 
> > Hmm, 'ignoreBOM' is rather confusing as a flag. I read the spec and can
> > understand the intent, but I guess we should come up with a better name for
> the
> > flag. Perhaps, I'll file a bug against the spec. 
> 
> Yes, a spec bug would be good. I don't know if FF has shipped this yet. Also,
> note that we prefer flags to default to 'false' since null/undefined are
falsy.
> 
> Can we land this as-is, but hold off on "Shipping" the feature until the spec
> issue is resolved?

Yup. LGTM. Thank you for fixing the comment.

[jsbell, 2014-05-27 17:42:31]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-05-27 17:42:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jsbell@chromium.org/232513003/60001

[commit-bot: I haz the power, 2014-05-27 18:56:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9308)

[commit-bot: I haz the power, 2014-05-27 19:54:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-27 19:54:42]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9328)

[jsbell, 2014-05-28 00:35:27]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-05-28 00:35:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jsbell@chromium.org/232513003/60001

[commit-bot: I haz the power, 2014-05-28 01:42:49]

Change committed as 174918