Bug 208610 – Construct fewer unnecessary temporary WebProcessPool objects in WebsiteDataStore implementation

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Construct less unnecessary temporary WebProcessPool objects in WebsiteDataStore implementation.

[Chris Dumez]

Created attachment 392502 [details]
Patch

[Chris Dumez]

Created attachment 392510 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 392510 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1380
> -    for (auto& processPool : ensureProcessPools()) {
> +    for (auto& processPool : processPools()) {
>          if (auto* process = processPool->networkProcess()) {
>              process->isRegisteredAsSubresourceUnder(m_sessionID, WebCore::RegistrableDomain { subresourceURL }, WebCore::RegistrableDomain { topFrameURL }, WTFMove(completionHandler));
> -            break;
> -        } else {
> -            ASSERT_NOT_REACHED();
> -            completionHandler(false);
> -            break;
> +            return;
>          }
> +        break;
>      }
> +    completionHandler(false);

I am not sure what this function is supposed to do. Does it expect (processPools().size() == 1 and processPools()[0]->networkProcess() != nullptr)? if this is the case why do we use a for-loop?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1481
> +    completionHandler();

completionHandler is moved above when creating callbackAggregator.

[Chris Dumez]

(In reply to Said Abou-Hallawa from comment #3)
> Comment on attachment 392510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392510&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1380
> > -    for (auto& processPool : ensureProcessPools()) {
> > +    for (auto& processPool : processPools()) {
> >          if (auto* process = processPool->networkProcess()) {
> >              process->isRegisteredAsSubresourceUnder(m_sessionID, WebCore::RegistrableDomain { subresourceURL }, WebCore::RegistrableDomain { topFrameURL }, WTFMove(completionHandler));
> > -            break;
> > -        } else {
> > -            ASSERT_NOT_REACHED();
> > -            completionHandler(false);
> > -            break;
> > +            return;
> >          }
> > +        break;
> >      }
> > +    completionHandler(false);
> 
> I am not sure what this function is supposed to do. Does it expect
> (processPools().size() == 1 and processPools()[0]->networkProcess() !=
> nullptr)? if this is the case why do we use a for-loop?
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1481
> > +    completionHandler();
> 
> completionHandler is moved above when creating callbackAggregator.

Thanks for catching this. Will fix before landing.

[Chris Dumez]

Created attachment 392514 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392514 [details]
Patch

Rejecting attachment 392514 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 392514, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13333735

[Chris Dumez]

Created attachment 392515 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392515 [details]
Patch

Clearing flags on attachment: 392515

Committed r257893: <https://trac.webkit.org/changeset/257893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60066720>