Add support for taking partial screenshot using touch

Add support for taking partial screenshot using touch

BUG=chromium:177857
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190738

[Jun Mukai, 2013-03-25 21:03:48]

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:205: if (current_touch_id_ != -1 &&
current_touch_id_ != event->touch_id())
So, this will ignore the second finger touch right?
That would be fine, but not so sure the expected behavior. When the user touch
the screen by two fingers, the user might expect to select the region by those
two fingers?
I'm okay to go ahead with this code, but please discuss about this later in the
crbug.

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:220: }
This is okay, but creating dummy mouse event doesn't look excellent. What about:
- extract the logic into other methods, for example
OnSelectionStarted()/OnSelectionChanged()/OnSelectionFinished(), which accepts
location only
- and call them both from Mouse event handlers and here

[sadrul, 2013-03-25 21:11:04]

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:202: void
PartialScreenshotView::OnTouchEvent(ui::TouchEvent* event) {
You should override OnGestureEvent, and use scroll-gesture events
(ET_GESTURE_SCROLL_BEGIN, SCROLL_UPDATE, and SCROLL_END/ET_SCROLL_FLING_START)
for this.

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:220: }
On 2013/03/25 21:03:48, Jun Mukai wrote:
> This is okay, but creating dummy mouse event doesn't look excellent. What
about:
> - extract the logic into other methods, for example
> OnSelectionStarted()/OnSelectionChanged()/OnSelectionFinished(), which accepts
> location only
> - and call them both from Mouse event handlers and here

+1. This is usually how it's done in other parts of the code.

[Yufeng Shen (Slow to review), 2013-03-25 22:03:34]

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:202: void
PartialScreenshotView::OnTouchEvent(ui::TouchEvent* event) {
On 2013/03/25 21:11:04, sadrul wrote:
> You should override OnGestureEvent, and use scroll-gesture events
> (ET_GESTURE_SCROLL_BEGIN, SCROLL_UPDATE, and SCROLL_END/ET_SCROLL_FLING_START)
> for this.

Done.

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:205: if (current_touch_id_ != -1 &&
current_touch_id_ != event->touch_id())
On 2013/03/25 21:03:48, Jun Mukai wrote:
> So, this will ignore the second finger touch right?
> That would be fine, but not so sure the expected behavior. When the user touch
> the screen by two fingers, the user might expect to select the region by those
> two fingers?
> I'm okay to go ahead with this code, but please discuss about this later in
the
> crbug.

I will put a note in the crbug.

https://codereview.chromium.org/13006010/diff/1/ash/wm/partial_screenshot_vie...
ash/wm/partial_screenshot_view.cc:220: }
On 2013/03/25 21:03:48, Jun Mukai wrote:
> This is okay, but creating dummy mouse event doesn't look excellent. What
about:
> - extract the logic into other methods, for example
> OnSelectionStarted()/OnSelectionChanged()/OnSelectionFinished(), which accepts
> location only
> - and call them both from Mouse event handlers and here

Done.

[sadrul, 2013-03-25 22:13:24]

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:216: case ui::ET_GESTURE_SCROLL_BEGIN:
Hm. On second thought: perhaps it makes more sense to do this on
ET_GESTURE_TAP_DOWN instead.

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
File ash/wm/partial_screenshot_view.h (right):

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.h:41: void OnSelectionStarted(gfx::Point
position);
const gfx::Point&

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.h:42: void OnSelectionChanged(gfx::Point
position);
ditto

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.h:53: // Overidden from EventHandler
.. from ui::EventHandler:

[Yufeng Shen (Slow to review), 2013-03-25 22:38:44]

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:216: case ui::ET_GESTURE_SCROLL_BEGIN:
On 2013/03/25 22:13:24, sadrul wrote:
> Hm. On second thought: perhaps it makes more sense to do this on
> ET_GESTURE_TAP_DOWN instead.

Done.

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
File ash/wm/partial_screenshot_view.h (right):

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.h:41: void OnSelectionStarted(gfx::Point
position);
On 2013/03/25 22:13:24, sadrul wrote:
> const gfx::Point&

Done.

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.h:42: void OnSelectionChanged(gfx::Point
position);
On 2013/03/25 22:13:24, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/13006010/diff/8001/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.h:53: // Overidden from EventHandler
On 2013/03/25 22:13:24, sadrul wrote:
> .. from ui::EventHandler:

Done.

[Jun Mukai, 2013-03-25 22:40:12]

lgtm

[sadrul, 2013-03-25 22:42:14]

LGTM

It looks like there are no current tests for the PartialScreenshotView. But it
would be good to add tests for both mouse and gesture cases.

Please add OWNERS for review.

https://codereview.chromium.org/13006010/diff/14001/ash/wm/partial_screenshot...
File ash/wm/partial_screenshot_view.h (right):

https://codereview.chromium.org/13006010/diff/14001/ash/wm/partial_screenshot...
ash/wm/partial_screenshot_view.h:53: // Overidden from ui::EventHandler
:

[Yufeng Shen (Slow to review), 2013-03-25 22:55:00]

added sky@ for OWNERS

https://codereview.chromium.org/13006010/diff/14001/ash/wm/partial_screenshot...
File ash/wm/partial_screenshot_view.h (right):

https://codereview.chromium.org/13006010/diff/14001/ash/wm/partial_screenshot...
ash/wm/partial_screenshot_view.h:53: // Overidden from ui::EventHandler
On 2013/03/25 22:42:14, sadrul wrote:
> :

Done.

[Yufeng Shen (Slow to review), 2013-03-25 22:59:21]

On 2013/03/25 22:42:14, sadrul wrote:
> LGTM
> 
> It looks like there are no current tests for the PartialScreenshotView. But it
> would be good to add tests for both mouse and gesture cases.
> 
> Please add OWNERS for review.
> 
>
https://codereview.chromium.org/13006010/diff/14001/ash/wm/partial_screenshot...
> File ash/wm/partial_screenshot_view.h (right):
> 
>
https://codereview.chromium.org/13006010/diff/14001/ash/wm/partial_screenshot...
> ash/wm/partial_screenshot_view.h:53: // Overidden from ui::EventHandler
> :

Will add tests in a separate CL.

[sky, 2013-03-25 23:42:29]

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:148: current_position_ = position;
Can this early out if is_dragging_ and current_position_ == position?

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:210: OnSelectionFinished();
What about if capture is lost?

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:214: gfx::Point position = event->location();
const gfx::Point&, although you only use this in two branches, maybe move it to
where needed (meaning 217 and 220).

[Yufeng Shen (Slow to review), 2013-03-26 01:24:03]

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
File ash/wm/partial_screenshot_view.cc (right):

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:148: current_position_ = position;
On 2013/03/25 23:42:29, sky wrote:
> Can this early out if is_dragging_ and current_position_ == position?

Done.

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:210: OnSelectionFinished();
On 2013/03/25 23:42:29, sky wrote:
> What about if capture is lost?

Added OnMouseCaptureLost() which basically cancels the screenshot.

https://codereview.chromium.org/13006010/diff/9003/ash/wm/partial_screenshot_...
ash/wm/partial_screenshot_view.cc:214: gfx::Point position = event->location();
On 2013/03/25 23:42:29, sky wrote:
> const gfx::Point&, although you only use this in two branches, maybe move it
to
> where needed (meaning 217 and 220).

Done.

[sky, 2013-03-26 14:43:35]

LGTM

https://codereview.chromium.org/13006010/diff/25001/ash/wm/partial_screenshot...
File ash/wm/partial_screenshot_view.h (right):

https://codereview.chromium.org/13006010/diff/25001/ash/wm/partial_screenshot...
ash/wm/partial_screenshot_view.h:54: // Overidden from ui::EventHandler:
Isn't this overriden from View too?

[Yufeng Shen (Slow to review), 2013-03-26 15:35:14]

https://codereview.chromium.org/13006010/diff/25001/ash/wm/partial_screenshot...
File ash/wm/partial_screenshot_view.h (right):

https://codereview.chromium.org/13006010/diff/25001/ash/wm/partial_screenshot...
ash/wm/partial_screenshot_view.h:54: // Overidden from ui::EventHandler:
On 2013/03/26 14:43:35, sky wrote:
> Isn't this overriden from View too?

right, removed.

[commit-bot: I haz the power, 2013-03-26 15:38:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/13006010/31001

[commit-bot: I haz the power, 2013-03-26 19:45:12]

Change committed as 190738