Bug 54313

Created attachment 82174 [details] Proposed patch

Comment on attachment 82174 [details] Proposed patch Wrong patch.

Comment on attachment 82174 [details] Proposed patch I need to fix the patch.

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

Created attachment 82178 [details] Proposed patch

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.

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

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 > +2011-02-11 Bill Budge <bbudge@chromium.org> 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.)

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

Created attachment 82479 [details] Proposed Patch

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

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.

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

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

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

Created attachment 82518 [details] Proposed Patch I added a protected isPreflight() method, so derived classes can filter out events related to the preflight request.

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.

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?

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

Comment on attachment 82518 [details] Proposed Patch r- per discussion in bug.

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?

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

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?

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

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.

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

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

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.

Created attachment 82567 [details] Proposed Patch

Created attachment 82568 [details] Proposed Patch

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

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

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

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.

Created attachment 82584 [details] Proposed Patch

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

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.

Created attachment 82633 [details] Proposed Patch

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

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

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

Created attachment 82724 [details] Proposed Patch There were 2 pages to your code search. I missed WorkerScriptLoader, which implements ThreadableLoaderClient.

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

Created attachment 82728 [details] Proposed Patch

Comment on attachment 82728 [details] Proposed Patch Clearing flags on attachment: 82728 Committed r78782: <http://trac.webkit.org/changeset/78782>

All reviewed patches have been landed. Closing bug.