API test for AppHighlights
Created attachment 421509 [details] Patch
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!!).
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`)
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.
(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.
(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).
Created attachment 421736 [details] Patch
Committed r273620: <https://commits.webkit.org/r273620> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421736 [details].
<rdar://problem/74822597>