Bug 54313 – Need didReceiveCachedMetadata, and finishTime for didFinishLoading exposed in ThreadableLoaderClient.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Bill Budge]

Needs a public setDefersLoading(bool value) method to allow clients to implement WebURLLoader using this class.

Need a protected override to the inherited SubresourceLoaderClient::didReceiveCachedMetada method to allow derived classes to invoke the assertion logic when overriding.

Grouping these 2 changes into one issue to expedite the upcoming patch.

[Bill Budge]

Created attachment 82174 [details]
Proposed patch

[David Levin]

Comment on attachment 82174 [details]
Proposed patch

Wrong patch.

[Bill Budge]

Comment on attachment 82174 [details]
Proposed patch

I need to fix the patch.

[David Levin]

You may want to try using "webkit-patch upload" at some point (and consider setting the commit-queue flag to ? to indicate that you want your patch to be committed automatically).

[Bill Budge]

Created attachment 82178 [details]
Proposed patch

[Bill Budge]

Comment on attachment 82178 [details]
Proposed patch

This change also involves making the SubresourceLoaderClient callbacks protected, rather than private, and the addition of an isActive() method, which is needed to implement the static 'create' method in derived classes.

[Bill Budge]

I definitely need to improve my workflow here David. Thanks for bearing with me.

[David Levin]

Comment on attachment 82178 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:12
> +        (WebCore::DocumentThreadableLoader::didReceiveCachedMetadata):

A comment here would say:
  Make this method as protected instead of private.

I wonder why we just don't change the inheritance of SubresourceLoaderClient to be protected instead of private.

> Source/WebCore/ChangeLog:16
> [phone]  Bill Budge  <[e-mail]>

This shouldn't be here.

> Source/WebCore/loader/DocumentThreadableLoader.h:58
> +        virtual void setDefersLoading(bool value);

No need for "value" here. (It adds no information.)

[Bill Budge]

I will try your idea of using protected inheritance. I won't be able to work on this until next Monday fyi. Thanks!

[Bill Budge]

Created attachment 82479 [details]
Proposed Patch

[David Levin]

Comment on attachment 82479 [details]
Proposed Patch

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

r- to remove unnecessary overload. (If it is needed, give some short explanation and I'll r+.)

> Source/WebCore/loader/DocumentThreadableLoader.cpp:240
> +void DocumentThreadableLoader::didReceiveCachedMetadata(SubresourceLoader* loader, const char* data, int lengthReceived)

Ditto (with my comment below).

> Source/WebCore/loader/DocumentThreadableLoader.h:80
> +        virtual void didReceiveCachedMetadata(SubresourceLoader*, const char*, int lengthReceived);

Do you need this overload? (I though it was only here because SubresourceLoaderClient was private but that is no longer true.)

[Bill Budge]

Comment on attachment 82479 [details]
Proposed Patch

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:240
>> +void DocumentThreadableLoader::didReceiveCachedMetadata(SubresourceLoader* loader, const char* data, int lengthReceived)
> 
> Ditto (with my comment below).

See my response, below.

>> Source/WebCore/loader/DocumentThreadableLoader.h:80
>> +        virtual void didReceiveCachedMetadata(SubresourceLoader*, const char*, int lengthReceived);
> 
> Do you need this overload? (I though it was only here because SubresourceLoaderClient was private but that is no longer true.)

I added it so that we could include the assertion logic of the other methods, which verify that it's not called improperly. The method doesn't do anything, but can then be called by any derived classes. In case we ever want to put some logic into this method in DocumentThreadableLoader, we wouldn't have to go change all the derived classes. But I will defer to you on this; if you feel it's better to leave it unimplemented, I will remove it.

[Bill Budge]

Another question: In didReceiveData, we check if it's a preflight request, why don't we do the same test for didReceiveCachedMetadata?

[Bill Budge]

Let me rephrase that: Do I need to worry about that before calling the client in a derived implementation of didReceiveCachedMetadata?

[David Levin]

(In reply to comment #14)
> Let me rephrase that: Do I need to worry about that before calling the client in a derived implementation of didReceiveCachedMetadata?

Good point! I would.

When a request is done that is cross origin, it may trigger a preflight request which is a request to get info from the site to see if the cross origin request is allowed.

This preflight request is a basically protocol detail that shouldn't be exposed to the clients so it is nice to check to see if this is happening and not forward along the info to the client.


(In reply to comment #12)
> (From update of attachment 82479 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82479&action=review
> 
> >> Source/WebCore/loader/DocumentThreadableLoader.h:80
> >> +        virtual void didReceiveCachedMetadata(SubresourceLoader*, const char*, int lengthReceived);
> > 
> > Do you need this overload? (I though it was only here because SubresourceLoaderClient was private but that is no longer true.)
> 
> I added it so that we could include the assertion logic of the other methods, which verify that it's not called improperly. 

Good point. Feel free to leave it.

[Bill Budge]

Created attachment 82518 [details]
Proposed Patch

I added a protected isPreflight() method, so derived classes can filter out events related to the preflight request.

[David Levin]

Now I finally get to higher level questions (for which I'm sorry... it took my mind a while to get there.)

Why are we doing derivation as opposed to just using the client interface (DocumentThreadableLoaderClient)?

I'm concerned about the derivation being fragile (and the client interface seems to support everything you need -- well, you'll need to add in didReceiveCacheMetadata and expose the finishTime to one api, but that seems ok). Also this will hide the preflight request from your callbacks which seems desirable.

[Bill Budge]

So you raise an excellent point. If I could modify ThreadableLoaderClient, I could probably do without all of the API changes to DocumentThreadableLoader. I've been operating on the assumption that we shouldn't change interfaces, but if this isn't the case, modifying ThreadableLoaderClient is the way to go. Can I?

[David Levin]

(In reply to comment #18)
> So you raise an excellent point. If I could modify ThreadableLoaderClient, I could probably do without all of the API changes to DocumentThreadableLoader. I've been operating on the assumption that we shouldn't change interfaces, but if this isn't the case, modifying ThreadableLoaderClient is the way to go. Can I?

Yes, sorry for not mentioning/noticing this sooner.  These interfaces are only used within WebKit (not some external api) so they aren't set in stone. It just happens that there is more than one implementation.


Just make sure to fix up those implementations: http://www.google.com/codesearch?as_q=ThreadableLoaderClient&btnG=Search+Code&hl=en&vert=chromium&filesuggest=&as_lang=&as_filename=&as_class=&as_function=&as_case=

[David Levin]

Comment on attachment 82518 [details]
Proposed Patch

r- per discussion in bug.

[Bill Budge]

I will be adding didReceiveCachedMetadata and didDownloadData methods, and modify the signature of didFinishLoading. I'm unclear about how clients want to handle didReceiveCachedMetadata - for example EventSource has the following code:


    append(m_receiveBuf, m_decoder->decode(data, length));
    parseEventStream();

What should happen in the didReceiveCachedMetadata method?

[David Levin]

(In reply to comment #21)
> I will be adding didReceiveCachedMetadata and didDownloadData methods, and modify the signature of didFinishLoading. I'm unclear about how clients want to handle didReceiveCachedMetadata - for example EventSource has the following code:
> 
> 
>     append(m_receiveBuf, m_decoder->decode(data, length));
>     parseEventStream();
> 
> What should happen in the didReceiveCachedMetadata method?

Why should they implement it at all?  (Just modify the appropriate signatures but don't add the new method. They don't care about this information now so let them continue not to care about it.)

[Bill Budge]

That makes sense. How about didDownloadData? Couldn't this be implemented at the top level, by AssociatedURLLoader, which could check for this case on didFinishLoading?

[David Levin]

(In reply to comment #23)
> That makes sense. How about didDownloadData? Couldn't this be implemented at the top level, by AssociatedURLLoader, which could check for this case on didFinishLoading?

I see that method here:
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/public/WebURLLoaderClient.h&q=WebURLLoaderClient&exact_package=chromium

but I didn't understand when that method is called. (I suspect that you can get all the information you need from ThreadableLoaderClient to call this at the right time but since I don't know the contract for the method, it is only a guess.)

[Bill Budge]

Created attachment 82553 [details]
Proposed Patch

Changed the name of the bug to reflect the fact that ThreadableLoaderClient is being changed.

The patch also adds setDefersLoading(bool value) to DocumentThreadableLoader. That could be a separate patch, but doesn't simplify things much.

[David Levin]

Comment on attachment 82553 [details]
Proposed Patch

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

Just a few things to clean up.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:243
> +    ASSERT_UNUSED(loader, loader == m_loader);

I think this is missing the following:

if (!m_actualRequest)
    m_client->didReceiveCachedMetadata(data, lengthReceived)

> Source/WebCore/loader/DocumentThreadableLoader.h:49
> +    class DocumentThreadableLoader : public RefCounted<DocumentThreadableLoader>, public ThreadableLoader, protected SubresourceLoaderClient  {

"protected SubresourceLoaderClient" should be "private SubresourceLoaderClient" again.

> Source/WebCore/loader/DocumentThreadableLoader.h:57
> +        virtual void setDefersLoading(bool);

I suspect that you'll need to make this method public.

> Source/WebCore/loader/DocumentThreadableLoader.h:78
> +        virtual void didDownloadData(SubresourceLoader*, int dataLength);

This isn't implemented in DocumentThreadableLoader.cpp (and I doubt that it should be here).

> Source/WebCore/xml/XMLHttpRequest.cpp:990
> +void XMLHttpRequest::didFinishLoading(unsigned long identifier, double finishTime)

This will likely result in a compile error since "finishTime" isn't used in this function (just omit the parameter name).

[WebKit Review Bot]

Attachment 82553 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7911997

[Bill Budge]

Comment on attachment 82553 [details]
Proposed Patch

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:243
>> +    ASSERT_UNUSED(loader, loader == m_loader);
> 
> I think this is missing the following:
> 
> if (!m_actualRequest)
>     m_client->didReceiveCachedMetadata(data, lengthReceived)

Good catch!

>> Source/WebCore/loader/DocumentThreadableLoader.h:49
>> +    class DocumentThreadableLoader : public RefCounted<DocumentThreadableLoader>, public ThreadableLoader, protected SubresourceLoaderClient  {
> 
> "protected SubresourceLoaderClient" should be "private SubresourceLoaderClient" again.

Done.

>> Source/WebCore/loader/DocumentThreadableLoader.h:57
>> +        virtual void setDefersLoading(bool);
> 
> I suspect that you'll need to make this method public.

It is.

>> Source/WebCore/loader/DocumentThreadableLoader.h:78
>> +        virtual void didDownloadData(SubresourceLoader*, int dataLength);
> 
> This isn't implemented in DocumentThreadableLoader.cpp (and I doubt that it should be here).

Right, and it breaks the build on Linux.

[Bill Budge]

Created attachment 82567 [details]
Proposed Patch

[Bill Budge]

Created attachment 82568 [details]
Proposed Patch

[Build Bot]

Attachment 82553 [details] did not build on win:
Build output: http://queues.webkit.org/results/7926004

[Early Warning System Bot]

Attachment 82553 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7917132

[David Levin]

Comment on attachment 82568 [details]
Proposed Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:990
> +void XMLHttpRequest::didFinishLoading(unsigned long identifier, double finishTime)

This will likely result in a compile error since "finishTime" isn't used in this function (just omit the parameter name).

[Bill Budge]

Comment on attachment 82568 [details]
Proposed Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:990
>> +void XMLHttpRequest::didFinishLoading(unsigned long identifier, double finishTime)
> 
> This will likely result in a compile error since "finishTime" isn't used in this function (just omit the parameter name).

Done.

[Bill Budge]

Created attachment 82584 [details]
Proposed Patch

[WebKit Commit Bot]

Comment on attachment 82584 [details]
Proposed Patch

Rejecting attachment 82584 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
 .................................................
fast/text/whitespace ..........................................
fast/tokenizer ........................................
fast/transforms .........................
fast/url ....................
fast/workers .
fast/workers/dedicated-worker-lifecycle.html -> failed

Exiting early after 1 failures. 11091 tests run.
302.89s total testing time

11090 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
7 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7917231

[David Levin]

Comment on attachment 82584 [details]
Proposed Patch

Looks like this breaks a layout test. (If you need to run that test in chromium, it is in ui_tests.)

Please consider changing the ChangeLog "ThreadableLoaderClient doesn't support WebURLLoader usage" to what I made the bug title.

Of course, your formulation is the answer to "Why?" which is nice but I think "What?" would be better for this case.

[Bill Budge]

Created attachment 82633 [details]
Proposed Patch

[David Levin]

Comment on attachment 82633 [details]
Proposed Patch

The important thing is that it "looks like this breaks a layout test." Since this lastest patch has no difference from the last patch (except the ChangeLog), that issue could not have been addressed.

(The ChangeLog comment was just an aside.)

[Bill Budge]

I was hoping it was a flaky test. I'll check it out.

[David Levin]

(In reply to comment #40)
> I was hoping it was a flaky test. I'll check it out.

The commit queue is pretty good about noting that a test was flaky. I think it only does this if the test fails twice.

[Bill Budge]

Created attachment 82724 [details]
Proposed Patch

There were 2 pages to your code search. I missed WorkerScriptLoader, which implements ThreadableLoaderClient.

[David Levin]

Comment on attachment 82724 [details]
Proposed Patch

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

> Source/WebCore/workers/WorkerScriptLoader.cpp:137
> +void WorkerScriptLoader::didFinishLoading(unsigned long identifier, double finishTime)

omit "finishTime" since it isn't used. (As is, this will fail the compile stage on OSX.)

[Bill Budge]

Created attachment 82728 [details]
Proposed Patch

[WebKit Commit Bot]

Comment on attachment 82728 [details]
Proposed Patch

Clearing flags on attachment: 82728

Committed r78782: <http://trac.webkit.org/changeset/78782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.