Bug 233798 - Follow-up to r286479 to add API test and address issues found by the test
Summary: Follow-up to r286479 to add API test and address issues found by the test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 233874
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-02 20:31 PST by Chris Dumez
Modified: 2021-12-06 07:46 PST (History)
4 users (show)

See Also:


Attachments
Patch (14.73 KB, patch)
2021-12-03 08:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.34 KB, patch)
2021-12-03 09:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.93 KB, patch)
2021-12-03 09:26 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.93 KB, patch)
2021-12-03 09:30 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-12-02 20:31:20 PST
Add API test for Bug 233783
Comment 1 Chris Dumez 2021-12-03 08:22:45 PST
Created attachment 445857 [details]
Patch
Comment 2 Alex Christensen 2021-12-03 08:47:20 PST
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.
Comment 3 Chris Dumez 2021-12-03 08:48:12 PST
(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?
Comment 4 Alex Christensen 2021-12-03 08:48:39 PST
We do
Comment 5 Chris Dumez 2021-12-03 09:00:52 PST
Created attachment 445863 [details]
Patch
Comment 6 Darin Adler 2021-12-03 09:04:18 PST
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?
Comment 7 Chris Dumez 2021-12-03 09:08:13 PST
(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.
Comment 8 Chris Dumez 2021-12-03 09:26:43 PST
Created attachment 445865 [details]
Patch
Comment 9 Alex Christensen 2021-12-03 09:28:12 PST
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.
Comment 10 Chris Dumez 2021-12-03 09:30:02 PST
Created attachment 445866 [details]
Patch
Comment 11 EWS 2021-12-03 11:39:11 PST
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].
Comment 12 Radar WebKit Bug Importer 2021-12-03 11:40:24 PST
<rdar://problem/86027625>