Bug 36882 - [chromium] move createApplicationCacheHost from WebKitClient to WebFrameClient
: [chromium] move createApplicationCacheHost from WebKitClient to WebFrameClient
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-31 06:29 PDT by jochen
Modified: 2010-04-07 00:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.41 KB, patch)
2010-03-31 06:30 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (3.73 KB, patch)
2010-04-06 07:49 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2010-04-07 00:14 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (2.64 KB, patch)
2010-04-07 00:25 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2010-03-31 06:29:25 PDT
[chromium] traverse from WebApplicationCacheHost to its associated WebDataSource and ultimately to its containing WebFrame
Comment 1 jochen 2010-03-31 06:30:24 PDT
Created attachment 52155 [details]
Patch
Comment 2 jochen 2010-03-31 06:38:21 PDT
chromium side is in http://codereview.chromium.org/1600002/show
Comment 3 Darin Fisher (:fishd, Google) 2010-03-31 07:58:00 PDT
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?
Comment 4 jochen 2010-03-31 09:14:13 PDT
(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
Comment 5 Michael Nordman 2010-03-31 12:25:36 PDT
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.
Comment 6 Michael Nordman 2010-03-31 14:34:08 PDT
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.
Comment 7 jochen 2010-03-31 14:42:31 PDT
(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 )
Comment 8 Michael Nordman 2010-03-31 14:56:26 PDT
> 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.
Comment 9 jochen 2010-04-06 07:49:15 PDT
Created attachment 52637 [details]
Patch
Comment 10 jochen 2010-04-06 07:52:32 PDT
Please also review http://codereview.chromium.org/1627003 which prepares chromium for this patch.
Comment 11 jochen 2010-04-06 08:29:24 PDT
Comment on attachment 52637 [details]
Patch

I've cancelled commit-queue because the chromium side needs to be landed first. please review anyway
Comment 12 Michael Nordman 2010-04-06 11:38:18 PDT
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)?
Comment 13 Darin Fisher (:fishd, Google) 2010-04-06 22:28:41 PDT
Comment on attachment 52637 [details]
Patch

R- based on michaeln's comments.
Comment 14 jochen 2010-04-07 00:14:40 PDT
Created attachment 52709 [details]
Patch
Comment 15 jochen 2010-04-07 00:25:31 PDT
Created attachment 52710 [details]
Patch
Comment 16 Michael Nordman 2010-04-07 00:28:12 PDT
(In reply to comment #15)
> Created an attachment (id=52710) [details]
> Patch

LGTM (fwiw)
Comment 17 WebKit Commit Bot 2010-04-07 00:39:36 PDT
Comment on attachment 52710 [details]
Patch

Clearing flags on attachment: 52710

Committed r57197: <http://trac.webkit.org/changeset/57197>
Comment 18 WebKit Commit Bot 2010-04-07 00:39:42 PDT
All reviewed patches have been landed.  Closing bug.