Skip to content

Commit

Permalink
[PaintWorklet] Build right paint callback according to paint_arguments
Browse files Browse the repository at this point in the history
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/880886
Reviewed-by: Robert Flack <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Commit-Queue: Xida Chen <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#531396}(cherry picked from commit 329d492)
Reviewed-on: https://chromium-review.googlesource.com/884554
Reviewed-by: Xida Chen <[email protected]>
Cr-Commit-Position: refs/branch-heads/3325@{#83}
Cr-Branched-From: bc084a8-refs/heads/master@{#530369}
  • Loading branch information
xidachen committed Jan 25, 2018
1 parent b88ee05 commit b701c2a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 12 deletions.
23 changes: 14 additions & 9 deletions third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ scoped_refptr<Image> CSSPaintDefinition::Paint(
const ImageResourceObserver& client,
const IntSize& container_size,
const CSSStyleValueVector* paint_arguments) {
if (!paint_arguments)
return nullptr;

// TODO: Break dependency on LayoutObject. Passing the Node should work.
const LayoutObject& layout_object = static_cast<const LayoutObject&>(client);

Expand Down Expand Up @@ -104,11 +101,19 @@ scoped_refptr<Image> CSSPaintDefinition::Paint(
native_invalidation_properties_,
custom_invalidation_properties_);

v8::Local<v8::Value> 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),
ToV8(*paint_arguments, script_state_->GetContext()->Global(), isolate)};
Vector<v8::Local<v8::Value>, 4> argv;
if (paint_arguments) {
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),
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);

Expand All @@ -117,7 +122,7 @@ scoped_refptr<Image> CSSPaintDefinition::Paint(

V8ScriptRunner::CallFunction(paint,
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
// invalid image.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ TEST_F(PaintWorkletTest, GarbageCollectionOfCSSPaintDefinition) {
// 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
// that the |paint_arguments| in the CSSPaintDefinition::Paint() becomes
// nullptr and we need to null check that. This is a regression test to ensure
// that we don't crash.
// nullptr and the renderer crashes. This is a regression test to ensure that
// we will never crash.
TEST_F(PaintWorkletTest, PaintWithNullPaintArguments) {
PaintWorkletGlobalScope* global_scope = GetProxy()->global_scope();
global_scope->ScriptController()->Evaluate(
Expand All @@ -157,7 +157,7 @@ TEST_F(PaintWorkletTest, PaintWithNullPaintArguments) {
const IntSize container_size(100, 100);
scoped_refptr<Image> image =
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,
Expand Down

0 comments on commit b701c2a

Please sign in to comment.