Bug 36882 – [chromium] move createApplicationCacheHost from WebKitClient to WebFrameClient

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] traverse from WebApplicationCacheHost to its associated WebDataSource and ultimately to its containing WebFrame

Created attachment 52155 [details]
Patch

chromium side is in http://codereview.chromium.org/1600002/show

[Darin Fisher (:fishd, Google)]

Why is it preferrable to pass WebDataSource instead of WebFrame to createApplicationCacheHost?  Also, can we instead introduce a method on WebFrameClient as we did for scripts, images, plugins and databases to check if appcaches are allowed?

(In reply to comment #3)
> Why is it preferrable to pass WebDataSource instead of WebFrame to
> createApplicationCacheHost?  Also, can we instead introduce a method on
> WebFrameClient as we did for scripts, images, plugins and databases to check if
> appcaches are allowed?

could point actually. Michael, is there a method that I could use for that? ApplicationCacheHost::isApplicationCacheEnabled looks like it

[Michael Nordman]

As mentioned in an earlier email, here's an overview of what's going on...

I've been noodling with how to get the icon to show up when an appcache is blocked by the policy. Respecting the policy and putting up prompts as needed is already done (see AppCachePolicy stuff).

Please refer to this work in progress stuff...
http://codereview.chromium.org/1599002/show
http://codereview.chromium.org/1580004/show

There are two parts to this.

* Be able to identify the 'frame' associated with an appcache from the chrome side of things.

* Notify the appcache frontend when something was blocked in the backend. When the msg gets to the frontend, look up the render view and tell it about the blockage.

-------------

> Why is it preferrable to pass WebDataSource instead of WebFrame to
> createApplicationCacheHost?

Frame->DocumentLoader->AppCacheHost is what happens in webcore.

WebFrame->WebDataSource->WebAppCacheHost is what i'm proposing we do in the webkit API. (Actually we have API that lets you traverse in the direction that the arrows show, this is adding stuff to allow traversal in the opposite direction as well).

(note: DocumentLoader is represented in our WebKit API as WebDataSource)

I don't see any reason to deviate from what webcore is doing under the covers. Also honestly, I'm not sure if we know which frame we're destined for at AppCacheHost ctor time (an example of the type of trouble that deviating from what happens under the covers can get you into).

> Also, can we instead introduce a method on
> WebFrameClient as we did for scripts, images, plugins and databases to check if
> appcaches are allowed?

You could but in Chrome it wouldn't be used. We test the policy in the backend far removed from the renderer, only after we're discovered we either have something to load (policy.CanLoadAppCache), or we're trying to create a new appcache (policy.CanCreateAppCache). The callsites for those methods are in place in the chrome code base for all to see. Imagine arranging to call back to the renderer and have it call back out thru some WebFrameClient method which calls back to the browser (i'm getting dizzy already).

It may make sense to have an API like that and hook webcore's impl up to it.

[Michael Nordman]

Darin had a good suggestion for how to establish the frame/datasource/appcache relationships. Instead of adding a WebFrame param to the factory method, move the createApplicationCacheHost() method from WebKitClient to WebFrameClient. That interface has a handful of similar factory methods that take a WebFrame param.

So we'd have...

WebFrameClient::createApplicationCacheHost(
  WebFrame*, WebDataSource*, WebApplicationCacheHostClient*);

... at the time of WebApplicationCacheHost construction (its done lazily in maybeLoadMainResource), we're guaranteed to know what our WebCore::Frame is.


And we wouldn't need the WebDataSource.frame() method which Darin wants to avoid introducing.

(In reply to comment #6)
> Darin had a good suggestion for how to establish the frame/datasource/appcache
> relationships. Instead of adding a WebFrame param to the factory method, move
> the createApplicationCacheHost() method from WebKitClient to WebFrameClient.
> That interface has a handful of similar factory methods that take a WebFrame
> param.
> 
> So we'd have...
> 
> WebFrameClient::createApplicationCacheHost(
>   WebFrame*, WebDataSource*, WebApplicationCacheHostClient*);
> 
> ... at the time of WebApplicationCacheHost construction (its done lazily in
> maybeLoadMainResource), we're guaranteed to know what our WebCore::Frame is.
> 
> 
> And we wouldn't need the WebDataSource.frame() method which Darin wants to
> avoid introducing.

what do we need the datasource for then? All I need to send the notification around is the render_view_id of the frame, e.g.

RenderView::FromWebView(frame->view())->routing_id(). This information is extracted on construction of the WebApplicationCacheHost (see http://codereview.chromium.org/1600002 )

[Michael Nordman]

> what do we need the datasource for then? All I need to send the notification
> around is the render_view_id of the frame, e.g.

You're right, we don't need that given the frame.

Created attachment 52637 [details]
Patch

Please also review http://codereview.chromium.org/1627003 which prepares chromium for this patch.

Comment on attachment 52637 [details]
Patch

I've cancelled commit-queue because the chromium side needs to be landed first. please review anyway

[Michael Nordman]

4446     ApplicationCacheHostInternal(ApplicationCacheHost* host)
4547         : m_innerHost(host)
4648     {
47          m_outerHost.set(WebKit::webKitClient()->createApplicationCacheHost(this));
 49       WebKit::WebFrameImpl* webFrame = WebKit::WebFrameImpl::fromFrame(host->m_documentLoader->frame());
 50         m_outerHost.set(webFrame->client()->createApplicationCacheHost(webFrame, this));
4851     }

Wunderbar!

Indents are off, webkit uses 4 space indentation. Maybe ASSERT(webFrame)?

[Darin Fisher (:fishd, Google)]

Comment on attachment 52637 [details]
Patch

R- based on michaeln's comments.

Created attachment 52709 [details]
Patch

Created attachment 52710 [details]
Patch

[Michael Nordman]

(In reply to comment #15)
> Created an attachment (id=52710) [details]
> Patch

LGTM (fwiw)

[WebKit Commit Bot]

Comment on attachment 52710 [details]
Patch

Clearing flags on attachment: 52710

Committed r57197: <http://trac.webkit.org/changeset/57197>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.