Stub out initial NavigationController browser implementation

Stub out initial NavigationController browser implementation

This is the initial cut for the browser-side NavigationControllerDispatcherHost,
and all the various related support plumbing to hook up two initial messages.

BUG=285976
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224585

[alecflett, 2013-09-05 18:31:53]

I'm just starting to stub this stuff out - modeled a little on AppCache, a
little on IDB, and a little on workers.. this is mostly just a boilerplate to
get a storage context and dispatcher in place, along with some stub messages.. 

I wasn't sure how best to begin this - I mean 2 of the 4 messages aren't even
referenced (good news is they'll get compiled out) but I did hook up the new
dispatcher to the render_host's BrowserMessageFilter.

[michaeln, 2013-09-05 21:15:39]

Looks like a great start to me. I opened an umbrella bug, can you add the bug
number to this CL and your blink side change too: crbug/285976

Also lets cc kinuko and gbillock on all changes too, I've added them to the cc
list.

https://codereview.chromium.org/23615009/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/23615009/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:66: #include
"content/browser/navigation_controller/navigation_controller_context.h"
For the naming convention in chromium code, nav_controller & NavController would
make for less typing and ugly line wrapping. I'd vote to use the shortened form
for those reasons on class names, filenames, directory names, etc... wdyt?

[alecflett, 2013-09-05 22:24:56]

https://codereview.chromium.org/23615009/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/23615009/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:66: #include
"content/browser/navigation_controller/navigation_controller_context.h"
On 2013/09/05 21:15:39, michaeln wrote:
> For the naming convention in chromium code, nav_controller & NavController
would
> make for less typing and ugly line wrapping. I'd vote to use the shortened
form
> for those reasons on class names, filenames, directory names, etc... wdyt?

I agree with shortening it - I struggled with this quite a bit. Theres also a
non-zero chance that the worker side of things may get renamed to
"ServiceWorker" or something similarly worker-related. 

We can cross that bridge when we get to it but in the mean time, NavController
sounds good to me.

I'll also add an OWNERS in the next rev here.

[alecflett, 2013-09-06 04:39:21]

On 2013/09/05 22:24:56, alecflett wrote:
>
https://codereview.chromium.org/23615009/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/23615009/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_process_host_impl.cc:66: #include
> "content/browser/navigation_controller/navigation_controller_context.h"
> On 2013/09/05 21:15:39, michaeln wrote:
> > For the naming convention in chromium code, nav_controller & NavController
> would
> > make for less typing and ugly line wrapping. I'd vote to use the shortened
> form
> > for those reasons on class names, filenames, directory names, etc... wdyt?
> 
> I agree with shortening it - I struggled with this quite a bit. Theres also a
> non-zero chance that the worker side of things may get renamed to
> "ServiceWorker" or something similarly worker-related. 
> 
> We can cross that bridge when we get to it but in the mean time, NavController
> sounds good to me.
> 
> I'll also add an OWNERS in the next rev here.

Done. michaeln@ PTAL?

[michaeln, 2013-09-06 18:41:16]

BUG=285976

https://codereview.chromium.org/23615009/diff/7001/content/browser/nav_contro...
File content/browser/nav_controller/OWNERS (right):

https://codereview.chromium.org/23615009/diff/7001/content/browser/nav_contro...
content/browser/nav_controller/OWNERS:2: michaeln@chromium.org
add kinuko here too?

https://codereview.chromium.org/23615009/diff/7001/content/browser/nav_contro...
File content/browser/nav_controller/nav_controller_context.h (right):

https://codereview.chromium.org/23615009/diff/7001/content/browser/nav_contro...
content/browser/nav_controller/nav_controller_context.h:24: class
NavControllerContext : public base::RefCounted<NavControllerContext> {
Since this is constructed on the UI thread, really should be using a thread safe
refcount.

[alecflett, 2013-09-06 22:39:31]

ok, addressed remaining comments.

jam@ - mind a hopefully quick review? some new stuff in content/ and new ipc
messages.

[michaeln, 2013-09-06 22:51:23]

https://codereview.chromium.org/23615009/diff/18001/content/browser/nav_contr...
File content/browser/nav_controller/nav_controller_context.h (right):

https://codereview.chromium.org/23615009/diff/18001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_context.h:24: class
NavControllerContext : public base::RefCountedThreadSafe<NavControllerContext> {
line length

[michaeln, 2013-09-09 18:34:13]

lgtm

[michaeln, 2013-09-09 20:55:49]

https://codereview.chromium.org/23615009/diff/36001/content/common/nav_contro...
File content/common/nav_controller/nav_controller_messages.h (right):

https://codereview.chromium.org/23615009/diff/36001/content/common/nav_contro...
content/common/nav_controller/nav_controller_messages.h:14:
IPC_MESSAGE_CONTROL3(NavControllerHostMsg_RegisterController,
Since you're defining messages that require a 'peer_id' parameter in this CL, it
might make sense to also define the two messages to create/delete the 'peer'
itself (analogs of AppCacheHostMsg_RegisterHost and
AppCacheHostMsg_UnRegisterHost).

For lack of a better name referring to this as the NavControllerPeer seems ok?
NavControllerHostMsg_CreatePeer(int peer_id);
NavControllerHostMsg_DeletePeer(int peer_id);

In anticipation of adding some c++ classes on either end in later CLs...
class NavControllerPeer // browser process c++ class
-----
class WebNavControllerPeer // WebKitAPI defined interface
class WebNavControllerPeerImpl // client process c++ class that impls the
WebKitAPI interface

[alecflett, 2013-09-12 21:04:56]

jam@ - ping on IPC?

https://codereview.chromium.org/23615009/diff/36001/content/common/nav_contro...
File content/common/nav_controller/nav_controller_messages.h (right):

https://codereview.chromium.org/23615009/diff/36001/content/common/nav_contro...
content/common/nav_controller/nav_controller_messages.h:14:
IPC_MESSAGE_CONTROL3(NavControllerHostMsg_RegisterController,
On 2013/09/09 20:55:49, michaeln wrote:
> Since you're defining messages that require a 'peer_id' parameter in this CL,
it
> might make sense to also define the two messages to create/delete the 'peer'
> itself (analogs of AppCacheHostMsg_RegisterHost and
> AppCacheHostMsg_UnRegisterHost).
> 
> For lack of a better name referring to this as the NavControllerPeer seems ok?
> NavControllerHostMsg_CreatePeer(int peer_id);
> NavControllerHostMsg_DeletePeer(int peer_id);
> 
> In anticipation of adding some c++ classes on either end in later CLs...
> class NavControllerPeer // browser process c++ class
> -----
> class WebNavControllerPeer // WebKitAPI defined interface
> class WebNavControllerPeerImpl // client process c++ class that impls the
> WebKitAPI interface

The peer is just a temporary object that exists in the renderer during
registration - it is 'created' when RegisterController() is called, and likely
destroyed (working out details there) after the
NavControllerMsg_ControllerRegistered message comes through.

[michaeln, 2013-09-12 23:29:04]

> The peer is just a temporary object that exists in the renderer during
> registration - it is 'created' when RegisterController() is called, and likely
> destroyed (working out details there) after the
> NavControllerMsg_ControllerRegistered message comes through.

I was expecting the peer_id would identify the Document context and for that id
would be used for all interactions with the backend on behalf of the document.

[alecflett, 2013-09-13 17:48:12]

Talked with jam@, gave me a few comments, addressed them here.

also, on the blink side, we're calling 'Peer' 'Registry' which is
semi-consistent with shared worker concepts, at least blink-side.

[alecflett, 2013-09-13 21:12:38]

On 2013/09/12 23:29:04, michaeln wrote:
> > The peer is just a temporary object that exists in the renderer during
> > registration - it is 'created' when RegisterController() is called, and
likely
> > destroyed (working out details there) after the
> > NavControllerMsg_ControllerRegistered message comes through.
> 
> I was expecting the peer_id would identify the Document context and for that
id
> would be used for all interactions with the backend on behalf of the document.

yes, though at the moment it's just a one-shot interaction... there may be
MessagePorts or something down the line attached to the 'active' controller, but
this patch doesn't touch that - registration is independent of the actual
controller/worker currently in play means these concepts have different id's.

In particular:
1) an 'uncontrolled' document can call registerController() but there will be no
controller visible until the next reload, so the document context is irrelevant
2) a 'controlled' document may call registerController() to register a different
controller, which means the 'registered' controller is a different object than
the 'active' controller

So all I'm writing in *this* patch is the registration flow, which should be
independent of the 'active' controller stuff.

[alecflett, 2013-09-13 21:34:49]

Adding tsepez for _messages.h review.

[michaeln, 2013-09-13 21:46:34]

On 2013/09/13 21:12:38, alecflett wrote:
> On 2013/09/12 23:29:04, michaeln wrote:
> > > The peer is just a temporary object that exists in the renderer during
> > > registration - it is 'created' when RegisterController() is called, and
> likely
> > > destroyed (working out details there) after the
> > > NavControllerMsg_ControllerRegistered message comes through.
> > 
> > I was expecting the peer_id would identify the Document context and for that
> id
> > would be used for all interactions with the backend on behalf of the
document.
> 
> yes, though at the moment it's just a one-shot interaction... there may be
> MessagePorts or something down the line attached to the 'active' controller,
but
> this patch doesn't touch that - registration is independent of the actual
> controller/worker currently in play means these concepts have different id's.

messageports or unicorns or something ;)

> In particular:
> 1) an 'uncontrolled' document can call registerController() but there will be
no
> controller visible until the next reload, so the document context is
irrelevant
> 2) a 'controlled' document may call registerController() to register a
different
> controller, which means the 'registered' controller is a different object than
> the 'active' controller
> 
> So all I'm writing in *this* patch is the registration flow, which should be
> independent of the 'active' controller stuff.

Got it... so the 'id' in this case is really just a registration request id.

Since we're renaming things to EventWorker should we rename classes and files
now too?

[Tom Sepez, 2013-09-13 21:48:34]

https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
File content/browser/nav_controller/nav_controller_context.cc (right):

https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_context.cc:16: const
base::FilePath& path,
Who gets to specify this path? Hardcoded in browser?  Sent by renderer?  How do
we know that it is legal to write here ... a path with ../../.. in it would be
bad.

https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
File content/browser/nav_controller/nav_controller_dispatcher_host.cc (right):

https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_dispatcher_host.cc:42: }
Are there constraints on what the script_url can be relative to the site
instance that is requesting to add a controller?  If so, please put a comment
here about that so that when you later go to implement it, someone will know to
check for this carefully.  Thanks.

https://codereview.chromium.org/23615009/diff/44001/content/common/nav_contro...
File content/common/nav_controller_messages.h (right):

https://codereview.chromium.org/23615009/diff/44001/content/common/nav_contro...
content/common/nav_controller_messages.h:14:
IPC_MESSAGE_CONTROL3(NavControllerHostMsg_RegisterController,
nit: Generally, I'd split these into browser -> renderer and renderer -> browser
sections, with a comment indicating which way the messages are going for each
section (even though the HostMsg_ indicates this).

https://codereview.chromium.org/23615009/diff/44001/content/common/nav_contro...
content/common/nav_controller_messages.h:16: string16 /* scope */,
What is scope?  Can you give an example?  In particluar, if its some structured
list we'd prefer that it not be tunnelled through IPC using strings.  If not,
then ... nevermind.

https://codereview.chromium.org/23615009/diff/44001/content/common/nav_contro...
content/common/nav_controller_messages.h:19:
IPC_MESSAGE_CONTROL1(NavControllerMsg_ControllerReady, int32 /* registry_id */)
nit: maybe a comment about "Response to ..." in all places.

[alecflett, 2013-09-13 21:50:12]

> 
> Since we're renaming things to EventWorker should we rename classes and files
> now too?

It's not fully settled if it's EventWorker or ServiceWorker, should know soon.

I'll just do a single rename pass when it's decided... but we can still land
this in the meantime.

[michaeln, 2013-09-13 22:00:02]

https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
File content/browser/nav_controller/nav_controller_dispatcher_host.cc (right):

https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_dispatcher_host.cc:42: }
On 2013/09/13 21:48:34, Tom Sepez wrote:
> Are there constraints on what the script_url can be relative to the site
> instance that is requesting to add a controller?  If so, please put a comment
> here about that so that when you later go to implement it, someone will know
to
> check for this carefully.  Thanks.

Actually this is a good example of why it's useful to have some context about
the Document from which this request to register something came from. I think it
would be good to set that context up in the initial CLs. I wish i had the
opportunity to make that comment in the Blink CL.

[alecflett, 2013-09-13 22:07:05]

On 2013/09/13 22:00:02, michaeln wrote:
>
https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
> File content/browser/nav_controller/nav_controller_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
> content/browser/nav_controller/nav_controller_dispatcher_host.cc:42: }
> On 2013/09/13 21:48:34, Tom Sepez wrote:
> > Are there constraints on what the script_url can be relative to the site
> > instance that is requesting to add a controller?  If so, please put a
comment
> > here about that so that when you later go to implement it, someone will know
> to
> > check for this carefully.  Thanks.
> 
> Actually this is a good example of why it's useful to have some context about
> the Document from which this request to register something came from. I think
it
> would be good to set that context up in the initial CLs. I wish i had the
> opportunity to make that comment in the Blink CL.

I agree.. and feel free to add a comment to the (closed) blink CL still. You can
also of course add comments to my future blink CLs to make sure the right info
is making it across the wire.

[alecflett, 2013-09-13 22:15:13]

On 2013/09/13 22:00:02, michaeln wrote:
>
https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
> File content/browser/nav_controller/nav_controller_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/23615009/diff/44001/content/browser/nav_contr...
> content/browser/nav_controller/nav_controller_dispatcher_host.cc:42: }
> On 2013/09/13 21:48:34, Tom Sepez wrote:
> > Are there constraints on what the script_url can be relative to the site
> > instance that is requesting to add a controller?  If so, please put a
comment
> > here about that so that when you later go to implement it, someone will know
> to
> > check for this carefully.  Thanks.
> 
> Actually this is a good example of why it's useful to have some context about
> the Document from which this request to register something came from. I think
it
> would be good to set that context up in the initial CLs. I wish i had the
> opportunity to make that comment in the Blink CL.

Also: while I agree that having context about the document is important, the
security issue is not addressed by sending this (via additional messages, or
with additional data in the message) from the renderer to the browser by
NavController code. The navigationcontext or the dispatcher host needs to
determine the document another way - I'm sure that mechanism is available and
I'll wire it up if someone can show me where to get that information.

[alecflett, 2013-09-13 22:20:09]

new patch - hopefully addressing all of tsepez@'s concerns.

tsepez@ - regarding scope, it is a url "pattern" which right now is a relative
url, like "/foo/*" - so no it's definitely not an encoding of any other data
structure..

[Tom Sepez, 2013-09-13 22:22:06]

Nice. LGTM.

[jam, 2013-09-13 22:44:39]

since this patch is minimal, I don't understand what NavControllerContext will
be used for. More specifically, it's not clear to me why we need this class at
all?

i.e. right now it has a path. it's not public, and i can't see who'll use it.
The other data member is something that can be taken out of a StoragePartition.
Whoever needs that proxy (the new dispathcerhost?) can just be passed a pointer
to that instead?

https://codereview.chromium.org/23615009/diff/27001/content/common/nav_contro...
File content/common/nav_controller/nav_controller_messages.h (right):

https://codereview.chromium.org/23615009/diff/27001/content/common/nav_contro...
content/common/nav_controller/nav_controller_messages.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
nit: ipc files have owners that are only security folks. so this file can just
be in content/common (I'm assuming you put it in a subdir for owners reasons)

https://codereview.chromium.org/23615009/diff/67001/content/browser/nav_contr...
File content/browser/nav_controller/nav_controller_context.h (right):

https://codereview.chromium.org/23615009/diff/67001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_context.h:8: #include
"base/files/file_path.h"
nit: no need

https://codereview.chromium.org/23615009/diff/67001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_context.h:12: // when this needs
to be accessed from chrome/
nit: remove this for now. if you need that in the future, it'll happen

https://codereview.chromium.org/23615009/diff/67001/content/browser/nav_contr...
content/browser/nav_controller/nav_controller_context.h:30: : public
base::RefCountedThreadSafe<NavControllerContext> {
why is this refcounted?

[alecflett, 2013-09-13 23:02:39]

On 2013/09/13 22:44:39, jam wrote:
> since this patch is minimal, I don't understand what NavControllerContext will
> be used for. More specifically, it's not clear to me why we need this class at
> all?
> 
> i.e. right now it has a path. it's not public, and i can't see who'll use it.
> The other data member is something that can be taken out of a
StoragePartition.
> Whoever needs that proxy (the new dispathcerhost?) can just be passed a
pointer
> to that instead?

I thought I clarified that in the patch, but I can clarify further. The comments
say:

// This class manages metadata associated with all navigation
// controllers, including:
// - persistent storage of pattern -> controller scripts

this basically means "[will] have a leveldb instance mapping patterns -> script
source files" - i.e. the leveldb actually contains javascript code that is
executed by the event worker. Access to the quota manager here is because we're
storing data on a per-domain basis, and we want to account for that.

// - initialization and initial installation of controllers
// - dispatching of non-fetch events to controllers

These two cover the various states that a controller can be in during
installation or upgrade - for instance when a new controller is registered,
there maybe old event workers running - and in fact the old controllers will
continue to be "active" until all pages they control are closed. (see some
details here:
https://github.com/slightlyoff/EventWorker/blob/master/explainer.md#event-wor...)

I'd really rather not pollute the StoragePartition *or* the
NavControllerDispatcherHost with any of these details.

If anything I expect this class to grow and sprout a number of additional
classes to manage these states. This particular CL is just stubbing that out.

I suppose it doesn't need to be refcounted though, I'll fix that.

[jam, 2013-09-18 01:43:15]

On Fri, Sep 13, 2013 at 4:02 PM, <alecflett@chromium.org> wrote:

> On 2013/09/13 22:44:39, jam wrote:
>
>> since this patch is minimal, I don't understand what NavControllerContext
>> will
>> be used for. More specifically, it's not clear to me why we need this
>> class at
>> all?
>>
>
>  i.e. right now it has a path. it's not public, and i can't see who'll use
>> it.
>> The other data member is something that can be taken out of a
>>
> StoragePartition.
>
>> Whoever needs that proxy (the new dispathcerhost?) can just be passed a
>>
> pointer
>
>> to that instead?
>>
>
> I thought I clarified that in the patch, but I can clarify further. The
> comments
> say:
>
> // This class manages metadata associated with all navigation
> // controllers, including:
> // - persistent storage of pattern -> controller scripts
>
> this basically means "[will] have a leveldb instance mapping patterns ->
> script
> source files" - i.e. the leveldb actually contains javascript code that is
> executed by the event worker. Access to the quota manager here is because
> we're
> storing data on a per-domain basis, and we want to account for that.
>
> // - initialization and initial installation of controllers
> // - dispatching of non-fetch events to controllers
>
> These two cover the various states that a controller can be in during
> installation or upgrade - for instance when a new controller is registered,
> there maybe old event workers running - and in fact the old controllers
> will
> continue to be "active" until all pages they control are closed. (see some
> details here:
> https://github.com/**slightlyoff/EventWorker/blob/**
>
master/explainer.md#event-**worker-installation--upgrade<https://github.com/slightlyoff/EventWorker/blob/master/explainer.md#event-worker-installation--upgrade>
> )
>
> I'd really rather not pollute the StoragePartition *or* the
> NavControllerDispatcherHost with any of these details.
>
> If anything I expect this class to grow and sprout a number of additional
> classes to manage these states. This particular CL is just stubbing that
> out.
>
> I suppose it doesn't need to be refcounted though, I'll fix that.
>

I see, ok lgtm if this class will expand. but before checkin, please remove
the refcounting (and the comment at the top of nav_controller_context.h).


>
>
https://codereview.chromium.**org/23615009/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[alecflett, 2013-09-19 22:17:02]

So I ended up leaving the refcounting in there, because it has to be held by
both the storage_partition and the dispatcher_host. 
This follows the model of the other storage subsystems. (and I think you and I
have clarified the role of the context in person, so hopefully that's ok)

[commit-bot: I haz the power, 2013-09-19 22:17:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/23615009/13001

[commit-bot: I haz the power, 2013-09-19 23:02:29]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-19 23:03:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/23615009/94001

[commit-bot: I haz the power, 2013-09-20 02:26:44]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-20 19:10:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/23615009/64001

[commit-bot: I haz the power, 2013-09-20 22:20:28]

Retried try job too often on win_rel for step(s) browser_tests,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-20 22:52:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/23615009/64001

[commit-bot: I haz the power, 2013-09-21 00:59:03]

Retried try job too often on win_rel for step(s) browser_tests,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-21 04:43:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/23615009/64001

[commit-bot: I haz the power, 2013-09-21 16:12:52]

Change committed as 224585