[webkit-reviews] review denied: [Bug 41539] Move BOM handling out of the lexer and parser : [Attachment 60402] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 41539: Move BOM handling out of the lexer and parser
https://bugs.webkit.org/show_bug.cgi?id=41539

Attachment 60402: Patch
https://bugs.webkit.org/attachment.cgi?id=60402&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +		   bool scratch = false;
> +		   m_source =
UString(m_source.rep()->copyStringWithoutBOMs(false, scratch));

It's a little awkward to have this on StringImpl and have to dig down to the
rep(). Normally we call functions directly on UString or String.

> +	       // ECMA-262 calls for stripping all Cf characters, but we only
strip BOM characters.
> +	       // See <https://bugs.webkit.org/show_bug.cgi?id=4931> for
details.

This comment seems to belong somewhere else. Obviously in a function named
copyStringWithoutBOMs we strip only BOMs. I think the comment should ideally go
at some call site that is making the policy decision. Sadly, there are multiple
places!

I think the interface, where you pass in a boolean to tell it whether to skip
the check for BOMs, is really awkward. Can we just break this into multiple
functions? I could see (a) one function that just answers the question of
whether there is a BOM, (b) another that strips the BOMs, and (c) a third for
the case where you want to strip in the unlikely case that a BOM exists. Then
we wouldn't need boolean arguments out or in at all.

CachedScript would use the (a) and (b) functions and the other call sites would
use the (c) function that combines (a) and (b).

The constant byteOrderMark ought to go into CharacterNames.h. I guess that
would need to move to WTF if this code is in WTF, though.

> +	       const size_t len = m_length;

We normally don’t use const in cases like this.

I really don't like the booleans arguments in copyStringWithoutBOMs, so I'm
going to say review-, but you could convince me to change my mind.