Introduce very beginning of navigation

Introduce very beginning of navigation.

Add an interface to allow embedders to navigate embedded views.
This only supports local/pushState-style navigation right now.

BUG=
R=darin@chromium.org, davemoore@chromium.org, sky@chromium.org

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

[DaveMoore, 2014-06-09 19:59:32]

lgtm

https://codereview.chromium.org/327523004/diff/40001/mojo/examples/embedded_a...
File mojo/examples/embedded_app/embedded_app.cc (right):

https://codereview.chromium.org/327523004/diff/40001/mojo/examples/embedded_a...
mojo/examples/embedded_app/embedded_app.cc:52: const std::string& url =
details->url.To<std::string>();
Nit: LOG(INFOS)'s should be removed before checkin

https://codereview.chromium.org/327523004/diff/40001/mojo/examples/embedded_a...
mojo/examples/embedded_app/embedded_app.cc:123: if (!view_manager_) {
Nit: remove extra braces

[Aaron Boodman, 2014-06-09 20:08:59]

https://codereview.chromium.org/327523004/diff/40001/mojo/examples/embedded_a...
File mojo/examples/embedded_app/embedded_app.cc (right):

https://codereview.chromium.org/327523004/diff/40001/mojo/examples/embedded_a...
mojo/examples/embedded_app/embedded_app.cc:52: const std::string& url =
details->url.To<std::string>();
On 2014/06/09 19:59:32, DaveMoore wrote:
> Nit: LOG(INFOS)'s should be removed before checkin

Done.

https://codereview.chromium.org/327523004/diff/40001/mojo/examples/embedded_a...
mojo/examples/embedded_app/embedded_app.cc:123: if (!view_manager_) {
On 2014/06/09 19:59:32, DaveMoore wrote:
> Nit: remove extra braces

Grump.

[Aaron Boodman, 2014-06-09 20:11:17]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-06-09 20:13:49]

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

[Ben Goodger (Google), 2014-06-09 20:17:05]

https://codereview.chromium.org/327523004/diff/120001/mojo/examples/window_ma...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/327523004/diff/120001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:85:
CreateWindow("mojo:mojo_embedded_app");
nit: mebbe swap this out with your new kEmbeddedAppURL

https://codereview.chromium.org/327523004/diff/120001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:127: ConnectTo(kEmbeddedAppURL,
&view_navigator_);
can't you resolve this by having view_navigator_ be a local & just connect every
time?

[Aaron Boodman, 2014-06-09 20:47:52]

The CQ bit was unchecked by aa@chromium.org

[Aaron Boodman, 2014-06-10 19:41:29]

dave: please especially review changes to service_connector.cc and
dynamic_service_loader.cc.

sky: review other files please.

Thanks!

https://codereview.chromium.org/327523004/diff/120001/mojo/examples/window_ma...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/327523004/diff/120001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:85:
CreateWindow("mojo:mojo_embedded_app");
On 2014/06/09 20:17:05, Ben Goodger (Google) wrote:
> nit: mebbe swap this out with your new kEmbeddedAppURL

Ugh. Merge fallout. Sorry.

https://codereview.chromium.org/327523004/diff/120001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:127: ConnectTo(kEmbeddedAppURL,
&view_navigator_);
On 2014/06/09 20:17:05, Ben Goodger (Google) wrote:
> can't you resolve this by having view_navigator_ be a local & just connect
every
> time?

I wasn't referring to the shared member variable. You're right that I can use a
local for this (I had to change something in application.cc for it to work, but
I think that was a bug).

What I was referring to was that connecting by URL implies that the URL alone is
sufficient to identify what you want. I don't think that is the case.

Think of like in Chrome, where we have multiple processes for a given domain. If
we want to support that, connecting by URL isn't going to work...

I think that when embed asks to connect to a service, it should get back a SP.
It should return that SP up to caller, and caller should use that to get the
interfaces it wants. That way both VN and the caller of Embed can be ignorant of
how exactly services are instantiated and shared.

I believe that the exactly rules for how we divvy up services will vary by
service, device, etc.

https://codereview.chromium.org/327523004/diff/180001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/service_connector.h (left):

https://codereview.chromium.org/327523004/diff/180001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/service_connector.h:127:
owner_->RemoveServiceConnector(this);
I believe this is incorrect. Just because last connection goes away, that
doesn't mean someone won't ask for one later. Connector should remain.

https://codereview.chromium.org/327523004/diff/180001/mojo/shell/dynamic_serv...
File mojo/shell/dynamic_service_loader.cc (right):

https://codereview.chromium.org/327523004/diff/180001/mojo/shell/dynamic_serv...
mojo/shell/dynamic_service_loader.cc:17: std::string MakeSharedLibraryName(const
std::string& base_name) {
I changed the mojo scheme to be a 'standard' scheme as defined by url_util.h.
So-called 'standard' schemes are different from non-standard schemes because
they have a host component.

I think this is important to make the origin concept be equivalent to the web
one, where origin == scheme+host+port.

With that change, all the pathnames started being empty, so I had to change
this.

[DaveMoore, 2014-06-10 21:38:11]

There are no changes to dynamic_service_loader.cc. Should there be?

https://codereview.chromium.org/327523004/diff/190001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/service_connector.h (left):

https://codereview.chromium.org/327523004/diff/190001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/service_connector.h:127:
owner_->RemoveServiceConnector(this);
Please leave a TODO about maybe killing the app at this point.

[Aaron Boodman, 2014-06-10 21:43:19]

https://codereview.chromium.org/327523004/diff/190001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/service_connector.h (left):

https://codereview.chromium.org/327523004/diff/190001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/service_connector.h:127:
owner_->RemoveServiceConnector(this);
On 2014/06/10 21:38:11, DaveMoore wrote:
> Please leave a TODO about maybe killing the app at this point.

Why would we want to kill it? Doesn't this just mean there are no connections
for this connector? There could be other connectors with connections.

[Aaron Boodman, 2014-06-10 21:44:08]

On Tue, Jun 10, 2014 at 2:38 PM, <davemoore@chromium.org> wrote:

> There are no changes to dynamic_service_loader.cc. Should there be?
>

They moved to mojo_url_resolver after rebasing darin's recent changes.


>
>
> https://codereview.chromium.org/327523004/diff/190001/
> mojo/public/cpp/application/lib/service_connector.h
> File mojo/public/cpp/application/lib/service_connector.h (left):
>
> https://codereview.chromium.org/327523004/diff/190001/
> mojo/public/cpp/application/lib/service_connector.h#oldcode127
> mojo/public/cpp/application/lib/service_connector.h:127:
> owner_->RemoveServiceConnector(this);
> Please leave a TODO about maybe killing the app at this point.
>
> https://codereview.chromium.org/327523004/
>

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

[darin (slow to review), 2014-06-10 21:45:43]

mojo_url_resolver.cc changes LGTM.

[Aaron Boodman, 2014-06-10 22:05:30]

Dave, I fixed service_manager_unittest.cc. Please see that too.

[sky, 2014-06-10 22:15:14]

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/embedded_...
File mojo/examples/embedded_app/embedded_app.cc (right):

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/embedded_...
mojo/examples/embedded_app/embedded_app.cc:70: };
DISALLOW (same comment in all similar files).

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/window_ma...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:129: ConnectTo(url,
&view_navigator);
This won't work for mojo_browser (line 89 above).

https://codereview.chromium.org/327523004/diff/190001/mojo/services/navigatio...
File mojo/services/navigation/navigation.mojom (right):

https://codereview.chromium.org/327523004/diff/190001/mojo/services/navigatio...
mojo/services/navigation/navigation.mojom:12: // Applications implement this
interface to support navigation of their views
Your use of view here and below is confusing (and I think wrong). AFAICT you're
navigating the node. See EmbeddedAPp, where view_id is the id of the node. At a
minimum rename view_id (and update your description too). But you should also
consider renaming this to just Navigator.

[Aaron Boodman, 2014-06-10 22:31:31]

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/embedded_...
File mojo/examples/embedded_app/embedded_app.cc (right):

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/embedded_...
mojo/examples/embedded_app/embedded_app.cc:70: };
On 2014/06/10 22:15:14, sky wrote:
> DISALLOW (same comment in all similar files).

Done.

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/window_ma...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/327523004/diff/190001/mojo/examples/window_ma...
mojo/examples/window_manager/window_manager.cc:129: ConnectTo(url,
&view_navigator);
On 2014/06/10 22:15:14, sky wrote:
> This won't work for mojo_browser (line 89 above).

Sorry, didn't see that get added. Fixed.

https://codereview.chromium.org/327523004/diff/190001/mojo/services/navigatio...
File mojo/services/navigation/navigation.mojom (right):

https://codereview.chromium.org/327523004/diff/190001/mojo/services/navigatio...
mojo/services/navigation/navigation.mojom:12: // Applications implement this
interface to support navigation of their views
On 2014/06/10 22:15:14, sky wrote:
> Your use of view here and below is confusing (and I think wrong). AFAICT
you're
> navigating the node. See EmbeddedAPp, where view_id is the id of the node. At
a
> minimum rename view_id (and update your description too). But you should also
> consider renaming this to just Navigator.

Ah, you are right, I started with view_id, but changed to node_id and forgot to
change here.

The interface name was intentional, but I see why you think it is confusing.
There is eventually going to be another interface, implemented by the embedder.
I was going to name this NodeNavigator (to parallel the Node/View relationship).

Anyway, I changed it to just Navigator for now.

[DaveMoore, 2014-06-10 22:41:01]

lgtm

[sky, 2014-06-10 22:45:53]

LGTM

[Aaron Boodman, 2014-06-10 23:15:05]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-06-10 23:17:37]

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

[commit-bot: I haz the power, 2014-06-11 05:06:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-11 06:30:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 06:30:11]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Aaron Boodman, 2014-06-11 15:30:07]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-06-11 15:32:46]

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

[commit-bot: I haz the power, 2014-06-11 21:03:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:25:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:25:03]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Aaron Boodman, 2014-06-12 03:43:51]

Committed patchset #12 manually as r276521 (presubmit successful).