Center the focus of the text input in magnifier screen.

Center the focus of the text input in magnifier screen when this feature is enabled in settings for magnifier mode.

BUG=446270
Committed: https://crrev.com/b894b6df6feee2b1126a3f1fdec3d44c82a15564
Cr-Commit-Position: refs/heads/master@{#332254}

[jennyz, 2015-05-28 21:55:25]

jennyz@chromium.org changed reviewers:
+ oshima@chromium.org

[jennyz, 2015-05-28 21:58:20]

jennyz@chromium.org changed reviewers:
+ stevenjb@chromium.org

[jennyz, 2015-05-28 21:59:41]

stevenjb, please review the files under:
chrome/browser/resources/options/
chrome/browser/ui/webui/options/

oshima, please review the rest of them for magnifier code.

[stevenjb, 2015-05-28 22:39:23]

stevenjb@chromium.org changed reviewers:
+ jlklein@chromium.org

[stevenjb, 2015-05-28 22:39:24]

lgtm
+jlklien@ FYI

[oshima, 2015-05-28 22:41:35]

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

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:135: bool Redraw(const gfx::PointF&
position, float scale, bool animate);
nit: new line

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:740: int y = window_rect.y() +
point.y() - window_rect.CenterPoint().y();
window_rect.origin() + point - window_rect.CenterPoint() ?

and move this inside if block.

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:836: this,
&MagnificationControllerImpl::OnMoveMagnifierTimer);
Am I correct that this depends on the timing and may not be deterministic?

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

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller_unittest.cc:74: }
Can you comment why this is necessary?

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller_unittest.cc:79: if
(!controller_->IsOnAnimationForTesting()) {
Animation is turned off in unittest by default.
(https://code.google.com/p/chromium/codesearch#chromium/src/ash/test/ash_test_...)

Is it enabled in this test?

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller_unittest.cc:81: }
nit: nuke{}

https://codereview.chromium.org/1157553003/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/1157553003/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/magnification_manager.cc:213:
SetMagniferKeepFocusCenteredInternal(keep_focus_centered);
can you keep the (reverse) order?

[jennyz, 2015-05-29 21:41:24]

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

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:135: bool Redraw(const gfx::PointF&
position, float scale, bool animate);
On 2015/05/28 22:41:35, oshima wrote:
> nit: new line

Done.

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:740: int y = window_rect.y() +
point.y() - window_rect.CenterPoint().y();
On 2015/05/28 22:41:35, oshima wrote:
> window_rect.origin() + point - window_rect.CenterPoint() ?
> 
> and move this inside if block.

Done.

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:836: this,
&MagnificationControllerImpl::OnMoveMagnifierTimer);
On 2015/05/28 22:41:35, oshima wrote:
> Am I correct that this depends on the timing and may not be deterministic?

Well, I guess it depends on how gmail code is implemented for this part? But
this must be done programatically, which should be really fast. So I just added
a little delay, and tested on link, it works well. On a slow machine, will this
become a problem? I have not tested it. Do you have any suggestion for other
approach which could be more deterministic?

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

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller_unittest.cc:74: }
On 2015/05/28 22:41:35, oshima wrote:
> Can you comment why this is necessary?

Removed this class.

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller_unittest.cc:79: if
(!controller_->IsOnAnimationForTesting()) {
On 2015/05/28 22:41:35, oshima wrote:
> Animation is turned off in unittest by default.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ash/test/ash_test_...)
> 
> Is it enabled in this test?

Good catch. I am not aware the animation is disabled for unittest. Great, so I
removed the MagnifierAnimationWaiterClass now, instead, just call
base::RunLoop().RunUntilIdle().

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller_unittest.cc:81: }
On 2015/05/28 22:41:35, oshima wrote:
> nit: nuke{}

Acknowledged.

https://codereview.chromium.org/1157553003/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/1157553003/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/magnification_manager.cc:213:
SetMagniferKeepFocusCenteredInternal(keep_focus_centered);
On 2015/05/28 22:41:35, oshima wrote:
> can you keep the (reverse) order?

Done.

[oshima, 2015-05-29 22:34:26]

lgtm with nits.

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

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:836: this,
&MagnificationControllerImpl::OnMoveMagnifierTimer);
On 2015/05/29 21:41:23, jennyz wrote:
> On 2015/05/28 22:41:35, oshima wrote:
> > Am I correct that this depends on the timing and may not be deterministic?
> 
> Well, I guess it depends on how gmail code is implemented for this part? But
> this must be done programatically, which should be really fast. So I just
added
> a little delay, and tested on link, it works well. On a slow machine, will
this
> become a problem? I have not tested it. Do you have any suggestion for other
> approach which could be more deterministic? 

I just wanted to double check if my understanding is correct. If gmail modifies
the text, which in turn causes the visual effect here, then there isn't much we
can do. I probably would increase a bit more (like 15~30 ms which roughly fits
within 2 frames at most) just in case though. I'll leave it to you.

https://codereview.chromium.org/1157553003/diff/20001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller_unittest.cc (right):

https://codereview.chromium.org/1157553003/diff/20001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller_unittest.cc:621:
base::RunLoop().RunUntilIdle();
nit: you can just call RunAllPendingInMessageLoop() and remove run_loop.h. (we
probably should rename it to shorter name)

[jennyz, 2015-05-30 00:12:58]

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

https://codereview.chromium.org/1157553003/diff/1/ash/magnifier/magnification...
ash/magnifier/magnification_controller.cc:836: this,
&MagnificationControllerImpl::OnMoveMagnifierTimer);
On 2015/05/29 22:34:25, oshima wrote:
> On 2015/05/29 21:41:23, jennyz wrote:
> > On 2015/05/28 22:41:35, oshima wrote:
> > > Am I correct that this depends on the timing and may not be deterministic?
> > 
> > Well, I guess it depends on how gmail code is implemented for this part? But
> > this must be done programatically, which should be really fast. So I just
> added
> > a little delay, and tested on link, it works well. On a slow machine, will
> this
> > become a problem? I have not tested it. Do you have any suggestion for other
> > approach which could be more deterministic? 
> 
> I just wanted to double check if my understanding is correct. If gmail
modifies
> the text, which in turn causes the visual effect here, then there isn't much
we
> can do. I probably would increase a bit more (like 15~30 ms which roughly fits
> within 2 frames at most) just in case though. I'll leave it to you.

Done.

https://codereview.chromium.org/1157553003/diff/20001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller_unittest.cc (right):

https://codereview.chromium.org/1157553003/diff/20001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller_unittest.cc:621:
base::RunLoop().RunUntilIdle();
On 2015/05/29 22:34:26, oshima wrote:
> nit: you can just call RunAllPendingInMessageLoop() and remove run_loop.h. (we
> probably should rename it to shorter name)

Done.

[jennyz, 2015-05-30 00:13:11]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-05-30 00:13:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1157553003/#ps40001
(title: "Fix nit and disable the delay for moving magnifier window in testing
mode.")

[commit-bot: I haz the power, 2015-05-30 00:13:49]

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

[commit-bot: I haz the power, 2015-05-30 00:48:38]

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

[commit-bot: I haz the power, 2015-05-30 00:48:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jennyz, 2015-06-01 17:14:49]

jennyz@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-06-01 17:18:18]

You need to write descriptions of your actions and put an owner on them.

--mark

[jennyz, 2015-06-01 17:54:13]

On 2015/06/01 17:18:18, Mark P (out sick) wrote:
> You need to write descriptions of your actions and put an owner on them.
> 
> --mark

Woops, added now. Please take a look actions.xml again. The rest of changes are
all from rebase.

[Mark P, 2015-06-01 18:30:00]

actions.xml lgtm

https://codereview.chromium.org/1157553003/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1157553003/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:8551: Settings: Accessibility: Enable
centering focus for text input in  magnifier
nit: extra space

[jennyz, 2015-06-01 18:36:26]

https://codereview.chromium.org/1157553003/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1157553003/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:8551: Settings: Accessibility: Enable
centering focus for text input in  magnifier
On 2015/06/01 18:30:00, Mark P (out sick) wrote:
> nit: extra space

Done.

[jennyz, 2015-06-01 18:36:31]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-06-01 18:36:32]

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

[commit-bot: I haz the power, 2015-06-01 18:41:15]

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

[commit-bot: I haz the power, 2015-06-01 20:12:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-01 20:13:20]

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