Bug 233490

Summary: Reuse navigation preload if service worker is fetching the corresponding navigation request
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ews-watchlist, japhet, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 233523    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2021-11-25 03:25:35 PST
Reuse navigation preload is service worker is fetching the corresponding navigation request
Comment 1 youenn fablet 2021-11-25 03:37:29 PST
Created attachment 445131 [details]
Patch
Comment 2 youenn fablet 2021-11-26 07:09:51 PST
Created attachment 445201 [details]
Patch
Comment 3 youenn fablet 2021-11-30 03:05:10 PST
Created attachment 445405 [details]
Patch
Comment 4 Chris Dumez 2021-11-30 07:58:11 PST
Comment on attachment 445405 [details]
Patch

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

r=me with comments.

> Source/WebCore/loader/ResourceLoaderOptions.h:226
> +    FetchIdentifier navigationPreloadIdentifier;

We probably want to move this data member before the bitfield?

> Source/WebKit/NetworkProcess/NetworkSession.h:178
> +    ServiceWorkerFetchTask* navigationPreloaderTaskFromFetchIdentifier(WebCore::FetchIdentifier);

Shouldn't this be const?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:83
> +    , m_timeoutTimer(makeUnique<Timer>(*this, &ServiceWorkerFetchTask::timeoutTimerFired))

I don't understand this change. You made it a pointer and you unconditionally initialize it. Yet, you never seem to null the pointer out. I don't understand the purpose of making it a pointer and null checking it every time then.
Comment 5 youenn fablet 2021-11-30 08:01:46 PST
Comment on attachment 445405 [details]
Patch

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

>> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:83
>> +    , m_timeoutTimer(makeUnique<Timer>(*this, &ServiceWorkerFetchTask::timeoutTimerFired))
> 
> I don't understand this change. You made it a pointer and you unconditionally initialize it. Yet, you never seem to null the pointer out. I don't understand the purpose of making it a pointer and null checking it every time then.

m_timeoutTimer is only created in one of the two ServiceWorkerFetchTask constructor.
Comment 6 Chris Dumez 2021-11-30 08:06:10 PST
(In reply to youenn fablet from comment #5)
> Comment on attachment 445405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445405&action=review
> 
> >> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:83
> >> +    , m_timeoutTimer(makeUnique<Timer>(*this, &ServiceWorkerFetchTask::timeoutTimerFired))
> > 
> > I don't understand this change. You made it a pointer and you unconditionally initialize it. Yet, you never seem to null the pointer out. I don't understand the purpose of making it a pointer and null checking it every time then.
> 
> m_timeoutTimer is only created in one of the two ServiceWorkerFetchTask
> constructor.

Oh, I missed that. All good then.
Comment 7 youenn fablet 2021-12-01 03:23:09 PST
Created attachment 445546 [details]
Patch for landing
Comment 8 EWS 2021-12-01 05:37:05 PST
Committed r286361 (244720@main): <https://commits.webkit.org/244720@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445546 [details].
Comment 9 Radar WebKit Bug Importer 2021-12-01 05:38:24 PST
<rdar://problem/85918450>