[webkit-reviews] review denied: [Bug 65902] Spell-checking doesn't recognize word boundaries on contests inserted by execCommand('insertHTML') : [Attachment 103744] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 65902: Spell-checking doesn't recognize word boundaries on contests
inserted by execCommand('insertHTML')
https://bugs.webkit.org/show_bug.cgi?id=65902

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103744&action=review


Sorry r- for various nits.

> ChangeLog:4
> +	   Spell-checking against execCommand() inserted HTML doesn't care word
boundary.
> +	   https://bugs.webkit.org/show_bug.cgi?id=65902

Please update the bug title.

> LayoutTests/ChangeLog:8
> +	   Existing expectation was wrong because some markers spans a part of
words.

maybe 'some markers on substrings of words'?

> LayoutTests/ChangeLog:9
> +	   With this fix, now Editor rejects such invalid markers.

Editor now rejects such markers.

> LayoutTests/editing/spelling/spelling-insert-html.html:14
> +description("The spellchecker shouldn't check words on the paste border but
check inside it.");

What does "paste border" mean?

> LayoutTests/editing/spelling/spelling-insert-html.html:35
> +    shouldBe("markedText", "'zz'");

Use shouldBeEqualToString instead?

> LayoutTests/editing/spelling/spelling-insert-html.html:40
> +    layoutTestController.notifyDone();

Why are we calling this?  By the way, should we wait until the page loads?  If
I remember correctly, some of spell-checking stuff won't run until the page is
fully loaded and we've had some flakiness in our tests because of that.

> Source/WebCore/ChangeLog:9
> +	   But these are low-level APIand caller should take care of word
boundary if input.

Nit: API and.
if input what?

> Source/WebCore/editing/Editor.cpp:1946
> +    markMisspellingsAndBadGrammar(movingSelection, markGrammar,
movingSelection);

This is all we have to do to fix this bug!?  That's quite amazing.  I guess you
had a really good insight into what refactoring we should be doing.

> Source/WebCore/testing/Internals.cpp:184
> +unsigned Internals::markedSizeOf(Node* node, ExceptionCode& ec)

I don't really understand what this function does.  Can we come up with a more
descriptive name?

> Source/WebKit2/win/WebKit2CFLite.def:142
> +    
?create at Range@WebCore@@SA?AV?$PassRefPtr at VRange@WebCore@@@WTF@@V?$PassRefPtr at VD
ocument at WebCore@@@4 at V?$PassRefPtr at VNode@WebCore@@@4 at H1H@Z

Wrong indentation?