Bug 54688 - ThreadableLoaderClient needs willSendRequest method.
Summary: ThreadableLoaderClient needs willSendRequest method.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 53925
  Show dependency treegraph
 
Reported: 2011-02-17 14:02 PST by Bill Budge
Modified: 2011-02-19 00:39 PST (History)
3 users (show)

See Also:


Attachments
Proposed Patch (6.10 KB, patch)
2011-02-17 14:14 PST, Bill Budge
levin: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (7.88 KB, patch)
2011-02-18 15:00 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (7.72 KB, patch)
2011-02-18 15:14 PST, Bill Budge
levin: review-
Details | Formatted Diff | Diff
Proposed Patch (7.67 KB, patch)
2011-02-18 16:42 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (7.36 KB, patch)
2011-02-18 16:56 PST, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Budge 2011-02-17 14:02:49 PST
Needed so clients can implement WebURLLoaderClient.
Comment 1 Bill Budge 2011-02-17 14:14:55 PST
Created attachment 82861 [details]
Proposed Patch

Forgot I need this method.
Comment 2 Alexey Proskuryakov 2011-02-17 19:46:40 PST
Comment on attachment 82861 [details]
Proposed Patch

+    } else {
+        m_client->willSendRequest(newRequest, redirectResponse);
     }

The coding style calls for no braces around single line blocks. Why didn't the style bot catch this?
Comment 3 Alexey Proskuryakov 2011-02-17 19:52:31 PST
Seems unfortunate that we have pass a willSendRequest from ResourceHandle to worker thread, and then go all the way back to make a client call.

> +    m_loaderProxy.postTaskForModeToWorkerContext(createCallbackTask(&workerContextWillSendRequest, m_workerClientWrapper, newRequest, redirectResponse), m_taskMode);

Does this make a synchronous call? I'm curious how we avoid deadlocking here.
Comment 4 David Levin 2011-02-17 20:25:38 PST
Comment on attachment 82861 [details]
Proposed Patch

Good catch Alexey!

As Alexey gently pointed out, this simply doesn't work. willSendRequest may modify the request (see http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoaderClient.h), but all calls to the worker are async, so the callback to the worker doesn't function correctly.

In fact, you can't do a sync call in that direction, so something else has to be done for this case.
Comment 5 David Levin 2011-02-17 20:36:28 PST
Here's a possible alternative:

In ThreadableLoaderClient add this method:
    virtual bool isDocumentThreadableLoaderClient() { return false; }


Create a new class:
    class DocumentThreadableLoaderClient : public ThreadableLoaderClient {
    public:
        virtual bool isDocumentThreadableLoaderClient() { return true; }

        // request may be modified
        virtual void willSendRequest(SubresourceLoader*, ResourceRequest&, const ResourceResponse& /*redirectResponse*/) { }
    };


In DocumentThreadableLoader::willSendRequest put the following in place of your current call to willSendRequest:

    if (m_client->isDocumentThreadableLoaderClient())
        static_cast<DocumentThreadableLoaderClient*>(m_client)->willSendRequest(....);
Comment 6 Bill Budge 2011-02-17 21:20:57 PST
Yes, good catch!

Is there a reason to pass the SubresourceLoader as a parameter to the client? It seems better to keep it an implementation detail. 

// request may be modified
        virtual void willSendRequest(SubresourceLoader*, ResourceRequest&, const ResourceResponse& /*redirectResponse*/) { }
Comment 7 Bill Budge 2011-02-17 21:30:20 PST
Another question David: If we create this derived DocumentThreadableLoaderClient, why not change DocumentThreadableLoader to only accept this kind of client? Is there a use case for passing in a ThreadableLoaderClient? It would be nice to avoid the up-casting.
Comment 8 David Levin 2011-02-17 21:40:56 PST
(In reply to comment #6)
> Is there a reason to pass the SubresourceLoader as a parameter to the client? It seems better to keep it an implementation detail. 

No, it was a copy/paste error.

(In reply to comment #7)
> Another question David: If we create this derived DocumentThreadableLoaderClient, why not change DocumentThreadableLoader to only accept this kind of client?

Not possible.

> Is there a use case for passing in a ThreadableLoaderClient? It would be nice to avoid the up-casting.
Yes and yes but unavoidable.

There are several places that create a ThreadableLoader (like XHR). They don't care if the underlying thing is a DocumentThreadableLoader or a WorkerThreadableLoader. It is totally transparent to them as it should be.

So they will pass in a ThreadableLoaderClient which is then passed to the DocumentThreadableLoader or WokerThreadableLoader. (The "Threadable" part of the name is a reference to the fact that the ThreadableLoader may be used on threads unlike all other loaders.)

Now, that I think about it, you could do make DocumentThreadableLoader only take a DocumentThreadableLoaderClient but you would have to make an adapter class which makes a ThreadableLoaderClient look like a DocumentThreadableLoaderClient (-- it would pass through all methods and ignore the willSendRequest). This feel uglier than the upcast (to me).
Comment 9 Bill Budge 2011-02-18 05:57:19 PST
You made me think: If I took out the implementation of willSendRequest in WorkerThreadableLoader that attempts the asynchronous call and just made it a no-op, wouldn't that be the same thing?
Comment 10 David Levin 2011-02-18 07:46:05 PST
(In reply to comment #9)
> You made me think: If I took out the implementation of willSendRequest in WorkerThreadableLoader that attempts the asynchronous call and just made it a no-op, wouldn't that be the same thing?

But then you'd have this method in ThreadableLoaderClient which doesn't work (for the worker case).

btw, I thought a bit more about why I didn't like the adapter pattern in this case:
1. It adds another level of virtuals to go through.
2. It adds another class that people have to understand and maintain.

On the plus side 
1. It allows for more flexibility. (But that flexibility is only needed for one method.)
2. No need to upcast (and the isDocumentThreadableLoaderClient method could go away).

At this point I feel like the negatives out weight the positives.
Comment 11 David Levin 2011-02-18 13:42:15 PST
(In reply to comment #2)
> (From update of attachment 82861 [details])
> +    } else {
> +        m_client->willSendRequest(newRequest, redirectResponse);
>      }
> 
> The coding style calls for no braces around single line blocks. Why didn't the style bot catch this?

As penance for missing this in my review, I improved the style checker to catch this: https://bugs.webkit.org/show_bug.cgi?id=54769
Comment 12 Bill Budge 2011-02-18 15:00:00 PST
Created attachment 83018 [details]
Proposed Patch

Derived a DocumentThreadableLoaderClient from ThreadableLoaderClient to add willSendRequest method, which can't be implemented for some clients.
Comment 13 Bill Budge 2011-02-18 15:14:05 PST
Created attachment 83021 [details]
Proposed Patch

I left an unnecessary forward declaration of ResourceRequest in ThreadableLoaderClient.
Comment 14 David Levin 2011-02-18 16:07:26 PST
Comment on attachment 83021 [details]
Proposed Patch

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

Feel free to add ", 2011" to all of the Google copyright years in DocumentThreadableLoader.cpp and ThreadableLoaderClient.h

r- for the changelog echo and the inverted if conditions.

> Source/WebCore/ChangeLog:21
> +2011-02-18  Bill Budge  <bbudge@chromium.org>

I sense an echo in here.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:176
> +void DocumentThreadableLoader::willSendRequest(SubresourceLoader* loader, ResourceRequest& newRequest, const ResourceResponse& redirectResponse)

I'm not sure why "request" became "newRequest".

It is the request that will be sent.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:238
> +    if (!m_actualRequest)

This looks wrong.

m_actualRequest is only set while the preflight request is happening.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:251
> +        return;

Ditto.
Comment 15 Bill Budge 2011-02-18 16:42:04 PST
Created attachment 83032 [details]
Proposed Patch

Fixed problems. PTAL.
Comment 16 Bill Budge 2011-02-18 16:56:47 PST
Created attachment 83036 [details]
Proposed Patch

Fixed copyrights, Changelog. Sorry, didn't get your echo ref.
Comment 17 David Levin 2011-02-18 17:30:44 PST
(In reply to comment #16)
> Created an attachment (id=83036) [details]
> Sorry, didn't get your echo ref.

It was about the duplicate changelog.
Comment 18 David Levin 2011-02-18 17:32:18 PST
Comment on attachment 83036 [details]
Proposed Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

fwiw, unlike Chromium code, WebKit typically keeps the old copyright year.
  Copyright (C) 2009, 2011 Google Inc. All rights reserved.
etc. (but not a big deal).
Comment 19 WebKit Commit Bot 2011-02-19 00:39:55 PST
Comment on attachment 83036 [details]
Proposed Patch

Clearing flags on attachment: 83036

Committed r79110: <http://trac.webkit.org/changeset/79110>
Comment 20 WebKit Commit Bot 2011-02-19 00:39:59 PST
All reviewed patches have been landed.  Closing bug.