Bug 36882 - [chromium] move createApplicationCacheHost from WebKitClient to WebFrameClient
: [chromium] move createApplicationCacheHost from WebKitClient to WebFrameClient
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-03-31 06:29 PST by
Modified: 2010-04-07 00:39 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-31 06:29:25 PST
[chromium] traverse from WebApplicationCacheHost to its associated WebDataSource and ultimately to its containing WebFrame
------- Comment #1 From 2010-03-31 06:30:24 PST -------
Created an attachment (id=52155) [details]
Patch
------- Comment #2 From 2010-03-31 06:38:21 PST -------
chromium side is in http://codereview.chromium.org/1600002/show
------- Comment #3 From 2010-03-31 07:58:00 PST -------
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 From 2010-03-31 09:14:13 PST -------
(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 From 2010-03-31 12:25:36 PST -------
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 From 2010-03-31 14:34:08 PST -------
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 From 2010-03-31 14:42:31 PST -------
(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 From 2010-03-31 14:56:26 PST -------
> 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 From 2010-04-06 07:49:15 PST -------
Created an attachment (id=52637) [details]
Patch
------- Comment #10 From 2010-04-06 07:52:32 PST -------
Please also review http://codereview.chromium.org/1627003 which prepares chromium for this patch.
------- Comment #11 From 2010-04-06 08:29:24 PST -------
(From update of attachment 52637 [details])
I've cancelled commit-queue because the chromium side needs to be landed first. please review anyway
------- Comment #12 From 2010-04-06 11:38:18 PST -------
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 From 2010-04-06 22:28:41 PST -------
(From update of attachment 52637 [details])
R- based on michaeln's comments.
------- Comment #14 From 2010-04-07 00:14:40 PST -------
Created an attachment (id=52709) [details]
Patch
------- Comment #15 From 2010-04-07 00:25:31 PST -------
Created an attachment (id=52710) [details]
Patch
------- Comment #16 From 2010-04-07 00:28:12 PST -------
(In reply to comment #15)
> Created an attachment (id=52710) [details] [details]
> Patch

LGTM (fwiw)
------- Comment #17 From 2010-04-07 00:39:36 PST -------
(From update of attachment 52710 [details])
Clearing flags on attachment: 52710

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