Fix size estimation for prerender in custom tabs

Fix size estimation for prerender in custom tabs

- Fix estimated size for prerender to not subtract the navigation bar size which is
actually included in the frameSize in Blink.
- Fix estimated size to send dp rather than pixel values.
- Cache the prerendered size (in pixels and including control container height) so
that we set the contentviewcore size before swap to avoid any unnecessary resizes.

BUG=525229
Committed: https://crrev.com/a7d2d881a00e5c9f9dcaa214e76e886964c91128
Cr-Commit-Position: refs/heads/master@{#349249}

[Yusuf, 2015-09-15 22:40:07]

yusufo@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Ted C, 2015-09-16 00:36:49]

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:193:
getContentViewCore().onSizeChanged(mPrerenderedSize.x, mPrerenderedSize.y, 0,
0);
Take a look at:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

I don't know if that applies here, but at least something to consider to see if
this'll be plagued by the same things as that.

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:195:
}
add a blank line after this

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:702:
private Point estimateContentSize(boolean forPrerenderedContents) {
can you add a description for this @param, I'm not sure why the values/logic
would be different based on this.

And prerendered seems to be in DP while non prerendered is in pixels...for some
reason.

[Yusuf, 2015-09-16 04:57:48]

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:193:
getContentViewCore().onSizeChanged(mPrerenderedSize.x, mPrerenderedSize.y, 0,
0);
On 2015/09/16 00:36:49, Ted C wrote:
> Take a look at:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> I don't know if that applies here, but at least something to consider to see
if
> this'll be plagued by the same things as that.

Seems like this is already called for the swapping case. ie when we swap the
compositor is initialized with the right contentviewcore and the container view
size is updated. Do we have any concerns beyond that?

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:195:
}
On 2015/09/16 00:36:49, Ted C wrote:
> add a blank line after this

Done.

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:702:
private Point estimateContentSize(boolean forPrerenderedContents) {
On 2015/09/16 00:36:49, Ted C wrote:
> can you add a description for this @param, I'm not sure why the values/logic
> would be different based on this.
> 
> And prerendered seems to be in DP while non prerendered is in pixels...for
some
> reason.

Done. Yes the web contents needs the size in dp and contentviewcore in pixels.

[Ted C, 2015-09-16 18:12:47]

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:193:
getContentViewCore().onSizeChanged(mPrerenderedSize.x, mPrerenderedSize.y, 0,
0);
On 2015/09/16 04:57:48, Yusuf wrote:
> On 2015/09/16 00:36:49, Ted C wrote:
> > Take a look at:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > 
> > I don't know if that applies here, but at least something to consider to see
> if
> > this'll be plagued by the same things as that.
> 
> Seems like this is already called for the swapping case. ie when we swap the
> compositor is initialized with the right contentviewcore and the container
view
> size is updated. Do we have any concerns beyond that?

Is the ContentViewCore attached to the view hierarchy at this point?  I think
the frightening comment in CompositorViewHolder makes me think this should be
calling measure/layout.  But if this is already attached to the view hierarchy,
then this is potentially even more dangerous since the size change could be
different from the view size.

I think you want to keep the view measured size of the ContentView the same as
the sized pushed to ContentViewCore.

https://codereview.chromium.org/1348473002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1348473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:691:
session, webContents, url, referrer, estimateContentSize(false), extras);
why is this false?  Isn't this for prerendered contents?

[Yusuf, 2015-09-16 20:54:43]

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1348473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:193:
getContentViewCore().onSizeChanged(mPrerenderedSize.x, mPrerenderedSize.y, 0,
0);
On 2015/09/16 18:12:47, Ted C wrote:
> On 2015/09/16 04:57:48, Yusuf wrote:
> > On 2015/09/16 00:36:49, Ted C wrote:
> > > Take a look at:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > > 
> > > I don't know if that applies here, but at least something to consider to
see
> > if
> > > this'll be plagued by the same things as that.
> > 
> > Seems like this is already called for the swapping case. ie when we swap the
> > compositor is initialized with the right contentviewcore and the container
> view
> > size is updated. Do we have any concerns beyond that?
> 
> Is the ContentViewCore attached to the view hierarchy at this point?  I think
> the frightening comment in CompositorViewHolder makes me think this should be
> calling measure/layout.  But if this is already attached to the view
hierarchy,
> then this is potentially even more dangerous since the size change could be
> different from the view size.
> 
> I think you want to keep the view measured size of the ContentView the same as
> the sized pushed to ContentViewCore.

OK. I hoped this would be a quick additional fix on top of the size estimation
but looks like more complicated then I had intended to be.

Since this is also for m46 and already fixes the two layout and zoom related
issues, I am removing the sizing of CVC stuff to investigate later.

Changed the estimation code to divide by density (which is the real cause of the
zoomed in bug) and kept it at that.

[Ted C, 2015-09-16 21:07:04]

lgtm

[Yusuf, 2015-09-16 21:09:49]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2015-09-16 21:11:37]

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

[commit-bot: I haz the power, 2015-09-16 22:12:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-16 22:13:35]

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