Bug 54688 – ThreadableLoaderClient needs willSendRequest method.

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

[Bill Budge]

Needed so clients can implement WebURLLoaderClient.

[Bill Budge]

Created attachment 82861 [details]
Proposed Patch

Forgot I need this method.

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

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.

[David Levin]

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.

[David Levin]

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

[Bill Budge]

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*/) { }

[Bill Budge]

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.

[David Levin]

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

[Bill Budge]

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?

[David Levin]

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

[David Levin]

(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

[Bill Budge]

Created attachment 83018 [details]
Proposed Patch

Derived a DocumentThreadableLoaderClient from ThreadableLoaderClient to add willSendRequest method, which can't be implemented for some clients.

[Bill Budge]

Created attachment 83021 [details]
Proposed Patch

I left an unnecessary forward declaration of ResourceRequest in ThreadableLoaderClient.

[David Levin]

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
> [phone]  Bill Budge  <[e-mail]>

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.

[Bill Budge]

Created attachment 83032 [details]
Proposed Patch

Fixed problems. PTAL.

[Bill Budge]

Created attachment 83036 [details]
Proposed Patch

Fixed copyrights, Changelog. Sorry, didn't get your echo ref.

[David Levin]

(In reply to comment #16)
> Created an attachment (id=83036) [details]
> Sorry, didn't get your echo ref.

It was about the duplicate changelog.

[David Levin]

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

[WebKit Commit Bot]

Comment on attachment 83036 [details]
Proposed Patch

Clearing flags on attachment: 83036

Committed r79110: <http://trac.webkit.org/changeset/79110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.