Bug 233490 – Reuse navigation preload if service worker is fetching the corresponding navigation request

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Reuse navigation preload is service worker is fetching the corresponding navigation request

[youenn fablet]

Created attachment 445131 [details]
Patch

[youenn fablet]

Created attachment 445201 [details]
Patch

[youenn fablet]

Created attachment 445405 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

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.

[Chris Dumez]

(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.

[youenn fablet]

Created attachment 445546 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85918450>