[webkit-reviews] review denied: [Bug 106815] [Chromium] Tests and fixes for spell checker behavior : [Attachment 184588] Proposed patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Rouslan Solomakhin
<rouslan+webkit at chromium.org>'s request for review:
Bug 106815: [Chromium] Tests and fixes for spell checker behavior
https://bugs.webkit.org/show_bug.cgi?id=106815

Attachment 184588: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=184588&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184588&action=review


You need a ChangeLog.  If you use "webkit-patch upload" it should create one
for you and open it in a text editor for you.  Otherwise, you can use
prepare-ChangeLog.

Why are these tests only run on Chromium?  Shouldn't the result be the same on
all WebKit ports?  Does this depend on features that are Chromium specific?  If
so, you could put them in a subdirectory (e.g.,
editing/spelling/unified-text-checker) and skip that subdir on other platforms
since they may enable it in the future.

>
LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-wit
h-underscores.html:16
> +    internals.settings.setUnifiedTextCheckerEnabled(false);

This shouldn't be necessary. The test harness (DRT) should automatically undo
any settings you set. If you see a different behavior, please file a bug about
it and cc me.

>
LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-wit
h-underscores.html:76
> +description("Spelling should work for double-clicked misspelled words with
underscores.");
> +if (window.internals)
> +    test();

It would be nice if there was some text describing how to run the test
manually.  You could write this in the else branch of "if (window.internals)"
or just put it in the description.