Bug 37057 – [chromium] Adding hasUserGesture flag to the ResourceRequest

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

[Pierre-Antoine LaFayette]

This patch adds the NavigationType to the ResourceRequestBase during loadURL so that it may be available in the request. This patch will help reliably determine whether a user has initiated a navigation.

[Pierre-Antoine LaFayette]

Created attachment 52479 [details]
Changes to add NavigationType to ResourceRequestBase during loadURL

[Pierre-Antoine LaFayette]

Created attachment 52481 [details]
The previous patch was incomplete.

[Alexey Proskuryakov]

This patch isn't marked for review, but anyway, I don't think that it's something we should do. A ResourceRequest isn't necessarily related to any navigation - it's also used for XMLHttpRequest, for example.

[Pierre-Antoine LaFayette]

Thanks. This patch is a dependency for a chromium patch that I am working on. I did not mark it for review because I'm waiting on the chromium patch review before I'm sure that this WebKit change is necessary. 

I'm trying to determine, from the browser side, if a download has been caused by a user clicking on a link. If the NavigationType is available in the load request, then we'll get this information synchronously. Otherwise, we have to store state for a bit; which could possibly be compromised by subsequent user clicks.

If this is addition is not reasonable, I suppose we'll have to figure out another way.

> This patch isn't marked for review, but anyway, I don't think that it's
> something we should do. A ResourceRequest isn't necessarily related to any
> navigation - it's also used for XMLHttpRequest, for example.

[Darin Fisher (:fishd, Google)]

Why isn't WebFrameClient::decidePolicyForNavigation sufficient?  That exposes a WebNavigationType parameter.

[Pierre-Antoine LaFayette]

Created attachment 72924 [details]
Patch

[Pierre-Antoine LaFayette]

As discussed with Darin Fisher, it would be more appropriate to have a hasUserGesture flag instead of storing the navigation type in the ResourceRequestBase.

[Alexey Proskuryakov]

Comment on attachment 72924 [details]
Patch

ResourceRequest is part of low level loader machinery that shouldn't know anything about the Web platform, users or other high level concepts.

[Darin Fisher (:fishd, Google)]

(In reply to comment #8)
> (From update of attachment 72924 [details])
> ResourceRequest is part of low level loader machinery that shouldn't know anything about the Web platform, users or other high level concepts.

Hi Alexey,

I spoke with Pierre.  We're going to move this to PLATFORM(CHROMIUM) specific code.  Thanks.

[Alexey Proskuryakov]

I guess I can live with that for now, but the situation where Chromium gets even more layering violations is disturbing. Is there a plan to rectify that?

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> I guess I can live with that for now, but the situation where Chromium gets even more layering violations is disturbing. Is there a plan to rectify that?

We are just adding this field here for convenience.  It was previously relayed in a different way, that was far more complicated.  Since this field is populated by the embedding app, I suspect a better solution (for this field as well as a number of other ResourceRequest fields) would be to add support for an opaque 'user data' field on ResourceRequest so that we can tag it with an external structure.  This would clean things up considerably.

[Pierre-Antoine LaFayette]

Created attachment 72956 [details]
Patch

[Alexey Proskuryakov]

> I suspect a better solution (for this field as well as a number of other ResourceRequest fields) 
> would be to add support for an opaque 'user data' field on ResourceRequest so that we can tag
>  it with an external structure.  This would clean things up considerably.

I agree that this would probably be an improvement, but the ultimate goal should be for loader to not know about Web platform concepts at all, not to change the way they are passed into it.

[Adam Barth]

Comment on attachment 72956 [details]
Patch

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

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

We can't land this patch with this line (because of the OOPS).  We need to either replace this line with a list of tests or an explanation of why this change isn't testable.

[Pierre-Antoine LaFayette]

(In reply to comment #14)
> (From update of attachment 72956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72956&action=review
> 
> > WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> We can't land this patch with this line (because of the OOPS).  We need to either replace this line with a list of tests or an explanation of why this change isn't testable.

Is it actually common that WebKit folks test accessors and mutators? The actual testing would come from the Chromium side browser use case.

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> > I suspect a better solution (for this field as well as a number of other ResourceRequest fields) 
> > would be to add support for an opaque 'user data' field on ResourceRequest so that we can tag
> >  it with an external structure.  This would clean things up considerably.
> 
> I agree that this would probably be an improvement, but the ultimate goal should be for loader to not know about Web platform concepts at all, not to change the way they are passed into it.

I disagree.  We derive a lot of benefits from passing this information to our resource loading layer in Chromium.  For one thing, it avoids the need to route data through WebKit when we need to download a file to disk.  I'd be happy to discuss this topic further with you in another forum.

[Darin Fisher (:fishd, Google)]

(In reply to comment #16)
> (In reply to comment #13)
> > > I suspect a better solution (for this field as well as a number of other ResourceRequest fields) 
> > > would be to add support for an opaque 'user data' field on ResourceRequest so that we can tag
> > >  it with an external structure.  This would clean things up considerably.
> > 
> > I agree that this would probably be an improvement, but the ultimate goal should be for loader to not know about Web platform concepts at all, not to change the way they are passed into it.
> 
> I disagree.

Ugh, what I really mean is for us to arrive at a solution that fits both of our constraints, something more substantial is going to need to change at the layers above ResourceHandle.  I'm not sure how it would look, but it would probably involve something radical.

[Alexey Proskuryakov]

That I agree with - it's the ResourceLoader level that knows about Web concepts, but it needs significant refactoring to be at process boundary itself.

Such refactoring probably needs to start soon to avoid blocking Chromium, as seen in bug 44722 (see bug 44722 comment 49 in particular).

[Pierre-Antoine LaFayette]

Created attachment 72992 [details]
Fixing ChangeLogs

[Darin Fisher (:fishd, Google)]

Comment on attachment 72992 [details]
Fixing ChangeLogs

This patch looks good, but I'm now questioning whether we should do it like
this.  The problem is that we are teaching WebKit to store fields that are
meaningless to it.  We are doing that just so we can pass information from
WebFrameClient::willSendRequest to WebURLLoader.  hasUserGesture is not the
first example of this, so I feel bad making this your problem, but we have
to clean up the code at some point.  m_requestorID and m_requestorProcessID
similarly exist only to support the embedder, Chromium.

Instead, I suspect we may be better served by jumping to the solution I
mentioned in comment #11.  We already do something like that for WebDataSource.
See WebDataSource::ExtraData.  One difference is that the ExtraData that would
be associated with a ResourceRequest needs to be copied when a ResourceRequest
is copied.  The DocumentLoader (which WebDataSource wraps) does not get copied
in the same way, so it does not have this problem.

Something like reference counting seems like the right answer...

class WebURLRequest {
public:
    class ExtraData {
    public:
        virtual void ref() = 0;
        virtual void deref() = 0;
    protected:
        ~ExtraData() { }
    };

    WEBKIT_API ExtraData* extraData() const;
    WEBKIT_API void setExtraData(ExtraData*);
    ...
};

Unfortunately, though, we do not permit referring to WebKit:: classes
from WebCore, so we wouldn't want to directly stash one of these
WebURLRequest::ExtraData pointers on ResourceRequest.  We'd need to
either create another interface like ExtraData at the ResourceRequest
level, or we'd need to find some way to stash the pointer in some
opaque fashion :-(  I don't have a good answer for this.

Given the current patch, even though you don't need it, perhaps you
should consider amending CrossThreadResourceRequestData.

[Pierre-Antoine LaFayette]

(In reply to comment #20)
> (From update of attachment 72992 [details])
> This patch looks good, but I'm now questioning whether we should do it like
> this.  The problem is that we are teaching WebKit to store fields that are
> meaningless to it.  We are doing that just so we can pass information from
> WebFrameClient::willSendRequest to WebURLLoader.  hasUserGesture is not the
> first example of this, so I feel bad making this your problem, but we have
> to clean up the code at some point.  m_requestorID and m_requestorProcessID
> similarly exist only to support the embedder, Chromium.
> 
> Instead, I suspect we may be better served by jumping to the solution I
> mentioned in comment #11.  We already do something like that for WebDataSource.
> See WebDataSource::ExtraData.  One difference is that the ExtraData that would
> be associated with a ResourceRequest needs to be copied when a ResourceRequest
> is copied.  The DocumentLoader (which WebDataSource wraps) does not get copied
> in the same way, so it does not have this problem.
> 
> Something like reference counting seems like the right answer...
> 
> class WebURLRequest {
> public:
>     class ExtraData {
>     public:
>         virtual void ref() = 0;
>         virtual void deref() = 0;
>     protected:
>         ~ExtraData() { }
>     };
> 
>     WEBKIT_API ExtraData* extraData() const;
>     WEBKIT_API void setExtraData(ExtraData*);
>     ...
> };
> 
> Unfortunately, though, we do not permit referring to WebKit:: classes
> from WebCore, so we wouldn't want to directly stash one of these
> WebURLRequest::ExtraData pointers on ResourceRequest.  We'd need to
> either create another interface like ExtraData at the ResourceRequest
> level, or we'd need to find some way to stash the pointer in some
> opaque fashion :-(  I don't have a good answer for this.
> 
> Given the current patch, even though you don't need it, perhaps you
> should consider amending CrossThreadResourceRequestData.

This refactoring would be a separate issue. Are you saying that it needs to be done before this patch or after?

[Darin Fisher (:fishd, Google)]

> This refactoring would be a separate issue. Are you saying that it needs to be done before this patch or after?

Do you prefer to do it before or after?  Are you interested in helping with this?

[Pierre-Antoine LaFayette]

(In reply to comment #22)
> > This refactoring would be a separate issue. Are you saying that it needs to be done before this patch or after?
> 
> Do you prefer to do it before or after?  Are you interested in helping with this?

Sure, I can help. I'd rather have this patch go through as is so that the issue on the chromium side doesn't get blocked waiting for this work. Then we can do the refactoring. Can you file a bug for the refactoring?

[Darin Fisher (:fishd, Google)]

Comment on attachment 72992 [details]
Fixing ChangeLogs

Actually, sorry... as I noted earlier, don't you need to amend CrossThreadResourceRequestData?

[Darin Fisher (:fishd, Google)]

> Can you file a bug for the refactoring?

Done.  See bug 49113.

[Pierre-Antoine LaFayette]

Created attachment 73168 [details]
Adding flag to CrossThreadResourceRequestData

[Pierre-Antoine LaFayette]

Created attachment 73181 [details]
Adding changes to ResourceRequest

[WebKit Commit Bot]

Comment on attachment 73181 [details]
Adding changes to ResourceRequest

Clearing flags on attachment: 73181

Committed r71587: <http://trac.webkit.org/changeset/71587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.