Remove visible viewport dependency in CompositorView

Remove visible viewport dependency in CompositorView

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

[mdjones, 2016-11-08 19:05:17]

Description was changed from

==========
Start removing visible viewport dependency in CompositorView

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

to

==========
Remove visible viewport dependency in CompositorView

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

[mdjones, 2016-11-08 19:07:07]

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

[mdjones, 2016-11-08 19:07:08]

ptal

[mdjones, 2016-11-08 19:07:44]

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

https://codereview.chromium.org/2482233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:279:
return mActiveLayout.getUpdatedSceneLayer(viewport, getViewportPixel(null),
layerTitleCache,
Think it is worth having a cached rect for this?

[aelias_OOO_until_Jul13, 2016-11-08 22:51:48]

Looks like a nice cleanup direction.

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

https://codereview.chromium.org/2482233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:279:
return mActiveLayout.getUpdatedSceneLayer(viewport, getViewportPixel(null),
layerTitleCache,
On 2016/11/08 at 19:07:43, mdjones wrote:
> Think it is worth having a cached rect for this?

Because of the "new Rect" inside getViewportPixel?  Would be nice to avoid, but
instead of adding state cruft for it, I'd prefer you fix the underlying problem,
which is that getUpdatedSceneLayer takes a Rect which apparently it doesn't use
for anything important (since it can be bogus empty rect with no harm done). 
Also, I'd change getViewportPixel() to crash if it's given a null pointer since
that default behavior doesn't seem like it should ever be used.

https://codereview.chromium.org/2482233002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2482233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:348:
// TODO(jscholler): change 1.0f to dpToPx once the native part is fully
supporting dp.
Actually, we are going the reverse direction, all platforms are moving to
physical pixels and nothing is moving over to DP.  See http://crbug.com/485650 .
 So please delete this TODO.

https://codereview.chromium.org/2482233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:349:
nativeSetLayoutBounds(mNativeCompositorView, 1.0f);
This is the only caller and the value is always 1.0f, so please delete the
dpToPixel argument.

[mdjones, 2016-11-08 23:57:59]

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

https://codereview.chromium.org/2482233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:279:
return mActiveLayout.getUpdatedSceneLayer(viewport, getViewportPixel(null),
layerTitleCache,
On 2016/11/08 22:51:47, aelias wrote:
> On 2016/11/08 at 19:07:43, mdjones wrote:
> > Think it is worth having a cached rect for this?
> 
> Because of the "new Rect" inside getViewportPixel?  Would be nice to avoid,
but
> instead of adding state cruft for it, I'd prefer you fix the underlying
problem,
> which is that getUpdatedSceneLayer takes a Rect which apparently it doesn't
use
> for anything important (since it can be bogus empty rect with no harm done). 
> Also, I'd change getViewportPixel() to crash if it's given a null pointer
since
> that default behavior doesn't seem like it should ever be used.

That rect is used by classes like StaticTabSceneLayer to size the background and
content. I can still modify the functions to crash if null is passed though.

https://codereview.chromium.org/2482233002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2482233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:348:
// TODO(jscholler): change 1.0f to dpToPx once the native part is fully
supporting dp.
On 2016/11/08 22:51:47, aelias wrote:
> Actually, we are going the reverse direction, all platforms are moving to
> physical pixels and nothing is moving over to DP.  See http://crbug.com/485650
.
>  So please delete this TODO.

Done.

https://codereview.chromium.org/2482233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:349:
nativeSetLayoutBounds(mNativeCompositorView, 1.0f);
On 2016/11/08 22:51:47, aelias wrote:
> This is the only caller and the value is always 1.0f, so please delete the
> dpToPixel argument.

Done.

[aelias_OOO_until_Jul13, 2016-11-09 00:20:20]

lgtm modulo comments

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

https://codereview.chromium.org/2482233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:279:
return mActiveLayout.getUpdatedSceneLayer(viewport, getViewportPixel(null),
layerTitleCache,
On 2016/11/08 at 23:57:59, mdjones wrote:
> That rect is used by classes like StaticTabSceneLayer to size the background
and content. I can still modify the functions to crash if null is passed though.

Yeah, OK, I misread getViewportPixel() to early-return there.  I guess then that
you can create a final static rect in this class just to avoid the new, and
remove the "new Rect" line in getViewportPixel() and make it return void to make
it a bit clearer.

https://codereview.chromium.org/2482233002/diff/60001/chrome/browser/android/...
File chrome/browser/android/compositor/compositor_view.cc (right):

https://codereview.chromium.org/2482233002/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/compositor_view.cc:167:
compositor_->setDeviceScaleFactor(1.0f);
Delete this line since it no longer belongs here, and CompositorImpl constructor
sets this to 1.

[mdjones, 2016-11-09 01:36:32]

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

https://codereview.chromium.org/2482233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java:279:
return mActiveLayout.getUpdatedSceneLayer(viewport, getViewportPixel(null),
layerTitleCache,
On 2016/11/09 00:20:20, aelias wrote:
> On 2016/11/08 at 23:57:59, mdjones wrote:
> > That rect is used by classes like StaticTabSceneLayer to size the background
> and content. I can still modify the functions to crash if null is passed
though.
> 
> Yeah, OK, I misread getViewportPixel() to early-return there.  I guess then
that
> you can create a final static rect in this class just to avoid the new, and
> remove the "new Rect" line in getViewportPixel() and make it return void to
make
> it a bit clearer.

Done. I also updated the dp version (thought this will be removed in the
future).

https://codereview.chromium.org/2482233002/diff/60001/chrome/browser/android/...
File chrome/browser/android/compositor/compositor_view.cc (right):

https://codereview.chromium.org/2482233002/diff/60001/chrome/browser/android/...
chrome/browser/android/compositor/compositor_view.cc:167:
compositor_->setDeviceScaleFactor(1.0f);
On 2016/11/09 00:20:20, aelias wrote:
> Delete this line since it no longer belongs here, and CompositorImpl
constructor
> sets this to 1.

Done.

[David Trainor- moved to gerrit, 2016-11-10 20:31:46]

lgtm thanks!

[mdjones, 2016-11-10 21:52:29]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-10 21:52:29]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2482233002/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2016-11-10 21:52:59]

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-10 22:22:11]

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

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

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

[mdjones, 2016-11-10 23:48:37]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-10 23:48: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/2482233002/#ps100001
(title: "remove unneeded override")

[commit-bot: I haz the power, 2016-11-10 23:49:22]

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-11 00:02:55]

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

[commit-bot: I haz the power, 2016-11-11 00:02:56]

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

[mdjones, 2016-11-11 01:25:38]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2016-11-11 01:26:14]

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-11 01:58:35]

Description was changed from

==========
Remove visible viewport dependency in CompositorView

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

to

==========
Remove visible viewport dependency in CompositorView

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

[commit-bot: I haz the power, 2016-11-11 01:58:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-11 02:14:17]

Description was changed from

==========
Remove visible viewport dependency in CompositorView

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

to

==========
Remove visible viewport dependency in CompositorView

BUG=662427

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

[commit-bot: I haz the power, 2016-11-11 02:14:18]

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