[Android WebView] Inform ContentViewCore about double tap support status

[Android WebView] Inform ContentViewCore about double tap support status

When WebSettings.UseWideviewport is turned off, double tap support is
disabled on the Blink side only, which means, ContentViewCore still
induces 300ms double tap detection delay, even though the recognized
double tap will be dumped on the Blink side.

This side makes ContentViewCore aware of double tap support status
change due to wide viewport mode update.

BUG=336765
R=boliu@chromium.org

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

[mnaganov (inactive), 2014-01-22 11:24:50]

Ben, PTAL!

[mnaganov (inactive), 2014-01-22 13:28:43]

On 2014/01/22 11:24:50, Mikhail Naganov (Cr) wrote:
> Ben, PTAL!

Hold on, there is a strange assertion failure, need to sort this out:

d02ae:  01-22 12:16:51.682 10239 10239 W System.err: java.lang.AssertionError
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
org.chromium.android_webview.AwSettings.supportsDoubleTapZoomLocked(AwSettings.java:1412)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
org.chromium.content.browser.ContentViewCore.nativeLoadUrl(Native Method)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
org.chromium.content.browser.ContentViewCore.loadUrl(ContentViewCore.java:1016)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
org.chromium.android_webview.AwContents.loadUrl(AwContents.java:897)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
org.chromium.android_webview.test.AwTestBase$4.run(AwTestBase.java:135)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1719)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
android.os.Handler.handleCallback(Handler.java:733)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
android.os.Handler.dispatchMessage(Handler.java:95)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
android.os.Looper.loop(Looper.java:136)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
android.app.ActivityThread.main(ActivityThread.java:5017)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
java.lang.reflect.Method.invokeNative(Native Method)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
java.lang.reflect.Method.invoke(Method.java:515)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
d02ae:  01-22 12:16:51.682 10239 10239 W System.err: 	at
dalvik.system.NativeStart.main(Native Method)
d02ae:  01-22 12:16:51.732 10239 10239 F chromium: [FATAL:jni_android.cc(204)]
Check failed: false.

[mnaganov (inactive), 2014-01-22 16:38:47]

Aha! This is actually a question to you, Ben. In
https://codereview.chromium.org/23619089/ you've introduced
AwWebPreferencesPopulater, which calls PopulateWebPreferences without holding
the Java lock. All "Locked" getters in Java lack an assert for checking the
lock, and I've added a new getter which does, and so it is failing.

So, the question is, whether this was by design, or have I just discovered an
issue? Looks like an issue to me, because WebSettings can be accessed from any
thread, thus lacking proper locking is bad.

[mnaganov (inactive), 2014-01-22 17:17:51]

On 2014/01/22 16:38:47, Mikhail Naganov (Cr) wrote:
> Aha! This is actually a question to you, Ben. In
> https://codereview.chromium.org/23619089/ you've introduced
> AwWebPreferencesPopulater, which calls PopulateWebPreferences without holding
> the Java lock. All "Locked" getters in Java lack an assert for checking the
> lock, and I've added a new getter which does, and so it is failing.
> 
> So, the question is, whether this was by design, or have I just discovered an
> issue? Looks like an issue to me, because WebSettings can be accessed from any
> thread, thus lacking proper locking is bad.

This is the call stack that causes the "unprotected" call (each stack entry is
on 2 lines: function name, then location):

android_webview::AwContentBrowserClient::OverrideWebkitPrefs(content::RenderViewHost*,
GURL const&, WebPreferences*)
android_webview/browser/aw_content_browser_client.cc:497
content::RenderViewHostImpl::GetWebkitPrefs(GURL const&)
content/browser/renderer_host/render_view_host_impl.cc:552
content::WebContentsImpl::GetWebkitPrefs()
content/browser/web_contents/web_contents_impl.cc:3380
content::RenderViewHostImpl::CreateRenderView(std::basic_string<unsigned short,
base::string16_char_traits, std::allocator<unsigned short> > const&, int, int)
content/browser/renderer_host/render_view_host_impl.cc:287
content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*,
int)
content/browser/web_contents/web_contents_impl.cc:3585
content::RenderFrameHostManager::InitRenderView(content::RenderViewHost*, int)
content/browser/frame_host/render_frame_host_manager.cc:921
content::RenderFrameHostManager::Navigate(content::NavigationEntryImpl const&)
content/browser/frame_host/render_frame_host_manager.cc:186
content::NavigatorImpl::NavigateToEntry(content::RenderFrameHostImpl*,
content::NavigationEntryImpl const&, content::NavigationController::ReloadType)
content/browser/frame_host/navigator_impl.cc:298
content::NavigatorImpl::NavigateToPendingEntry(content::RenderFrameHostImpl*,
content::NavigationController::ReloadType)
content/browser/frame_host/navigator_impl.cc:357
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
content/browser/web_contents/web_contents_impl.cc:1606
content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
content/browser/frame_host/navigation_controller_impl.cc:1572
content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams
const&)
content/browser/frame_host/navigation_controller_impl.cc:724
content::ContentViewCoreImpl::LoadUrl(content::NavigationController::LoadURLParams&)
content/browser/android/content_view_core_impl.cc:763
content::ContentViewCoreImpl::LoadUrl(_JNIEnv*, _jobject*, _jstring*, int, int,
int, _jstring*, _jbyteArray*, _jstring*, _jstring*, unsigned char)
content/browser/android/content_view_core_impl.cc:873
LoadUrl
/usr/local/google/code/chrome/src/out/Debug/gen/content/jni/ContentViewCore_jni.h:66

[mnaganov (inactive), 2014-01-23 10:38:54]

On 2014/01/22 17:17:51, Mikhail Naganov (Cr) wrote:
> On 2014/01/22 16:38:47, Mikhail Naganov (Cr) wrote:
> > Aha! This is actually a question to you, Ben. In
> > https://codereview.chromium.org/23619089/ you've introduced
> > AwWebPreferencesPopulater, which calls PopulateWebPreferences without
holding
> > the Java lock. All "Locked" getters in Java lack an assert for checking the
> > lock, and I've added a new getter which does, and so it is failing.
> > 
> > So, the question is, whether this was by design, or have I just discovered
an
> > issue? Looks like an issue to me, because WebSettings can be accessed from
any
> > thread, thus lacking proper locking is bad.
> 
> This is the call stack that causes the "unprotected" call (each stack entry is
> on 2 lines: function name, then location):
> 
>
android_webview::AwContentBrowserClient::OverrideWebkitPrefs(content::RenderViewHost*,
> GURL const&, WebPreferences*)
> android_webview/browser/aw_content_browser_client.cc:497
> content::RenderViewHostImpl::GetWebkitPrefs(GURL const&)
> content/browser/renderer_host/render_view_host_impl.cc:552
> content::WebContentsImpl::GetWebkitPrefs()
> content/browser/web_contents/web_contents_impl.cc:3380
> content::RenderViewHostImpl::CreateRenderView(std::basic_string<unsigned
short,
> base::string16_char_traits, std::allocator<unsigned short> > const&, int, int)
> content/browser/renderer_host/render_view_host_impl.cc:287
>
content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*,
> int)
> content/browser/web_contents/web_contents_impl.cc:3585
> content::RenderFrameHostManager::InitRenderView(content::RenderViewHost*, int)
> content/browser/frame_host/render_frame_host_manager.cc:921
> content::RenderFrameHostManager::Navigate(content::NavigationEntryImpl const&)
> content/browser/frame_host/render_frame_host_manager.cc:186
> content::NavigatorImpl::NavigateToEntry(content::RenderFrameHostImpl*,
> content::NavigationEntryImpl const&,
content::NavigationController::ReloadType)
> content/browser/frame_host/navigator_impl.cc:298
> content::NavigatorImpl::NavigateToPendingEntry(content::RenderFrameHostImpl*,
> content::NavigationController::ReloadType)
> content/browser/frame_host/navigator_impl.cc:357
>
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
> content/browser/web_contents/web_contents_impl.cc:1606
>
content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
> content/browser/frame_host/navigation_controller_impl.cc:1572
>
content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams
> const&)
> content/browser/frame_host/navigation_controller_impl.cc:724
>
content::ContentViewCoreImpl::LoadUrl(content::NavigationController::LoadURLParams&)
> content/browser/android/content_view_core_impl.cc:763
> content::ContentViewCoreImpl::LoadUrl(_JNIEnv*, _jobject*, _jstring*, int,
int,
> int, _jstring*, _jbyteArray*, _jstring*, _jstring*, unsigned char)
> content/browser/android/content_view_core_impl.cc:873
> LoadUrl
>
/usr/local/google/code/chrome/src/out/Debug/gen/content/jni/ContentViewCore_jni.h:66

Addressed thread safety issues here: https://codereview.chromium.org/143803016/
Will add the assert back to the new getter before committing.

[mnaganov (inactive), 2014-01-24 10:20:26]

@benm: PTAL!

[mnaganov (inactive), 2014-01-29 09:31:24]

Bo, can you please take a look, since Ben is OOO.

[boliu, 2014-01-29 17:06:09]

lgtm

lgtm

fyi cc jdduke

https://codereview.chromium.org/144923002/diff/100001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/144923002/diff/100001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:1412:
return mSupportZoom && mBuiltInZoomControls && mUseWideViewport;
assert holdsLock is coming in https://codereview.chromium.org/143803016/, right?

[mnaganov (inactive), 2014-01-29 17:48:46]

Thanks, Bo!

https://codereview.chromium.org/144923002/diff/100001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/144923002/diff/100001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:1412:
return mSupportZoom && mBuiltInZoomControls && mUseWideViewport;
On 2014/01/29 17:06:09, boliu wrote:
> assert holdsLock is coming in https://codereview.chromium.org/143803016/,
right?

Yes, I'll add it there after this one gets in.

[commit-bot: I haz the power, 2014-01-29 17:49:48]

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

[commit-bot: I haz the power, 2014-01-29 18:48:43]

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

[mnaganov (inactive), 2014-01-29 19:17:36]

Committed patchset #2 manually as r247730 (presubmit successful).