Bug 233798 – Follow-up to r286479 to add API test and address issues found by the test

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Dumez]

Add API test for Bug 233783

[Chris Dumez]

Created attachment 445857 [details]
Patch

[Alex Christensen]

Comment on attachment 445857 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:419
> +- (void)_isLayerTreeFrozen:(void (^)(BOOL frozen))completionHandler;

availability macros, WKWebViewPrivateForTesting.h

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:405
> +    isLayerTreeFrozenForTesting() -> (bool isFrozen) Async

The freezing isn't for testing, the query is.  I'm not sure if the naming could be more clear.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:7275
> +    sourceHeaders.add("Content-Type"_s, "text/html"_s);

These can be put in the initializer list.  That would clean up the syntax a lot.

[Chris Dumez]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 445857 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445857&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:419
> > +- (void)_isLayerTreeFrozen:(void (^)(BOOL frozen))completionHandler;
> 
> availability macros, WKWebViewPrivateForTesting.h

I thought we didn't need TBA macros for SPI?

[Alex Christensen]

We do

[Chris Dumez]

Created attachment 445863 [details]
Patch

[Darin Adler]

Comment on attachment 445863 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Add API test for Bug 233783

Looks like you found something additional to fix in ProvisionalPageProxy, so this is not just adding a test any more. Is that correct? Should we retitle?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:419
> +- (void)_isLayerTreeFrozen:(void (^)(BOOL frozen))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Why did you decide to put this into Private rather than PrivateForTesting?

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 445863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445863&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        Add API test for Bug 233783
> 
> Looks like you found something additional to fix in ProvisionalPageProxy, so
> this is not just adding a test any more. Is that correct? Should we retitle?

Correct, I will retitle.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:419
> > +- (void)_isLayerTreeFrozen:(void (^)(BOOL frozen))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Why did you decide to put this into Private rather than PrivateForTesting?

Because I wasn't aware of the WKWebViewPrivateForTesting.h header. I will move it there.

[Chris Dumez]

Created attachment 445865 [details]
Patch

[Alex Christensen]

Comment on attachment 445865 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:405
> +    isLayerTreeFrozen() -> (bool isFrozen) Async

nit: for some reason, all the messages.in names start with a capital letter.

[Chris Dumez]

Created attachment 445866 [details]
Patch

[EWS]

Committed r286505 (244844@main): <https://commits.webkit.org/244844@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/86027625>