Plumb ViewportMetrics change notifications around the world and back.

Plumb ViewportMetrics change notifications around the world and back.

This fixes the race which is seen where mojo_shell will sometimes
launch with the wrong viewport metrics since the DisplayManager
will happily respond to GetViewportMetrics with default values
before the NativeViewport has fully booted and told it what the
actual values are.

I considered making DisplayManager hang until the NativeViewport
was ready, but I decided that it does make sense for the
ViewportMetrics (device pixel ratio, mostly) to change for a
view if it were to move between displays (as exists on desktop
OSes today).

R=abarth@chromium.org, sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/2b961563d09c1972d8fafe91d9de5c1a38d546e6

[eseidel, 2015-01-27 00:48:26]

Just uploading this to sanity check it with sky before pouring more time into
typing. :p

[sky, 2015-01-27 16:35:08]

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/cpp/lib/view.cc (right):

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/cpp/lib/view.cc:492: void
View::LocalSetViewportMetrics(const ViewportMetrics& old_metrics,
Why does this need to take old_metrics? Isn't old_metrics the same as
viewport_metrics_?

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/cpp/lib/view.cc:494:
DCHECK(viewport_metrics_->size == viewport_metrics_->size);
I think there is a bug where viewport_metrics_ is null for newly created views.
It can be fixed separately.

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:164:
mojo.ViewportMetrics old_metrics,
make sure you run git cl format.

https://codereview.chromium.org/880743002/diff/1/services/view_manager/connec...
File services/view_manager/connection_manager.h (right):

https://codereview.chromium.org/880743002/diff/1/services/view_manager/connec...
services/view_manager/connection_manager.h:133: void
ProcessViewportMetricsChanged(const ServerView* view,
Did you finish wiring this all up? AFAICT this isn't invoked.

How do you plan on invoking this? For all views? Only for the root?

https://codereview.chromium.org/880743002/diff/1/services/view_manager/view_m...
File services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/880743002/diff/1/services/view_manager/view_m...
services/view_manager/view_manager_service_impl.cc:183: if (originated_change ||
!IsViewKnown(view))
This is only going to notify clients that know of |view|. As metrics are
inherited you need to notify everyone. I would notify each connection using the
connections root (don't notify if the connection doesn't have a root), eg:

if (!root())
  return;

client()->OnViewViewportMetricsChanged(ViewIdToTransportId(*root());

And be sure and clarify in the mojom docs the client is only notified for it's
root.

[eseidel, 2015-01-27 18:50:05]

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/cpp/lib/view.cc (right):

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/cpp/lib/view.cc:492: void
View::LocalSetViewportMetrics(const ViewportMetrics& old_metrics,
On 2015/01/27 16:35:08, sky wrote:
> Why does this need to take old_metrics? Isn't old_metrics the same as
> viewport_metrics_?

I just tried to make it match what LocalSetBounds does. :/  I'm happy to remove
the old_metrics.

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/cpp/lib/view.cc:494:
DCHECK(viewport_metrics_->size == viewport_metrics_->size);
On 2015/01/27 16:35:08, sky wrote:
> I think there is a bug where viewport_metrics_ is null for newly created
views.
> It can be fixed separately.

Yes, there is.  It's fixed a little in this patch, not completely.  The trouble
when I tried to model after Bounds is that Bounds uses a mojo:Rect which because
it only contains POD types can be allocated as part of View.  Since
ViewportMetrics contains a SizePtr it can't, thus we have bugs like the null bug
you mention.

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/880743002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:164:
mojo.ViewportMetrics old_metrics,
On 2015/01/27 16:35:08, sky wrote:
> make sure you run git cl format.

Acknowledged.

https://codereview.chromium.org/880743002/diff/1/services/view_manager/connec...
File services/view_manager/connection_manager.h (right):

https://codereview.chromium.org/880743002/diff/1/services/view_manager/connec...
services/view_manager/connection_manager.h:133: void
ProcessViewportMetricsChanged(const ServerView* view,
On 2015/01/27 16:35:08, sky wrote:
> Did you finish wiring this all up? AFAICT this isn't invoked.
> 
> How do you plan on invoking this? For all views? Only for the root?

No, the wiring isn't finished.  I figured all views (since they can be owned by
separate applications, no?)  Unless you think views should be responsible for
telling their children.

https://codereview.chromium.org/880743002/diff/1/services/view_manager/view_m...
File services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/880743002/diff/1/services/view_manager/view_m...
services/view_manager/view_manager_service_impl.cc:183: if (originated_change ||
!IsViewKnown(view))
On 2015/01/27 16:35:08, sky wrote:
> This is only going to notify clients that know of |view|. As metrics are
> inherited you need to notify everyone. I would notify each connection using
the
> connections root (don't notify if the connection doesn't have a root), eg:
> 
> if (!root())
>   return;
> 
> client()->OnViewViewportMetricsChanged(ViewIdToTransportId(*root());
> 
> And be sure and clarify in the mojom docs the client is only notified for it's
> root.

I just copied this from ProcessBoundsChanged. :/

[eseidel, 2015-01-27 20:45:02]

I thought I was done, but it doesn't seem to work yet.

[eseidel, 2015-01-27 22:43:13]

This should be ready for review.

[eseidel, 2015-01-27 22:46:58]

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
File mojo/services/view_manager/public/cpp/lib/view.cc (right):

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
mojo/services/view_manager/public/cpp/lib/view.cc:387: ViewportMetricsPtr
CreateEmptyViewportMetrics() {
This is unfortunate, but makes it so the caller never gets back a null handle,
only a somewhat nonsensical one.

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
mojo/services/view_manager/public/cpp/lib/view.cc:505: // TODO(eseidel): We
could check old_metrics against viewport_metrics_.
I tried, but wasn't able to get them to pass.

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
File mojo/services/view_manager/public/cpp/lib/view_private.cc (right):

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
mojo/services/view_manager/public/cpp/lib/view_private.cc:11: CHECK(view);
This will just make crash stacks easier to read as we'll crash faster.

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/s...
File services/view_manager/server_view.cc (right):

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/s...
services/view_manager/server_view.cc:112: for (size_t i = 0; i <
children_.size(); ++i)
Unknown if this is needed or not.

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/t...
File services/view_manager/test_change_tracker.cc (right):

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/t...
services/view_manager/test_change_tracker.cc:212: // NOT IMPLEMENTED
Not sure if this is needed?

[abarth-chromium, 2015-01-27 22:47:36]

sky/viewer/ LGTM

[sky, 2015-01-27 23:40:23]

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
File mojo/services/view_manager/public/cpp/lib/view_manager_client_impl.cc
(right):

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
mojo/services/view_manager/public/cpp/lib/view_manager_client_impl.cc:304: View*
view = GetViewById(view_id);
It's possible view isn't around anymore. Only invoke LocalSetViewportMetrics if
view is valid.

Also, because we're only sending this for the root you need to apply it to all
views.

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
File mojo/services/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
mojo/services/view_manager/public/interfaces/view_manager.mojom:162: // Invoked
when the viewport metrics for the view have changed.
Document this is only sent for the root. It's assumed the client will propagate
as necessary.

https://codereview.chromium.org/880743002/diff/140001/mojo/services/view_mana...
mojo/services/view_manager/public/interfaces/view_manager.mojom:164:
mojo.ViewportMetrics old_metrics,
nit: indent

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/c...
File services/view_manager/connection_manager.cc (right):

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/c...
services/view_manager/connection_manager.cc:397: // TODO(sky): Do we need to
repaint here?
I would think not.

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/d...
File services/view_manager/display_manager.cc (right):

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/d...
services/view_manager/display_manager.cc:179: // TODO(sky): Is SetBounds still
necessary now with SetViewportMetrics?
Yes, the two are different.

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/s...
File services/view_manager/server_view.h (right):

https://codereview.chromium.org/880743002/diff/140001/services/view_manager/s...
services/view_manager/server_view.h:43: const mojo::ViewportMetrics&
viewport_metrics() { return *viewport_metrics_; }
Since we currently only support metrics on the root you shouldn't need to add
anything on ServerView. At some point we may want to support metrics on certain
nodes, but lets wait until we actually need that.

[eseidel, 2015-01-28 21:14:56]

ptal

[sky, 2015-01-28 21:38:16]

LGTM

[eseidel, 2015-01-28 21:43:35]

Committed patchset #10 (id:180001) manually as
2b961563d09c1972d8fafe91d9de5c1a38d546e6 (presubmit successful).

[eseidel, 2015-01-28 21:58:40]

This broke the tree due to a typo.  Fixed in
d60312f362cd6ed618293259f3df6e5f4128b820