[webkit-reviews] review granted: [Bug 17475] Error with line break inside ?» pair of characters. : [Attachment 47371] Change rules for allowing a line break after a question mark

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 25 17:32:06 PST 2010


Darin Adler <darin at apple.com> has granted mitz at webkit.org's request for review:
Bug 17475: Error with line break inside ?&raquo; pair of characters.
https://bugs.webkit.org/show_bug.cgi?id=17475

Attachment 47371: Change rules for allowing a line break after a question mark
https://bugs.webkit.org/attachment.cgi?id=47371&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +static const unsigned char internetExplorerBreaksBetweenQuestionMarkAnd[] =
{

I think it would be better to have a size of 0x80 on this array definition.

I think it's a bit curious to name this array with a verb as if it was a
function. A mathematical way of thinking about an array. Programmers tend to
think of them as objects instead.

> +	   // Internet Explorer allows breaking afer a question mark preceding
one of a subset of
> +	   // characters in ASCII. For characters outside ASCII, defer to the
Unicode algorithm by returning false.

This comment should somehow state that the array is used for enhanced speed as
well as to match IE. If all we cared about was matching IE, then the code could
say just:

    case '?':
	return nextCh == '|';

>	   case '?':
> +	       return nextCh <= 0x7f &&
internetExplorerBreaksBetweenQuestionMarkAnd[nextCh];

I prefer capital hex, myself. It would be nice if the check here was more tied
to the size of the array.

r=me


More information about the webkit-reviews mailing list