[Android] Add supportMultipleWindows setting

[Android] Add supportMultipleWindows setting

Add supportMultipleWindows settings for reusing the same view
when opening popups. This is required for emulating the behavior
of Android WebView.

Depends on the WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=99716

R=joth@chromium.org
BUG=156600
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164909

[joth, 2012-10-18 20:06:00]

fly-by

https://codereview.chromium.org/11192057/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11192057/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2421: #if !defined(OS_ANDROID)
Maybe easier to follow as:

if (did_show_) {
#if defined(OS_ANDROID)
  if (webkit_preferences_.support_multiple_windows)
    return;
#endif
  NOTREACHED();  // received extraneous show call
  return;
}
did_show_ = true;

[benm (inactive), 2012-10-19 11:02:59]

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

https://codereview.chromium.org/11192057/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:809:
class AwSettingsJavaScriptPopupsTestHelper extends AwSettingsTestHelper<Boolean>
{
This test seems to be verifying the behaviour of
ContentSettings.get/setJavaScriptCanOpenWindowsAutomatically, not
setSupportMultipleWindows?

https://codereview.chromium.org/11192057/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11192057/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:867:
public synchronized void setSupportMultipleWindows(boolean support) {
no need for the synchronized here, we use mContentSettingsLock below

https://codereview.chromium.org/11192057/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

https://codereview.chromium.org/11192057/diff/1/webkit/glue/webpreferences.cc...
webkit/glue/webpreferences.cc:106: support_multiple_windows(true),
should this default to false to match the legacy WebView?

[mnaganov (inactive), 2012-10-19 15:46:39]

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

https://codereview.chromium.org/11192057/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:809:
class AwSettingsJavaScriptPopupsTestHelper extends AwSettingsTestHelper<Boolean>
{
On 2012/10/19 11:02:59, benm wrote:
> This test seems to be verifying the behaviour of
> ContentSettings.get/setJavaScriptCanOpenWindowsAutomatically, not
> setSupportMultipleWindows?

The test relies on the fact that when SupportMultipleWindows is false (the
default value), popups are opened in the same WebView. This is impossible
without the change I'm making.

I can't write a full test for SupportMultipleWindows, because we haven't
implemented WebChromeClient.onCreateWindow callback yet.

https://codereview.chromium.org/11192057/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11192057/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:867:
public synchronized void setSupportMultipleWindows(boolean support) {
On 2012/10/19 11:02:59, benm wrote:
> no need for the synchronized here, we use mContentSettingsLock below

Sorry, copied the code from the interface definition. Removed.

https://codereview.chromium.org/11192057/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11192057/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2421: #if !defined(OS_ANDROID)
On 2012/10/18 20:06:00, joth wrote:
> Maybe easier to follow as:
> 
> if (did_show_) {
> #if defined(OS_ANDROID)
>   if (webkit_preferences_.support_multiple_windows)
>     return;
> #endif
>   NOTREACHED();  // received extraneous show call
>   return;
> }
> did_show_ = true;

Changed to an alternative shorter version.

https://codereview.chromium.org/11192057/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

https://codereview.chromium.org/11192057/diff/1/webkit/glue/webpreferences.cc...
webkit/glue/webpreferences.cc:106: support_multiple_windows(true),
On 2012/10/19 11:02:59, benm wrote:
> should this default to false to match the legacy WebView?

For WebView, we have default settings in ContentSettings.java. These default
settings are for Chrome, where support_multiple_windows should obviously be
'true'.

[mnaganov (inactive), 2012-10-19 16:22:55]

https://codereview.chromium.org/11192057/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11192057/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2421: #if !defined(OS_ANDROID)
On 2012/10/19 15:46:39, Mikhail Naganov (Chromium) wrote:
> On 2012/10/18 20:06:00, joth wrote:
> > Maybe easier to follow as:
> > 
> > if (did_show_) {
> > #if defined(OS_ANDROID)
> >   if (webkit_preferences_.support_multiple_windows)
> >     return;
> > #endif
> >   NOTREACHED();  // received extraneous show call
> >   return;
> > }
> > did_show_ = true;
> 
> Changed to an alternative shorter version.

Ouch. My approach doesn't always compile, changing to yours.

[mnaganov (inactive), 2012-10-25 13:49:29]

The WebKit part of the patch has landed.

Ben and Joth, do you have any questions to the patch?

Adam, are you interested in reviewing 'content' and 'webkit' parts? I realize, I
will need an owner's review anyway, but as you are familiar with the WebKit part
of the change, taking a look might make sense.

[joth, 2012-10-25 17:27:17]

lgtm % nits

https://codereview.chromium.org/11192057/diff/25001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11192057/diff/25001/content/common/view_messa...
content/common/view_messages.h:150:
IPC_STRUCT_TRAITS_MEMBER(supports_multiple_windows)
ditto comment from other thread about putting this at the end of the struct

https://codereview.chromium.org/11192057/diff/25001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11192057/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:863:
* Tells the WebView whether it supports multiple windows. TRUE means
nit: 'true'

no need to capitalize

https://codereview.chromium.org/11192057/diff/25001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11192057/diff/25001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2414: #if defined(OS_ANDROID)
maybe add a comment to explain the reasoning here.

[mnaganov (inactive), 2012-10-26 10:05:42]

https://codereview.chromium.org/11192057/diff/25001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11192057/diff/25001/content/common/view_messa...
content/common/view_messages.h:150:
IPC_STRUCT_TRAITS_MEMBER(supports_multiple_windows)
On 2012/10/25 17:27:18, joth wrote:
> ditto comment from other thread about putting this at the end of the struct 

Done.

https://codereview.chromium.org/11192057/diff/25001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11192057/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:863:
* Tells the WebView whether it supports multiple windows. TRUE means
On 2012/10/25 17:27:18, joth wrote:
> nit: 'true'
> 
> no need to capitalize 

Done.

https://codereview.chromium.org/11192057/diff/25001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11192057/diff/25001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2414: #if defined(OS_ANDROID)
On 2012/10/25 17:27:18, joth wrote:
> maybe add a comment to explain the reasoning here.

Done.

[mnaganov (inactive), 2012-10-26 10:09:09]

John, can you please look at the contents/ part and on fields rearrangement in
WebPreferences.

Darin, can you please look at the webkit/ part. I've moved conditional
WebPreferences fields to the end following John's advice.

[jam, 2012-10-26 20:11:35]

content lgtm, and thanks for rearranging the struct

[mnaganov (inactive), 2012-10-29 16:23:56]

Darin, ping. Can you please have a look at webkit/ ?

[darin (slow to review), 2012-10-30 04:51:35]

LGTM for src/webkit/

[commit-bot: I haz the power, 2012-10-30 09:45:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/11192057/37001

[commit-bot: I haz the power, 2012-10-30 13:59:28]

Change committed as 164909