ScrollableArea::pixelsPerLine should return viewport pixels in use-zoom-for-dsf mode

ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf mode

BUG=485650
TEST=manual. Use arrow key to scroll on 2x device.

Committed: https://crrev.com/8269081fddb6ba6f281294562073c85a4ce59514
Cr-Commit-Position: refs/heads/master@{#375761}

[oshima, 2016-02-06 01:53:16]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-06 01:54:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/1

[commit-bot: I haz the power, 2016-02-06 02:36:26]

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

[commit-bot: I haz the power, 2016-02-06 02:36:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[oshima, 2016-02-08 18:32:38]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 18:34:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/20001

[oshima, 2016-02-08 18:45:19]

Patchset #1 (id:1) has been deleted

[oshima, 2016-02-08 19:39:08]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 19:42:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/40001

[commit-bot: I haz the power, 2016-02-08 20:12:46]

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

[commit-bot: I haz the power, 2016-02-08 20:12:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[oshima, 2016-02-08 21:28:57]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[oshima, 2016-02-08 21:29:03]

Patchset #2 (id:40001) has been deleted

[oshima, 2016-02-08 21:29:11]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-02-08 21:35:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/60001

[oshima, 2016-02-08 22:26:48]

Patchset #1 (id:60001) has been deleted

[oshima, 2016-02-08 22:26:52]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 22:41:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/80001

[oshima, 2016-02-08 23:54:47]

Description was changed from

==========
Arrow Key scroll

BUG=485650

Arrow key scroll
==========

to

==========
ScrollableArea::pixelsPerLine/pixelsSteps should return viewport pixels in
use-zoom-for-dsf mode

BUG=485650
==========

[oshima, 2016-02-08 23:54:47]

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

[oshima, 2016-02-09 00:01:55]

oshima@chromium.org changed reviewers:
- skobes@chromium.org

[oshima, 2016-02-09 00:02:08]

Description was changed from

==========
ScrollableArea::pixelsPerLine/pixelsSteps should return viewport pixels in
use-zoom-for-dsf mode

BUG=485650
==========

to

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
==========

[oshima, 2016-02-09 00:02:17]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-09 00:04:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/100001

[oshima, 2016-02-09 00:09:02]

Description was changed from

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
==========

to

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
TEST=manual. Use arrow key to scroll on 2x device.
==========

[oshima, 2016-02-09 02:05:01]

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

[oshima, 2016-02-09 02:05:02]

This is not for review yet. Can you look at my question below?

https://codereview.chromium.org/1672973002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1672973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:592: return 1;
Looks like this is DIP, but I'm not sure converting to viewport pixels is the
right thing here. skobes@, should I keep this as is or should I convert this as
well?

[commit-bot: I haz the power, 2016-02-09 02:14:37]

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

[commit-bot: I haz the power, 2016-02-09 02:14:38]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-02-09 18:36:06]

https://codereview.chromium.org/1672973002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1672973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:592: return 1;
On 2016/02/09 02:05:02, oshima wrote:
> Looks like this is DIP, but I'm not sure converting to viewport pixels is the
> right thing here. skobes@, should I keep this as is or should I convert this
as
> well?

As far as I know ScrollableArea only deals in DIPs, not physical pixels.  What
is the reason for this change?

[oshima, 2016-02-09 23:12:49]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[oshima, 2016-02-09 23:15:07]

PTAL. Based on your answer, I changed to scale in pixelStep to physical pixels
when zooming is used to scale the content for device scale factor .

https://codereview.chromium.org/1672973002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1672973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:592: return 1;
On 2016/02/09 18:36:06, skobes wrote:
> On 2016/02/09 02:05:02, oshima wrote:
> > Looks like this is DIP, but I'm not sure converting to viewport pixels is
the
> > right thing here. skobes@, should I keep this as is or should I convert this
> as
> > well?
> 
> As far as I know ScrollableArea only deals in DIPs, not physical pixels.  What
> is the reason for this change?

In use-zoom-for-dsf mode, the viewport pixel becomes DIP* device scale factor.
The arrow scroll for exmaple becomes 2x slower because it only moves half
without these changes.

[commit-bot: I haz the power, 2016-02-09 23:15:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/120001

[skobes, 2016-02-09 23:42:32]

https://codereview.chromium.org/1672973002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1672973002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:67: FloatRect
tmpRectInViewport = host->windowToViewport(FloatRect(1, 0, 0, 0));
Can we introduce a clearer API that doesn't use rects for one-dimensional unit
conversions?  Something like HostWindow::convertDIPToViewportPx(float) -> float?

[commit-bot: I haz the power, 2016-02-10 00:37:50]

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

[commit-bot: I haz the power, 2016-02-10 00:37:51]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-02-12 18:20:31]

Patchset #4 (id:140001) has been deleted

[oshima, 2016-02-12 18:53:31]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 18:54:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/160001

[commit-bot: I haz the power, 2016-02-12 20:24:02]

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

[commit-bot: I haz the power, 2016-02-12 20:24:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2016-02-13 00:42:04]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-13 00:43:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/160001

[commit-bot: I haz the power, 2016-02-13 01:19:20]

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

[commit-bot: I haz the power, 2016-02-13 01:19:20]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-02-16 06:09:53]

https://codereview.chromium.org/1672973002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1672973002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:67: FloatRect
tmpRectInViewport = host->windowToViewport(FloatRect(1, 0, 0, 0));
On 2016/02/09 23:42:32, skobes wrote:
> Can we introduce a clearer API that doesn't use rects for one-dimensional unit
> conversions?  Something like HostWindow::convertDIPToViewportPx(float) ->
float?

I was asked to either made this symmetric to viewportToWindow in
https://codereview.chromium.org/1603253003/ (or return scale factor, which I
didn't want because it's not correct in traditional mode). 

I changed the method to return scale var in HostWindow only because all user of
HostWindow are scalar. Please let me know what you think.

[skobes, 2016-02-16 18:03:12]

lgtm w/ nit

https://codereview.chromium.org/1672973002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/HostWindow.h (right):

https://codereview.chromium.org/1672973002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/HostWindow.h:53: // Converts the scalar value
from the window coordinates to the viewport scale.
Is it documented anywhere what window and viewport coordinates are?  If I
understand our chat discussion correctly it is

  window coordinates == DIPs
  viewport coordinates == (use-zoom-for-dsf ? physical pixels : DIPs)

but this was not at all clear to me before.  It would be good for the comment to
explain this or link to a doc.

[oshima, 2016-02-16 23:32:36]

On 2016/02/16 18:03:12, skobes wrote:
> lgtm w/ nit
> 
>
https://codereview.chromium.org/1672973002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/HostWindow.h (right):
> 
>
https://codereview.chromium.org/1672973002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/HostWindow.h:53: // Converts the scalar
value
> from the window coordinates to the viewport scale.
> Is it documented anywhere what window and viewport coordinates are?  If I
> understand our chat discussion correctly it is
> 
>   window coordinates == DIPs
>   viewport coordinates == (use-zoom-for-dsf ? physical pixels : DIPs)
> 
> but this was not at all clear to me before.  It would be good for the comment
to
> explain this or link to a doc.

I was going to update the
https://www.chromium.org/developers/design-documents/blink-coordinate-spaces
when this is completed (at least for chromeos), but I probably should do this
now, with schedule information.

I'll work on it this week.

[oshima, 2016-02-16 23:35:49]

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

[oshima, 2016-02-16 23:35:50]

pfeldman@ -> owners review

[pfeldman, 2016-02-16 23:39:03]

owner lgtm

[oshima, 2016-02-16 23:57:29]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2016-02-16 23:59:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672973002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672973002/160001

[commit-bot: I haz the power, 2016-02-17 02:20:47]

Description was changed from

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
TEST=manual. Use arrow key to scroll on 2x device.
==========

to

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
TEST=manual. Use arrow key to scroll on 2x device.
==========

[commit-bot: I haz the power, 2016-02-17 02:20:48]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-02-17 02:22:07]

Description was changed from

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
TEST=manual. Use arrow key to scroll on 2x device.
==========

to

==========
ScrollableArea::pixelsPerLine  should return viewport pixels in use-zoom-for-dsf
mode

BUG=485650
TEST=manual. Use arrow key to scroll on 2x device.

Committed: https://crrev.com/8269081fddb6ba6f281294562073c85a4ce59514
Cr-Commit-Position: refs/heads/master@{#375761}
==========

[commit-bot: I haz the power, 2016-02-17 02:22:08]

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