Bug 195130 – Introduce ServiceWorkerFetchTask

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Introduce ServiceWorkerFetchTask so that later on we can link it with NetworkResourceLoader.

[youenn fablet]

Created attachment 363155 [details]
Patch

[youenn fablet]

Created attachment 363164 [details]
Patch

[youenn fablet]

Created attachment 363181 [details]
Patch

[youenn fablet]

Created attachment 363184 [details]
Patch

[youenn fablet]

Created attachment 363258 [details]
Patch

[Daniel Bates]

Comment on attachment 363258 [details]
Patch

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

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:60
> +        inline unsigned hash() const

Inline?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:62
> +            unsigned hashes[2];

Old style. New hotness is std::array.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:65
> +            

Ok as-is. Empty line to rich for me. I don’t think it improves readability.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:77
> +    ServiceWorkerFetchTask(PAL::SessionID sessionID, Ref<IPC::Connection>&& connection, WebCore::SWServerConnectionIdentifier connectionIdentifier, WebCore::FetchIdentifier fetchIdentifier)
> +        : m_sessionID(sessionID)
> +        , m_connection(WTFMove(connection))
> +        , m_identifier(Identifier {connectionIdentifier, fetchIdentifier})
> +    { }

Uniform initialized syntax (UIS)?

Also, you’re use of UIS is in bad taste. Put spaces after { and before }.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:117
> +    typedef ServiceWorkerFetchTaskIdentifierHash Hash;

Using?

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:63
> +    auto fetches = WTFMove(m_ongoingFetches);

std::exchange()? Or old object left in sane state?

[youenn fablet]

Created attachment 363654 [details]
Patch

[youenn fablet]

Thanks for the review.

(In reply to Daniel Bates from comment #6)
> Comment on attachment 363258 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363258&action=review
> 
> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:60
> > +        inline unsigned hash() const
> 
> Inline?

Removed.

> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:62
> > +            unsigned hashes[2];
> 
> Old style. New hotness is std::array.

That probably requires to have hashMemory take a std::array, otherwise this makes the code more complex.
Might be better tackled outside of this patch.

> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:65
> > +            
> 
> Ok as-is. Empty line to rich for me. I don’t think it improves readability.

Removed the line.

> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:77
> > +    ServiceWorkerFetchTask(PAL::SessionID sessionID, Ref<IPC::Connection>&& connection, WebCore::SWServerConnectionIdentifier connectionIdentifier, WebCore::FetchIdentifier fetchIdentifier)
> > +        : m_sessionID(sessionID)
> > +        , m_connection(WTFMove(connection))
> > +        , m_identifier(Identifier {connectionIdentifier, fetchIdentifier})
> > +    { }
> 
> Uniform initialized syntax (UIS)?
> 
> Also, you’re use of UIS is in bad taste. Put spaces after { and before }.

Fixed

> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:117
> > +    typedef ServiceWorkerFetchTaskIdentifierHash Hash;
> 
> Using?

OK

> > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:63
> > +    auto fetches = WTFMove(m_ongoingFetches);
> 
> std::exchange()? Or old object left in sane state?

I kept WTFMove, it reads better to me and m_ongoingFetches is a map so is left in a good state.

[Chris Dumez]

Comment on attachment 363654 [details]
Patch

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

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:69
> +    Identifier identifier() const { return m_identifier; }

const Identifier&

[youenn fablet]

Created attachment 363669 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363669 [details]
Patch for landing

Clearing flags on attachment: 363669

Committed r242503: <https://trac.webkit.org/changeset/242503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48612687>