AppCachePlumbing

Adds new messages and widens some existing messages between the renderer and browser processes to support an implementation of the HTML5AppCache spec with most of the logic running in the browser process.  The gist of most of the changes are to indicate which frame each resource request is coming from, and to indicate which appcache each response was retrieved from (if any).See https://docs.google.com/a/google.com/Doc?docid=agv6ghfsqr_15f749cgt3&hl=en

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13258

[darin (slow to review), 2009-02-18 21:43:46]

This all looks very reasonable.

http://codereview.chromium.org/9712/diff/1201/1212
File chrome/browser/resource_message_filter.cc (right):

http://codereview.chromium.org/9712/diff/1201/1212#newcode124
Line 124: // TODO(michaeln): plumb to profile->app_cache_manager
we may actually be better off placing the app_cache_manager on the
ChromeURLRequestContext since that object is available from the IO thread
whereas the Profile is not.

http://codereview.chromium.org/9712/diff/1201/1225
File chrome/common/app_cache_common.h (right):

http://codereview.chromium.org/9712/diff/1201/1225#newcode6
Line 6: #define CHROME_COMMON_APP_CACHE_COMMON_H__
nit: google style changed so that these should end with "_" instead of "__"

http://codereview.chromium.org/9712/diff/1201/1225#newcode21
Line 21: const GlobalAppCacheContextId kGlobalNoAppCacheContextId(0,
kNoAppCacheId);
we generally try to avoid static initializers.  can you do this using a POD
type?  maybe by defining a structure instead?

struct GlobalAppCacheContextId {
  int foo;  // helps to give these good names too
  int32 bar;
};

const GlobalAppCacheContextId kGlobalNoAppCacheContextId = { 0, kNoAppCacheId };

http://codereview.chromium.org/9712/diff/1201/1215
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/9712/diff/1201/1215#newcode468
Line 468:
//----------------------------------------------------------------------------
nit: this divider is a bit out-of-place.  we only presently use such a divider
to separate the two IPC_BEGIN_MESSAGES blocks.

http://codereview.chromium.org/9712/diff/1201/1215#newcode476
Line 476: int32 /* context_id */,
please define these IDs in the comments above.

http://codereview.chromium.org/9712/diff/1201/1215#newcode1106
Line 1106: // These are messages sent from the renderer/worker to the browser
process.
would be good to define the various IDs

http://codereview.chromium.org/9712/diff/1201/1224
File chrome/renderer/app_cache_context.cc (right):

http://codereview.chromium.org/9712/diff/1201/1224#newcode15
Line 15: IDMap<AppCacheContext> AppCacheContext::all_contexts;
this should be OK.  i assume that webworkers will have to post a task back to
the main webkit thread in order to interact with the appcache APIs.  we don't
want to have to support getting called on any thread.

http://codereview.chromium.org/9712/diff/1201/1208
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/9712/diff/1201/1208#newcode373
Line 373: virtual void SelectAppCacheWithManifest(const GURL& manifest_url) = 0;
are there any rules about when it is OK to call these methods?

http://codereview.chromium.org/9712/diff/1201/1208#newcode377
Line 377: virtual AppCacheContext *app_cache_context() const = 0;
nit: virtual functions should not use unix_hacker naming since the function may
not be cheap to call.

nit: classes exposed by webkit/glue typically have the Web prefix.  perhaps this
should be WebAppCacheContext.

http://codereview.chromium.org/9712/diff/1201/1209
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/9712/diff/1201/1209#newcode35
Line 35: #include "chrome/renderer/app_cache_context.h"
webkit/glue cannot depend on chrome/renderer or anything from under chrome/ for
that matter.  we need to create abstractions for things that we wish for the
embedder (i.e., chrome) to provide.

http://codereview.chromium.org/9712/diff/1201/1205
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/9712/diff/1201/1205#newcode280
Line 280: void SelectAppCacheWithManifestForFrame(WebCore::Frame* frame,
this function doesn't appear to be referenced.

[michaeln, 2009-03-27 00:18:01]

Finally getting back to this stuff. New snapshot uploaded.

I have two webcore files (in platform/chromium) that need to go upstream that
this CL has compile time dependencies on.
- WebCore\platform\network\chromium\ResourceRequest.h
- WebCore\platform\network\chromium\ResourceResponse.h
They contain trivial changes that aren't out for review yet.

http://codereview.chromium.org/9712/diff/1201/1212
File chrome/browser/resource_message_filter.cc (right):

http://codereview.chromium.org/9712/diff/1201/1212#newcode124
Line 124: // TODO(michaeln): plumb to profile->app_cache_manager
Sounds good, will do

http://codereview.chromium.org/9712/diff/1201/1225
File chrome/common/app_cache_common.h (right):

http://codereview.chromium.org/9712/diff/1201/1225#newcode6
Line 6: #define CHROME_COMMON_APP_CACHE_COMMON_H__
On 2009/02/18 21:43:46, darin wrote:
> nit: google style changed so that these should end with "_" instead of "__"

Done.

http://codereview.chromium.org/9712/diff/1201/1225#newcode21
Line 21: const GlobalAppCacheContextId kGlobalNoAppCacheContextId(0,
kNoAppCacheId);
On 2009/02/18 21:43:46, darin wrote:
> we generally try to avoid static initializers.  can you do this using a POD
> type?  maybe by defining a structure instead?
> 
> struct GlobalAppCacheContextId {
>   int foo;  // helps to give these good names too
>   int32 bar;
> };
> 
> const GlobalAppCacheContextId kGlobalNoAppCacheContextId = { 0, kNoAppCacheId
};

Done.

http://codereview.chromium.org/9712/diff/1201/1215
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/9712/diff/1201/1215#newcode468
Line 468:
//----------------------------------------------------------------------------
On 2009/02/18 21:43:46, darin wrote:
> nit: this divider is a bit out-of-place.  we only presently use such a divider
> to separate the two IPC_BEGIN_MESSAGES blocks.

Done.

http://codereview.chromium.org/9712/diff/1201/1224
File chrome/renderer/app_cache_context.cc (right):

http://codereview.chromium.org/9712/diff/1201/1224#newcode15
Line 15: IDMap<AppCacheContext> AppCacheContext::all_contexts;
Probably so. I'll coordinate with john about this when that time comes.

http://codereview.chromium.org/9712/diff/1201/1208
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/9712/diff/1201/1208#newcode373
Line 373: virtual void SelectAppCacheWithManifest(const GURL& manifest_url) = 0;
Yes, these should be called shortly after a new document is committed to a
frame, prior to the initiation of any sub resource loads.

Actually the WithoutManifest variant is called automatically by WebFrameImpl at
commit time... so perhaps I should just remove that one from the public
interface.

The WithManifest variant needs to be called at HTML parsing time when coming
across the <html manifest=xxx> tag. The code to do that is not in this CL. I'll
need to upstream some webcore mods to have that happen.

http://codereview.chromium.org/9712/diff/1201/1208#newcode377
Line 377: virtual AppCacheContext *app_cache_context() const = 0;
done and done

http://codereview.chromium.org/9712/diff/1201/1209
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/9712/diff/1201/1209#newcode35
Line 35: #include "chrome/renderer/app_cache_context.h"
Done

http://codereview.chromium.org/9712/diff/1201/1205
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/9712/diff/1201/1205#newcode280
Line 280: void SelectAppCacheWithManifestForFrame(WebCore::Frame* frame,
See earlier comment about <html manifest=xxx> parse time.

[jam, 2009-03-31 01:20:03]

some initial comments (also totally agree about splitting up the files)

http://codereview.chromium.org/9712/diff/5201/5224
File chrome/common/app_cache_context_impl.h (right):

http://codereview.chromium.org/9712/diff/5201/5224#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
should be 2009

http://codereview.chromium.org/9712/diff/5201/5224#newcode55
Line 55: class AppCacheMessageDispatcher {
why message, this should really be AppCacheDispatcher, to match
ResourceDispatcher.

http://codereview.chromium.org/9712/diff/5201/5224#newcode70
Line 70: class AppCacheMessageFilter {
the naming is off here.  I agree that ResourceDispatcherHost should really be
something like ResourceMessageFilter, and the class now known as
ResourceMessageFilter should be something like IOThreadFilter.  but given where
we are, this should be called AppCacheDispatcherHost?

http://codereview.chromium.org/9712/diff/5201/5224#newcode73
Line 73: : is_initialized_(false), child_process_id_(-1), sender_(NULL) {}
nit need two more spaces

http://codereview.chromium.org/9712/diff/5201/5211
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/9712/diff/5201/5211#newcode1202
Line 1202: IPC_MESSAGE_CONTROL5(AppCacheMsg_SelectAppCacheWithManifest,
can't this be combined with the above message, with an empty manifest indicating
no manifest?

http://codereview.chromium.org/9712/diff/5201/5212
File chrome/common/resource_dispatcher.h (right):

http://codereview.chromium.org/9712/diff/5201/5212#newcode125
Line 125: AppCacheMessageDispatcher app_cache_message_dispatcher_;
I think it's better if you keep ResourceDispatcher from AppCacheDispatcher
separate since they're not related (and also to help testing).  The code in
child_thread that currently calls ResourceDispatcher::OnMessageReceived should
do the same to AppCacheDispatcher...

[michaeln, 2009-03-31 21:24:28]

Still haven't split the .cc file into three, but will do so prior to submitting.

Also still have to get my WebKit patch landed which this CL has a compile time
dependency on.

http://codereview.chromium.org/9712/diff/5201/5224
File chrome/common/app_cache_context_impl.h (right):

http://codereview.chromium.org/9712/diff/5201/5224#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Done

http://codereview.chromium.org/9712/diff/5201/5224#newcode55
Line 55: class AppCacheMessageDispatcher {
On 2009/03/31 01:20:03, John Abd-El-Malek wrote:
> why message, this should really be AppCacheDispatcher, to match
> ResourceDispatcher.

Done.

http://codereview.chromium.org/9712/diff/5201/5224#newcode70
Line 70: class AppCacheMessageFilter {
On 2009/03/31 01:20:03, John Abd-El-Malek wrote:
> the naming is off here.  I agree that ResourceDispatcherHost should really be
> something like ResourceMessageFilter, and the class now known as
> ResourceMessageFilter should be something like IOThreadFilter.  but given
where
> we are, this should be called AppCacheDispatcherHost?

Done.

http://codereview.chromium.org/9712/diff/5201/5224#newcode73
Line 73: : is_initialized_(false), child_process_id_(-1), sender_(NULL) {}
On 2009/03/31 01:20:03, John Abd-El-Malek wrote:
> nit need two more spaces

Done.

http://codereview.chromium.org/9712/diff/5201/5211
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/9712/diff/5201/5211#newcode1202
Line 1202: IPC_MESSAGE_CONTROL5(AppCacheMsg_SelectAppCacheWithManifest,
On 2009/03/31 01:20:03, John Abd-El-Malek wrote:
> can't this be combined with the above message, with an empty manifest
indicating
> no manifest?

Of course they can be combined... done

Why do you prefer to combine them?
* so we don't exhaust the msgID space?
* so we have fewer cases to look thru when dispatching?

I've also combined the ContextCreated messages and switched to using 'int' over
'int32' like we talked about offline.

http://codereview.chromium.org/9712/diff/5201/5212
File chrome/common/resource_dispatcher.h (right):

http://codereview.chromium.org/9712/diff/5201/5212#newcode125
Line 125: AppCacheMessageDispatcher app_cache_message_dispatcher_;
On 2009/03/31 01:20:03, John Abd-El-Malek wrote:
> I think it's better if you keep ResourceDispatcher from AppCacheDispatcher
> separate since they're not related (and also to help testing).  The code in
> child_thread that currently calls ResourceDispatcher::OnMessageReceived should
> do the same to AppCacheDispatcher...

Done (nice suggestion)

[jam, 2009-04-02 23:31:48]

http://codereview.chromium.org/9712/diff/5201/5211
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/9712/diff/5201/5211#newcode1202
Line 1202: IPC_MESSAGE_CONTROL5(AppCacheMsg_SelectAppCacheWithManifest,
On 2009/03/31 21:24:29, michaeln wrote:
> On 2009/03/31 01:20:03, John Abd-El-Malek wrote:
> > can't this be combined with the above message, with an empty manifest
> indicating
> > no manifest?
> 
> Of course they can be combined... done
> 
> Why do you prefer to combine them?
> * so we don't exhaust the msgID space?
that's not really a problem, we have 2^12 for now..

> * so we have fewer cases to look thru when dispatching?

it just makes the IPC code use less codepaths in sending/dispatching the
messages.

> 
> I've also combined the ContextCreated messages and switched to using 'int'
over
> 'int32' like we talked about offline.
>

http://codereview.chromium.org/9712/diff/7201/7223
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/9712/diff/7201/7223#newcode194
Line 194: DCHECK(msg_is_ok);  // should not have been squashed by delegates
nit: I think this dcheck is too paranoid (i.e. we don't check that they changed
message too)...

http://codereview.chromium.org/9712/diff/7201/7226
File chrome/common/app_cache_context_impl.cc (right):

http://codereview.chromium.org/9712/diff/7201/7226#newcode12
Line 12: return new AppCacheContextImpl(ChildThread::current());
I think using RenderThread::current() is fine for now since it's only being used
in the renderer process for now.

http://codereview.chromium.org/9712/diff/7201/7224
File chrome/common/app_cache_context_impl.h (right):

http://codereview.chromium.org/9712/diff/7201/7224#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
this should be 2009 instead of 2006-2009 since it's a new file (same for the
other new files)

http://codereview.chromium.org/9712/diff/7201/7224#newcode44
Line 44: bool is_initialized_;
I think this flag (and the one in AppCacheDispatcher) aren't necessary.  We have
this pattern in a lot of different parts of the code, and we usually expect the
caller to null their reference to an object whose Initialize() function failed. 
It's simpler than having the class check in all its functions that it has been
initialized.

http://codereview.chromium.org/9712/diff/7201/7224#newcode94
Line 94: int child_process_id_;
this isn't used anywhere

http://codereview.chromium.org/9712/diff/7201/7229
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/9712/diff/7201/7229#newcode38
Line 38: static ChildThread* current();
I think this should remain protected as before, and the one caller should use
RenderThread::current() until we need this in other processes

http://codereview.chromium.org/9712/diff/7201/7229#newcode87
Line 87: scoped_ptr<AppCacheDispatcher> app_cache_dispatcher_;
ChildThread is used for all child processes, so it should only contain objects
that they all need.  Since AppCache is definitely not needed in the plugin
process, and not in the worker process for now, can you move this code to
RenderThread?

http://codereview.chromium.org/9712/diff/7201/7210
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/9712/diff/7201/7210#newcode278
Line 278: int32 app_cache_context_id;
this should be an int to match the other places

http://codereview.chromium.org/9712/diff/7201/7227
File chrome/renderer/media/data_source_impl.cc (right):

http://codereview.chromium.org/9712/diff/7201/7227#newcode219
Line 219: // For now don't service media resource requests from the appcache.
I'm just curious, can't this be done right now as per the comment or is that not
enough?

http://codereview.chromium.org/9712/diff/7201/7219
File webkit/glue/webappcachecontext.h (right):

http://codereview.chromium.org/9712/diff/7201/7219#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009

[michaeln, 2009-04-03 23:07:27]

New snapshot uploaded.

* Split the files like we talked about.

* I found a few more places to touch after doing a full build, mostly around
various test harnesses.

http://codereview.chromium.org/9712/diff/7201/7223
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/9712/diff/7201/7223#newcode194
Line 194: DCHECK(msg_is_ok);  // should not have been squashed by delegates
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> nit: I think this dcheck is too paranoid (i.e. we don't check that they
changed
> message too)... 

Done.

http://codereview.chromium.org/9712/diff/7201/7226
File chrome/common/app_cache_context_impl.cc (right):

http://codereview.chromium.org/9712/diff/7201/7226#newcode12
Line 12: return new AppCacheContextImpl(ChildThread::current());
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> I think using RenderThread::current() is fine for now since it's only being
used
> in the renderer process for now.

Done.

It might be nice for formalize the use of message dispatching delegates in the
ChildThread at some point. We now have two uses of that pattern and may well get
more.

http://codereview.chromium.org/9712/diff/7201/7224
File chrome/common/app_cache_context_impl.h (right):

http://codereview.chromium.org/9712/diff/7201/7224#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> this should be 2009 instead of 2006-2009 since it's a new file (same for the
> other new files)

Done.

http://codereview.chromium.org/9712/diff/7201/7224#newcode44
Line 44: bool is_initialized_;
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> I think this flag (and the one in AppCacheDispatcher) aren't necessary.  We
have
> this pattern in a lot of different parts of the code, and we usually expect
the
> caller to null their reference to an object whose Initialize() function
failed. 
> It's simpler than having the class check in all its functions that it has been
> initialized.

Done.

I'm still DCHECK'ing my assumptions (which was 90% of what this was used for),
but don't have an is_initialized flag for that purpose anymore.

http://codereview.chromium.org/9712/diff/7201/7224#newcode94
Line 94: int child_process_id_;
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> this isn't used anywhere

I've removed it for now since this class doesn't actually do anything yet.

http://codereview.chromium.org/9712/diff/7201/7229
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/9712/diff/7201/7229#newcode38
Line 38: static ChildThread* current();
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> I think this should remain protected as before, and the one caller should use
> RenderThread::current() until we need this in other processes

Done.

http://codereview.chromium.org/9712/diff/7201/7229#newcode87
Line 87: scoped_ptr<AppCacheDispatcher> app_cache_dispatcher_;
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> ChildThread is used for all child processes, so it should only contain objects
> that they all need.  Since AppCache is definitely not needed in the plugin
> process, and not in the worker process for now, can you move this code to
> RenderThread?

Done.

http://codereview.chromium.org/9712/diff/7201/7210
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/9712/diff/7201/7210#newcode278
Line 278: int32 app_cache_context_id;
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> this should be an int to match the other places

Yes, thank you

http://codereview.chromium.org/9712/diff/7201/7227
File chrome/renderer/media/data_source_impl.cc (right):

http://codereview.chromium.org/9712/diff/7201/7227#newcode219
Line 219: // For now don't service media resource requests from the appcache.
On 2009/04/02 23:31:48, John Abd-El-Malek wrote:
> I'm just curious, can't this be done right now as per the comment or is that
not
> enough?

There are some special requirements for 'media' resource loading. There's a
custom URLRequestContext and a separate HTTP DiskCache in the picture for media
requests that conspire to send a FileHandle over the ipc channel instead of
actually moving the bytes.

I just don't want to provide for this interface quite yet. There's plenty of
other stuff todo before that.

[jam, 2009-04-04 00:07:36]

lgtm

[darin (slow to review), 2009-04-13 18:37:54]

sorry for not commenting when this change was initially in review.  can you
please fix up the style?

http://codereview.chromium.org/9712/diff/11274/11284
File webkit/glue/webappcachecontext.h (right):

http://codereview.chromium.org/9712/diff/11274/11284#newcode52
Line 52: virtual int context_id() = 0;
nit: virtual methods should not be named unix_hacker style since you cannot by
definition be cheap to call.

http://codereview.chromium.org/9712/diff/11274/11284#newcode57
Line 57: virtual int64 app_cache_id() = 0;
ditto

[darin (slow to review), 2009-04-13 18:39:00]

http://codereview.chromium.org/9712/diff/11274/11279
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/9712/diff/11274/11279#newcode384
Line 384: // Initiates app cache selection for the context with the resource
currently
nit: seems bad to wedge these app cache specific methods in the middle of some
methods that have to do with printing.

[michaeln, 2009-04-15 06:18:29]

just now saw your comments... (this one was out of sight and out of mind by now)

yes, i'll make those changes, i hadn't realized i was splitting up a related
block of methods in webframe.h