Bug 38665 – [chromium] Update chromium port to send/receive cached metadata

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Tony Gentilcore]

Created attachment 55270 [details]
Update chromium port to send/receive cached metadata

Chromium port implementation of API introduced in https://bugs.webkit.org/show_bug.cgi?id=37874

[Tony Gentilcore]

Created attachment 55305 [details]
Merge with changes from 37874

[WebKit Review Bot]

Attachment 55305 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2166019

[Adam Barth]

Is this patch ready for review, or did you want to fix the red chromium bubble first?

[Tony Gentilcore]

Created attachment 55320 [details]
Fix typo -- should fix redness

[WebKit Review Bot]

Attachment 55320 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2235024

[Tony Gentilcore]

It looks like the build failed because 37874 had not landed yet (and this patch depends on it).

It has landed now. Is there a way I can kick off the build again (other than uploading an identical patch)?

[Tony Gentilcore]

Created attachment 55380 [details]
Retry build now that 37874 has landed

[WebKit Review Bot]

Attachment 55380 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2209060

[Tony Gentilcore]

Oops, 37874 hasn't really landed yet. I'll upload the patch again once it lands.

[Tony Gentilcore]

Created attachment 55596 [details]
Patch

[Tony Gentilcore]

FYI -- These two chromium patches may be useful during this review (e.g. they demonstrate why response time is necessary):

http://codereview.chromium.org/1742004
http://codereview.chromium.org/1698001

[Adam Barth]

Comment on attachment 55596 [details]
Patch

WebKit/chromium/src/ResourceHandle.cpp:181
 +          CRASH();
This seems a bit harsh.  Is this what other code in this file does?

WebCore/platform/chromium/ChromiumBridge.h:76
 +          static void cacheMetadata(const KURL& url, long long responseTime, const Vector<char>&);
Does a pair of URL+responseTime uniquely determine an HTTP response?  What if the clock gets reset?  This seems like a proxy for a response ID.  Why not create a response ID?

[Adam Barth]

This patch requires review by the great and powerful fishd because it touches the WebKit API.

[Tony Gentilcore]

(In reply to comment #12)
> (From update of attachment 55596 [details])
> WebKit/chromium/src/ResourceHandle.cpp:181
>  +          CRASH();
> This seems a bit harsh.  Is this what other code in this file does?

Yeah, didReceiveResponse, didReceiveData, didFinishLoading, all have similar statements that ensure the state is expected. I'm fine with removing it if it is less important here because things will work even if the cached metadata is never received.

> 
> WebCore/platform/chromium/ChromiumBridge.h:76
>  +          static void cacheMetadata(const KURL& url, long long responseTime, const Vector<char>&);
> Does a pair of URL+responseTime uniquely determine an HTTP response?  What if the clock gets reset?  This seems like a proxy for a response ID.  Why not create a response ID?

Response time is what is required by the HTTP Cache API that fishd and rvargas put together here: http://codereview.chromium.org/600167/diff/9001/9008

I don't think clock reset is a factor unless it was reset in the very slim time between the load of the resource and the generation of metadata. And even in that case, we'd just miss an opportunity to cache the metadata. Once the metadata is stored in the cache, the response time doesn't matter for future loads.

[Adam Barth]

Okiedokes.  Re: CRASH: if that's what this file wants, that's fine with me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 55596 [details]
Patch

WebKit/chromium/public/WebKitClient.h:196
 +      virtual void cacheMetadata(const WebURL&, long long, const char*, size_t) { }
please document the non-obvious parameters by giving them a name :-)

virtual void cacheMetadata(const WebURL&, long long responseTime, const char* data, size_t dataLength) { }

ordinarily, we use 'double' to represent timestamps in webkit.  is there a reason for using 'long long' instead of 'double'?  i realize that chromium uses long long.

WebKit/chromium/public/WebURLLoaderClient.h:60
 +      virtual void didReceiveCachedMetadata(WebURLLoader*, const char* data, int dataLength) { }
i presume it is promised that didReceiveCachedMetadata will not be called after didFinishLoading or didFail, right?

WebKit/chromium/public/WebURLResponse.h:75
 +      WEBKIT_API long long responseTime() const;
same comment about 'long long' versus 'double'

WebKit/chromium/src/ResourceHandle.cpp:176
 +  void ResourceHandleInternal::didReceiveCachedMetadata(
no line break here in webkit style

[Tony Gentilcore]

(In reply to comment #16)
> (From update of attachment 55596 [details])
> WebKit/chromium/public/WebKitClient.h:196
>  +      virtual void cacheMetadata(const WebURL&, long long, const char*, size_t) { }
> please document the non-obvious parameters by giving them a name :-)

Done.

> 
> virtual void cacheMetadata(const WebURL&, long long responseTime, const char* data, size_t dataLength) { }
> 
> ordinarily, we use 'double' to represent timestamps in webkit.  is there a reason for using 'long long' instead of 'double'?  i realize that chromium uses long long.

I will completely defer to your recommendation here. Basically this is just an identifier that has to survive a round trip in order to make sure the metadata gets stored with the correct cache entry. The fact that it is a responseTime is really an implementation detail. WebCore never actually touches this field.

So I could see looking at this as either (a) an opaque identifier in which case it could be renamed or (b) as a response time that just happens to be used as this identifier. Please let me know if you prefer I keep it as-is or static_cast it to a double and back. I'll upload the updated patch after we figure this out.

> 
> WebKit/chromium/public/WebURLLoaderClient.h:60
>  +      virtual void didReceiveCachedMetadata(WebURLLoader*, const char* data, int dataLength) { }
> i presume it is promised that didReceiveCachedMetadata will not be called after didFinishLoading or didFail, right?

Yes. That is my intent. Here is where I'm planning to send the metadata:
http://codereview.chromium.org/1698001/diff/39001/28009

> 
> WebKit/chromium/public/WebURLResponse.h:75
>  +      WEBKIT_API long long responseTime() const;
> same comment about 'long long' versus 'double'

Yes. I'll keep them in sync based on your answer to my question above.

> 
> WebKit/chromium/src/ResourceHandle.cpp:176
>  +  void ResourceHandleInternal::didReceiveCachedMetadata(
> no line break here in webkit style

Done.

[Darin Fisher (:fishd, Google)]

> I will completely defer to your recommendation here. Basically this is just an identifier that has to survive a round trip in order to make sure the metadata gets stored with the correct cache entry. The fact that it is a responseTime is really an implementation detail. WebCore never actually touches this field.
> 
> So I could see looking at this as either (a) an opaque identifier in which case it could be renamed or (b) as a response time that just happens to be used as this identifier. Please let me know if you prefer I keep it as-is or static_cast it to a double and back. I'll upload the updated patch after we figure this out.

Converting 'long long' to/from 'double' just means that we can only have 52 bits of precision, right?  That seems plenty sufficient for this use case, right?

By the way, I actually had the same thought as you after submitting my comments.  It is attractive to view response-time as an opaque identifier.  That said, the response-time is actually a piece of information that would be useful to the cache expiration calculations performed by WebCore's memory cache.  Presently it just has to guess, which can lead to skewed results.  (Maybe it should really be set on ResourceResponseBase, or we can do that as a separate patch since other ports won't set it yet.)

Does the Chromium side require an exact match for the response-time to validate that the cached response is the same?  It probably is sensible for it to do so.  Anyways, I think it is safe to use double.  Note, on the Chromium side we have Time::ToDoubleT and FromDoubleT.

[Tony Gentilcore]

Created attachment 55859 [details]
Patch

[Tony Gentilcore]

(In reply to comment #18)
> > I will completely defer to your recommendation here. Basically this is just an identifier that has to survive a round trip in order to make sure the metadata gets stored with the correct cache entry. The fact that it is a responseTime is really an implementation detail. WebCore never actually touches this field.
> > 
> > So I could see looking at this as either (a) an opaque identifier in which case it could be renamed or (b) as a response time that just happens to be used as this identifier. Please let me know if you prefer I keep it as-is or static_cast it to a double and back. I'll upload the updated patch after we figure this out.
> 
> Converting 'long long' to/from 'double' just means that we can only have 52 bits of precision, right?  That seems plenty sufficient for this use case, right?
> 
> By the way, I actually had the same thought as you after submitting my comments.  It is attractive to view response-time as an opaque identifier.  That said, the response-time is actually a piece of information that would be useful to the cache expiration calculations performed by WebCore's memory cache.  Presently it just has to guess, which can lead to skewed results.  (Maybe it should really be set on ResourceResponseBase, or we can do that as a separate patch since other ports won't set it yet.)

I think that is a good argument for leaving it as responseTime. Then we can move it to ResponseResponseBase when we are ready to make the cache fix. But I won't conflate that fix with this patch for now.

> 
> Does the Chromium side require an exact match for the response-time to validate that the cached response is the same?  It probably is sensible for it to do so.  Anyways, I think it is safe to use double.  Note, on the Chromium side we have Time::ToDoubleT and FromDoubleT.

Yes, it needs an exact match. The ToDoubleT and FromDoubleT functions should work perfectly.

New patch with doubles instead of long longs should be ready for another look.

[WebKit Commit Bot]

Comment on attachment 55859 [details]
Patch

Clearing flags on attachment: 55859

Committed r59465: <http://trac.webkit.org/changeset/59465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.