Update view_manager and window_manager to make use of content handling.

Update view_manager and window_manager to make use of content handling.

This required ripping out the old concept of app navigation,
since it is no longer compatible with how apps get connected 
to.

BUG=

Committed: https://crrev.com/d098906547cc70f8626709ae1243be7f8c1e0e22
Cr-Commit-Position: refs/heads/master@{#293452}

[Aaron Boodman, 2014-08-29 02:16:21]

aa@chromium.org changed reviewers:
+ darin@chromium.org, sky@chromium.org

[Aaron Boodman, 2014-08-29 02:16:21]

sky: please review changes to viewmanager and examples/window_manager.

darin: everything else.

hans: I didn't actually end up looking into teardown in this CL. I think that it
might work as-is, but there's a bug in window manager where you can't actually
close windows ... so it's impossible to know whether teardown of apps is
working. And fixing that bug also seemed like a good separate change.

This change doesn't convert media_viewer, I figured that made sense as a
separate change.

[Aaron Boodman, 2014-08-29 02:20:48]

Patchset #1 (id:1) has been deleted

[Aaron Boodman, 2014-08-29 02:20:54]

Patchset #1 (id:20001) has been deleted

[Aaron Boodman, 2014-08-29 02:20:59]

Patchset #1 (id:40001) has been deleted

[darin (slow to review), 2014-08-29 05:59:43]

https://codereview.chromium.org/514063003/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/service_provider_impl.h (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/service_provider_impl.h:21:
ServiceProviderImpl(ApplicationConnection* application_connection);
nit: explicit

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
File mojo/services/html_viewer/html_document_view.h (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
mojo/services/html_viewer/html_document_view.h:34: // application (e.g., the
application that called
do you mean the application that called ConnectToApplication instead?

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
mojo/services/html_viewer/html_document_view.h:40: // |shell_connection| is the
original ServiceProvider that was passed to
nit: ServiceProvider -> ApplicationConnection

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
mojo/services/html_viewer/html_document_view.h:47: scoped_ptr<ServiceProvider>
imported_from_connector,
in the .cc file you use the variable names imported_services and
exported_services instead. not sure if you want to be consistent.

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
mojo/services/html_viewer/html_viewer.cc:68:
content_handler_factory_.set_context(connection);
I'm a bit confused by this code that keeps changing the context associated with
the content_handler_factory_. If ConfigureIncomingConnection is called more than
once but the ContentHandlerImpl is instantiated in reverse order, then wouldn't
we have a bug?

Note: This is probably a good reason why ApplicationConnection should not have
the Shell functionality of ConnectToApplication. That functionality does not
vary from ApplicationConnection instance to ApplicationConnection instance.
Rather, it is just a function of the Shell. It is the Shell that you should make
be a context parameter of the content_handler_factory_. Then it would be
something set once rather than reset like this. The |set_context| method
probably shouldn't exist as it creates dangerous situations.

[Aaron Boodman, 2014-08-29 07:45:11]

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/html_viewe...
mojo/services/html_viewer/html_viewer.cc:68:
content_handler_factory_.set_context(connection);
On 2014/08/29 05:59:42, darin wrote:
> I'm a bit confused by this code that keeps changing the context associated
with
> the content_handler_factory_. If ConfigureIncomingConnection is called more
than
> once but the ContentHandlerImpl is instantiated in reverse order, then
wouldn't
> we have a bug?

Good point.

> Note: This is probably a good reason why ApplicationConnection should not have
> the Shell functionality of ConnectToApplication. That functionality does not
> vary from ApplicationConnection instance to ApplicationConnection instance.
> Rather, it is just a function of the Shell. It is the Shell that you should
make
> be a context parameter of the content_handler_factory_.

I agree.

The main reason I was trying to get ApplicationConnection* into
ContentHandlerImpl was actually not so much for HTMLDocumentView, but so that I
could pass it into ServiceProviderImpl. This is because SPI passes it to
ServiceConnector, who passes it to InterfaceFactory::Create(), where it finally
makes it way to ViewManagerClient, who uses it to get to WindowManager.

I was trying to figure out a temporary way to meet that expectation, then go
back and rethink how this should actually work in a later change.

===

Stepping back now though...

I really like your idea of just using Shell to make outbound "global"
connections, rather than ApplicationConnection. There are so many ways to make
outbound connections, it seems like reducing that would be good in general.

What if I remove ApplicationConnection* from InterfaceFactory::Create()
entirely. There are only two implementations that use it
(ViewManagerClientFactory and NetworkApplicationLoader), but those can be
reworked so that the requirements are passed into the constructors instead.

I was also wondering if maybe ApplicationConnection shouldn't just go away. It
seems to overlap in functionality with ServiceProviderImpl... or am I
misunderstanding?

[sky, 2014-09-02 15:47:14]

sky@chromium.org changed reviewers:
+ ben@chromium.org

[sky, 2014-09-02 15:47:16]

+ben now that he is back. Ben, the interesting thing is the change to the
viewmanager that won't attempt to reuse an existing connection when asking to
connect to the same url twice.

https://codereview.chromium.org/514063003/diff/60001/mojo/examples/window_man...
File mojo/examples/window_manager/debug_panel.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/debug_panel.cc:120: next_color_++;
You removed next_color_ from the initializer. Should it be removed entirely?

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (left):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:483:
connection_with_view_as_root->RemoveRoot(view_id);
You should be able to get rid of AddRoot now.
Also, I believe you can

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:467: if (!view &&
access_policy_->CanEmbed(view)) {
Shouldn't this be:

if (!view || !...CanEmbed(view)) ?

[Aaron Boodman, 2014-09-02 16:22:19]

On 2014/09/02 15:47:16, sky wrote:
> +ben now that he is back. Ben, the interesting thing is the change to the
> viewmanager that won't attempt to reuse an existing connection when asking to
> connect to the same url twice.

It does actually, most of the time, it just happens lower in the stack ... in
ApplicationManager.

[Aaron Boodman, 2014-09-02 19:10:44]

https://codereview.chromium.org/514063003/diff/60001/mojo/examples/window_man...
File mojo/examples/window_manager/debug_panel.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/examples/window_man...
mojo/examples/window_manager/debug_panel.cc:120: next_color_++;
On 2014/09/02 15:47:15, sky wrote:
> You removed next_color_ from the initializer. Should it be removed entirely?

ah yes, thanks.

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (left):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:483:
connection_with_view_as_root->RemoveRoot(view_id);
On 2014/09/02 15:47:16, sky wrote:
> You should be able to get rid of AddRoot now.
> Also, I believe you can 

Done.

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:467: if (!view &&
access_policy_->CanEmbed(view)) {
On 2014/09/02 15:47:15, sky wrote:
> Shouldn't this be:
> 
> if (!view || !...CanEmbed(view)) ?

Done.

[sky, 2014-09-02 19:22:11]

LGTM

[Ben Goodger (Google), 2014-09-02 20:50:14]

lgtm

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/514063003/diff/60001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:472: // Only allow a
node to be the root for one connection.
I think this is a good simplification. I will ponder the implications on access
policy. We had been using the connection to track view visibility. I think it
was imperfect anyway since different embed chains would result in unique
connections. I think at the very least what you are doing here makes it more
consistent.

[Aaron Boodman, 2014-09-03 00:08:19]

Patchset #3 (id:100001) has been deleted

[Aaron Boodman, 2014-09-03 00:12:15]

Darin PTAL 

Here is a different approach to connecting to WindowManager from inside
ViewManager which relies on Shell*, not ApplicationConnection*.

[darin (slow to review), 2014-09-04 21:33:51]

LGTM

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
File mojo/examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
mojo/examples/png_viewer/png_viewer.cc:201: new PNGView(response.Pass(),
exported_services, remote.Pass(), shell_);
nit: add a comment here about memory management?

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
mojo/examples/png_viewer/png_viewer.cc:216: content_handler_factory_.reset(
food for thought...

it seems like it'd be possible to have a stock implementation of ContentHandler
and InterfaceFactory such that one only needs to supply a function that should
be called in response to OnConnect. this function would get the parameters of
the PNGView's constructor perhaps. maybe something to do in a follow-up CL.

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/wm_flow/a...
File mojo/examples/wm_flow/app/app.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/wm_flow/a...
mojo/examples/wm_flow/app/app.cc:62: view_manager_client_factory_.reset(
it seems a bit unfortunate that we have to lazily allocate
ViewManagerClientFactory. i suppose an alternative would have been a setter for
the app->shell()?

  virtual void Initialize(mojo::ApplicationImpl* app) MOJO_OVERRIDE {
    view_manager_client_factory_.Initialize(app);
  }

had you considered something like that?

https://codereview.chromium.org/514063003/diff/140001/mojo/services/html_view...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:60: content_handler_factory_.reset(
InterfaceFactoryImplWithContext could also have an Initialize method. then it
wouldn't need to be separately allocated here. just thinking this might reduce
code a bit.

https://codereview.chromium.org/514063003/diff/140001/mojo/services/public/cp...
File mojo/services/public/cpp/view_manager/view_manager_client_factory.h
(right):

https://codereview.chromium.org/514063003/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/view_manager_client_factory.h:21: explicit
ViewManagerClientFactory(Shell* shell,
nit: no longer need explicit keyword

[Aaron Boodman, 2014-09-04 22:31:43]

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
File mojo/examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
mojo/examples/png_viewer/png_viewer.cc:216: content_handler_factory_.reset(
On 2014/09/04 21:33:51, darin wrote:
> food for thought...
> 
> it seems like it'd be possible to have a stock implementation of
ContentHandler
> and InterfaceFactory such that one only needs to supply a function that should
> be called in response to OnConnect. this function would get the parameters of
> the PNGView's constructor perhaps. maybe something to do in a follow-up CL.

I'm keeping it in mind. I wanted to flush things out a bit more before I started
generalizing (see subsequent CL).

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/wm_flow/a...
File mojo/examples/wm_flow/app/app.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/wm_flow/a...
mojo/examples/wm_flow/app/app.cc:62: view_manager_client_factory_.reset(
On 2014/09/04 21:33:51, darin wrote:
> it seems a bit unfortunate that we have to lazily allocate
> ViewManagerClientFactory. i suppose an alternative would have been a setter
for
> the app->shell()?
> 
>   virtual void Initialize(mojo::ApplicationImpl* app) MOJO_OVERRIDE {
>     view_manager_client_factory_.Initialize(app);
>   }
> 
> had you considered something like that?

I can do that if you like. I assumed it was a design goal that the factories
were never partially init'd. ViewManagerClientFactory is totally useless without
a |shell|.

https://codereview.chromium.org/514063003/diff/140001/mojo/services/html_view...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:60: content_handler_factory_.reset(
On 2014/09/04 21:33:51, darin wrote:
> InterfaceFactoryImplWithContext could also have an Initialize method. then it
> wouldn't need to be separately allocated here. just thinking this might reduce
> code a bit.

Yeah, I mean personally I like to avoid the separate Initialize() methods. But I
don't feel that strongly about it. I guess it does save one allocation to have
the Initialize() method.

[Aaron Boodman, 2014-09-05 02:41:31]

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
File mojo/examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/514063003/diff/140001/mojo/examples/png_viewe...
mojo/examples/png_viewer/png_viewer.cc:201: new PNGView(response.Pass(),
exported_services, remote.Pass(), shell_);
On 2014/09/04 21:33:51, darin wrote:
> nit: add a comment here about memory management?

Yeah good point -- instead, I made the ctor private and hid it behind a static
factory that doesn't look so weird (and added a comment there).

https://codereview.chromium.org/514063003/diff/140001/mojo/services/public/cp...
File mojo/services/public/cpp/view_manager/view_manager_client_factory.h
(right):

https://codereview.chromium.org/514063003/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/view_manager_client_factory.h:21: explicit
ViewManagerClientFactory(Shell* shell,
On 2014/09/04 21:33:51, darin wrote:
> nit: no longer need explicit keyword

blargh, done.

[Aaron Boodman, 2014-09-05 02:43:22]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-09-05 02:45:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aa@chromium.org/514063003/200001

[Aaron Boodman, 2014-09-05 03:39:16]

The CQ bit was unchecked by aa@chromium.org

[Aaron Boodman, 2014-09-05 03:39:16]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-09-05 03:39:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aa@chromium.org/514063003/200001

[commit-bot: I haz the power, 2014-09-05 05:18:08]

Committed patchset #7 (id:200001) as 342280004829791be13f1ba2443d8ba636ddf529

[commit-bot: I haz the power, 2014-09-10 03:37:14]

Patchset 7 (id:??) landed as
https://crrev.com/d098906547cc70f8626709ae1243be7f8c1e0e22
Cr-Commit-Position: refs/heads/master@{#293452}