Commit 709e3c43 authored by igsolla's avatar igsolla Committed by Commit bot

Revert of [WebView] Add unique visual state request ids. (patchset #4 id:60001...

Revert of [WebView] Add unique visual state request ids. (patchset #4 id:60001 of https://codereview.chromium.org/900303002/)

Reason for revert:
This is breaking some bots, see for example:
https://chromegw.corp.google.com/i/chromium.linux/builders/Android%20Arm64%20Builder%20(dbg)/builds/8923

https://chromegw.corp.google.com/i/chromium.linux/builders/Android%20Builder%20(dbg)/builds/73663

The reason is:
M D ST: Write to static field org.chromium.android_webview.AwContents.sNextVisualStateRequestId from instance method org.chromium.android_webview.AwContents.flushVisualState(AwContents$VisualStateFlushCallback)  At AwContents.java

Original issue's description:
> [WebView] Add unique visual state request ids.
>
> Each flushVisualState request returns a unique id
> that allows callers to match requests with callbacks.
> The reasoning behind is to potentially allow clients
> to reuse VisualStateFlushCallback objects to avoid
> additional java allocations.
>
> BUG=455651
>
> Committed: https://crrev.com/d011b93e7c8f966ba068b0c77c8a1400bdedee88
> Cr-Commit-Position: refs/heads/master@{#315304}

TBR=boliu@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=455651

Review URL: https://codereview.chromium.org/907723004

Cr-Commit-Position: refs/heads/master@{#315321}
parent 9dcc0401
...@@ -183,18 +183,13 @@ public class AwContents implements SmartClipProvider { ...@@ -183,18 +183,13 @@ public class AwContents implements SmartClipProvider {
/** /**
* Callback used when flushing the visual state, see {@link #flushVisualState}. * Callback used when flushing the visual state, see {@link #flushVisualState}.
*
* <p>The {@code requestId} is the unique request id returned by
* {@link AwContents#flushVisualState} which can be used to match callbacks with requests.
*/ */
@VisibleForTesting @VisibleForTesting
public abstract static class VisualStateFlushCallback { public abstract static class VisualStateFlushCallback {
public abstract void onComplete(long requestId); public abstract void onComplete();
public abstract void onFailure(long requestId); public abstract void onFailure();
} }
private static long sNextVisualStateRequestId = 1;
private long mNativeAwContents; private long mNativeAwContents;
private final AwBrowserContext mBrowserContext; private final AwBrowserContext mBrowserContext;
private ViewGroup mContainerView; private ViewGroup mContainerView;
...@@ -2054,14 +2049,9 @@ public class AwContents implements SmartClipProvider { ...@@ -2054,14 +2049,9 @@ public class AwContents implements SmartClipProvider {
* 1. The DOM tree is committed becoming the pending tree - see ThreadProxy::BeginMainFrame * 1. The DOM tree is committed becoming the pending tree - see ThreadProxy::BeginMainFrame
* 2. The pending tree is activated becoming the active tree * 2. The pending tree is activated becoming the active tree
* 3. A frame swap happens that draws the active tree into the screen * 3. A frame swap happens that draws the active tree into the screen
*
* @return an unique id that identifies this request. It can be used to match this request
* to the corresponding callback to allow reuse of {@link VisualStateFlushCallback} objects.
*/ */
public long flushVisualState(VisualStateFlushCallback callback) { public void flushVisualState(VisualStateFlushCallback callback) {
long requestId = sNextVisualStateRequestId++; nativeFlushVisualState(mNativeAwContents, callback);
nativeFlushVisualState(mNativeAwContents, callback, requestId);
return requestId;
} }
//-------------------------------------------------------------------------------------------- //--------------------------------------------------------------------------------------------
...@@ -2174,16 +2164,16 @@ public class AwContents implements SmartClipProvider { ...@@ -2174,16 +2164,16 @@ public class AwContents implements SmartClipProvider {
*/ */
@CalledByNative @CalledByNative
public void flushVisualStateCallback( public void flushVisualStateCallback(
final VisualStateFlushCallback callback, final long requestId, final boolean result) { final VisualStateFlushCallback callback, final boolean result) {
// Posting avoids invoking the callback inside invoking_composite_ // Posting avoids invoking the callback inside invoking_composite_
// (see synchronous_compositor_impl.cc and crbug/452530). // (see synchronous_compositor_impl.cc and crbug/452530).
mContainerView.getHandler().post(new Runnable() { mContainerView.getHandler().post(new Runnable() {
@Override @Override
public void run() { public void run() {
if (result) { if (result) {
callback.onComplete(requestId); callback.onComplete();
} else { } else {
callback.onFailure(requestId); callback.onFailure();
} }
} }
}); });
...@@ -2746,7 +2736,7 @@ public class AwContents implements SmartClipProvider { ...@@ -2746,7 +2736,7 @@ public class AwContents implements SmartClipProvider {
private native long nativeCapturePicture(long nativeAwContents, int width, int height); private native long nativeCapturePicture(long nativeAwContents, int width, int height);
private native void nativeEnableOnNewPicture(long nativeAwContents, boolean enabled); private native void nativeEnableOnNewPicture(long nativeAwContents, boolean enabled);
private native void nativeFlushVisualState( private native void nativeFlushVisualState(
long nativeAwContents, VisualStateFlushCallback callback, long requestId); long nativeAwContents, VisualStateFlushCallback callback);
private native void nativeClearView(long nativeAwContents); private native void nativeClearView(long nativeAwContents);
private native void nativeSetExtraHeadersForUrl(long nativeAwContents, private native void nativeSetExtraHeadersForUrl(long nativeAwContents,
String url, String extraHeaders); String url, String extraHeaders);
......
...@@ -18,6 +18,7 @@ import org.chromium.content_public.browser.LoadUrlParams; ...@@ -18,6 +18,7 @@ import org.chromium.content_public.browser.LoadUrlParams;
import java.util.concurrent.CountDownLatch; import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
/** /**
* Visual state related tests. * Visual state related tests.
...@@ -25,8 +26,6 @@ import java.util.concurrent.TimeUnit; ...@@ -25,8 +26,6 @@ import java.util.concurrent.TimeUnit;
public class VisualStateTest extends AwTestBase { public class VisualStateTest extends AwTestBase {
private TestAwContentsClient mContentsClient = new TestAwContentsClient(); private TestAwContentsClient mContentsClient = new TestAwContentsClient();
private long mExpectedFlushRequestId = -1;
private AwContents mAwContents;
@Feature({"AndroidWebView"}) @Feature({"AndroidWebView"})
@SmallTest @SmallTest
...@@ -40,19 +39,17 @@ public class VisualStateTest extends AwTestBase { ...@@ -40,19 +39,17 @@ public class VisualStateTest extends AwTestBase {
runTestOnUiThread(new Runnable() { runTestOnUiThread(new Runnable() {
@Override @Override
public void run() { public void run() {
mExpectedFlushRequestId = awContents.flushVisualState(new AwContents.VisualStateFlushCallback() {
awContents.flushVisualState(new AwContents.VisualStateFlushCallback() { @Override
@Override public void onComplete() {
public void onComplete(long requestId) { ch.notifyCalled();
assertEquals(mExpectedFlushRequestId, requestId); }
ch.notifyCalled();
}
@Override @Override
public void onFailure(long requestId) { public void onFailure() {
fail("onFailure received"); fail("onFailure received");
} }
}); });
} }
}); });
ch.waitForCallback(chCount); ch.waitForCallback(chCount);
...@@ -68,44 +65,44 @@ public class VisualStateTest extends AwTestBase { ...@@ -68,44 +65,44 @@ public class VisualStateTest extends AwTestBase {
final LoadUrlParams bluePageUrl = createTestPageUrl("blue"); final LoadUrlParams bluePageUrl = createTestPageUrl("blue");
final CountDownLatch testFinishedSignal = new CountDownLatch(1); final CountDownLatch testFinishedSignal = new CountDownLatch(1);
final AtomicReference<AwContents> awContentsRef = new AtomicReference<>();
final AwTestContainerView testView = final AwTestContainerView testView =
createAwTestContainerViewOnMainSync(new TestAwContentsClient() { createAwTestContainerViewOnMainSync(new TestAwContentsClient() {
@Override @Override
public void onPageFinished(String url) { public void onPageFinished(String url) {
if (bluePageUrl.getUrl().equals(url)) { if (bluePageUrl.getUrl().equals(url)) {
mExpectedFlushRequestId = awContentsRef.get().flushVisualState(new VisualStateFlushCallback() {
mAwContents.flushVisualState(new VisualStateFlushCallback() { @Override
@Override public void onFailure() {
public void onFailure(long requestId) { fail("onFailure received");
fail("onFailure received"); }
}
@Override @Override
public void onComplete(long requestId) { public void onComplete() {
assertEquals(mExpectedFlushRequestId, requestId); Bitmap blueScreenshot = GraphicsTestUtils.drawAwContents(
Bitmap blueScreenshot = awContentsRef.get(), 1, 1);
GraphicsTestUtils.drawAwContents( assertEquals(Color.BLUE, blueScreenshot.getPixel(0, 0));
mAwContents, 1, 1); testFinishedSignal.countDown();
assertEquals(Color.BLUE, blueScreenshot.getPixel(0, 0)); }
testFinishedSignal.countDown(); });
}
});
} }
} }
}); });
mAwContents = testView.getAwContents(); final AwContents awContents = testView.getAwContents();
awContentsRef.set(awContents);
runTestOnUiThread(new Runnable() { runTestOnUiThread(new Runnable() {
@Override @Override
public void run() { public void run() {
mAwContents.setBackgroundColor(Color.RED); awContents.setBackgroundColor(Color.RED);
mAwContents.loadUrl(bluePageUrl); awContents.loadUrl(bluePageUrl);
// We have just loaded the blue page, but the graphics pipeline is asynchronous // We have just loaded the blue page, but the graphics pipeline is asynchronous
// so at this point the WebView still draws red, ie. the View background color. // so at this point the WebView still draws red, ie. the View background color.
// Only when the flush callback is received will we know for certain that the // Only when the flush callback is received will we know for certain that the
// blue page contents are on screen. // blue page contents are on screen.
Bitmap redScreenshot = GraphicsTestUtils.drawAwContents(mAwContents, 1, 1); Bitmap redScreenshot = GraphicsTestUtils.drawAwContents(
awContentsRef.get(), 1, 1);
assertEquals(Color.RED, redScreenshot.getPixel(0, 0)); assertEquals(Color.RED, redScreenshot.getPixel(0, 0));
} }
}); });
......
...@@ -1037,27 +1037,22 @@ void AwContents::EnableOnNewPicture(JNIEnv* env, ...@@ -1037,27 +1037,22 @@ void AwContents::EnableOnNewPicture(JNIEnv* env,
namespace { namespace {
void FlushVisualStateCallback(const JavaObjectWeakGlobalRef& java_ref, void FlushVisualStateCallback(const JavaObjectWeakGlobalRef& java_ref,
ScopedJavaGlobalRef<jobject>* callback, ScopedJavaGlobalRef<jobject>* callback,
int64 request_id,
bool result) { bool result) {
JNIEnv* env = AttachCurrentThread(); JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = java_ref.get(env); ScopedJavaLocalRef<jobject> obj = java_ref.get(env);
if (obj.is_null()) if (obj.is_null())
return; return;
Java_AwContents_flushVisualStateCallback(env, obj.obj(), callback->obj(), Java_AwContents_flushVisualStateCallback(
request_id, result); env, obj.obj(), callback->obj(), result);
} }
} // namespace } // namespace
void AwContents::FlushVisualState(JNIEnv* env, void AwContents::FlushVisualState(JNIEnv* env, jobject obj, jobject callback) {
jobject obj,
jobject callback,
int64 request_id) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
ScopedJavaGlobalRef<jobject>* j_callback = new ScopedJavaGlobalRef<jobject>(); ScopedJavaGlobalRef<jobject>* j_callback = new ScopedJavaGlobalRef<jobject>();
j_callback->Reset(env, callback); j_callback->Reset(env, callback);
web_contents_->GetMainFrame()->FlushVisualState( web_contents_->GetMainFrame()->FlushVisualState(base::Bind(
base::Bind(&FlushVisualStateCallback, java_ref_, base::Owned(j_callback), &FlushVisualStateCallback, java_ref_, base::Owned(j_callback)));
request_id));
} }
void AwContents::ClearView(JNIEnv* env, jobject obj) { void AwContents::ClearView(JNIEnv* env, jobject obj) {
......
...@@ -129,10 +129,7 @@ class AwContents : public FindHelper::Listener, ...@@ -129,10 +129,7 @@ class AwContents : public FindHelper::Listener,
jlong GetAwDrawGLViewContext(JNIEnv* env, jobject obj); jlong GetAwDrawGLViewContext(JNIEnv* env, jobject obj);
jlong CapturePicture(JNIEnv* env, jobject obj, int width, int height); jlong CapturePicture(JNIEnv* env, jobject obj, int width, int height);
void EnableOnNewPicture(JNIEnv* env, jobject obj, jboolean enabled); void EnableOnNewPicture(JNIEnv* env, jobject obj, jboolean enabled);
void FlushVisualState(JNIEnv* env, void FlushVisualState(JNIEnv* env, jobject obj, jobject callback);
jobject obj,
jobject callback,
int64 request_id);
void ClearView(JNIEnv* env, jobject obj); void ClearView(JNIEnv* env, jobject obj);
void SetExtraHeadersForUrl(JNIEnv* env, jobject obj, void SetExtraHeadersForUrl(JNIEnv* env, jobject obj,
jstring url, jstring extra_headers); jstring url, jstring extra_headers);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment