Skip to content

GitLab

T
tangled
Commit 8d503c0a authored by tobiasjs's avatar tobiasjs Committed by Commit bot
Browse files
Options

[WebView] Remove onFailure from VisualStateCallback. 

onFailure is only invoked when the render frame has
been destroyed, but it is not really useful to
distinguish onComplete from onFailure in that case.
This also addresses the feedback that we received
from API Council.

This is a resubmit of CL 943633003

BUG=459539, 457184

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

Cr-Commit-Position: refs/heads/master@{#319068}
parent 9b841666
Hide whitespace changes
Inline Side-by-side
Showing with 46 additions and 61 deletions
android_webview/java/src/org/chromium/android_webview/AwContents.java
View file @ 8d503c0a
......@@ -183,13 +183,14 @@ public class AwContents implements SmartClipProvider,
/**
* Visual state callback, see {@link #insertVisualStateCallback} for details.
*
* <p>The {@code requestId} is the id passed to {@link AwContents#insertVisualStateCallback}
* which can be used to match requests with the corresponding callbacks.
*/
@VisibleForTesting
public abstract static class VisualStateCallback {
/**
* @param requestId the id passed to {@link AwContents#insertVisualStateCallback}
* which can be used to match requests with the corresponding callbacks.
*/
public abstract void onComplete(long requestId);
public abstract void onFailure(long requestId);
}
private long mNativeAwContents;
......@@ -2212,17 +2213,13 @@ public class AwContents implements SmartClipProvider,
*/
@CalledByNative
public void invokeVisualStateCallback(
final VisualStateCallback callback, final long requestId, final boolean result) {
final VisualStateCallback callback, final long requestId) {
// Posting avoids invoking the callback inside invoking_composite_
// (see synchronous_compositor_impl.cc and crbug/452530).
mContainerView.getHandler().post(new Runnable() {
@Override
public void run() {
if (result) {
callback.onComplete(requestId);
} else {
callback.onFailure(requestId);
}
callback.onComplete(requestId);
}
});
}
......
android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java
View file @ 8d503c0a
......@@ -59,11 +59,6 @@ public class VisualStateTest extends AwTestBase {
assertEquals(requestId, id);
ch.notifyCalled();
}
@Override
public void onFailure(long id) {
fail("onFailure received");
}
});
}
});
......@@ -89,11 +84,6 @@ public class VisualStateTest extends AwTestBase {
final long requestId = 10;
awContentsRef.get().insertVisualStateCallback(requestId,
new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
assertEquals(requestId, id);
......@@ -146,11 +136,6 @@ public class VisualStateTest extends AwTestBase {
super.onPageFinished(url);
awContentsRef.get().insertVisualStateCallback(10,
new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
Bitmap blueScreenshot = GraphicsTestUtils.drawAwContents(
......@@ -190,11 +175,6 @@ public class VisualStateTest extends AwTestBase {
public void run() {
awContents.insertVisualStateCallback(20,
new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
Bitmap redScreenshot = GraphicsTestUtils.drawAwContents(
......@@ -228,11 +208,6 @@ public class VisualStateTest extends AwTestBase {
public void onPageFinished(String url) {
super.onPageFinished(url);
awContentsRef.get().insertVisualStateCallback(10, new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
Bitmap blueScreenshot =
......@@ -270,11 +245,6 @@ public class VisualStateTest extends AwTestBase {
@Override
public void run() {
awContents.insertVisualStateCallback(20, new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
// NOTE: We cannot use drawAwContents here because the web contents
......@@ -309,11 +279,6 @@ public class VisualStateTest extends AwTestBase {
public void onPageFinished(String url) {
super.onPageFinished(url);
awContentsRef.get().insertVisualStateCallback(10, new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
Bitmap blueScreenshot =
......@@ -328,11 +293,6 @@ public class VisualStateTest extends AwTestBase {
public void onShowCustomView(View view, WebChromeClient.CustomViewCallback callback) {
super.onShowCustomView(view, callback);
awContentsRef.get().insertVisualStateCallback(20, new VisualStateCallback() {
@Override
public void onFailure(long id) {
fail("onFailure received");
}
@Override
public void onComplete(long id) {
// NOTE: We cannot use drawAwContents here because the web contents are
......
android_webview/native/aw_contents.cc
View file @ 8d503c0a
......@@ -1039,15 +1039,15 @@ void AwContents::EnableOnNewPicture(JNIEnv* env,
namespace {
void InvokeVisualStateCallback(const JavaObjectWeakGlobalRef& java_ref,
long request_id,
ScopedJavaGlobalRef<jobject>* callback,
bool result) {
long request_id,
ScopedJavaGlobalRef<jobject>* callback,
bool result) {
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = java_ref.get(env);
if (obj.is_null())
return;
Java_AwContents_invokeVisualStateCallback(
env, obj.obj(), callback->obj(), request_id, result);
env, obj.obj(), callback->obj(), request_id);
}
} // namespace
......
content/renderer/gpu/frame_swap_message_queue.cc
View file @ 8d503c0a
......@@ -161,10 +161,14 @@ void FrameSwapMessageQueue::DidNotSwap(int source_frame_number,
case cc::SwapPromise::SWAP_FAILS:
case cc::SwapPromise::COMMIT_NO_UPDATE:
swap_queue_->DrainMessages(source_frame_number, messages);
// fallthrough
case cc::SwapPromise::COMMIT_FAILS:
visual_state_queue_->DrainMessages(source_frame_number, messages);
break;
case cc::SwapPromise::COMMIT_FAILS:
// Do not queue any responses here.
// If COMMIT_FAILS the renderer is shutting down, which will
// result in the RenderFrameHostImpl destructor firing the
// remaining response callbacks itself.
break;
default:
NOTREACHED();
}
......
content/renderer/gpu/frame_swap_message_queue_unittest.cc
View file @ 8d503c0a
......@@ -237,13 +237,11 @@ TEST_F(FrameSwapMessageQueueTest, TestDidNotSwapCommitFails) {
QueueVisualStateMessage(3, CloneMessage(third_message_));
queue_->DidNotSwap(2, cc::SwapPromise::COMMIT_FAILS, &messages);
ASSERT_EQ(1u, messages.size());
ASSERT_TRUE(HasMessageForId(messages, second_message_.routing_id()));
ASSERT_EQ(0u, messages.size());
messages.clear();
queue_->DidNotSwap(3, cc::SwapPromise::COMMIT_FAILS, &messages);
ASSERT_EQ(1u, messages.size());
ASSERT_TRUE(HasMessageForId(messages, third_message_.routing_id()));
ASSERT_EQ(0u, messages.size());
messages.clear();
DrainMessages(1, &messages);
......
content/renderer/gpu/queue_message_swap_promise_unittest.cc
View file @ 8d503c0a
......@@ -294,8 +294,34 @@ TEST_F(QueueMessageSwapPromiseTest, VisalStateSwapPromiseDidNotSwapNoUpdate) {
}
TEST_F(QueueMessageSwapPromiseTest,
VisalStateSwapPromiseDidNotSwapCommitFails) {
VisualStateSwapPromiseDidNotSwap(cc::SwapPromise::COMMIT_FAILS);
VisualStateSwapPromiseDidNotSwapCommitFails) {
// COMMIT_FAILS is treated differently:
// If we fail to swap with COMMIT_FAILS, then the renderer is
// shutting down, which implies that the RenderFrameHostImpl
// destructor will eventually be called, firing the remaining
// response callbacks (with swap_success = false) itself.
QueueMessageData data[] = {
/* { policy, source_frame_number } */
{MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE, 1},
{MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE, 1},
{MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE, 2},
};
QueueMessages(data, arraysize(data));
promises_[0]->DidNotSwap(cc::SwapPromise::COMMIT_FAILS);
ASSERT_FALSE(promises_[1]);
EXPECT_TRUE(NextSwapMessages().empty());
EXPECT_EQ(0u, DirectSendMessages().size());
EXPECT_FALSE(ContainsMessage(DirectSendMessages(), messages_[0]));
EXPECT_FALSE(ContainsMessage(DirectSendMessages(), messages_[1]));
EXPECT_FALSE(ContainsMessage(DirectSendMessages(), messages_[2]));
promises_[2]->DidNotSwap(cc::SwapPromise::COMMIT_FAILS);
EXPECT_TRUE(NextSwapMessages().empty());
EXPECT_FALSE(ContainsMessage(DirectSendMessages(), messages_[2]));
EXPECT_TRUE(NextSwapMessages().empty());
EXPECT_FALSE(frame_swap_message_queue_->Empty());
}
TEST_F(QueueMessageSwapPromiseTest, VisalStateSwapPromiseDidNotSwapSwapFails) {
......
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