Bug 25028 - Restore FrameLoader::policyDocumentLoader to fix the Chromium build
Summary: Restore FrameLoader::policyDocumentLoader to fix the Chromium build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-03 09:40 PDT by Darin Fisher (:fishd, Google)
Modified: 2009-04-03 10:55 PDT (History)
1 user (show)

See Also:


Attachments
v1 patch (1.21 KB, patch)
2009-04-03 09:42 PDT, Darin Fisher (:fishd, Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2009-04-03 09:40:51 PDT
Restore FrameLoader::policyDocumentLoader to fix the Chromium build

r42158 removed FrameLoader::policyDocumentLoader which was originally added in r32320 as part of upstreaming changes to WebCore required by the Chromium build.

We found that there are some FrameLoaderClient callbacks that occur when the current DocumentLoader is m_policyDocumentLoader, and without an accessor to that, it is not possible from FrameLoaderClient to access the DocumentLoader.  This is important since the FrameLoaderClient may have subclassed DocumentLoader to provide additional state (which is what Chromium does).

I understand that the code I'm referring to is not yet upstreamed, so it makes sense that this change would have been made.  It surely looked like there were no other consumers.

Hopefully this is a reasonable method to restore on FrameLoader since it makes sense to be able to access the current DocumentLoader during policy callbacks to the FrameLoaderClient.

Patch coming up...
Comment 1 Darin Fisher (:fishd, Google) 2009-04-03 09:42:00 PDT
Created attachment 29229 [details]
v1 patch
Comment 2 Darin Fisher (:fishd, Google) 2009-04-03 09:43:46 PDT
> This is important since the FrameLoaderClient may have subclassed
> DocumentLoader to provide additional state

Clarification:  What I mean is that from FrameLoaderClient::createDocumentLoader, the FrameLoaderClient implementation can allocate a class that subclasses DocumentLoader.  Then from other FrameLoaderClient methods, that subclass is exposed via a down cast from DocumentLoader.
Comment 3 Darin Adler 2009-04-03 09:56:56 PDT
Comment on attachment 29229 [details]
v1 patch

It would be good to know why this is needed. I think it’s likely a mistake.

r=me on bringing it back
Comment 4 Darin Adler 2009-04-03 09:57:52 PDT
(In reply to comment #0)
> We found that there are some FrameLoaderClient callbacks that occur when the
> current DocumentLoader is m_policyDocumentLoader, and without an accessor to
> that, it is not possible from FrameLoaderClient to access the DocumentLoader.

We should consider adding the DocumentLoader as an argument to those functions to solve the problem.
Comment 5 Darin Fisher (:fishd, Google) 2009-04-03 10:53:02 PDT
> We should consider adding the DocumentLoader as an argument to those functions
> to solve the problem.

That sounds like a good idea.  The FrameLoaderClient method in question is dispatchDecidePolicyForNavigationAction.

Landed as:  http://trac.webkit.org/changeset/42202
Comment 6 Darin Fisher (:fishd, Google) 2009-04-03 10:55:49 PDT
The key issue is that the policyDocumentLoader doesn't become provisional until after dispatchDecidePolicyForNavigationAction OKs the navigation.