Bug 104586 - [chromium] Expose quickRedirectComing via WebDataSource
Summary: [chromium] Expose quickRedirectComing via WebDataSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Charles Reis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-10 12:11 PST by Charles Reis
Modified: 2012-12-12 17:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2012-12-10 12:18 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (3.18 KB, patch)
2012-12-11 15:57 PST, Charles Reis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Reis 2012-12-10 12:11:16 PST
In Chromium, there are cases that the WebFrameClient needs to know whether the current navigation is a redirect that should replace the current history item.  This is kept in FrameLoader::quickRedirectComing().  We'd like to expose it via WebDataSource so that the client can use it in decidePolicyForNavigation.

Note that quickRedirectComing() is also exposed via DocumentLoader::isClientRedirect().  However, this isn't set until after decidePolicyForNavigation is called.  It's also confusingly named, since WebFrameClient::willPerformClientRedirect is called whether isClientRedirect() is true or not.  (willPerformClientRedirect refers to JavaScript navigations, whether they occur before or after the current document finishes loading, and thus it may sometimes leave the current history item in place.)

For reference, this is needed to fix http://crbug.com/164419.
Comment 1 Charles Reis 2012-12-10 12:18:00 PST
Created attachment 178609 [details]
Patch
Comment 2 WebKit Review Bot 2012-12-10 12:23:04 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Darin Fisher (:fishd, Google) 2012-12-10 22:01:49 PST
I think we should fix willPerformClientRedirect to only be called when indeed there is a client redirect (a.k.a., a navigation that replaces the current history item).  

I haven't studied the logic in willPerformClientRedirect very carefully on the Chromium side, but I wouldn't be surprised if it is confused / buggy.

I think we should expose the method on WebDataSource as isClientRedirect, and I think the DocumentLoader can have its isClientRedirect field set very early on (before decidePolicyForNavigation).  We know when we create a DocumentLoader if it is going to be a client redirect or not.  (That's not something that changes over the course of the lifetime of a DocumentLoader.)
Comment 4 Charles Reis 2012-12-11 15:56:50 PST
(In reply to comment #3)
> I think we should fix willPerformClientRedirect to only be called when indeed there is a client redirect (a.k.a., a navigation that replaces the current history item).  
> 
> I haven't studied the logic in willPerformClientRedirect very carefully on the Chromium side, but I wouldn't be surprised if it is confused / buggy.
> 
> I think we should expose the method on WebDataSource as isClientRedirect, and I think the DocumentLoader can have its isClientRedirect field set very early on (before decidePolicyForNavigation).  We know when we create a DocumentLoader if it is going to be a client redirect or not.  (That's not something that changes over the course of the lifetime of a DocumentLoader.)

I think those are great ideas, though I'm concerned about the risk of changing this behavior just before a branch point.  Can I file a separate bug for changing willPerformClientRedirect and setting DocumentLoader::isClientRedirect earlier?

I've changed this patch to expose WebDataSource::isClientRedirect, which for now actually exposes FrameLoader::quickRedirectComing.  We can change it to return DocumentLoader::isClientRedirect once we set that value earlier.
Comment 5 Charles Reis 2012-12-11 15:57:05 PST
Created attachment 178904 [details]
Patch
Comment 6 Nate Chapin 2012-12-12 16:30:25 PST
Comment on attachment 178904 [details]
Patch

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

> Source/WebKit/chromium/src/WebDataSourceImpl.cpp:93
> +bool WebDataSourceImpl::isClientRedirect() const
> +{
> +    // FIXME: This should return DocumentLoader::isClientRedirect() once that is
> +    // changed to be set earlier than the call to WebFrameClient::decidePolicyForNavigation.
> +    return frameLoader() ? frameLoader()->quickRedirectComing() : false;
> +}
> +

I worry about depending on quickRedirectComing(), but I understand that using DocumentLoader::isClientRedirect() is a much more complex undertaking. So long as we tackle this FIXME sometime soon, this LGTM.
Comment 7 Charles Reis 2012-12-12 16:42:23 PST
(In reply to comment #6)
> (From update of attachment 178904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178904&action=review
> 
> > Source/WebKit/chromium/src/WebDataSourceImpl.cpp:93
> > +bool WebDataSourceImpl::isClientRedirect() const
> > +{
> > +    // FIXME: This should return DocumentLoader::isClientRedirect() once that is
> > +    // changed to be set earlier than the call to WebFrameClient::decidePolicyForNavigation.
> > +    return frameLoader() ? frameLoader()->quickRedirectComing() : false;
> > +}
> > +
> 
> I worry about depending on quickRedirectComing(), but I understand that using DocumentLoader::isClientRedirect() is a much more complex undertaking. So long as we tackle this FIXME sometime soon, this LGTM.

Thanks.  I filed bug 104860 to tackle it.  Darin Fisher is on board with this plan, so I'll go ahead with landing it.
Comment 8 WebKit Review Bot 2012-12-12 17:29:39 PST
Comment on attachment 178904 [details]
Patch

Clearing flags on attachment: 178904

Committed r137542: <http://trac.webkit.org/changeset/137542>
Comment 9 WebKit Review Bot 2012-12-12 17:29:43 PST
All reviewed patches have been landed.  Closing bug.