Bug 222408 – API test for AppHighlights

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

API test for AppHighlights

[Megan Gardner]

Created attachment 421509 [details]
Patch

[Tim Horton]

Comment on attachment 421509 [details]
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!!).

[Wenson Hsieh]

Comment on attachment 421509 [details]
Patch

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

>> 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.

(I suggested `PlatformUtilities.h`)

[Wenson Hsieh]

Comment on attachment 421509 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:5923
> +    auto appHighlightRegister = document->appHighlightRegisterIfExists();

This seems like it should probably return 0 instead of null dereffing if the `appHighlightRegister` does not exist yet.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:59
> +@interface AppHighlightDelegateImpl : NSObject <_WKAppHighlightDelegate>

Nit - maybe just `AppHighlightDelegate`?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:60
> +@property (nonatomic, copy) void (^storeAppHighlightCallback)(WKWebView *, _WKAppHighlight *, BOOL);

Since this is marked `copy`, this should probably be overridden to set and get from a `BlockPtr<void(WKWebView *, _WKAppHighlight *, BOOL)>`.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:63
> +
> +

Nit - couple of extra newlines here.

[Tim Horton]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 421509 [details]
> Patch
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:59
> > +@interface AppHighlightDelegateImpl : NSObject <_WKAppHighlightDelegate>
> 
> Nit - maybe just `AppHighlightDelegate`?

Agree (this was my name, but it was bad).

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:60
> > +@property (nonatomic, copy) void (^storeAppHighlightCallback)(WKWebView *, _WKAppHighlight *, BOOL);
> 
> Since this is marked `copy`, this should probably be overridden to set and
> get from a `BlockPtr<void(WKWebView *, _WKAppHighlight *, BOOL)>`.

hmm, this is following TestNavigationDelegate. Maybe we have this mistake in many places.

[Wenson Hsieh]

(In reply to Tim Horton from comment #5)
> (In reply to Wenson Hsieh from comment #4)
> > Comment on attachment 421509 [details]
> > Patch
> > 
> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:59
> > > +@interface AppHighlightDelegateImpl : NSObject <_WKAppHighlightDelegate>
> > 
> > Nit - maybe just `AppHighlightDelegate`?
> 
> Agree (this was my name, but it was bad).
> 
> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:60
> > > +@property (nonatomic, copy) void (^storeAppHighlightCallback)(WKWebView *, _WKAppHighlight *, BOOL);
> > 
> > Since this is marked `copy`, this should probably be overridden to set and
> > get from a `BlockPtr<void(WKWebView *, _WKAppHighlight *, BOOL)>`.
> 
> hmm, this is following TestNavigationDelegate. Maybe we have this mistake in
> many places.

Ah, never mind — since the property is synthesized, I think this should be okay (the compiler adds the calls to copy automatically).

[Megan Gardner]

Created attachment 421736 [details]
Patch

[EWS]

Committed r273620: <https://commits.webkit.org/r273620>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421736 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74822597>