[webkit-reviews] review denied: [Bug 222408] API test for AppHighlights : [Attachment 421509] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has denied Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 222408: API test for AppHighlights
https://bugs.webkit.org/show_bug.cgi?id=222408

Attachment 421509: Patch

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




--- Comment #2 from Tim Horton <thorton at apple.com> ---
Comment on attachment 421509
  --> https://bugs.webkit.org/attachment.cgi?id=421509
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:243
> +    if (range)

This should come in the second patch, with a second test that validates it (the
current test doesn't). This patch should just add the testing infrastructure
and the basic test.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:40
> +static void waitForConditionWithLogging(BOOL(^condition)(), NSTimeInterval
loggingTimeout, NSString *message, ...)

Now that there are two clients, I wonder if we should share it. Not sure where
to put it though, maybe Wenson has an idea.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:92
> +	   waitForConditionWithLogging([&] () -> BOOL {
> +	       return [webView
stringByEvaluatingJavaScript:@"internals.numberOfAppHighlights"].intValue == 1;
> +	   }, 2, @"Expected Highlights to be populated.");

This is totally not doing what it's supposed to. The point was to wait /after
you call _restoreAppHighlights/ and ensure that it worked. But you're waiting
beforehand, and it'll still work because you're restoring into the same webview
you created the highlight in originally (so it has 1 even before restoring)

You should either restore into a second WebView (that's what I would do, I
think), or navigate before restoring (and make sure that the count goes to 0
when you navigate!!).