Bug 104586 – [chromium] Expose quickRedirectComing via WebDataSource

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Charles Reis]

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.

[Charles Reis]

Created attachment 178609 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

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.)

[Charles Reis]

(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.

[Charles Reis]

Created attachment 178904 [details]
Patch

[Nate Chapin]

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.

[Charles Reis]

(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.

[WebKit Review Bot]

Comment on attachment 178904 [details]
Patch

Clearing flags on attachment: 178904

Committed r137542: <http://trac.webkit.org/changeset/137542>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.