[webkit-reviews] review granted: [Bug 217548] REGRESSION (r267761): editing/mac/spelling/autocorrection-contraction.html is a constant timeout on macOS wk2 Debug : [Attachment 411027] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 217548: REGRESSION (r267761):
editing/mac/spelling/autocorrection-contraction.html is a constant timeout on
macOS wk2 Debug
https://bugs.webkit.org/show_bug.cgi?id=217548

Attachment 411027: Patch

https://bugs.webkit.org/attachment.cgi?id=411027&action=review




--- Comment #12 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 411027
  --> https://bugs.webkit.org/attachment.cgi?id=411027
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411027&action=review

Nice!

> Tools/ChangeLog:9
> +	   (ShouldBuildTest): Removed test case that depended on a file that is
now deleted.

Can it be changed to use another file, that's not deleted? There are other
-expected.txt in this directory.

At some point, we should remove all tests from LayoutTests/platform to avoid
the confusing situation with platform-specific results for platform-specific
tests, but until then, tests are helpful to keep some sanity.

> Tools/TestRunnerShared/Bindings/JSBasics.cpp:92
> +    return value && JSValueIsObject(context, value) ?
const_cast<JSObjectRef>(value) : nullptr;

I would rewrite this without a ternary operator, or at the very least, would
use parentheses around the condition. I don't think that saving two bytes is
worth decreased readability.

> LayoutTests/ChangeLog:9
> +	   * editing/mac/spelling/autocorrection-contraction-expected.png:
Removed.

The removed PNG shows that the original test also included "wouldn" and
"wouldn'", also without underlines. Do you know what's up with that? Does the
test even work at all?