Adding four bezel gestures

Adding four bezel gestures


BUG=126061, 126060, 123167
TEST=Unit test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137968

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138000

[Mr4D (OOO till 08-26), 2012-05-10 15:26:14]

Actually I also added in the footwork for the fifth gesture, but I am waiting on
the decision towards what it will do in the end.

Please have a look!

[sky, 2012-05-11 17:00:01]

http://codereview.chromium.org/10383108/diff/3001/ash/test/test_launcher_dele...
File ash/test/test_launcher_delegate.cc (right):

http://codereview.chromium.org/10383108/diff/3001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:33: aura::Window* window, LauncherItemStatus
status) {
nit: wrap status to next line.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.h (right):

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:21: typedef enum {
use C++ style, eg enum BezelStart ... Also, the values should be UPPER_CASE.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:55: const int kOverlapPercent;
overlap_percent_

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:71: bool
HandleDeviceControl(aura::GestureEvent* event);
methods before fields.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:6: #include
"ash/accelerators/accelerator_controller.h"
newline between 5 and 6.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:42: private:
newline between 41/42 and DISALLOW_COPY_AND...

http://codereview.chromium.org/10383108/diff/3001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/volume_controller_chromeos.h (right):

http://codereview.chromium.org/10383108/diff/3001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos.h:24: 
remove newline.

[Mr4D (OOO till 08-26), 2012-05-11 18:09:59]

Addressed. Have another look!

http://codereview.chromium.org/10383108/diff/3001/ash/test/test_launcher_dele...
File ash/test/test_launcher_delegate.cc (right):

http://codereview.chromium.org/10383108/diff/3001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:33: aura::Window* window, LauncherItemStatus
status) {
On 2012/05/11 17:00:01, sky wrote:
> nit: wrap status to next line.

Done.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.h (right):

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:21: typedef enum {
On 2012/05/11 17:00:01, sky wrote:
> use C++ style, eg enum BezelStart ... Also, the values should be UPPER_CASE.

Done.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:55: const int kOverlapPercent;
On 2012/05/11 17:00:01, sky wrote:
> overlap_percent_

Done.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:71: bool
HandleDeviceControl(aura::GestureEvent* event);
On 2012/05/11 17:00:01, sky wrote:
> methods before fields.

Done.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:6: #include
"ash/accelerators/accelerator_controller.h"
On 2012/05/11 17:00:01, sky wrote:
> newline between 5 and 6.

Done.

http://codereview.chromium.org/10383108/diff/3001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:42: private:
On 2012/05/11 17:00:01, sky wrote:
> newline between 41/42 and DISALLOW_COPY_AND...

Done.

http://codereview.chromium.org/10383108/diff/3001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/volume_controller_chromeos.h (right):

http://codereview.chromium.org/10383108/diff/3001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos.h:24: 
On 2012/05/11 17:00:01, sky wrote:
> remove newline.

Done.

[sadrul, 2012-05-11 22:49:33]

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.cc (right):

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:54: if (start_location_ ==
BEZEL_START_UNSET) {
Should this be a CHECK that start_location_ == BEZEL_START_UNSET?

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:85: else
braces

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.h (right):

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:55: bool
HandleDeviceControl(aura::GestureEvent* event);
I wonder if it makes sense to move these into a separate class owned by
SystemGestureEventFilter, e.g. BezelGestureHandler (or some such). I am changing
my patch to have a similar SystemPinchHandler to handle some gestures. That way,
the different kinds of system-level gesture handling code will be isolated and
easier to maintain.

[Mr4D (OOO till 08-26), 2012-05-13 23:19:05]

Can you please have a look again? Thanks!

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.cc (right):

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:54: if (start_location_ ==
BEZEL_START_UNSET) {
I don't know what you mean by that. It is a check - and it might come in there
when it is already assigned. - in which case the check would fail. So no DCHECK.

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:85: else
I was told to not use braces for a single line. Am I missing something?

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.h (right):

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.h:55: bool
HandleDeviceControl(aura::GestureEvent* event);
Well - I was thinking about various ways to do it. But in the end I thought that
there will be nothing added to the bezel code since there isn't much more
possible to do with the few pixels at the border. (without over convoluting it).
That said - if you want me to change it - let me know and I will change it once
I am back from the conference next week (or somewhere in between).

[sadrul, 2012-05-14 14:41:43]

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.cc (right):

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:54: if (start_location_ ==
BEZEL_START_UNSET) {
On 2012/05/13 23:19:05, Mr4D wrote:
> I don't know what you mean by that. It is a check - and it might come in there
> when it is already assigned. - in which case the check would fail. So no
DCHECK.

Does that imply we can get a scroll-begin, and then another scroll-begin before
a scroll-end? That should not happen, and as I understand this code, a DCHECK
here would catch cases where this does happen.

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:69: touch_id_ = event->GetLowestTouchId();
It's not clear to me what touch_id_ is used for. Why is it necessary to keep
track of the touch-id?

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:85: else
On 2012/05/13 23:19:05, Mr4D wrote:
> I was told to not use braces for a single line. Am I missing something?

My understanding is 'a single physical line' as opposed to 'a single statement'.

[Mr4D (OOO till 08-26), 2012-05-15 00:22:01]

Addressed! Please have another look!

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter.cc (right):

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:54: if (start_location_ ==
BEZEL_START_UNSET) {
Ahhh. Okay. I was under the impression that it would be possible to get two
gestures at a time. Did even create a unit test to cover this case. In that case
everything gets much simpler. Done.

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:69: touch_id_ = event->GetLowestTouchId();
Ditto. Done.

http://codereview.chromium.org/10383108/diff/7001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter.cc:85: else
I don't want to argue about brackets. You want some - you get some. :)

[sadrul, 2012-05-18 18:34:24]

LGTM (want moar braces)

http://codereview.chromium.org/10383108/diff/15001/ash/wm/system_gesture_even...
File ash/wm/system_gesture_event_filter.cc (right):

http://codereview.chromium.org/10383108/diff/15001/ash/wm/system_gesture_even...
ash/wm/system_gesture_event_filter.cc:79: else {
'if' should also have braces!

[Mr4D (OOO till 08-26), 2012-05-18 18:40:22]

Done! Thanks!

http://codereview.chromium.org/10383108/diff/15001/ash/wm/system_gesture_even...
File ash/wm/system_gesture_event_filter.cc (right):

http://codereview.chromium.org/10383108/diff/15001/ash/wm/system_gesture_even...
ash/wm/system_gesture_event_filter.cc:79: else {
On 2012/05/18 18:34:24, sadrul wrote:
> 'if' should also have braces!

Done.

[sky, 2012-05-18 19:49:30]

LGTM

http://codereview.chromium.org/10383108/diff/18002/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller_unittest.cc (left):

http://codereview.chromium.org/10383108/diff/18002/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller_unittest.cc:129:
handle_volume_up_count_(0) {
nit: leave the space here.

[Mr4D (OOO till 08-26), 2012-05-18 20:30:04]

Done. Thanks!

http://codereview.chromium.org/10383108/diff/18002/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller_unittest.cc (left):

http://codereview.chromium.org/10383108/diff/18002/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller_unittest.cc:129:
handle_volume_up_count_(0) {
On 2012/05/18 19:49:31, sky wrote:
> nit: leave the space here.

Done.

[commit-bot: I haz the power, 2012-05-18 20:30:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10383108/4022

[commit-bot: I haz the power, 2012-05-18 21:48:41]

Change committed as 137968

[commit-bot: I haz the power, 2012-05-18 23:35:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10383108/1017

[commit-bot: I haz the power, 2012-05-19 00:59:45]

Change committed as 138000