Explicitly coerce PostDispatchAction to uint32_t in DispatchEvent()

Explicitly coerce PostDispatchAction to uint32_t in DispatchEvent()

Intermingling enums with integer types leads to warnings on some compilers. Explicitly coercing an enum to an integer before returing it clears up the problem.

Broader changes (using constants instead of the enum, returning an enum instead of an integer type) were considered but deemed too sweeping.

BUG=424334
TEST=ui_unittests, events_unittests
R=sky@chromium.org,sadrul@chromium.org

Committed: https://crrev.com/ecbafd248eb7a2e5e735482261d14455a3fa61a5
Cr-Commit-Position: refs/heads/master@{#300588}

[Chris Masone, 2014-10-20 18:56:31]

Sky, how do you feel about using kOnstants here instead of an enum?

[sky, 2014-10-20 20:58:37]

https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
File ui/events/platform/platform_event_dispatcher.h (left):

https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
ui/events/platform/platform_event_dispatcher.h:17: POST_DISPATCH_NONE = 0x0,
I prefer the enum. It's more typical of how we do these sort of things in
Chrome.

[Chris Masone, 2014-10-20 21:05:55]

On 2014/10/20 20:58:37, sky wrote:
>
https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
> File ui/events/platform/platform_event_dispatcher.h (left):
> 
>
https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
> ui/events/platform/platform_event_dispatcher.h:17: POST_DISPATCH_NONE = 0x0,
> I prefer the enum. It's more typical of how we do these sort of things in
> Chrome.

It errors with -Wtype-limits, though, which is currently used by the default
CrOS toolchain. In the chromium-dev discussion of allowing 'enum class', there
was also some conversation about moving stuff like this away from enums, since
this code is not actually enumerating all possible values, but rather just
specifying some bitflags.

[sky, 2014-10-20 22:21:36]

Can you paste the error you're getting?

On Mon, Oct 20, 2014 at 2:05 PM,  <cmasone@chromium.org> wrote:
> On 2014/10/20 20:58:37, sky wrote:
>
>
https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
>>
>> File ui/events/platform/platform_event_dispatcher.h (left):
>
>
>
>
https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
>>
>> ui/events/platform/platform_event_dispatcher.h:17: POST_DISPATCH_NONE =
>> 0x0,
>> I prefer the enum. It's more typical of how we do these sort of things in
>> Chrome.
>
>
> It errors with -Wtype-limits, though, which is currently used by the default
> CrOS toolchain. In the chromium-dev discussion of allowing 'enum class',
> there
> was also some conversation about moving stuff like this away from enums,
> since
> this code is not actually enumerating all possible values, but rather just
> specifying some bitflags.
>
> https://codereview.chromium.org/666673005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Chris Masone, 2014-10-20 22:24:20]

It's in the linked bug:

../ui/wm/core/nested_accelerator_dispatcher_linux.cc: In member function
'virtual uint32_t
wm::NestedAcceleratorDispatcherLinux::DispatchEvent(XEvent* const&)':
../ui/wm/core/nested_accelerator_dispatcher_linux.cc:89:23: warning:
enumeral and non-enumeral type in conditional expression [enabled by
default]
                 : ui::POST_DISPATCH_PERFORM_DEFAULT;

I've got another patch that makes DispatchEvent return a
PostDispatchAction. In my searching, I didn't find anywhere that ORed
values together, so it seems safe. I may have missed something.

On Mon, Oct 20, 2014 at 3:21 PM, Scott Violet <sky@chromium.org> wrote:

> Can you paste the error you're getting?
>
> On Mon, Oct 20, 2014 at 2:05 PM,  <cmasone@chromium.org> wrote:
> > On 2014/10/20 20:58:37, sky wrote:
> >
> >
>
https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
> >>
> >> File ui/events/platform/platform_event_dispatcher.h (left):
> >
> >
> >
> >
>
https://codereview.chromium.org/666673005/diff/40001/ui/events/platform/platf...
> >>
> >> ui/events/platform/platform_event_dispatcher.h:17: POST_DISPATCH_NONE =
> >> 0x0,
> >> I prefer the enum. It's more typical of how we do these sort of things
> in
> >> Chrome.
> >
> >
> > It errors with -Wtype-limits, though, which is currently used by the
> default
> > CrOS toolchain. In the chromium-dev discussion of allowing 'enum class',
> > there
> > was also some conversation about moving stuff like this away from enums,
> > since
> > this code is not actually enumerating all possible values, but rather
> just
> > specifying some bitflags.
> >
> > https://codereview.chromium.org/666673005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-10-20 22:31:25]

On 2014/10/20 22:24:20, Chris Masone wrote:
> It's in the linked bug:
> 
> ../ui/wm/core/nested_accelerator_dispatcher_linux.cc: In member function
> 'virtual uint32_t
> wm::NestedAcceleratorDispatcherLinux::DispatchEvent(XEvent* const&)':
> ../ui/wm/core/nested_accelerator_dispatcher_linux.cc:89:23: warning:
> enumeral and non-enumeral type in conditional expression [enabled by
> default]
>                  : ui::POST_DISPATCH_PERFORM_DEFAULT;
> 

Would something like this work instead:

diff --git a/ui/wm/core/nested_accelerator_dispatcher_linux.cc
b/ui/wm/core/nested_accelerator_dispatcher_linux.cc
index ee5b587..16a4efb 100644
--- a/ui/wm/core/nested_accelerator_dispatcher_linux.cc
+++ b/ui/wm/core/nested_accelerator_dispatcher_linux.cc
@@ -85,8 +85,8 @@ class NestedAcceleratorDispatcherLinux : public
NestedAcceleratorDispatcher,
     }
     ui::PlatformEventDispatcher* prev = *restore_dispatcher_;
 
-    return prev ? prev->DispatchEvent(event)
-                : ui::POST_DISPATCH_PERFORM_DEFAULT;
+    uint32_t perform_default = ui::POST_DISPATCH_PERFORM_DEFAULT;
+    return prev ? prev->DispatchEvent(event) : perform_default;
   }
 
   scoped_ptr<ui::ScopedEventDispatcher> restore_dispatcher_;

[Chris Masone, 2014-10-20 22:34:05]

On 2014/10/20 22:31:25, sadrul (OOO until late Oct) wrote:
> On 2014/10/20 22:24:20, Chris Masone wrote:
> > It's in the linked bug:
> > 
> > ../ui/wm/core/nested_accelerator_dispatcher_linux.cc: In member function
> > 'virtual uint32_t
> > wm::NestedAcceleratorDispatcherLinux::DispatchEvent(XEvent* const&)':
> > ../ui/wm/core/nested_accelerator_dispatcher_linux.cc:89:23: warning:
> > enumeral and non-enumeral type in conditional expression [enabled by
> > default]
> >                  : ui::POST_DISPATCH_PERFORM_DEFAULT;
> > 
> 
> Would something like this work instead:
> 
> diff --git a/ui/wm/core/nested_accelerator_dispatcher_linux.cc
> b/ui/wm/core/nested_accelerator_dispatcher_linux.cc
> index ee5b587..16a4efb 100644
> --- a/ui/wm/core/nested_accelerator_dispatcher_linux.cc
> +++ b/ui/wm/core/nested_accelerator_dispatcher_linux.cc
> @@ -85,8 +85,8 @@ class NestedAcceleratorDispatcherLinux : public
> NestedAcceleratorDispatcher,
>      }
>      ui::PlatformEventDispatcher* prev = *restore_dispatcher_;
>  
> -    return prev ? prev->DispatchEvent(event)
> -                : ui::POST_DISPATCH_PERFORM_DEFAULT;
> +    uint32_t perform_default = ui::POST_DISPATCH_PERFORM_DEFAULT;
> +    return prev ? prev->DispatchEvent(event) : perform_default;
>    }
>  
>    scoped_ptr<ui::ScopedEventDispatcher> restore_dispatcher_;

Probably :-)

It'd certainly be a smaller change, but it felt lazy to just fix the local
problem without looking at whether using an enum for this is the best thing to
do anyhow. Is there anywhere in the code that actually composes these flags with
bitwise operations? I haven't seen any yet.

[Chris Masone, 2014-10-20 23:04:29]

cmasone@chromium.org changed reviewers:
+ sadrul@chromium.org

[Chris Masone, 2014-10-20 23:05:38]

Eh, ok. This is waaaaay simpler.

I'm happy to re-open consideration of changing to constants, or making the
method return an enum like I did in https://codereview.chromium.org/667823002

But I'm fine with the simple thing for now. WDYT?

[sky, 2014-10-20 23:10:37]

I like simple: LGTM

[Chris Masone, 2014-10-21 00:20:21]

The CQ bit was checked by cmasone@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:21:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666673005/60001

[commit-bot: I haz the power, 2014-10-21 01:34:39]

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

[commit-bot: I haz the power, 2014-10-21 01:34:40]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Chris Masone, 2014-10-21 22:44:51]

The CQ bit was checked by cmasone@chromium.org

[commit-bot: I haz the power, 2014-10-21 22:46:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666673005/60001

[commit-bot: I haz the power, 2014-10-22 00:55:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-22 04:04:23]

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