mozart: Add a simple tiling view manager.

mozart: Add a simple tiling view manager.

The tile view can be used to launch several applications based on the
Mozart view system side by side all contained within the same view.  It
is a simple demonstration of view composition.

Exempt all example code from logspam presubmit checks because it's
often quite useful to be able to illustrate the operation of an
example program to an interactive user.

R=abarth@google.com

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

[jeffbrown, 2015-10-24 03:06:44]

jeffbrown@google.com changed reviewers:
+ abarth@google.com, jamesr@chromium.org

[abarth, 2015-10-24 06:51:12]

LGTM

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_a...
File examples/ui/tile/tile_app.cc (right):

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.cc:51: base::SplitString(url.query(), ',',
&view_urls);
This is kind of lame.  You should probably use the Application args system even
though it's ugly on the command line.  That just means we should make it
prettier on the command line not that some apps should use query parameters to
receive their args.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:45: for (auto it = view_urls_.begin(); it !=
view_urls_.end(); it++) {
for (const std::string& url : view_urls_) {

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:47: const uint32_t key = it - view_urls_.begin();
Oh, I see...  I probably would have just used the ++ operator as we were running
through the loop.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:60:
mojo::InterfaceRequest<mojo::ServiceProvider>(),
there's no implicit conversion from |nullptr| ?

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:61: mojo::ServiceProviderPtr(),
ditto

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:82: std::make_pair(child_key,
std::unique_ptr<ViewData>(new ViewData(url))));
We don't have std::make_unique<ViewData>(url) but I thought trung made an
equivalent that we can use.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:120: }
This stanza has come up several times now...

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:124: it != children_needing_layout.end(); ++it) {
for (uint32_t id : children_needing_layout) {

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:136: uint32_t child_width = new_size.width /
views_.size();
If new_size.width doesn't divide evenly by views_.size(), you're not going to
fill your width.

One approach is to hoist this expression out of the loop (given that it's
loop-invariant) and then compute the remainder at the same time.  As you loop
through the views, you can allocate the remainder one pixel at a time until it's
gone.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:145: params->device_pixel_ratio =
layout_params->device_pixel_ratio;
This would be more future-proof if you made |params| a clone of |layout_params|
and then mutated the fields you wanted to change.

That actually raises an issue about the extensibility of ViewLayoutParams.  Do
all the views in the hierarchy have to agree about exactly which version of the
protocol they're using?

Relatedly, is it legal to LayoutChild with a different device_pixel_ratio than
you were given?  I wonder if we should break ViewLayoutParams into two pieces:
one that's information from your parent and one that's information from the view
manager.  For example, children_needing_layout would then move into that second
struct.  When you call LayoutChild, you just need to provide the first struct
because the view manager will provide the second struct for the child.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:148: continue;  // no work to do
This seems dubious.  What if one of our grandchildren requested layout?  Will
the view manager call it directly?  If so, why not move this check into the view
manager since the view manager needs to have conditional logic about calling
OnLayout?

I guess what I'm saying is that we should remove the children_needing_layout
parameter and ask views to call LayoutChild for each of their children if they
want to change the child's layout parameters.  The view manager will then use
the updated layout parameters and the dirtiness of the child to decide whether
to actually call OnLayout on the child or to short-circuit the OnLayout call and
call the completion callback directly.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:191: // Produce a new frame.
This system would be cleaner if the parent allocated the surface ids for the
children.  Then the parent could submit its frame without waiting for the
children to finish layout.

I guess if it's common to determine your size based on your children you need to
wait for them anyway...

[jamesr, 2015-10-26 20:31:21]

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:60:
mojo::InterfaceRequest<mojo::ServiceProvider>(),
On 2015/10/24 at 06:51:12, abarth wrote:
> there's no implicit conversion from |nullptr| ?

there is. provider->CreateView(nullptr, nullptr, ...) should do what you want

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:82: std::make_pair(child_key,
std::unique_ptr<ViewData>(new ViewData(url))));
On 2015/10/24 at 06:51:12, abarth wrote:
> We don't have std::make_unique<ViewData>(url) but I thought trung made an
equivalent that we can use.

there's this:

https://github.com/domokit/mojo/blob/master/mojo/edk/util/make_unique.h

you use it like

std::make_pair(child_key, mojo::util::MakeUnique<ViewData>(url));

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:185: if (std::any_of(views_.begin(), views_.end(),
this scales linearly with views, which might be a problem if this needs to
handle lots of views. since this class manages this container you could keep a
count of pending on the side and makes the O(1)

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:186: [](const decltype(views_)::value_type& pair)
{
this is an odd use of decltype, imo. this isn't generic code that needs to work
for many different definitions of |views_|. i think if the type of |views_| is
too irritating to print out here the right fix is to provide a type alias for
the map, i.e.

// tile_view.h
using ViewMap = <uint32_t, std::unique_ptr<ViewData>>;
ViewMap views_;

and then here do

   [](const ViewMap::value_type& pair)

which makes this line shorter and less magical and makes it easier to spell out
the iterator type if you need to do that elsewhere

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:193: frame->resources.resize(0u);
i *hate* that mojo arrays are nullable - the fact that this line changes
behavior is insano

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:202: for (auto it = views_.begin(); it !=
views_.end(); it++) {
'const auto& it' ?

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:203: const ViewData* view_data = it->second.get();
const ViewData& view_data ? it's not useful to null-test this value and having a
local ref makes access a bit terser

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:231: quad->solid_color_quad_state->color->rgba =
0xffff00ff;
i think we have some named colors somewhere (not that it matters too much, this
is definitely going to change a bunch)

[jeffbrown, 2015-10-27 03:13:08]

Lots of good comments, thanks.  :)

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_a...
File examples/ui/tile/tile_app.cc (right):

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.cc:51: base::SplitString(url.query(), ',',
&view_urls);
On 2015/10/24 06:51:12, abarth wrote:
> This is kind of lame.  You should probably use the Application args system
even
> though it's ugly on the command line.  That just means we should make it
> prettier on the command line not that some apps should use query parameters to
> receive their args.

Args won't work here.  Suppose I wanted to embed tile view within itself.  I
could have two instances of the application and each will need its own args. 
Moreover there's no way to specify args when connecting to another service.

So perhaps args need to go away entirely.

Still, I foresee the query parameters getting slammed if we use them like this a
lot so we might need a better way to pass data to a service at connection time.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:47: const uint32_t key = it - view_urls_.begin();
On 2015/10/24 06:51:12, abarth wrote:
> Oh, I see...  I probably would have just used the ++ operator as we were
running
> through the loop.

Honestly I just didn't know the for iterator syntax...

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:60:
mojo::InterfaceRequest<mojo::ServiceProvider>(),
On 2015/10/26 20:31:21, jamesr wrote:
> On 2015/10/24 at 06:51:12, abarth wrote:
> > there's no implicit conversion from |nullptr| ?
> 
> there is. provider->CreateView(nullptr, nullptr, ...) should do what you want

Done.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:82: std::make_pair(child_key,
std::unique_ptr<ViewData>(new ViewData(url))));
On 2015/10/26 20:31:21, jamesr wrote:
> On 2015/10/24 at 06:51:12, abarth wrote:
> > We don't have std::make_unique<ViewData>(url) but I thought trung made an
> equivalent that we can use.
> 
> there's this:
> 
> https://github.com/domokit/mojo/blob/master/mojo/edk/util/make_unique.h
> 
> you use it like
> 
> std::make_pair(child_key, mojo::util::MakeUnique<ViewData>(url));

Hardly seems worth it here.  I think I'll just leave this alone.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:120: }
On 2015/10/24 06:51:12, abarth wrote:
> This stanza has come up several times now...

Yup.  I find it helpful to be explicit in example code.  Once the dust settles
we can see whether there's any utility code worth deduping.  I suspect this
pattern will go away at some point if we introduce different synchronization
mechanisms.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:136: uint32_t child_width = new_size.width /
views_.size();
On 2015/10/24 06:51:12, abarth wrote:
> If new_size.width doesn't divide evenly by views_.size(), you're not going to
> fill your width.
> 
> One approach is to hoist this expression out of the loop (given that it's
> loop-invariant) and then compute the remainder at the same time.  As you loop
> through the views, you can allocate the remainder one pixel at a time until
it's
> gone.

Good point!

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:145: params->device_pixel_ratio =
layout_params->device_pixel_ratio;
On 2015/10/24 06:51:12, abarth wrote:
> This would be more future-proof if you made |params| a clone of
|layout_params|
> and then mutated the fields you wanted to change.
> 
> That actually raises an issue about the extensibility of ViewLayoutParams.  Do
> all the views in the hierarchy have to agree about exactly which version of
the
> protocol they're using?
> 
> Relatedly, is it legal to LayoutChild with a different device_pixel_ratio than
> you were given?  I wonder if we should break ViewLayoutParams into two pieces:
> one that's information from your parent and one that's information from the
view
> manager.  For example, children_needing_layout would then move into that
second
> struct.  When you call LayoutChild, you just need to provide the first struct
> because the view manager will provide the second struct for the child.

Discussed in person.  I'll see about pushing more of the high level layout
strategy into the view manager to allow the protocol to evolve.

And yes, we should split up the layout params.  I expect we'll see even more
asymmetry here where the outgoing information that the parent gives to the view
manager regarding the child may change representation before actually being
delivered to the child.  (The parent might tell the view manager things about
its children that don't need to be passed along, such as whether the parent
cares about the child's size, etc.)

Not sure about device_pixel_ratio though.  There are good reasons to keep it
fixed but it might depend on the projection.  There could also be some value in
being able to fake it (for magnification).

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:148: continue;  // no work to do
On 2015/10/24 06:51:12, abarth wrote:
> This seems dubious.  What if one of our grandchildren requested layout?  Will
> the view manager call it directly?  If so, why not move this check into the
view
> manager since the view manager needs to have conditional logic about calling
> OnLayout?
> 
> I guess what I'm saying is that we should remove the children_needing_layout
> parameter and ask views to call LayoutChild for each of their children if they
> want to change the child's layout parameters.  The view manager will then use
> the updated layout parameters and the dirtiness of the child to decide whether
> to actually call OnLayout on the child or to short-circuit the OnLayout call
and
> call the completion callback directly.

This is just an optimization to avoid an extra round-trip.  The view manager
already does what you suggest.

I'm not entirely satisfied with |children_needing_layout|.  Right now it's a way
to propagate dirtiness to the parent.  Specifically telling the parent that the
child's size or surface may have changed and that the parent *must* call
LayoutChild to get that new information regardless of whether the parent wants
to provide any new parameters.

Perhaps this would be better expressed as a two-phase protocol of some kind.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:185: if (std::any_of(views_.begin(), views_.end(),
On 2015/10/26 20:31:21, jamesr wrote:
> this scales linearly with views, which might be a problem if this needs to
> handle lots of views. since this class manages this container you could keep a
> count of pending on the side and makes the O(1)

True.  This is simpler (state can't get out of sync) but perhaps I should make
the example a little smarter.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:186: [](const decltype(views_)::value_type& pair)
{
On 2015/10/26 20:31:21, jamesr wrote:
> this is an odd use of decltype, imo. this isn't generic code that needs to
work
> for many different definitions of |views_|. i think if the type of |views_| is
> too irritating to print out here the right fix is to provide a type alias for
> the map, i.e.
> 
> // tile_view.h
> using ViewMap = <uint32_t, std::unique_ptr<ViewData>>;
> ViewMap views_;
> 
> and then here do
> 
>    [](const ViewMap::value_type& pair)
> 
> which makes this line shorter and less magical and makes it easier to spell
out
> the iterator type if you need to do that elsewhere

Hmm.  I think the intention is pretty clear.  I don't think decltype needs to be
limited to generic programming.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:191: // Produce a new frame.
On 2015/10/24 06:51:12, abarth wrote:
> This system would be cleaner if the parent allocated the surface ids for the
> children.  Then the parent could submit its frame without waiting for the
> children to finish layout.
> 
> I guess if it's common to determine your size based on your children you need
to
> wait for them anyway...

It might be.  Depends on whether a new surface is really needed.  For example,
the child might just be pushing video frames of fixed size so it really doesn't
care about layout other than ensuring it gets scaled appropriately.

I'm not satisfied with the layout and drawing synchronization mechanisms right
now.  We should discuss this further.

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:193: frame->resources.resize(0u);
On 2015/10/26 20:31:21, jamesr wrote:
> i *hate* that mojo arrays are nullable - the fact that this line changes
> behavior is insano

Yup.  It's nuts.  And if I omit this line then the program crashes if there are
no views because resources will remain null.  Whee!

https://codereview.chromium.org/1425543002/diff/20001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:203: const ViewData* view_data = it->second.get();
On 2015/10/26 20:31:21, jamesr wrote:
> const ViewData& view_data ? it's not useful to null-test this value and having
a
> local ref makes access a bit terser

Sure, why not.

[qsr, 2015-10-27 09:01:27]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-10-27 09:01:27]

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:45: for (auto url : view_urls_) {
const auto& ?

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:85:
provider.set_connection_error_handler(base::Closure());
Why is this needed? The provider will be deleted as soon as this function ends.

[jeffbrown, 2015-10-27 22:54:24]

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:45: for (auto url : view_urls_) {
On 2015/10/27 09:01:27, qsr wrote:
> const auto& ?

Done.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:85:
provider.set_connection_error_handler(base::Closure());
On 2015/10/27 09:01:27, qsr wrote:
> Why is this needed? The provider will be deleted as soon as this function
ends.

Good point, this is redundant.

[jamesr, 2015-10-27 22:58:28]

Some more style and general C++ comments below

we talked about redoing layout today but I think it's fine to land this as is to
make progress

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
File examples/ui/tile/tile_app.cc (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.cc:31: };
probably want to DISALLOW_COPY_AND_ASSIGN this too

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
File examples/ui/tile/tile_app.h (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.h:35: mojo::ApplicationImpl* app_impl_;
make members of a class private

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.h:38: DISALLOW_COPY_AND_ASSIGN(TileApp);
this macro in particular only makes sense in the 'private' section

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:81: std::make_pair(child_key,
std::unique_ptr<ViewData>(new ViewData(url))));
there's a school of thought that C++ code using smart pointers should never
invoke 'new' or 'delete' by hand, it should always use the wrappers. if you buy
into that school, you would write this

mojo::util::MakeUnique<ViewData>(url)

which has the advantage of being slightly terser than what you wrote.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.h (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.h:37: ViewData(const std::string& url);
explicit

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.h:42: bool layout_pending;
general style is to either be a struct with public members or be a class with
private members and setters/getters:

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._...

this looks like's a struct but 'url' is a const member, perhaps?

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.h:50: DISALLOW_COPY_AND_ASSIGN(ViewData);
since you have ..Ptr members this won't be copyable anyway

[jeffbrown, 2015-10-27 23:24:19]

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
File examples/ui/tile/tile_app.cc (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.cc:31: };
On 2015/10/27 22:58:27, jamesr wrote:
> probably want to DISALLOW_COPY_AND_ASSIGN this too

Done.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
File examples/ui/tile/tile_app.h (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_a...
examples/ui/tile/tile_app.h:35: mojo::ApplicationImpl* app_impl_;
On 2015/10/27 22:58:27, jamesr wrote:
> make members of a class private

Fixed.  Accidental omission.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.cc (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.cc:81: std::make_pair(child_key,
std::unique_ptr<ViewData>(new ViewData(url))));
On 2015/10/27 22:58:27, jamesr wrote:
> there's a school of thought that C++ code using smart pointers should never
> invoke 'new' or 'delete' by hand, it should always use the wrappers. if you
buy
> into that school, you would write this
> 
> mojo::util::MakeUnique<ViewData>(url)
> 
> which has the advantage of being slightly terser than what you wrote.

Feels weird to depend on an EDK header here so I'll just wait for c++14.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
File examples/ui/tile/tile_view.h (right):

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.h:37: ViewData(const std::string& url);
On 2015/10/27 22:58:27, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.h:42: bool layout_pending;
On 2015/10/27 22:58:27, jamesr wrote:
> general style is to either be a struct with public members or be a class with
> private members and setters/getters:
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._...
> 
> this looks like's a struct but 'url' is a const member, perhaps?

Done.

https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
examples/ui/tile/tile_view.h:50: DISALLOW_COPY_AND_ASSIGN(ViewData);
On 2015/10/27 22:58:27, jamesr wrote:
> since you have ..Ptr members this won't be copyable anyway

True.  Should I state that explicitly or let the compiler infer it?

[jamesr, 2015-10-27 23:27:52]

On 2015/10/27 at 23:24:19, jeffbrown wrote:
> Done.
> 
>
https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
> examples/ui/tile/tile_view.h:50: DISALLOW_COPY_AND_ASSIGN(ViewData);
> On 2015/10/27 22:58:27, jamesr wrote:
> > since you have ..Ptr members this won't be copyable anyway
> 
> True.  Should I state that explicitly or let the compiler infer it?

If it's a struct then you aren't supposed to define members, you should just let
the compiler do what it does.

should i be seeing a new patch with this as a struct?

[jamesr, 2015-10-27 23:27:53]

On 2015/10/27 at 23:24:19, jeffbrown wrote:
> Done.
> 
>
https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
> examples/ui/tile/tile_view.h:50: DISALLOW_COPY_AND_ASSIGN(ViewData);
> On 2015/10/27 22:58:27, jamesr wrote:
> > since you have ..Ptr members this won't be copyable anyway
> 
> True.  Should I state that explicitly or let the compiler infer it?

If it's a struct then you aren't supposed to define members, you should just let
the compiler do what it does.

should i be seeing a new patch with this as a struct?

[jeffbrown, 2015-10-27 23:35:19]

On 2015/10/27 23:27:53, jamesr wrote:
> On 2015/10/27 at 23:24:19, jeffbrown wrote:
> > Done.
> > 
> >
>
https://codereview.chromium.org/1425543002/diff/40001/examples/ui/tile/tile_v...
> > examples/ui/tile/tile_view.h:50: DISALLOW_COPY_AND_ASSIGN(ViewData);
> > On 2015/10/27 22:58:27, jamesr wrote:
> > > since you have ..Ptr members this won't be copyable anyway
> > 
> > True.  Should I state that explicitly or let the compiler infer it?
> 
> If it's a struct then you aren't supposed to define members, you should just
let
> the compiler do what it does.
> 
> should i be seeing a new patch with this as a struct?

I haven't uploaded the change yet.  But yes, I made it a struct.

[jeffbrown, 2015-10-28 01:48:36]

Committed patchset #5 (id:80001) manually as
2df6b30903d2b030da6aa2f0a47ecfd4c83746a2 (presubmit successful).