Bug 206426 - Add logging of the source of a response used to respond a fetch event
Summary: Add logging of the source of a response used to respond a fetch event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-17 09:48 PST by youenn fablet
Modified: 2020-01-22 06:27 PST (History)
9 users (show)

See Also:


Attachments
Patch (10.44 KB, patch)
2020-01-17 09:54 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.33 KB, patch)
2020-01-21 07:24 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.40 KB, patch)
2020-01-22 05:05 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-01-17 09:48:46 PST
Add logging of the source of a response used to respond a fetch event
Comment 1 youenn fablet 2020-01-17 09:54:38 PST
Created attachment 388052 [details]
Patch
Comment 2 Darin Adler 2020-01-20 12:35:17 PST
Comment on attachment 388052 [details]
Patch

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

> Source/WebCore/Modules/cache/DOMCache.cpp:578
> +                auto resourceResponse = WTFMove(record.response);
> +                resourceResponse.setSource(ResourceResponse::Source::DOMCache);

We do not need to move this into a local variable to modify it. We can just write:

    record.response.setSource(ResourceResponse::Source::DOMCache);

and not make the other changes. Basically, an rvalue reference gives us the permission to modify and/or take the value of something. No reason we need to move it to modify it.

If we were copying rather than moving, then yes, it’s important to copy before modifying something. But that’s not the case here.

> Source/WebCore/Modules/cache/DOMCache.cpp:591
> +            auto resourceResponse = WTFMove(record.response);
> +            resourceResponse.setSource(ResourceResponse::Source::DOMCache);

Ditto.
Comment 3 youenn fablet 2020-01-21 07:24:41 PST
Created attachment 388297 [details]
Patch for landing
Comment 4 youenn fablet 2020-01-21 07:25:33 PST
> We do not need to move this into a local variable to modify it. We can just
> write:
> 
>     record.response.setSource(ResourceResponse::Source::DOMCache);

Right, fixed in latest patch.

I also added a small internals test to verify the response source is correctly set if coming from DOM cache.
Comment 5 youenn fablet 2020-01-22 05:05:41 PST
Created attachment 388416 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2020-01-22 06:26:15 PST
Comment on attachment 388416 [details]
Patch for landing

Clearing flags on attachment: 388416

Committed r254919: <https://trac.webkit.org/changeset/254919>
Comment 7 WebKit Commit Bot 2020-01-22 06:26:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2020-01-22 06:27:15 PST
<rdar://problem/58795155>