Bug 25028 – Restore FrameLoader::policyDocumentLoader to fix the Chromium build

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Created attachment 29229 [details]
v1 patch

[Darin Fisher (:fishd, Google)]

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

[Darin Adler]

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

[Darin Adler]

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

[Darin Fisher (:fishd, Google)]

> 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

[Darin Fisher (:fishd, Google)]

The key issue is that the policyDocumentLoader doesn't become provisional until after dispatchDecidePolicyForNavigationAction OKs the navigation.