Bug 211520 – ResourceLoadStatistics data summary call should create a web process pool if one doesn't exist

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

This call should be available even if a web process pool doesn't already exist.

[Kate Cheney]

<rdar://problem/59869619>

[Kate Cheney]

Created attachment 398639 [details]
Patch

[Chris Dumez]

Comment on attachment 398639 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This does not need to be asynchronous or take a completion handler in.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> +    for (auto& processPool : ensureProcessPools()) {

ensureProcessPools() does NOT create a persistent process pool. As soon as this loop is over, the process pool that was created should get destroyed (unless some other object is ref'ing it).

As a result, I am perplexed by your new hasProcessPool getter and testing.

[Kate Cheney]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 398639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398639&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> > +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This does not need to be asynchronous or take a completion handler in.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> > +    for (auto& processPool : ensureProcessPools()) {
> 
> ensureProcessPools() does NOT create a persistent process pool. As soon as
> this loop is over, the process pool that was created should get destroyed
> (unless some other object is ref'ing it).
> 
> As a result, I am perplexed by your new hasProcessPool getter and testing.

I am only using the getter to confirm that the test is setup correctly before calling the API. It should have no existing process pools to start with.

[Chris Dumez]

(In reply to katherine_cheney from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 398639 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=398639&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> > > +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > This does not need to be asynchronous or take a completion handler in.
> > 
> > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> > > +    for (auto& processPool : ensureProcessPools()) {
> > 
> > ensureProcessPools() does NOT create a persistent process pool. As soon as
> > this loop is over, the process pool that was created should get destroyed
> > (unless some other object is ref'ing it).
> > 
> > As a result, I am perplexed by your new hasProcessPool getter and testing.
> 
> I am only using the getter to confirm that the test is setup correctly
> before calling the API. It should have no existing process pools to start
> with.

I see. Then I think it may be overkill to add a new SPI just to check that your API test did not create a process pool.

[Kate Cheney]

(In reply to Chris Dumez from comment #5)
> (In reply to katherine_cheney from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 398639 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=398639&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> > > > +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > > 
> > > This does not need to be asynchronous or take a completion handler in.
> > > 
> > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> > > > +    for (auto& processPool : ensureProcessPools()) {
> > > 
> > > ensureProcessPools() does NOT create a persistent process pool. As soon as
> > > this loop is over, the process pool that was created should get destroyed
> > > (unless some other object is ref'ing it).
> > > 
> > > As a result, I am perplexed by your new hasProcessPool getter and testing.
> > 
> > I am only using the getter to confirm that the test is setup correctly
> > before calling the API. It should have no existing process pools to start
> > with.
> 
> I see. Then I think it may be overkill to add a new SPI just to check that
> your API test did not create a process pool.

Got it. I was just trying to be thorough :)

[Kate Cheney]

Created attachment 398645 [details]
Patch

[Chris Dumez]

Comment on attachment 398645 [details]
Patch

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

Ok if bots are happy.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:-82
> -

Unnecessary change.

[Kate Cheney]

Created attachment 398647 [details]
Patch

[Kate Cheney]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 398645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398645&action=review
> 
> Ok if bots are happy.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:-82
> > -
> 
> Unnecessary change.

Thanks! Waiting for bots before I cq+

[EWS]

Committed r261250: <https://trac.webkit.org/changeset/261250>

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