Bug 33381 – [Android] FrameLoaderClientAndroid::canHandleRequest needs to know whether the request was initiated by a user gesture

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Steve Block]

FrameLoaderClientAndroid::canHandleRequest needs to know whether the request was initiated by a user gesture. This is required because the Android browser does not consider using an external application to handle the request if the request was not user-initiated.

[Steve Block]

Created attachment 46138 [details]
Patch 1 for Bug 33381

This patch adds a new 'isUserGesture' flag to the ResourceRequest. This is set by the FrameLoader, and can be read by the FrameLoaderClient. Due to the many different pathways from FrameLoader to FrameLoaderClient::canHandleRequest, this is simpler than adding userGesture arguments to many of the FrameLoader methods.

Note that a new userGesture argument is required for loadURL as this method creates a new ResourceRequest object.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46138 [details] without any errors.

[Adam Barth]

Comment on attachment 46138 [details]
Patch 1 for Bug 33381

This seems related to Bug 33188.  We're not accepting changes to FrameLoader without tests.

Also, in general, you can't add new data members to ResourceRequestBase because that class needs to round-trip all it's state through some CFNetwork object.  I think the roundtriping requirement is dumb, but I've not succeeded in removing it.

[Steve Block]

> We're not accepting changes to FrameLoader
> without tests.
I'll look into adding unit tests but I was hoping you could take a look at the changes to FrameLoader and let me know if the approach looks reasonable?

> Also, in general, you can't add new data members to ResourceRequestBase because
> that class needs to round-trip all it's state through some CFNetwork object.
Is this a hard rule? Is it not possible to update whatever code interfaces with the CF object to include the changes to ResourceRequestBase? Or would you prefer me to add it to each platform's implementation of ResourceRequest?

Thanks

[Adam Barth]

> I'll look into adding unit tests but I was hoping you could take a look at the
> changes to FrameLoader and let me know if the approach looks reasonable?

The problem is the FrameLoader is not well designed.  You should coordinate with ddklizer.  He's got a patch in flight that looks to be trying to solve the same problem, although he's using a slightly different approach.

> > Also, in general, you can't add new data members to ResourceRequestBase because
> > that class needs to round-trip all it's state through some CFNetwork object.
> Is this a hard rule?

A number of patches (including some of mine) were rejected for this reason.  We managed to find other ways of solving the problems we were trying to solve without adding members to ResourceRequestBase.

> Is it not possible to update whatever code interfaces with
> the CF object to include the changes to ResourceRequestBase?

I believe this is the current long-term solution.  I haven't looked into this part of the Mac port enough to know how hard this is.

> Or would you
> prefer me to add it to each platform's implementation of ResourceRequest?

That's not the right approach either.  This is not platform-specific information.

The underlying design problem is that ResourceRequest is two low-level a concept because ResourceRequest is just information for the network layer.  We have FrameLoadRequest, but I suspect that's too high-level a concept.  We need something in between which is the ResourceRequest + the WebCore context for that request.  The context should include things like "generated by user gesture" or "generated by this security origin."

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Created an attachment (id=46138) [details]
> Patch 1 for Bug 33381
> 
> This patch adds a new 'isUserGesture' flag to the ResourceRequest. This is set
> by the FrameLoader, and can be read by the FrameLoaderClient. Due to the many
> different pathways from FrameLoader to FrameLoaderClient::canHandleRequest,
> this is simpler than adding userGesture arguments to many of the FrameLoader
> methods.
> 
> Note that a new userGesture argument is required for loadURL as this method
> creates a new ResourceRequest object.

A lot of the userGesture arguments were removed from the FrameLoader methods in r35377.  <http://trac.webkit.org/changeset/35377>

The iPhone WebKit port also kept these flags in the methods (plus some additional ones) until Greg Bolsinga determined it was better to ask the FrameLoader about the isProcessingUserGesture() state when we needed it rather than passing around a boolean all the time.  This was the reason -[WebView _isProcessingUserGesture] was added in r45634.  <http://trac.webkit.org/changeset/45634>

I think what you want to do is the same as what I described in Bug 33188 Comment #12, which is that you want to know when the content for a given frame was loaded by a user action.  This can be determined by the same userGesture boolean passed into FrameLoader::changeLocation() and urlSelected(), but the remaining challenge (which I haven't determined yet) is to know when to reset a flag on the FrameLoader to cover all the various load paths.

Does that sound correct?

[Steve Block]

> I think what you want to do is the same as what I described in Bug 33188
> Comment #12, which is that you want to know when the content for a given frame
> was loaded by a user action.  This can be determined by the same userGesture
> boolean passed into FrameLoader::changeLocation() and urlSelected(), but the
> remaining challenge (which I haven't determined yet) is to know when to reset a
> flag on the FrameLoader to cover all the various load paths.
Yes, that sounds like exactly what I need to fix this bug.

I think a little extra plumbing will still be required to pass the userEnabled state through to canHandleRequest, so I've marked this bug as blocking on Bug 33188.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> > I think what you want to do is the same as what I described in Bug 33188
> > Comment #12, which is that you want to know when the content for a given frame
> > was loaded by a user action.  This can be determined by the same userGesture
> > boolean passed into FrameLoader::changeLocation() and urlSelected(), but the
> > remaining challenge (which I haven't determined yet) is to know when to reset a
> > flag on the FrameLoader to cover all the various load paths.
> Yes, that sounds like exactly what I need to fix this bug.
> 
> I think a little extra plumbing will still be required to pass the userEnabled
> state through to canHandleRequest, so I've marked this bug as blocking on Bug
> 33188.

A UserGestureIndicator object was added in r57045 for Bug 37008.  Perhaps a similar concept could be used instead of plumbing a state variable through every method?

[Steve Block]

The Android port is being removed in Bug 66688.