[android_webview] Fixes for wrap_content layouts.

[android_webview] Fixes for wrap_content layouts.

This changes AwContents to work correctly when in a WRAP_CONTENT
layout mode.

TBR=danakj@chromium.org
BUG=246621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223386

[mkosiba (inactive), 2013-09-05 18:28:23]

This patch depends on Blink changes (https://codereview.chromium.org/23441020/)
which are still pending/subject to change, so no hurry with this one.

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:241:
int fixedLayoutOverrideHeight = FIXED_LAYOUT_HEIGHT;
this might be 'controversial' thing about the CL - the Classic WebView would use
0 here, but that doesn't seem to work that well with Blink.

[aelias_OOO_until_Jul13, 2013-09-06 02:01:35]

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:241:
int fixedLayoutOverrideHeight = FIXED_LAYOUT_HEIGHT;
On 2013/09/05 18:28:24, mkosiba wrote:
> this might be 'controversial' thing about the CL - the Classic WebView would
use
> 0 here, but that doesn't seem to work that well with Blink.

I don't mind as long as it works in practice for every app we test.

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.h:358: gfx::Size
viewport_dip_size_override_;
I don't like this and it shouldn't be needed.  The intended design with respect
to rounding is that the calculation in GetViewportSizeDip() gives the canonical
width in DIP pixels and every other part of Chrome should use the value produced
there.  By having nobody do the rounding anywhere else, it eliminates the
possibility of off-by-one errors.

It looks like your problem is that some Android WebView code is doing its own
DIP calculation in a different way.  I suggest either making the WebView logic
round the same way or, even better plumb the canonical DIP value from
ContentViewCore to WebView.

[aelias_OOO_until_Jul13, 2013-09-06 02:14:21]

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.h:358: gfx::Size
viewport_dip_size_override_;
On second thought, I guess it probably has more to do with page scale.

Could you maybe change GetViewportSizeDip() to compute from the physical backing
size that you've already overridden?  For Clank there's no difference between
physical backing size and viewport size.

[joth, 2013-09-06 02:15:17]

basically l2tm ping me when you have resolve aelias comment if you want another
detailed review from me.
(also remember to add palmer for the ipc change)

https://codereview.chromium.org/23478022/diff/1/android_webview/renderer/DEPS
File android_webview/renderer/DEPS (right):

https://codereview.chromium.org/23478022/diff/1/android_webview/renderer/DEPS...
android_webview/renderer/DEPS:14: "+ui/gfx/size.h",
ui/gfx is fine. (I don't know why we're listing gpu_memory_buffer.h explicitly
below)

https://codereview.chromium.org/23478022/diff/1/android_webview/renderer/aw_r...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://codereview.chromium.org/23478022/diff/1/android_webview/renderer/aw_r...
android_webview/renderer/aw_render_view_ext.cc:314:
DCHECK(render_view()->GetWebView()->isFixedLayoutModeEnabled());
how do we get into this 'enabled' state to start with?

[mkosiba (inactive), 2013-09-06 18:23:08]

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:241:
int fixedLayoutOverrideHeight = FIXED_LAYOUT_HEIGHT;
On 2013/09/06 02:01:35, aelias wrote:
> On 2013/09/05 18:28:24, mkosiba wrote:
> > this might be 'controversial' thing about the CL - the Classic WebView would
> use
> > 0 here, but that doesn't seem to work that well with Blink.
> 
> I don't mind as long as it works in practice for every app we test.

Unfortunately it doesn't. If I set this to 0 then sometimes the WebView will
size itself correctly but not render any contents. I'll file a bug to
investigate using 0 for maxx WebViewClassic compat.

https://codereview.chromium.org/23478022/diff/1/android_webview/renderer/aw_r...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://codereview.chromium.org/23478022/diff/1/android_webview/renderer/aw_r...
android_webview/renderer/aw_render_view_ext.cc:314:
DCHECK(render_view()->GetWebView()->isFixedLayoutModeEnabled());
On 2013/09/06 02:15:17, joth wrote:
> how do we get into this 'enabled' state to start with?

the RenderViewImpl sets it on initialization:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.h:358: gfx::Size
viewport_dip_size_override_;
On 2013/09/06 02:14:22, aelias wrote:
> On second thought, I guess it probably has more to do with page scale.

yes and no. The reason I did this is that when the android_webview is to be
sized to exactly match content, we know what the viewport is supposed to be in
DIP pixels but (unless you assume DIP > 1.0) you can't be sure that the physical
size you come up with will be converted back to the right CSS size.

Clank keeps the viewport in physical pixels and then converts it to DIP pixels
by doing the following:
gfx::ToCeiledSize(gfx::ScaleSize(GetViewportSizePix(), 1.0f / GetDpiScale()))

This means that in the AwLayoutSizer we'd have to find a physical size that we
know will result back in the DIP size we want. I don't think that we can do that
for all DIP and size combinations though.

[aelias_OOO_until_Jul13, 2013-09-06 22:27:46]

lgtm

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:241:
int fixedLayoutOverrideHeight = FIXED_LAYOUT_HEIGHT;
On 2013/09/06 18:23:09, mkosiba wrote:
> On 2013/09/06 02:01:35, aelias wrote:
> > On 2013/09/05 18:28:24, mkosiba wrote:
> > > this might be 'controversial' thing about the CL - the Classic WebView
would
> > use
> > > 0 here, but that doesn't seem to work that well with Blink.
> > 
> > I don't mind as long as it works in practice for every app we test.
> 
> Unfortunately it doesn't. If I set this to 0 then sometimes the WebView will
> size itself correctly but not render any contents. I'll file a bug to
> investigate using 0 for maxx WebViewClassic compat.

I meant it's fine by me if we use the value "8", if concretely there are no
compat issues.

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/23478022/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.h:358: gfx::Size
viewport_dip_size_override_;
On 2013/09/06 18:23:09, mkosiba wrote:
> On 2013/09/06 02:14:22, aelias wrote:
> > On second thought, I guess it probably has more to do with page scale.
> 
> yes and no. The reason I did this is that when the android_webview is to be
> sized to exactly match content, we know what the viewport is supposed to be in
> DIP pixels but (unless you assume DIP > 1.0) you can't be sure that the
physical
> size you come up with will be converted back to the right CSS size.
> 
> Clank keeps the viewport in physical pixels and then converts it to DIP pixels
> by doing the following:
> gfx::ToCeiledSize(gfx::ScaleSize(GetViewportSizePix(), 1.0f / GetDpiScale()))
> 
> This means that in the AwLayoutSizer we'd have to find a physical size that we
> know will result back in the DIP size we want. I don't think that we can do
that
> for all DIP and size combinations though.

OK.  I agree you need a way to specify both values independently after all.

https://codereview.chromium.org/23478022/diff/11001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwLayoutSizerTest.java
(right):

https://codereview.chromium.org/23478022/diff/11001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwLayoutSizerTest.java:21:
public boolean fixedLayoutOverrideEnabled;
nit: please name it consistently using the word "lock"

[mkosiba (inactive), 2013-09-09 16:40:03]

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://codereview.chromium.org/23478022/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:241:
int fixedLayoutOverrideHeight = FIXED_LAYOUT_HEIGHT;
On 2013/09/06 22:27:47, aelias wrote:
> On 2013/09/06 18:23:09, mkosiba wrote:
> > On 2013/09/06 02:01:35, aelias wrote:
> > > On 2013/09/05 18:28:24, mkosiba wrote:
> > > > this might be 'controversial' thing about the CL - the Classic WebView
> would
> > > use
> > > > 0 here, but that doesn't seem to work that well with Blink.
> > > 
> > > I don't mind as long as it works in practice for every app we test.
> > 
> > Unfortunately it doesn't. If I set this to 0 then sometimes the WebView will
> > size itself correctly but not render any contents. I'll file a bug to
> > investigate using 0 for maxx WebViewClassic compat.
> 
> I meant it's fine by me if we use the value "8", if concretely there are no
> compat issues.

ah, ok.

https://codereview.chromium.org/23478022/diff/11001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwLayoutSizerTest.java
(right):

https://codereview.chromium.org/23478022/diff/11001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwLayoutSizerTest.java:21:
public boolean fixedLayoutOverrideEnabled;
On 2013/09/06 22:27:47, aelias wrote:
> nit: please name it consistently using the word "lock"

Done.

[mkosiba (inactive), 2013-09-10 12:47:36]

joth - PTAL again
+yfriedman for OWNERS LGTM
+palmer for IPC OWNERS LGTM
+asvitkine for incoming +ui/gfx DEPS OWNERS LGTM

[Yaron, 2013-09-10 17:43:52]

content lgtm

[aelias_OOO_until_Jul13, 2013-09-12 19:04:14]

Still lgtm with page scale simplification.  Thanks for exploring that route
further.

I'd suggest double-checking that selection handles are positioned properly in
WRAP_CONTENTS views, as that's another scaling-related thing handled in
ContentViewCore.

[palmer, 2013-09-12 19:14:41]

IPC security LGTM. Some nits.

https://codereview.chromium.org/23478022/diff/27001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://codereview.chromium.org/23478022/diff/27001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:41: //
Was our heightSpec is AT_MOST the last time onMeasure was called?
Nit: Grammar ("was... is...")

https://codereview.chromium.org/23478022/diff/27001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:196: //
If the WebView's measuredDimension depends on the size of it's contents (which
is the
Nit: Typo: it's --> its

[joth, 2013-09-12 20:31:12]

lgtm

https://codereview.chromium.org/23478022/diff/27001/android_webview/renderer/...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://codereview.chromium.org/23478022/diff/27001/android_webview/renderer/...
android_webview/renderer/aw_render_view_ext.cc:168: OnSetFixedLayoutSize)
ubernit: all on one line

[mkosiba (inactive), 2013-09-13 15:29:04]

On 2013/09/12 19:04:14, aelias wrote:
> Still lgtm with page scale simplification.  Thanks for exploring that route
> further.
> 
> I'd suggest double-checking that selection handles are positioned properly in
> WRAP_CONTENTS views, as that's another scaling-related thing handled in
> ContentViewCore.

Thanks for pointing this out. I did a quick check and didn't notice anything
being obviously wrong.

[mkosiba (inactive), 2013-09-13 15:31:00]

+danakj for incoming ui/gfx DEPS OWNER approval.

https://chromiumcodereview.appspot.com/23478022/diff/27001/android_webview/ja...
File android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java
(right):

https://chromiumcodereview.appspot.com/23478022/diff/27001/android_webview/ja...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:41: //
Was our heightSpec is AT_MOST the last time onMeasure was called?
On 2013/09/12 19:14:41, Chromium Palmer wrote:
> Nit: Grammar ("was... is...")

Done.

https://chromiumcodereview.appspot.com/23478022/diff/27001/android_webview/ja...
android_webview/java/src/org/chromium/android_webview/AwLayoutSizer.java:196: //
If the WebView's measuredDimension depends on the size of it's contents (which
is the
On 2013/09/12 19:14:41, Chromium Palmer wrote:
> Nit: Typo: it's --> its

Done.

https://chromiumcodereview.appspot.com/23478022/diff/27001/android_webview/re...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://chromiumcodereview.appspot.com/23478022/diff/27001/android_webview/re...
android_webview/renderer/aw_render_view_ext.cc:168: OnSetFixedLayoutSize)
On 2013/09/12 20:31:12, joth wrote:
> ubernit: all on one line

Done.

[commit-bot: I haz the power, 2013-09-16 01:44:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/23478022/38001

[commit-bot: I haz the power, 2013-09-16 04:20:15]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-16 16:50:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/23478022/38001

[commit-bot: I haz the power, 2013-09-16 19:56:50]

Change committed as 223386