Use contents size for android_webview layout.

Use contents size for android_webview layout size.

Currently android_webview uses RenderView::preferredSize (which is
based off of documentElement.height) to drive the android.view.View
layout size.
WebViewClassic would use content size for that purpose which meant
that absolute-positioned elements can cause the Classic WebView to
grow.
This CL makes AwContents use the contents size for driving layout.

BUG=286336
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223984

[mkosiba (inactive), 2013-09-06 18:03:37]

+benm@ for overall review

+aelias@ for reviewing the difference between
  webview()->mainFrame()->contentsSize()
and
  document->documentElement()->scrollHeight()

The one difference we've already found was the inclusion of absolute-positioned
elements in contentsSize, what else can we expect (and should test)?
Also - John Mellor suggested we use whatever iframe seamless uses - do you
happen to know what that is?

[benm (inactive), 2013-09-06 19:27:53]

Thanks Marcin, overall flow seems reasonable to me.

https://codereview.chromium.org/23899004/diff/1/android_webview/common/render...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/23899004/diff/1/android_webview/common/render...
android_webview/common/render_view_messages.h:81:
IPC_MESSAGE_ROUTED0(AwViewMsg_DisableContentsSizeNotifications)
Do we need two IPCs for this? Suggest just one with a bool param

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

https://codereview.chromium.org/23899004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:168:
if (mPreferredSizeChangedListener != null) {
Add a comment (maybe up with the field declaration as to what it means for this
to be null. Maybe an int or something to define what size change mode we're in
might be clearer?

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/AndroidViewIntegrationTest.java
(right):

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidViewIntegrationTest.java:287:
// Absolute-positioned elements shouldn't contribute to the height in non-quirks
mode.
Maybe move this comment to the top?

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidViewIntegrationTest.java:319:
// Absolute-positioned elements do contribute to the height in quirks mode.
ditto

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
android_webview/native/aw_settings.cc:238: if (!support_quirks &&
web_contents()->GetRenderViewHost()) {
cache wc->GRVH?

[mnaganov (inactive), 2013-09-06 21:07:04]

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
(right):

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:270:
mInQuirksMode = value;
nit: Perhaps, name the property "mQuirksMode" for consistency?

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
android_webview/native/aw_settings.cc:238: if (!support_quirks &&
web_contents()->GetRenderViewHost()) {
nit: perhaps write as:

if (support_quirks) { ... } else if (web_contents()->GetRenderViewHost()) { ...
}

[aelias_OOO_until_Jul13, 2013-09-06 23:29:22]

Hmm, that distinction is new to me.  I've always used contentsSize for scroll
bounds in Clank.  So I don't have a problem with WebView using it, it's always
given reasonable values in my experience.  But I am confused and curious why the
other one is slightly different and if there's a good reason why PreferredSize
chose that one instead.

[mkosiba (inactive), 2013-09-09 13:46:21]

So after a bit more thought about this I decided to move to contentSize.
The reasoning is that in wrap_content mode the WebView is not supposed to be
scrollable, and using preferredSize would allow for that.

https://codereview.chromium.org/23899004/diff/1/android_webview/common/render...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/23899004/diff/1/android_webview/common/render...
android_webview/common/render_view_messages.h:81:
IPC_MESSAGE_ROUTED0(AwViewMsg_DisableContentsSizeNotifications)
On 2013/09/06 19:27:53, benm wrote:
> Do we need two IPCs for this? Suggest just one with a bool param

Done.

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

https://codereview.chromium.org/23899004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:168:
if (mPreferredSizeChangedListener != null) {
On 2013/09/06 19:27:53, benm wrote:
> Add a comment (maybe up with the field declaration as to what it means for
this
> to be null. Maybe an int or something to define what size change mode we're in
> might be clearer?

just removed all of this.

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
(right):

https://codereview.chromium.org/23899004/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:270:
mInQuirksMode = value;
On 2013/09/06 21:07:04, Mikhail Naganov (Chromium) wrote:
> nit: Perhaps, name the property "mQuirksMode" for consistency?

Done.

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
android_webview/native/aw_settings.cc:238: if (!support_quirks &&
web_contents()->GetRenderViewHost()) {
On 2013/09/06 21:07:04, Mikhail Naganov (Chromium) wrote:
> nit: perhaps write as:
> 
> if (support_quirks) { ... } else if (web_contents()->GetRenderViewHost()) {
...
> }

Done.

https://codereview.chromium.org/23899004/diff/1/android_webview/native/aw_set...
android_webview/native/aw_settings.cc:238: if (!support_quirks &&
web_contents()->GetRenderViewHost()) {
On 2013/09/06 19:27:53, benm wrote:
> cache wc->GRVH?

Done.

[mkosiba (inactive), 2013-09-09 13:48:12]

+jamesr@ for the following question:

On 2013/09/06 23:29:22, aelias wrote:
> Hmm, that distinction is new to me.  I've always used contentsSize for scroll
> bounds in Clank.  So I don't have a problem with WebView using it, it's always
> given reasonable values in my experience.  But I am confused and curious why
the
> other one is slightly different and if there's a good reason why PreferredSize
> chose that one instead.

James, do you know why WebView::contentsMinimumPreferredSize uses the
documentElement scrollHeight and not the contents height?

[jamesr, 2013-09-10 05:43:14]

I don't know.  I think aa@ is the originator of this logic.

[benm (inactive), 2013-09-12 19:13:05]

https://codereview.chromium.org/23899004/diff/19001/android_webview/native/aw...
File android_webview/native/aw_settings.h (right):

https://codereview.chromium.org/23899004/diff/19001/android_webview/native/aw...
android_webview/native/aw_settings.h:37: void
UpdateContentSizeNotificationMode(JNIEnv* env, jobject obj);
nit: if I follow this should be TurnOnContentSizeNotification ? We enable it
unconditionally and I can't see us turning it off anywhere?

In which case, do we need an IPC to enable it at all?

[mkosiba (inactive), 2013-09-16 01:49:08]

new version up. PTAL

https://codereview.chromium.org/23899004/diff/19001/android_webview/native/aw...
File android_webview/native/aw_settings.h (right):

https://codereview.chromium.org/23899004/diff/19001/android_webview/native/aw...
android_webview/native/aw_settings.h:37: void
UpdateContentSizeNotificationMode(JNIEnv* env, jobject obj);
On 2013/09/12 19:13:06, benm wrote:
> nit: if I follow this should be TurnOnContentSizeNotification ? We enable it
> unconditionally and I can't see us turning it off anywhere?
> 
> In which case, do we need an IPC to enable it at all?

ah, yes, good spot.

[benm (inactive), 2013-09-16 02:55:07]

thanks marcin! lgtm % nits.

maybe add something into the commit message that we are switching from preferred
size to content size.

https://codereview.chromium.org/23899004/diff/31001/android_webview/common/re...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/23899004/diff/31001/android_webview/common/re...
android_webview/common/render_view_messages.h:72: SkColor)
intentional?

https://codereview.chromium.org/23899004/diff/31001/android_webview/renderer/...
File android_webview/renderer/aw_render_view_ext.h (right):

https://codereview.chromium.org/23899004/diff/31001/android_webview/renderer/...
android_webview/renderer/aw_render_view_ext.h:59: void
OnEnableContentsSizeNotifications();
remove me

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

https://codereview.chromium.org/23899004/diff/31001/android_webview/common/re...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/23899004/diff/31001/android_webview/common/re...
android_webview/common/render_view_messages.h:72: SkColor)
On 2013/09/16 02:55:07, benm wrote:
> intentional?

yes

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

+Joi for content/ layer API change.

[Jói, 2013-09-16 08:55:17]

//content/public LGTM.

[commit-bot: I haz the power, 2013-09-16 17:10:00]

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

[commit-bot: I haz the power, 2013-09-16 17:22:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mkosiba (inactive), 2013-09-16 18:47:18]

+ jochen for content/renderer
+ cdn for android_webview IPC

[jochen (gone - plz use gerrit), 2013-09-17 08:40:28]

lgtm

[commit-bot: I haz the power, 2013-09-17 23:34:22]

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

[commit-bot: I haz the power, 2013-09-17 23:54:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mkosiba (inactive), 2013-09-18 00:14:42]

+cdn@ for android_webview/common/render_view_messages.h

[Cris Neckar, 2013-09-18 19:01:14]

IPC changes LGTM

[commit-bot: I haz the power, 2013-09-18 19:03:37]

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

[commit-bot: I haz the power, 2013-09-18 23:21:36]

Change committed as 223984