[webkit-reviews] review denied: [Bug 61799] Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html : [Attachment 95608] Adds simplifyHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Philip Rogers <pdr at google.com>'s
request for review:
Bug 61799: Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html
https://bugs.webkit.org/show_bug.cgi?id=61799

Attachment 95608: Adds simplifyHTMLWhiteSpace and call it in drawText as well
as measureText, which makes us pass 5 of Philip's canvas tests
https://bugs.webkit.org/attachment.cgi?id=95608&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=95608&action=review

Wow, is the behavior really specified this way? That sounds like a mistake in
the design of this API. Seems bizarre to collapse the spaces out of a string
that is being passed in from JavaScript.

review- because I’d like to see a faster version of the “no collapsing needed”
code path.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1879
> +    const String& str = simplifyHTMLWhiteSpace(text);

We frown on abbreviations like “str”. Maybe “simplifiedText” or the argument
could be named rawText and this local could be text.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:65
> +    const UChar* fromend = from + length;

We’d normally capitalize the E here.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:66
> +    int outc = 0;

We try to avoid variable names like this with abbreviations rather than words.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:69
> +    StringBuffer data(length);

Unfortunately, this allocates a new buffer in memory every time, even if
nothing changes. I think we want to further optimize the no-collapse case so
the buffer is only created the first time we have to change something.

Since most strings won’t need to be collapsed, I think we could just do a
complete pass checking if there is anything to collapse before actually
creating the buffer.