Remove cached dp viewports in LayoutManager

Remove cached dp viewports in LayoutManager

BUG=662427
Committed: https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1
Cr-Commit-Position: refs/heads/master@{#433741}

[mdjones, 2016-11-14 21:52:31]

mdjones@chromium.org changed reviewers:
+ aelias@chromium.org, dtrainor@chromium.org

[mdjones, 2016-11-14 21:52:31]

This just removes the cached dp versions of the viewports in layout manager.
Locations that us dp convert from px on the fly. ptal

[aelias_OOO_until_Jul13, 2016-11-14 23:51:21]

Could you refrain from changing any types to int as part of this?  floats can
represent integers exactly at these magnitudes and using ints for geometry only
ever causes problems.  As one example, the position part of the rects may be
between pixels during an animation.

[mdjones, 2016-11-15 18:05:25]

On 2016/11/14 23:51:21, aelias wrote:
> Could you refrain from changing any types to int as part of this?  floats can
> represent integers exactly at these magnitudes and using ints for geometry
only
> ever causes problems.  As one example, the position part of the rects may be
> between pixels during an animation.

Do you mean both the primative types or the Rects as well? In any case, all of
this is only sizing information that starts off as ints anyway; the size of the
viewport from the CompositorViewHolder and the browser controls are both
computed in px. I'll still make the change if you want.

[aelias_OOO_until_Jul13, 2016-11-15 20:27:18]

On 2016/11/15 at 18:05:25, mdjones wrote:
> On 2016/11/14 23:51:21, aelias wrote:
> > Could you refrain from changing any types to int as part of this?  floats
can
> > represent integers exactly at these magnitudes and using ints for geometry
only
> > ever causes problems.  As one example, the position part of the rects may be
> > between pixels during an animation.
> 
> Do you mean both the primative types or the Rects as well? In any case, all of
this is only sizing information that starts off as ints anyway; the size of the
viewport from the CompositorViewHolder and the browser controls are both
computed in px. I'll still make the change if you want.

I mean both primitive types and rects.  In general I think it's a best practice
for all geometry to be float, with the sole exception of texture sizes.  That's
how OpenGL does it and that should carry though in the rest of our code.  If
there's a desire to pixel-snap at some point it can always be done as an
explicit floor()/ceil() call on floats, which more clearly expresses author
intent than an implicit cast anyway.  So if you want to uniformize the types I'd
prefer you change the starting point to be float-based as well.

[mdjones, 2016-11-15 22:24:47]

Switched to use float and RectF where relevant. ptal

[mdjones, 2016-11-18 17:24:54]

friendly ping!

[aelias_OOO_until_Jul13, 2016-11-18 19:35:20]

Sorry, forgot to hit publish on my draft comments...

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:681:
public int getOverlayTranslateY() {
float

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:152:
private final float mPxToDp;
Could you store this as mDpToPx and divide everywhere instead?  That means the
value would be the same as context.getResources().getDisplayMetrics().density
and deviceScaleFactor, helping have a bit of consistency (and possibly avoiding
floating point epsilon mismatch problems).

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:341:
public final void sizeChanged(RectF visibleViewport, RectF screenViewport,
visibleViewportPx, screenViewportPx

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:355:
mHeightMinusBrowserControls = heightMinusBrowserControls * mPxToDp;
mHeightMinusBrowserControlsDp

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
(right):

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:92:
private final Point mCachedPoint = new Point();
PointF

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:183:
mCachedPoint.set((int) -mCachedRect.left, (int) -mCachedRect.top);
No need for (int) here if you use PointF

[mdjones, 2016-11-18 21:37:41]

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:681:
public int getOverlayTranslateY() {
On 2016/11/18 19:35:20, aelias wrote:
> float

Done.

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:152:
private final float mPxToDp;
On 2016/11/18 19:35:20, aelias wrote:
> Could you store this as mDpToPx and divide everywhere instead?  That means the
> value would be the same as context.getResources().getDisplayMetrics().density
> and deviceScaleFactor, helping have a bit of consistency (and possibly
avoiding
> floating point epsilon mismatch problems).

Done.

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:341:
public final void sizeChanged(RectF visibleViewport, RectF screenViewport,
On 2016/11/18 19:35:20, aelias wrote:
> visibleViewportPx, screenViewportPx

Done.

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:355:
mHeightMinusBrowserControls = heightMinusBrowserControls * mPxToDp;
On 2016/11/18 19:35:20, aelias wrote:
> mHeightMinusBrowserControlsDp

Done.

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
(right):

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:92:
private final Point mCachedPoint = new Point();
On 2016/11/18 19:35:20, aelias wrote:
> PointF

Done.

https://codereview.chromium.org/2499733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:183:
mCachedPoint.set((int) -mCachedRect.left, (int) -mCachedRect.top);
On 2016/11/18 19:35:20, aelias wrote:
> No need for (int) here if you use PointF

Done.

[aelias_OOO_until_Jul13, 2016-11-18 22:01:20]

lgtm

[David Trainor- moved to gerrit, 2016-11-21 20:19:31]

lgtm

https://codereview.chromium.org/2499733002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2499733002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:567:
* @return The width of the drawing area.
" in dp."  Same with getHeight?

[mdjones, 2016-11-21 21:12:18]

https://codereview.chromium.org/2499733002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2499733002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:567:
* @return The width of the drawing area.
On 2016/11/21 20:19:31, David Trainor wrote:
> " in dp."  Same with getHeight?

Done.

[mdjones, 2016-11-21 21:12:31]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-21 21:12:31]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2499733002/#ps120001
(title: "update doc")

[commit-bot: I haz the power, 2016-11-21 21:12:58]

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, 2016-11-21 23:03:27]

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

[commit-bot: I haz the power, 2016-11-21 23:03:29]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[mdjones, 2016-11-21 23:28:38]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2016-11-21 23:29:20]

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, 2016-11-22 00:54:15]

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

[commit-bot: I haz the power, 2016-11-22 00:54:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[mdjones, 2016-11-22 01:34:36]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-22 01:34:37]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2499733002/#ps140001
(title: "use Float.compare")

[commit-bot: I haz the power, 2016-11-22 01:35:08]

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, 2016-11-22 02:07:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1479778476678730,
"parent_rev": "4f3ba54976e02efbf1162c40045a0d32a49df825", "commit_rev":
"708b174491c869a7085c0909cd1c66af85cc5db1"}

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

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-22 02:12:22]

Description was changed from

==========
Remove cached dp viewports in LayoutManager

BUG=662427
==========

to

==========
Remove cached dp viewports in LayoutManager

BUG=662427

Committed: https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1
Cr-Commit-Position: refs/heads/master@{#433741}
==========

[commit-bot: I haz the power, 2016-11-22 02:12:23]

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