Initial patch set to implement improved touch support for screen magnification

Initial patch set to implement improved touch support for screen
magnification

BUG=670526
TEST=none

Review-Url: https://codereview.chromium.org/2641733002
Cr-Commit-Position: refs/heads/master@{#450259}
Committed: https://chromium.googlesource.com/chromium/src/+/1a5774e06f5d0a8794594e28a70e7c6b00106730

[yawano, 2017-01-18 06:12:49]

yawano@chromium.org changed reviewers:
+ yoshiki@chromium.org

[yawano, 2017-01-18 06:12:50]

yoshiki@: PTAL. Thank you! I'll ask owner reviews later.

[yoshiki, 2017-01-19 06:30:20]

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:716: } else if (details.type() ==
ui::ET_GESTURE_PINCH_BEGIN &&
As talked offline, could you check if this logic works when the number of
touched fingers are increased (or decreased) during pinch?

[yawano, 2017-01-19 06:45:56]

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:716: } else if (details.type() ==
ui::ET_GESTURE_PINCH_BEGIN &&
I've tested this and we get gesture end event and begin event when user increase
or decrease number of fingers. But it may be safer not to check number of touch
points for toggling the flag. I'll upload new patch set to fix this.

[yawano, 2017-01-19 06:55:35]

Description was changed from

==========
Initial patch set to implement improved touch support in screen
magnification

BUG=670526
TEST=none
==========

to

==========
Initial patch set to implement improved touch support in screen
magnification

BUG=670526
TEST=none
==========

[yawano, 2017-01-19 07:51:47]

Description was changed from

==========
Initial patch set to implement improved touch support in screen
magnification

BUG=670526
TEST=none
==========

to

==========
Initial patch set to implement improved touch support for screen
magnification

BUG=670526
TEST=none
==========

[yawano, 2017-01-19 07:53:02]

Description was changed from

==========
Initial patch set to implement improved touch support for screen
magnification

BUG=670526
TEST=none
==========

to

==========
Initial patch set to implement improved touch support for screen
magnification

BUG=670526
TEST=none
==========

[yawano, 2017-01-19 07:54:25]

Uploaded new patch set to address comment. PTAL. Thank you!

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:716: } else if (details.type() ==
ui::ET_GESTURE_PINCH_BEGIN &&
On 2017/01/19 06:45:55, yawano wrote:
> I've tested this and we get gesture end event and begin event when user
increase
> or decrease number of fingers. But it may be safer not to check number of
touch
> points for toggling the flag. I'll upload new patch set to fix this.

Done.

[yoshiki, 2017-01-20 05:26:34]

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:716: } else if (details.type() ==
ui::ET_GESTURE_PINCH_BEGIN &&
On 2017/01/19 07:54:25, yawano wrote:
> On 2017/01/19 06:45:55, yawano wrote:
> > I've tested this and we get gesture end event and begin event when user
> increase
> > or decrease number of fingers. But it may be safer not to check number of
> touch
> > points for toggling the flag. I'll upload new patch set to fix this.
> 
> Done.

If "we get gesture end event and begin event when user increase
or decrease number of fingers" is correct, I think we can remove the
"is_performing_pinch_gesture_" flag entirely, because the pinch uses 3 fingers
and scroll uses 2 fingers, right?

And could you add comments about the gesture event behavior to prevent feature
us from having same question like current us.

https://codereview.chromium.org/2641733002/diff/40001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/40001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.cc:733: point_of_interest_ =
event->root_location();
Should we move the mouse cursor to the point of interest here (or in
GESTURE_PINCH_END handler)? Otherwise the magnification window may jump suddenly
from here to the mouse cursor position just after user moves mouse, I think.

https://codereview.chromium.org/2641733002/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2641733002/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2184:
{"enable-touch-support-for-screen-magnification",
Optional: The feature name is "screen magnifier", so I think
"for-screen-magnifier" is better. But the decision is up to you.

[yawano, 2017-02-06 06:49:04]

PTAL. Thank you!

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:716: } else if (details.type() ==
ui::ET_GESTURE_PINCH_BEGIN &&
Removed is_performing_pinch_gesture_ flag. We can distinguish them by number of
fingers.

The logic is very simple now. If user is performing scroll with 2 fingers, we
will scroll the screen. If user is performing pinch gesture with 3 fingers, we
will change zoom level.

Do we still need to add comment about the behavior of gesture events, especially
about begin and end events? We no longer handle gesture begin and end events
here.

https://codereview.chromium.org/2641733002/diff/40001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/40001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.cc:733: point_of_interest_ =
event->root_location();
I don't think we need to do it. IIUC, the mouse cursor is managed in the actual
(physical) screen coordinate, i.e. the cursor will stick to the current physical
location on the screen even if we change zoom level or scroll.

https://codereview.chromium.org/2641733002/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2641733002/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2184:
{"enable-touch-support-for-screen-magnification",
On 2017/01/20 05:26:33, yoshiki wrote:
> Optional: The feature name is "screen magnifier", so I think
> "for-screen-magnifier" is better. But the decision is up to you.

Done.

[yoshiki, 2017-02-06 17:22:07]

lgtm

https://codereview.chromium.org/2641733002/diff/40001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/40001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.cc:733: point_of_interest_ =
event->root_location();
On 2017/02/06 06:49:04, yawano wrote:
> I don't think we need to do it. IIUC, the mouse cursor is managed in the
actual
> (physical) screen coordinate, i.e. the cursor will stick to the current
physical
> location on the screen even if we change zoom level or scroll.

Thank you for explanation. Then, ok.

[yawano, 2017-02-07 00:18:57]

Thank you!

[yawano, 2017-02-07 00:24:38]

yawano@chromium.org changed reviewers:
+ isherman@chromium.org, oshima@chromium.org

[yawano, 2017-02-07 00:24:39]

PTAL.

isherman@: tools/metrics/histograms/histograms.xml
oshima@: the rest of files

Thank you!

[Ilya Sherman, 2017-02-07 01:41:39]

histograms.xml lgtm

[oshima, 2017-02-09 07:15:45]

lgtm with nits

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:706:
viewport_rect_in_dip.Offset(details.scroll_x() * -1,
- details.scroll_x()

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:708: const gfx::Rect&
viewport_rect_in_pixel =
IIRC, it returns a object, so it shouldn't be reference.

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:716: if (scale <
kMinMagnifiedScaleThreshold) {
or min/max to up you tho.

[yawano, 2017-02-10 05:32:47]

Thank you!

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:706:
viewport_rect_in_dip.Offset(details.scroll_x() * -1,
On 2017/02/09 07:15:44, oshima wrote:
> - details.scroll_x()

Done.

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:708: const gfx::Rect&
viewport_rect_in_pixel =
On 2017/02/09 07:15:44, oshima wrote:
> IIRC, it returns a object, so it shouldn't be reference.

Done.

https://codereview.chromium.org/2641733002/diff/100001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:716: if (scale <
kMinMagnifiedScaleThreshold) {
On 2017/02/09 07:15:45, oshima wrote:
> or min/max to up you tho.

Done.

[yawano, 2017-02-14 01:14:21]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-02-14 01:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, yoshiki@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2641733002/#ps120001
(title: "Fix nits.")

[commit-bot: I haz the power, 2017-02-14 01:15:02]

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-14 01:18:34]

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

[commit-bot: I haz the power, 2017-02-14 01:18:35]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[yawano, 2017-02-14 03:59:23]

Rebased and formatted by git cl format with Patch Set 8 and 9.

[yawano, 2017-02-14 04:00:06]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-02-14 04:00:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, yoshiki@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2641733002/#ps160001
(title: "Run git cl format.")

[commit-bot: I haz the power, 2017-02-14 04:01:11]

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-14 05:17:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487044806476590,
"parent_rev": "cdb17900987210db6248a3163f25341ba72daa84", "commit_rev":
"1a5774e06f5d0a8794594e28a70e7c6b00106730"}

[commit-bot: I haz the power, 2017-02-14 05:18:19]

Description was changed from

==========
Initial patch set to implement improved touch support for screen
magnification

BUG=670526
TEST=none
==========

to

==========
Initial patch set to implement improved touch support for screen
magnification

BUG=670526
TEST=none

Review-Url: https://codereview.chromium.org/2641733002
Cr-Commit-Position: refs/heads/master@{#450259}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a5774e06f5d0a8794594e28a70e...
==========

[commit-bot: I haz the power, 2017-02-14 05:18:20]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1a5774e06f5d0a8794594e28a70e...