Commit 329d4921 authored by Xida Chen's avatar Xida Chen Committed by Commit Bot

[PaintWorklet] Build right paint callback according to paint_arguments

In my previous CL:
https://chromium-review.googlesource.com/c/chromium/src/+/879110

I simply did null check for the |paint_arguments|, and return a nullptr
when it is null. There is a better way to handle it, which is to build
the paint callback function without the |paint_arguments| if it is null.

This CL should not change any behavior. We can use the existing tests
to verify this. We already have a PaintWorkletTest for that and a bunch
of layout tests to ensure the correct behavior.

Bug: 803026, 804206
Change-Id: I07b2f58dfe88ccbb5ac27d7268eb228ea101f5fc
Reviewed-on: https://chromium-review.googlesource.com/880886Reviewed-by: default avatarRobert Flack <flackr@chromium.org>
Reviewed-by: default avatarIan Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531396}
parent f3dfbe6f
...@@ -69,9 +69,6 @@ scoped_refptr<Image> CSSPaintDefinition::Paint( ...@@ -69,9 +69,6 @@ scoped_refptr<Image> CSSPaintDefinition::Paint(
const ImageResourceObserver& client, const ImageResourceObserver& client,
const IntSize& container_size, const IntSize& container_size,
const CSSStyleValueVector* paint_arguments) { const CSSStyleValueVector* paint_arguments) {
if (!paint_arguments)
return nullptr;
// TODO: Break dependency on LayoutObject. Passing the Node should work. // TODO: Break dependency on LayoutObject. Passing the Node should work.
const LayoutObject& layout_object = static_cast<const LayoutObject&>(client); const LayoutObject& layout_object = static_cast<const LayoutObject&>(client);
...@@ -104,11 +101,19 @@ scoped_refptr<Image> CSSPaintDefinition::Paint( ...@@ -104,11 +101,19 @@ scoped_refptr<Image> CSSPaintDefinition::Paint(
native_invalidation_properties_, native_invalidation_properties_,
custom_invalidation_properties_); custom_invalidation_properties_);
v8::Local<v8::Value> argv[] = { Vector<v8::Local<v8::Value>, 4> argv;
ToV8(rendering_context, script_state_->GetContext()->Global(), isolate), if (paint_arguments) {
ToV8(paint_size, script_state_->GetContext()->Global(), isolate), argv = {
ToV8(style_map, script_state_->GetContext()->Global(), isolate), ToV8(rendering_context, script_state_->GetContext()->Global(), isolate),
ToV8(*paint_arguments, script_state_->GetContext()->Global(), isolate)}; ToV8(paint_size, script_state_->GetContext()->Global(), isolate),
ToV8(style_map, script_state_->GetContext()->Global(), isolate),
ToV8(*paint_arguments, script_state_->GetContext()->Global(), isolate)};
} else {
argv = {
ToV8(rendering_context, script_state_->GetContext()->Global(), isolate),
ToV8(paint_size, script_state_->GetContext()->Global(), isolate),
ToV8(style_map, script_state_->GetContext()->Global(), isolate)};
}
v8::Local<v8::Function> paint = paint_.NewLocal(isolate); v8::Local<v8::Function> paint = paint_.NewLocal(isolate);
...@@ -117,7 +122,7 @@ scoped_refptr<Image> CSSPaintDefinition::Paint( ...@@ -117,7 +122,7 @@ scoped_refptr<Image> CSSPaintDefinition::Paint(
V8ScriptRunner::CallFunction(paint, V8ScriptRunner::CallFunction(paint,
ExecutionContext::From(script_state_.get()), ExecutionContext::From(script_state_.get()),
instance, WTF_ARRAY_LENGTH(argv), argv, isolate); instance, argv.size(), argv.data(), isolate);
// The paint function may have produced an error, in which case produce an // The paint function may have produced an error, in which case produce an
// invalid image. // invalid image.
......
...@@ -141,8 +141,8 @@ TEST_F(PaintWorkletTest, GarbageCollectionOfCSSPaintDefinition) { ...@@ -141,8 +141,8 @@ TEST_F(PaintWorkletTest, GarbageCollectionOfCSSPaintDefinition) {
// This is a crash test for crbug.com/803026. At some point, we shipped the // This is a crash test for crbug.com/803026. At some point, we shipped the
// CSSPaintAPI without shipping the CSSPaintAPIArguments, the result of it is // CSSPaintAPI without shipping the CSSPaintAPIArguments, the result of it is
// that the |paint_arguments| in the CSSPaintDefinition::Paint() becomes // that the |paint_arguments| in the CSSPaintDefinition::Paint() becomes
// nullptr and we need to null check that. This is a regression test to ensure // nullptr and the renderer crashes. This is a regression test to ensure that
// that we don't crash. // we will never crash.
TEST_F(PaintWorkletTest, PaintWithNullPaintArguments) { TEST_F(PaintWorkletTest, PaintWithNullPaintArguments) {
PaintWorkletGlobalScope* global_scope = GetProxy()->global_scope(); PaintWorkletGlobalScope* global_scope = GetProxy()->global_scope();
global_scope->ScriptController()->Evaluate( global_scope->ScriptController()->Evaluate(
...@@ -157,7 +157,7 @@ TEST_F(PaintWorkletTest, PaintWithNullPaintArguments) { ...@@ -157,7 +157,7 @@ TEST_F(PaintWorkletTest, PaintWithNullPaintArguments) {
const IntSize container_size(100, 100); const IntSize container_size(100, 100);
scoped_refptr<Image> image = scoped_refptr<Image> image =
definition->Paint(*observer, container_size, nullptr); definition->Paint(*observer, container_size, nullptr);
EXPECT_EQ(image, nullptr); EXPECT_NE(image, nullptr);
} }
// In this test, we set a list of "paints_to_switch" numbers, and in each frame, // In this test, we set a list of "paints_to_switch" numbers, and in each frame,
......
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