Expand aura::PropertyConverter support.

Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Allow clients to register additional properties.
Add a few more properties and lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org

Committed: https://crrev.com/4f5e732f00545db86e98aaff3245a2b3611d9168
Cr-Commit-Position: refs/heads/master@{#432792}

[msw, 2016-11-16 06:16:52]

Description was changed from

==========
Expand aura::PropertyConverter support.

BUG=663522
==========

to

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Add simple client extensibility to PropertyConverter.

Change aura/wm window string properties to string16.
Change kAppIdKey to base::string16. Add lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org
==========

[msw, 2016-11-16 06:16:52]

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

[msw, 2016-11-16 06:18:58]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 06:19:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 06:49:00]

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

[commit-bot: I haz the power, 2016-11-16 06:49:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-11-16 17:08:13]

Description was changed from

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Add simple client extensibility to PropertyConverter.

Change aura/wm window string properties to string16.
Change kAppIdKey to base::string16. Add lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org
==========

to

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Allow clients to register additional properties.
Add a few more properties and lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org
==========

[msw, 2016-11-16 17:08:30]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2016-11-16 17:08:44]

Hey Scott, please take a look; thanks!

[commit-bot: I haz the power, 2016-11-16 17:09:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 17:37:42]

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

[commit-bot: I haz the power, 2016-11-16 17:37:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-11-16 17:56:13]

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
File ui/aura/mus/property_converter.cc (right):

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:20: std::unique_ptr<std::vector<uint8_t>>
GetArray(Window* window,
optional: naming this as get implies it isn't doing conversion, where as this is
really a conversion function. Maybe ConvertWindowValueToArray? I would be
inclined to name all of these functions with Convert in them. But I'm listing
this as optional, so it's up to you.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:41: transport_value[7] = (value >> 56) & 0xFF;
optional: it's more common that the higher bits are first. See
services/ui/public/cpp/property_type_converters for what I mean. Doing that
would mean you don't have to iterate backwards on line 33 (which is more error
prone with size_t IMO).

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:77: DVLOG(2) << "Unknown aura property key: "
<< key;
I'm ok with a DVLOG, but be warned there are going to be plenty of properties
that shouldn't be mapped, so if this is log level 2 is enabled this is going to
log a bunch of stuff.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:123: CHECK_LE(data->size(), 8u) << " Property
too big: " << transport_name;
I think this should DVLOG and ignore the value. The reason is if you have code
like this, then a malicious client can easily crash ash by sending a value with
a length < 8.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:135: const gfx::Rect value =
mojo::ConvertTo<gfx::Rect>(*data);
Similar comment here and 143 about verifying type. I'm ok if you want to tackle
it later, just add TODOs.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
File ui/aura/mus/property_converter.h (right):

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:55: void RegisterPrimitiveProperty(const void*
key,
Document what |key| is. Or better yet, is it possible to define multiple
RegisterProperty() functions with the types are are supported? e.g.

RegisterProperty(const WindowProperty<bool>* property ..);
RegisterProperty(const WindowProperty<std::string*> property...);

?

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:65: typedef std::pair<const char*, const char*>
PropertyNames;
using?

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:66: std::map<const void*, PropertyNames>
primitive_properties_;
Document what the key is as it isn't readily obvious what the key is here.

[msw, 2016-11-16 22:47:29]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2016-11-16 22:47:45]

Comments addressed; please take another look; thanks!

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
File ui/aura/mus/property_converter.cc (right):

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:20: std::unique_ptr<std::vector<uint8_t>>
GetArray(Window* window,
On 2016/11/16 17:56:13, sky wrote:
> optional: naming this as get implies it isn't doing conversion, where as this
is
> really a conversion function. Maybe ConvertWindowValueToArray? I would be
> inclined to name all of these functions with Convert in them. But I'm listing
> this as optional, so it's up to you.

Done.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:41: transport_value[7] = (value >> 56) & 0xFF;
On 2016/11/16 17:56:13, sky wrote:
> optional: it's more common that the higher bits are first. See
> services/ui/public/cpp/property_type_converters for what I mean. Doing that
> would mean you don't have to iterate backwards on line 33 (which is more error
> prone with size_t IMO).

Done. I can use existing type converters with static casts to int64_t.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:77: DVLOG(2) << "Unknown aura property key: "
<< key;
On 2016/11/16 17:56:13, sky wrote:
> I'm ok with a DVLOG, but be warned there are going to be plenty of properties
> that shouldn't be mapped, so if this is log level 2 is enabled this is going
to
> log a bunch of stuff.

Yeah, this logging probably isn't too helpful; removed.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:123: CHECK_LE(data->size(), 8u) << " Property
too big: " << transport_name;
On 2016/11/16 17:56:13, sky wrote:
> I think this should DVLOG and ignore the value. The reason is if you have code
> like this, then a malicious client can easily crash ash by sending a value
with
> a length < 8.

Done.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:135: const gfx::Rect value =
mojo::ConvertTo<gfx::Rect>(*data);
On 2016/11/16 17:56:13, sky wrote:
> Similar comment here and 143 about verifying type. I'm ok if you want to
tackle
> it later, just add TODOs.

I added a size check for rect, but I'm not sure what to do for string, so I
added a TODO there. Let me know if you have thoughts for stronger validation, or
how to validate array->string.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
File ui/aura/mus/property_converter.h (right):

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:55: void RegisterPrimitiveProperty(const void*
key,
On 2016/11/16 17:56:13, sky wrote:
> Document what |key| is. Or better yet, is it possible to define multiple
> RegisterProperty() functions with the types are are supported? e.g.
> 
> RegisterProperty(const WindowProperty<bool>* property ..);
> RegisterProperty(const WindowProperty<std::string*> property...);
> 
> ?

Done.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:65: typedef std::pair<const char*, const char*>
PropertyNames;
On 2016/11/16 17:56:13, sky wrote:
> using?

Done.

https://codereview.chromium.org/2499933003/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:66: std::map<const void*, PropertyNames>
primitive_properties_;
On 2016/11/16 17:56:13, sky wrote:
> Document what the key is as it isn't readily obvious what the key is here.

Done.

[commit-bot: I haz the power, 2016-11-16 22:48:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 23:30:06]

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

[commit-bot: I haz the power, 2016-11-16 23:30:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-11-17 00:14:50]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 00:15:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-11-17 00:44:35]

LGTM - given we some times treat a string as a bag of bytes I'm not sure there
is any validation. I suppose we could check the size and disallow really big
sizes. But not sure.

https://codereview.chromium.org/2499933003/diff/180001/ui/aura/mus/property_c...
File ui/aura/mus/property_converter.cc (right):

https://codereview.chromium.org/2499933003/diff/180001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:39: RegisterProperty(client::kAppIdKey,
Maybe sort these?

[commit-bot: I haz the power, 2016-11-17 01:05:04]

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

[commit-bot: I haz the power, 2016-11-17 01:05:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msw, 2016-11-17 02:24:11]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 02:25:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-11-17 04:22:57]

The CQ bit was checked by msw@chromium.org

[msw, 2016-11-17 04:22:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2499933003/#ps140002
(title: "Fix the other new unit test.")

[msw, 2016-11-17 04:23:27]

Done; also fixed up the new unit tests.

https://codereview.chromium.org/2499933003/diff/180001/ui/aura/mus/property_c...
File ui/aura/mus/property_converter.cc (right):

https://codereview.chromium.org/2499933003/diff/180001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:39: RegisterProperty(client::kAppIdKey,
On 2016/11/17 00:44:35, sky wrote:
> Maybe sort these?

Done.

[commit-bot: I haz the power, 2016-11-17 04:23:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 07:01:06]

Description was changed from

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Allow clients to register additional properties.
Add a few more properties and lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org
==========

to

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Allow clients to register additional properties.
Add a few more properties and lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-11-17 07:01:07]

Committed patchset #12 (id:140002)

[commit-bot: I haz the power, 2016-11-17 07:05:35]

Description was changed from

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Allow clients to register additional properties.
Add a few more properties and lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org
==========

to

==========
Expand aura::PropertyConverter support.

Add PropertyConverter support for strings and rects.
Allow clients to register additional properties.
Add a few more properties and lots of testing.

BUG=663522
TEST=Automated
R=sky@chromium.org

Committed: https://crrev.com/4f5e732f00545db86e98aaff3245a2b3611d9168
Cr-Commit-Position: refs/heads/master@{#432792}
==========

[commit-bot: I haz the power, 2016-11-17 07:05:37]

Patchset 12 (id:??) landed as
https://crrev.com/4f5e732f00545db86e98aaff3245a2b3611d9168
Cr-Commit-Position: refs/heads/master@{#432792}