Simple support for routing in Sky

Simple support for routing in Sky

R=abarth@chromium.org, abarth, hixie

Committed: https://chromium.googlesource.com/external/mojo/+/80ad2e1649383cd8c40f14f7ba1112fc4f9f8f25

[abarth-chromium, 2015-06-16 02:07:55]

lgtm

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
File sky/sdk/lib/widgets/navigator.dart (right):

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:7: typedef UINode Builder(Navigator
navigator);
Please add a blank line below this line.

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:24: currentRoute = initialRoute == null ?
this.routes[0] : initialRoute;
There's actually a subtle bug in this class.  The problem is that if the app
rebuilds, this logic will set the currentRoute and syncFields will copy that
current route into the existing Navigator.  That will blow away whatever route
history we have.

Instead, I'd recommend renaming initialRoute to currentRoute and removing the
routes[0] logic.  In build(), below, if currentRoute is null, then we can
default to routes[0].

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:55: return currentRoute.build(this);
For example,

if (currentRoute == null)
  return routes[0].build(this);
return currentRoute.build(this);

[jackson, 2015-06-16 16:46:37]

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
File sky/sdk/lib/widgets/navigator.dart (right):

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:7: typedef UINode Builder(Navigator
navigator);
On 2015/06/16 02:07:55, abarth wrote:
> Please add a blank line below this line.

Done.

https://codereview.chromium.org/1186273002/diff/20001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:24: currentRoute = initialRoute == null ?
this.routes[0] : initialRoute;
On 2015/06/16 02:07:54, abarth wrote:
> There's actually a subtle bug in this class.  The problem is that if the app
> rebuilds, this logic will set the currentRoute and syncFields will copy that
> current route into the existing Navigator.  That will blow away whatever route
> history we have.
> 
> Instead, I'd recommend renaming initialRoute to currentRoute and removing the
> routes[0] logic.  In build(), below, if currentRoute is null, then we can
> default to routes[0].

Done.

[jackson, 2015-06-16 16:48:03]

Committed patchset #3 (id:40001) manually as
80ad2e1649383cd8c40f14f7ba1112fc4f9f8f25 (presubmit successful).

[Hixie, 2015-06-16 16:55:09]

lgtm

https://codereview.chromium.org/1186273002/diff/40001/sky/examples/raw/naviga...
File sky/examples/raw/navigation.dart (right):

https://codereview.chromium.org/1186273002/diff/40001/sky/examples/raw/naviga...
sky/examples/raw/navigation.dart:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
This example should be in examples/widgets.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
File sky/sdk/lib/widgets/navigator.dart (right):

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:10: RouteBase({this.name});
Spaces around the parameters.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:16: Route({String name, this.builder}) :
super(name: name);
Spaces here too.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:22: Navigator({Object key, this.currentRoute,
this.routes})
Here too.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:26: List<RouteBase> routes;
Maybe add a comment that "routes" is just the _named_ routes.

Also, why is this a list, and not a map? The only use for it is looking things
up by key, no?

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:33: void pushNamedRoute(String name) {
Why "push"?

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:35: for (RouteBase route in routes) {
You can do this way more efficiently if you use a map.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:53: Route route = currentRoute == null ?
routes[0] : currentRoute;
If you switch to using a map, then maybe have an implicitly default key rather
than taking the first in the list.

[jackson, 2015-06-16 18:27:28]

Thanks for the comments. I'm working on a new changelist to add transitions and
will address this feedback in that code review as well.

https://codereview.chromium.org/1186273002/diff/40001/sky/examples/raw/naviga...
File sky/examples/raw/navigation.dart (right):

https://codereview.chromium.org/1186273002/diff/40001/sky/examples/raw/naviga...
sky/examples/raw/navigation.dart:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2015/06/16 16:55:09, Hixie wrote:
> This example should be in examples/widgets.

Done.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
File sky/sdk/lib/widgets/navigator.dart (right):

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:10: RouteBase({this.name});
On 2015/06/16 16:55:09, Hixie wrote:
> Spaces around the parameters.

Done.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:16: Route({String name, this.builder}) :
super(name: name);
On 2015/06/16 16:55:09, Hixie wrote:
> Spaces here too.

Done.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:22: Navigator({Object key, this.currentRoute,
this.routes})
On 2015/06/16 16:55:09, Hixie wrote:
> Here too.

Done.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:26: List<RouteBase> routes;
On 2015/06/16 16:55:09, Hixie wrote:
> Maybe add a comment that "routes" is just the _named_ routes.
> 
> Also, why is this a list, and not a map? The only use for it is looking things
> up by key, no?

I'll rename it to namedRoutes for now and make it a map with a default key. I
think we're going to come up with a more advanced routing system, and a map data
structure might not work well for that, but right now it's fine.

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:33: void pushNamedRoute(String name) {
On 2015/06/16 16:55:09, Hixie wrote:
> Why "push"?

Because I'm implementing pop shortly. :)

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:35: for (RouteBase route in routes) {
On 2015/06/16 16:55:09, Hixie wrote:
> You can do this way more efficiently if you use a map.

Agree, but see above

https://codereview.chromium.org/1186273002/diff/40001/sky/sdk/lib/widgets/nav...
sky/sdk/lib/widgets/navigator.dart:53: Route route = currentRoute == null ?
routes[0] : currentRoute;
On 2015/06/16 16:55:09, Hixie wrote:
> If you switch to using a map, then maybe have an implicitly default key rather
> than taking the first in the list.

OK