Bug 37057

Summary: [chromium] Adding hasUserGesture flag to the ResourceRequest
Product: WebKit Reporter: Pierre-Antoine LaFayette <pierre.lafayette>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, fishd
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Changes to add NavigationType to ResourceRequestBase during loadURL
none
The previous patch was incomplete.
none
Patch
none
Patch
none
Fixing ChangeLogs
none
Adding flag to CrossThreadResourceRequestData
none
Adding changes to ResourceRequest none

Description Pierre-Antoine LaFayette 2010-04-02 22:43:14 PDT
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.
Comment 1 Pierre-Antoine LaFayette 2010-04-02 22:44:41 PDT
Created attachment 52479 [details]
Changes to add NavigationType to ResourceRequestBase during loadURL
Comment 2 Pierre-Antoine LaFayette 2010-04-02 22:48:44 PDT
Created attachment 52481 [details]
The previous patch was incomplete.
Comment 3 Alexey Proskuryakov 2010-04-03 12:16:36 PDT
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.
Comment 4 Pierre-Antoine LaFayette 2010-04-03 13:33:15 PDT
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.
Comment 5 Darin Fisher (:fishd, Google) 2010-11-02 14:20:52 PDT
Why isn't WebFrameClient::decidePolicyForNavigation sufficient?  That exposes a WebNavigationType parameter.
Comment 6 Pierre-Antoine LaFayette 2010-11-04 03:30:19 PDT
Created attachment 72924 [details]
Patch
Comment 7 Pierre-Antoine LaFayette 2010-11-04 03:33:20 PDT
As discussed with Darin Fisher, it would be more appropriate to have a hasUserGesture flag instead of storing the navigation type in the ResourceRequestBase.
Comment 8 Alexey Proskuryakov 2010-11-04 08:54:10 PDT
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.
Comment 9 Darin Fisher (:fishd, Google) 2010-11-04 09:36:56 PDT
(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.
Comment 10 Alexey Proskuryakov 2010-11-04 09:41:21 PDT
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?
Comment 11 Darin Fisher (:fishd, Google) 2010-11-04 10:09:15 PDT
(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.
Comment 12 Pierre-Antoine LaFayette 2010-11-04 10:10:15 PDT
Created attachment 72956 [details]
Patch
Comment 13 Alexey Proskuryakov 2010-11-04 10:48:50 PDT
> 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.
Comment 14 Adam Barth 2010-11-04 13:43:07 PDT
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.
Comment 15 Pierre-Antoine LaFayette 2010-11-04 13:54:22 PDT
(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.
Comment 16 Darin Fisher (:fishd, Google) 2010-11-04 15:11:41 PDT
(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.
Comment 17 Darin Fisher (:fishd, Google) 2010-11-04 15:15:27 PDT
(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.
Comment 18 Alexey Proskuryakov 2010-11-04 15:26:38 PDT
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).
Comment 19 Pierre-Antoine LaFayette 2010-11-04 15:47:47 PDT
Created attachment 72992 [details]
Fixing ChangeLogs
Comment 20 Darin Fisher (:fishd, Google) 2010-11-05 13:57:25 PDT
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.
Comment 21 Pierre-Antoine LaFayette 2010-11-05 14:17:32 PDT
(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?
Comment 22 Darin Fisher (:fishd, Google) 2010-11-05 14:58:22 PDT
> 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?
Comment 23 Pierre-Antoine LaFayette 2010-11-05 16:25:35 PDT
(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?
Comment 24 Darin Fisher (:fishd, Google) 2010-11-05 16:44:54 PDT
Comment on attachment 72992 [details]
Fixing ChangeLogs

Actually, sorry... as I noted earlier, don't you need to amend CrossThreadResourceRequestData?
Comment 25 Darin Fisher (:fishd, Google) 2010-11-05 16:46:43 PDT
> Can you file a bug for the refactoring?

Done.  See bug 49113.
Comment 26 Pierre-Antoine LaFayette 2010-11-06 03:41:21 PDT
Created attachment 73168 [details]
Adding flag to CrossThreadResourceRequestData
Comment 27 Pierre-Antoine LaFayette 2010-11-06 19:24:25 PDT
Created attachment 73181 [details]
Adding changes to ResourceRequest
Comment 28 WebKit Commit Bot 2010-11-08 17:05:55 PST
Comment on attachment 73181 [details]
Adding changes to ResourceRequest

Clearing flags on attachment: 73181

Committed r71587: <http://trac.webkit.org/changeset/71587>
Comment 29 WebKit Commit Bot 2010-11-08 17:06:01 PST
All reviewed patches have been landed.  Closing bug.