Summary: | [chromium] Adding hasUserGesture flag to the ResourceRequest | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pierre-Antoine LaFayette <pierre.lafayette> | ||||||||||||||||
Component: | Platform | Assignee: | 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
Pierre-Antoine LaFayette
2010-04-02 22:43:14 PDT
Created attachment 52479 [details]
Changes to add NavigationType to ResourceRequestBase during loadURL
Created attachment 52481 [details]
The previous patch was incomplete.
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. 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.
Why isn't WebFrameClient::decidePolicyForNavigation sufficient? That exposes a WebNavigationType parameter. Created attachment 72924 [details]
Patch
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 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.
(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. 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? (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. Created attachment 72956 [details]
Patch
> 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 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. (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. (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. (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. 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). Created attachment 72992 [details]
Fixing ChangeLogs
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. (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? > 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?
(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 on attachment 72992 [details]
Fixing ChangeLogs
Actually, sorry... as I noted earlier, don't you need to amend CrossThreadResourceRequestData?
> Can you file a bug for the refactoring? Done. See bug 49113. Created attachment 73168 [details]
Adding flag to CrossThreadResourceRequestData
Created attachment 73181 [details]
Adding changes to ResourceRequest
Comment on attachment 73181 [details] Adding changes to ResourceRequest Clearing flags on attachment: 73181 Committed r71587: <http://trac.webkit.org/changeset/71587> All reviewed patches have been landed. Closing bug. |