Validate incoming window properties.

aura-mus: Validate incoming window properties.

This adds a validator function to the integral type version of
PropertyConverter::RegisterProperty() and rejects changes if the
incoming value, passed to the validator, returns false.

BUG=654924
Review-Url: https://codereview.chromium.org/2702423004
Cr-Commit-Position: refs/heads/master@{#452990}
Committed: https://chromium.googlesource.com/chromium/src/+/19e1e20024109653910be72bd7ccbcc866a47c26

[Elliot Glaysher, 2017-02-21 22:46:23]

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

[commit-bot: I haz the power, 2017-02-21 22:47:38]

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, 2017-02-21 23:35:57]

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

[commit-bot: I haz the power, 2017-02-21 23:35:59]

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

[Elliot Glaysher, 2017-02-21 23:41:36]

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

[commit-bot: I haz the power, 2017-02-21 23:43:17]

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

[Elliot Glaysher, 2017-02-22 00:14:46]

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

[commit-bot: I haz the power, 2017-02-22 00:16:24]

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

[Elliot Glaysher, 2017-02-22 00:33:26]

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

[commit-bot: I haz the power, 2017-02-22 00:35:09]

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, 2017-02-22 02:07:01]

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

[commit-bot: I haz the power, 2017-02-22 02:07:01]

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_...)

[Elliot Glaysher, 2017-02-22 20:23:29]

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

[commit-bot: I haz the power, 2017-02-22 20:24:21]

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, 2017-02-22 21:42:57]

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

[commit-bot: I haz the power, 2017-02-22 21:42:58]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-02-22 23:42:26]

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

[commit-bot: I haz the power, 2017-02-22 23:43:19]

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, 2017-02-23 01:30:47]

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

[commit-bot: I haz the power, 2017-02-23 01:30:48]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-02-23 18:48:45]

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

[commit-bot: I haz the power, 2017-02-23 18:49:16]

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

[Elliot Glaysher, 2017-02-23 18:50:32]

Description was changed from

==========
Validate incoming window properties.

BUG=654924
==========

to

==========
aura-mus: Validate incoming window properties.

This adds a validator function to the integral type version of
PropertyConverter::RegisterProperty() and rejects changes if the
incoming value, passed to the validator, returns false.

BUG=654924
==========

[Elliot Glaysher, 2017-02-23 18:50:33]

erg@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[Elliot Glaysher, 2017-02-23 18:51:20]

This validates those enums and rejects bad values, but it doesn't close the pipe
on a bad value. That's the next patch in this series.

[Tom Sepez, 2017-02-23 18:52:52]

lgtm

[commit-bot: I haz the power, 2017-02-23 19:58:46]

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

[commit-bot: I haz the power, 2017-02-23 19:58:47]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-23 21:02:24]

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

[sky, 2017-02-23 21:02:25]

+msw as he has worked on PropertyConverter.

https://codereview.chromium.org/2702423004/diff/120001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2702423004/diff/120001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:124: ASH_PUBLIC_EXPORT
base::RepeatingCallback<bool(int32_t)>
Why expose a callback here? I certainly get wanting a function like
IsValidShelfItemType(int32_t), but whether that's done using a callback seems
like something the consumers of this function should do.

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

https://codereview.chromium.org/2702423004/diff/120001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:37: static
base::RepeatingCallback<bool(int32_t)> AcceptAnyValue();
After constructor/destructor, and add a comment.
WDYT of CreateAcceptAnyValueCallback?

https://codereview.chromium.org/2702423004/diff/120001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:81: const
base::RepeatingCallback<bool(int32_t)>& validator) {
Document what the validator is, and mention using AcceptAnyValue. As this code
ends up using int64, should the validator take an int64?

https://codereview.chromium.org/2702423004/diff/120001/ui/wm/core/shadow_types.h
File ui/wm/core/shadow_types.h (right):

https://codereview.chromium.org/2702423004/diff/120001/ui/wm/core/shadow_type...
ui/wm/core/shadow_types.h:36: WM_EXPORT base::RepeatingCallback<bool(int32_t)>
ValidateShadowElevation();
Similar comment about exposing a function, not a callback.

[Elliot Glaysher, 2017-02-23 21:32:00]

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

[commit-bot: I haz the power, 2017-02-23 21:33:12]

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

[Elliot Glaysher, 2017-02-23 22:12:43]

https://codereview.chromium.org/2702423004/diff/120001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2702423004/diff/120001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:124: ASH_PUBLIC_EXPORT
base::RepeatingCallback<bool(int32_t)>
On 2017/02/23 21:02:25, sky wrote:
> Why expose a callback here? I certainly get wanting a function like
> IsValidShelfItemType(int32_t), but whether that's done using a callback seems
> like something the consumers of this function should do.

Done.

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

https://codereview.chromium.org/2702423004/diff/120001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:37: static
base::RepeatingCallback<bool(int32_t)> AcceptAnyValue();
On 2017/02/23 21:02:25, sky wrote:
> After constructor/destructor, and add a comment.
> WDYT of CreateAcceptAnyValueCallback?

Done.

https://codereview.chromium.org/2702423004/diff/120001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:81: const
base::RepeatingCallback<bool(int32_t)>& validator) {
On 2017/02/23 21:02:25, sky wrote:
> Document what the validator is, and mention using AcceptAnyValue. As this code
> ends up using int64, should the validator take an int64?

Commented, and you are right that all the int32_ts in this patch should be
int64_ts.

https://codereview.chromium.org/2702423004/diff/120001/ui/wm/core/shadow_types.h
File ui/wm/core/shadow_types.h (right):

https://codereview.chromium.org/2702423004/diff/120001/ui/wm/core/shadow_type...
ui/wm/core/shadow_types.h:36: WM_EXPORT base::RepeatingCallback<bool(int32_t)>
ValidateShadowElevation();
On 2017/02/23 21:02:25, sky wrote:
> Similar comment about exposing a function, not a callback.

Done.

[msw, 2017-02-23 22:35:12]

Thanks for working on this; I have some minor comments.

https://codereview.chromium.org/2702423004/diff/140001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2702423004/diff/140001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:122: ASH_PUBLIC_EXPORT bool
IsValidShelfItemType(int64_t i);
optional nit: |i| -> |type| or |value| (in comment and definition too)

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

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:36: // Resize behaviour is a 3 bitfield.
nit: consistent Behavior/behaviour spelling

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:37: return value >= 0 && value <= 7;
optional nit: use value >= 0 && value <= ui::mojom::kResizeBehaviorCanMaximize |
ui::mojom::kResizeBehaviorCanMinimize | ui::mojom::kResizeBehaviorCanResize;

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:41: return value ==
int64_t(ui::mojom::ShowState::DEFAULT) ||
It's a bummer that validating each enum requires listing all values.

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:69: CreateAcceptAnyValueCallback());
Should we have a special callback for validating bool values?

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:199: return;
We should probably log when invalid property values are encountered.

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:279: return false;
Ditto

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

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:78: // Register a property to support
conversion between mus and
nit: line wrapping

https://codereview.chromium.org/2702423004/diff/140001/ui/wm/core/shadow_types.h
File ui/wm/core/shadow_types.h (right):

https://codereview.chromium.org/2702423004/diff/140001/ui/wm/core/shadow_type...
ui/wm/core/shadow_types.h:33: // Returns true if |i| is a valid element of
ShadowElevation elements.
ditto optional nit: |type| or |value|

[Elliot Glaysher, 2017-02-23 22:57:26]

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

[Elliot Glaysher, 2017-02-23 22:57:35]

https://codereview.chromium.org/2702423004/diff/140001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2702423004/diff/140001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:122: ASH_PUBLIC_EXPORT bool
IsValidShelfItemType(int64_t i);
On 2017/02/23 22:35:11, msw wrote:
> optional nit: |i| -> |type| or |value| (in comment and definition too)

Done.

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

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:36: // Resize behaviour is a 3 bitfield.
On 2017/02/23 22:35:12, msw wrote:
> nit: consistent Behavior/behaviour spelling

Done.

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:37: return value >= 0 && value <= 7;
On 2017/02/23 22:35:12, msw wrote:
> optional nit: use value >= 0 && value <= ui::mojom::kResizeBehaviorCanMaximize
|
> ui::mojom::kResizeBehaviorCanMinimize | ui::mojom::kResizeBehaviorCanResize;

Done.

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:41: return value ==
int64_t(ui::mojom::ShowState::DEFAULT) ||
On 2017/02/23 22:35:12, msw wrote:
> It's a bummer that validating each enum requires listing all values.

Acknowledged.

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:69: CreateAcceptAnyValueCallback());
On 2017/02/23 22:35:11, msw wrote:
> Should we have a special callback for validating bool values?

Unsure; are there cases where a non-0, non-1 value is used as true?

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:199: return;
On 2017/02/23 22:35:12, msw wrote:
> We should probably log when invalid property values are encountered.

Done.

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:279: return false;
On 2017/02/23 22:35:12, msw wrote:
> Ditto

Done.

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

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.h:78: // Register a property to support
conversion between mus and
On 2017/02/23 22:35:12, msw wrote:
> nit: line wrapping

Weird that neither emacs nor cl-format correct this. cl-format doesn't undo the
manual reflow, so Done.

https://codereview.chromium.org/2702423004/diff/140001/ui/wm/core/shadow_types.h
File ui/wm/core/shadow_types.h (right):

https://codereview.chromium.org/2702423004/diff/140001/ui/wm/core/shadow_type...
ui/wm/core/shadow_types.h:33: // Returns true if |i| is a valid element of
ShadowElevation elements.
On 2017/02/23 22:35:12, msw wrote:
> ditto optional nit: |type| or |value|

Done.

[commit-bot: I haz the power, 2017-02-23 22:58:04]

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

[msw, 2017-02-23 23:22:35]

lgtm with a comment nit

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

https://codereview.chromium.org/2702423004/diff/140001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:69: CreateAcceptAnyValueCallback());
On 2017/02/23 22:57:35, Elliot Glaysher wrote:
> On 2017/02/23 22:35:11, msw wrote:
> > Should we have a special callback for validating bool values?
> 
> Unsure; are there cases where a non-0, non-1 value is used as true?

Yeah, perhaps there's no value in doing so... I was just pondering.

https://codereview.chromium.org/2702423004/diff/160001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2702423004/diff/160001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:121: // Returns true if |i| is a valid
ShelfItemType.
nit: i->type

[Elliot Glaysher, 2017-02-23 23:26:02]

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

[Elliot Glaysher, 2017-02-23 23:26:21]

https://codereview.chromium.org/2702423004/diff/160001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2702423004/diff/160001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:121: // Returns true if |i| is a valid
ShelfItemType.
On 2017/02/23 23:22:35, msw wrote:
> nit: i->type

Done.

[commit-bot: I haz the power, 2017-02-23 23:27:02]

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, 2017-02-24 02:43:46]

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

[commit-bot: I haz the power, 2017-02-24 02:43:47]

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_...)

[sky, 2017-02-24 04:09:42]

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

https://codereview.chromium.org/2702423004/diff/180001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:289: *value =
mojo::ConvertTo<PrimitiveType>(transport_data);
Isn't this line the same as 282? PrimitiveType is an int64_t, so I'm wondering
why we need both? Can line 289 move above the conditional and you use *value? Or
move 289 above the conditional but use a temp?

[Elliot Glaysher, 2017-02-24 18:47:41]

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

[commit-bot: I haz the power, 2017-02-24 18: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, 2017-02-24 19:10:15]

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

[commit-bot: I haz the power, 2017-02-24 19:10:17]

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

[Elliot Glaysher, 2017-02-24 20:11:18]

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

[commit-bot: I haz the power, 2017-02-24 20:12:01]

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

[Elliot Glaysher, 2017-02-24 20:50:43]

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

https://codereview.chromium.org/2702423004/diff/180001/ui/aura/mus/property_c...
ui/aura/mus/property_converter.cc:289: *value =
mojo::ConvertTo<PrimitiveType>(transport_data);
On 2017/02/24 04:09:42, sky wrote:
> Isn't this line the same as 282? PrimitiveType is an int64_t, so I'm wondering
> why we need both? Can line 289 move above the conditional and you use *value?
Or
> move 289 above the conditional but use a temp?

Removed duplicates here and in SetPropertyFromTransportValue

[sky, 2017-02-24 21:27:44]

LGTM

[Elliot Glaysher, 2017-02-24 21:28:31]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2017-02-24 21:28:36]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2017-02-24 21:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2702423004/#ps200001
(title: "sky comments")

[commit-bot: I haz the power, 2017-02-24 21:29:16]

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, 2017-02-24 23:24:17]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487971716345590,
"parent_rev": "60b01374443cd1e14598ed8fd554109c697a096f", "commit_rev":
"19e1e20024109653910be72bd7ccbcc866a47c26"}

[commit-bot: I haz the power, 2017-02-24 23:24:53]

Description was changed from

==========
aura-mus: Validate incoming window properties.

This adds a validator function to the integral type version of
PropertyConverter::RegisterProperty() and rejects changes if the
incoming value, passed to the validator, returns false.

BUG=654924
==========

to

==========
aura-mus: Validate incoming window properties.

This adds a validator function to the integral type version of
PropertyConverter::RegisterProperty() and rejects changes if the
incoming value, passed to the validator, returns false.

BUG=654924

Review-Url: https://codereview.chromium.org/2702423004
Cr-Commit-Position: refs/heads/master@{#452990}
Committed:
https://chromium.googlesource.com/chromium/src/+/19e1e20024109653910be72bd7cc...
==========

[commit-bot: I haz the power, 2017-02-24 23:24:54]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/19e1e20024109653910be72bd7cc...