Summary: | [chromium] Expose quickRedirectComing via WebDataSource | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Charles Reis <creis> | ||||||
Component: | WebKit API | Assignee: | Charles Reis <creis> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Charles Reis
2012-12-10 12:11:16 PST
Created attachment 178609 [details]
Patch
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. 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.) (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. Created attachment 178904 [details]
Patch
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. (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 on attachment 178904 [details] Patch Clearing flags on attachment: 178904 Committed r137542: <http://trac.webkit.org/changeset/137542> All reviewed patches have been landed. Closing bug. |