Hook up CSSPaintValue::image to CSS Paint API callback.

Hook up CSSPaintValue::image to CSS Paint API callback.

This introduces the plumbing required for custom paint.

PaintGeneratedImage - Holds onto a SkPicture for painting. This is
produced as the result of custom paint.

CSSPaintImageGeneratorImpl - Generates a PaintGeneratedImage based on a
LayoutObject and size. Invokes the custom paint callback inside
CSSPaintDefinition.

BUG=578252
Committed: https://crrev.com/065941e38a7ec787b7c1e13e50128816e1d422eb
Cr-Commit-Position: refs/heads/master@{#389838}

[ikilpatrick, 2016-04-06 04:44:37]

ikilpatrick@chromium.org changed reviewers:
+ chrishtr@chromium.org, junov@chromium.org

[ikilpatrick, 2016-04-06 04:44:38]

Justin, Chris sending this to you first for reviewing paint&graphics related
changes.

Justin - I've just added some basic 'smoke' tests for the
PaintRenderingContext2d.

I'll add in haraken for bindings, and timloh for style changes after your
initial review. :)

[Justin Novosad, 2016-04-06 15:52:45]

platform/graphics and modules/canvas2d lgtm
I really like the test design too.

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-simple-paint-worklet.js
(right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-simple-paint-worklet.js:16:
testRunner.notifyDone();
I am a bit concerned about this call to notifyDone() because it could  interfere
with tests that want trigger more async work after the worklet has run. Maybe
you could just rename the function to make it clearer that it has async
termination behavior. Something like
"importPaintWorkletAndTerminateTestAfterAsyncPaint", or something shorter that
means the same thing.

[chrishtr, 2016-04-06 18:55:20]

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.h (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.h:17: class
CORE_EXPORT CSSPaintImageGenerator : public
GarbageCollectedFinalized<CSSPaintImageGenerator> {
Why do you need both a CSSPaintImageGenerator and CSSPaintImageGeneratorImpl?

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:25: //
TODO(ikilpatrick): Modify RecordingImageBufferSurface so that it cannot
Cannot fall back, or has a required un-accelerated mode? Don't you
simply require an un-accelerated surface?

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:23:
PassRefPtr<Image> paint(const IntSize&);
This class is under-documented.

Please also add Markdown documentation for this directory, explaining
the class structure and how it implements the CSS paint API. And
link to it from README.md in core/paint/

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h:14: class
PLATFORM_EXPORT PaintGeneratedImage : public GeneratedImage {
Add comments explaining what this class is for.

[ikilpatrick, 2016-04-07 23:02:59]

ikilpatrick@chromium.org changed reviewers:
+ haraken@chromium.org

[ikilpatrick, 2016-04-07 23:03:00]

+haraken for bindings.

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-simple-paint-worklet.js
(right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/csspaint/resources/test-runner-simple-paint-worklet.js:16:
testRunner.notifyDone();
On 2016/04/06 15:52:45, Justin Novosad wrote:
> I am a bit concerned about this call to notifyDone() because it could 
interfere
> with tests that want trigger more async work after the worklet has run. Maybe
> you could just rename the function to make it clearer that it has async
> termination behavior. Something like
> "importPaintWorkletAndTerminateTestAfterAsyncPaint", or something shorter that
> means the same thing.

I couldn't think of a shorter name :) done.

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.h (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.h:17: class
CORE_EXPORT CSSPaintImageGenerator : public
GarbageCollectedFinalized<CSSPaintImageGenerator> {
On 2016/04/06 18:55:20, chrishtr wrote:
> Why do you need both a CSSPaintImageGenerator and CSSPaintImageGeneratorImpl?

Source/core cannot depend on Source/modules. Hence defining the interface in
core, then implementation in modules.
An analogous class is core/html/canvas/CanvasRenderingContext.h &
modules/canvas2d/CanvasRenderingContext2D.h

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:25: //
TODO(ikilpatrick): Modify RecordingImageBufferSurface so that it cannot
On 2016/04/06 18:55:20, chrishtr wrote:
> Cannot fall back, or has a required un-accelerated mode? Don't you
> simply require an un-accelerated surface?

At the moment when you invoke RecordingImageBufferSurface#getPicture it can
potentially fallback to a UnaccelleratedImageBufferSurface for a few reasons (it
assumes that it'll be reused, always has a prev/next frame).

See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I've talked with junov@ about modifying RecordingIBS to prevent this, then this
can disappear.

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:23:
PassRefPtr<Image> paint(const IntSize&);
On 2016/04/06 18:55:20, chrishtr - vacation wrote:
> This class is under-documented.
> 
> Please also add Markdown documentation for this directory, explaining
> the class structure and how it implements the CSS paint API. And
> link to it from README.md in core/paint/

PTAL.

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h (right):

https://codereview.chromium.org/1866623002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PaintGeneratedImage.h:14: class
PLATFORM_EXPORT PaintGeneratedImage : public GeneratedImage {
On 2016/04/06 18:55:20, chrishtr - vacation wrote:
> Add comments explaining what this class is for.

Done. PTAL

[haraken, 2016-04-08 04:31:19]

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintValue.h (right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintValue.h:66: Observer
m_paintImageGeneratorObserver;

How about just making CSSPaintValue CSSPaintImageGenerator::Observer instead of
introducing a dedicated Observer class?

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:54:
ScopedPersistent<v8::Object> m_instance;

I guess it's a bit nasty to add m_instance to both CSSPaintDefinition and
CSSPaintImageGeneratorImpl. Why can't we just make CSSPaintDefinition hold a
strong reference?

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h
(right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h:38:
Observer* m_observer;

Can you make the Observer a GarbageCollectedMixin and use Member<Observer> here?

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h:41:
ScopedPersistent<v8::Object> m_instance;

Who clears out m_instance when the worklet dies? How is it guaranteed that
m_instance does not cause a cycle?

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:110:
Member<GeneratorHashSet> set = m_pendingGenerators.get(name);

Member<GeneratorHashSet> => GeneratorHashSet*

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:112: for
(const WeakMember<CSSPaintImageGeneratorImpl> generator : *set) {

WeakMember<CSSPaintImageGeneratorImpl> => auto&

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.h
(right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.h:41: typedef
HeapHashSet<WeakMember<CSSPaintImageGeneratorImpl>> GeneratorHashSet;

Just to confirm: PaintWorkletGlobalScope is expected to outlive
CSSPaintImageGeneratorImpl, so you're using WeakMembers, right?

[ikilpatrick, 2016-04-08 18:43:21]

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintValue.h (right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintValue.h:66: Observer
m_paintImageGeneratorObserver;
On 2016/04/08 04:31:19, haraken wrote:
> 
> How about just making CSSPaintValue CSSPaintImageGenerator::Observer instead
of
> introducing a dedicated Observer class?

CSSPaintValue inherits from CSSValue which is a traceAfterDispatch class (no
vtable).

I don't think I can make CSSPaintValue inherit from Observer due to this? I.e.
in TRACE would need cast to CSSValue?

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:54:
ScopedPersistent<v8::Object> m_instance;
On 2016/04/08 04:31:19, haraken wrote:
> 
> I guess it's a bit nasty to add m_instance to both CSSPaintDefinition and
> CSSPaintImageGeneratorImpl. Why can't we just make CSSPaintDefinition hold a
> strong reference?
> 

The initial thinking was when a CSSPaintValue was GC'd it would also clean up
the instance if nessecery. This is probably a bit premature, under memory
pressure we'd more likely kill the entire workletGlobalScope.

I've deleted the CSSPaintImageGeneratorImpl and made this persistent.

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h
(right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h:38:
Observer* m_observer;
On 2016/04/08 04:31:19, haraken wrote:
> 
> Can you make the Observer a GarbageCollectedMixin and use Member<Observer>
here?

Made GarbageCollectedFinalized, see comment in CSSPaintValue.

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h:41:
ScopedPersistent<v8::Object> m_instance;
On 2016/04/08 04:31:19, haraken wrote:
> 
> Who clears out m_instance when the worklet dies? How is it guaranteed that
> m_instance does not cause a cycle?
> 

Removed.

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:110:
Member<GeneratorHashSet> set = m_pendingGenerators.get(name);
On 2016/04/08 04:31:19, haraken wrote:
> 
> Member<GeneratorHashSet> => GeneratorHashSet*

Done.

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:112: for
(const WeakMember<CSSPaintImageGeneratorImpl> generator : *set) {
On 2016/04/08 04:31:19, haraken wrote:
> 
> WeakMember<CSSPaintImageGeneratorImpl> => auto&

Done.

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.h
(right):

https://codereview.chromium.org/1866623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.h:41: typedef
HeapHashSet<WeakMember<CSSPaintImageGeneratorImpl>> GeneratorHashSet;
On 2016/04/08 04:31:19, haraken wrote:
> 
> Just to confirm: PaintWorkletGlobalScope is expected to outlive
> CSSPaintImageGeneratorImpl, so you're using WeakMembers, right?

Yup that's correct. Added note.

[haraken, 2016-04-11 02:18:15]

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp:19:
s_createFunction = createFunction;

It's a bit nasty to bypass the core/ => modules/ dependency using a global
variable. Alternately, can we use Supplement<WorkerClient>?

See how IndexedDBClient addresses the dependency problem:
https://codereview.chromium.org/1865213006/

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:60:
v8::MicrotasksScope microtasksScope(isolate,
v8::MicrotasksScope::kRunMicrotasks);

v8::MicrotasksScope is handled by V8ScriptRunner::xxx, so you won't need this.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:72: Geometry*
geom = Geometry::create(size);

geom => geometry

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:81:
ScriptForbiddenScope::AllowUserAgentScript allowScripting;

Why do you need ScriptForbiddenScope::AllowUserAgentScript? It should not be
used unless you really need to execute scripts in a place where script execution
is forbidden.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:82:
v8::TryCatch block(isolate);

You won't need this TryCatch.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:88: if
(block.HasCaught()) {

What happens if the paint function didn't throw a JS exception but didn't
something wrong with painting? It looks a bit strange to special-case a case
where the paint function throws a JS exception (because that wouldn't be the
only case where we should return an invalid image).

Maybe do we want to just remove the block.HasCaught branch and unconditionally
return PaintGeneratedImage::create()?

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:113:
v8::TryCatch block(isolate);

You won't need this v8::TryCatch.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:115: if
(!v8Call(constructor->NewInstance(m_scriptState->context()), paintInstance,
block)) {

Can we use V8ObjectConstructor::newInstance? Then it will handle subtle stuff
like MicrotaskScope etc.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:117: //
TODO(ikilpatrick): messageHandlerInMainThread doesn't currently

Or maybe you can use v8CallOrCrash until you find a better way to handle
scenarios where v8Call fails.

[ikilpatrick, 2016-04-11 22:34:24]

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp:19:
s_createFunction = createFunction;
On 2016/04/11 02:18:14, haraken wrote:
> 
> It's a bit nasty to bypass the core/ => modules/ dependency using a global
> variable. Alternately, can we use Supplement<WorkerClient>?
> 
> See how IndexedDBClient addresses the dependency problem:
> https://codereview.chromium.org/1865213006/

I just wanted to check if this is what you meant before doing this:

Create a Supplementable inside CSSPaintImageGenerator i.e.

class CSSPaintImageGenerator : /* etc */ {
public:
    class SupplementableHost : public Supplementable<SupplementableHost> {
        static SupplentableHost& instance()
        {
            DEFINE_STATIC_LOCAL(/* etc */);
            return instance;
        }
        static const char* supplementName() { return
"CSSPaintImageGenerator::Factory"; }
        static CSSPaintImageGenerator* create(name, document, observer)
        {
            CSSPaintImageGenerator::Factory* factory =
                  static_cast<CSSPaintImageGenerator::Factory*>
                  (Supplement<CSSPaintImageGenerator::Factory>::from(instance(),
supplementName()));
            return factory->create(name, document, observer);
        }
    };

    class Factory {
        virtual CSSPaintImageGenerator* create(name, document, observer);
    }
};

And hook up in ModulesInitializer like:

void ModulesInitializer::initialize()
{
    CSSPaintImageGenerator::SupplementableHost.instance().provideSupplement(
        CSSPaintImageGeneratorImpl::Factory.instance(),
CSSPaintImageGenerator::SupplementableHost.supplementName());
}

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:60:
v8::MicrotasksScope microtasksScope(isolate,
v8::MicrotasksScope::kRunMicrotasks);
On 2016/04/11 02:18:14, haraken wrote:
> 
> v8::MicrotasksScope is handled by V8ScriptRunner::xxx, so you won't need this.

Done.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:72: Geometry*
geom = Geometry::create(size);
On 2016/04/11 02:18:14, haraken wrote:
> 
> geom => geometry

Done.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:81:
ScriptForbiddenScope::AllowUserAgentScript allowScripting;
On 2016/04/11 02:18:14, haraken wrote:
> 
> Why do you need ScriptForbiddenScope::AllowUserAgentScript? It should not be
> used unless you really need to execute scripts in a place where script
execution
> is forbidden.

Done.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:88: if
(block.HasCaught()) {
On 2016/04/11 02:18:14, haraken wrote:
> 
> What happens if the paint function didn't throw a JS exception but didn't
> something wrong with painting? It looks a bit strange to special-case a case
> where the paint function throws a JS exception (because that wouldn't be the
> only case where we should return an invalid image).
> 
> Maybe do we want to just remove the block.HasCaught branch and unconditionally
> return PaintGeneratedImage::create()?

Sorry didn't get "but didn't wrong with painting?"

We talked about this at the Houdini F2F, most people thought that it made more
sense to have an invalid image instead of a partially drawn image for JS Errors.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:113:
v8::TryCatch block(isolate);
On 2016/04/11 02:18:14, haraken wrote:
> 
> You won't need this v8::TryCatch.

Done.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:115: if
(!v8Call(constructor->NewInstance(m_scriptState->context()), paintInstance,
block)) {
On 2016/04/11 02:18:14, haraken wrote:
> 
> Can we use V8ObjectConstructor::newInstance? Then it will handle subtle stuff
> like MicrotaskScope etc.
> 

Done.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:117: //
TODO(ikilpatrick): messageHandlerInMainThread doesn't currently
On 2016/04/11 02:18:14, haraken wrote:
> 
> Or maybe you can use v8CallOrCrash until you find a better way to handle
> scenarios where v8Call fails.
> 

Just removed the log at the moment. Filed crbug.com/602445 to track
messageHandlerInMainThread issue.

[haraken, 2016-04-12 00:44:20]

LGTM

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp:19:
s_createFunction = createFunction;
On 2016/04/11 22:34:23, ikilpatrick wrote:
> On 2016/04/11 02:18:14, haraken wrote:
> > 
> > It's a bit nasty to bypass the core/ => modules/ dependency using a global
> > variable. Alternately, can we use Supplement<WorkerClient>?
> > 
> > See how IndexedDBClient addresses the dependency problem:
> > https://codereview.chromium.org/1865213006/
> 
> I just wanted to check if this is what you meant before doing this:
> 
> Create a Supplementable inside CSSPaintImageGenerator i.e.
> 
> class CSSPaintImageGenerator : /* etc */ {
> public:
>     class SupplementableHost : public Supplementable<SupplementableHost> {
>         static SupplentableHost& instance()
>         {
>             DEFINE_STATIC_LOCAL(/* etc */);
>             return instance;
>         }
>         static const char* supplementName() { return
> "CSSPaintImageGenerator::Factory"; }
>         static CSSPaintImageGenerator* create(name, document, observer)
>         {
>             CSSPaintImageGenerator::Factory* factory =
>                   static_cast<CSSPaintImageGenerator::Factory*>
>                  
(Supplement<CSSPaintImageGenerator::Factory>::from(instance(),
> supplementName()));
>             return factory->create(name, document, observer);
>         }
>     };
> 
>     class Factory {
>         virtual CSSPaintImageGenerator* create(name, document, observer);
>     }
> };
> 
> And hook up in ModulesInitializer like:
> 
> void ModulesInitializer::initialize()
> {
>     CSSPaintImageGenerator::SupplementableHost.instance().provideSupplement(
>         CSSPaintImageGeneratorImpl::Factory.instance(),
> CSSPaintImageGenerator::SupplementableHost.supplementName());
> }
> 

Ah, thanks -- I now understand it would be more nasty than the current static
variable hack...

Final question: Can we use ChromeClient to bypass the dependency from core/ to
modules/?

  frame()->page()->chromeClient()->youCanCallWhateverMethodFromWebLayer();

ChromeClient is (one of the) standard ways to bypass the dependency from core/
to modules/.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:88: if
(block.HasCaught()) {
On 2016/04/11 22:34:23, ikilpatrick wrote:
> On 2016/04/11 02:18:14, haraken wrote:
> > 
> > What happens if the paint function didn't throw a JS exception but didn't
> > something wrong with painting? It looks a bit strange to special-case a case
> > where the paint function throws a JS exception (because that wouldn't be the
> > only case where we should return an invalid image).
> > 
> > Maybe do we want to just remove the block.HasCaught branch and
unconditionally
> > return PaintGeneratedImage::create()?
> 
> Sorry didn't get "but didn't wrong with painting?"
> 
> We talked about this at the Houdini F2F, most people thought that it made more
> sense to have an invalid image instead of a partially drawn image for JS
Errors.

Thanks, makes sense.

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintValue.cpp (right):

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintValue.cpp:54: for (const auto& curr :
clients()) {

for (LayoutObject* client : clients().keys())

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:97: if
(m_invalidConstructor)

I'd replace m_invalidConstructor with m_didCallConstructor, which just checks if
the constructor has been called or not.

Just to confirm: You don't need to call the constructor multiple times, right?
i.e., what happens if a user script updates the constructor function after
instantiating the CSSPaintDefinition object?

[ikilpatrick, 2016-04-12 03:08:07]

ikilpatrick@chromium.org changed reviewers:
+ timloh@chromium.org

[ikilpatrick, 2016-04-12 03:08:09]

+tim for style changes.

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp:19:
s_createFunction = createFunction;
On 2016/04/12 00:44:20, haraken wrote:
> On 2016/04/11 22:34:23, ikilpatrick wrote:
> > On 2016/04/11 02:18:14, haraken wrote:
> > > 
> > > It's a bit nasty to bypass the core/ => modules/ dependency using a global
> > > variable. Alternately, can we use Supplement<WorkerClient>?
> > > 
> > > See how IndexedDBClient addresses the dependency problem:
> > > https://codereview.chromium.org/1865213006/
> > 
> > I just wanted to check if this is what you meant before doing this:
> > 
> > Create a Supplementable inside CSSPaintImageGenerator i.e.
> > 
> > class CSSPaintImageGenerator : /* etc */ {
> > public:
> >     class SupplementableHost : public Supplementable<SupplementableHost> {
> >         static SupplentableHost& instance()
> >         {
> >             DEFINE_STATIC_LOCAL(/* etc */);
> >             return instance;
> >         }
> >         static const char* supplementName() { return
> > "CSSPaintImageGenerator::Factory"; }
> >         static CSSPaintImageGenerator* create(name, document, observer)
> >         {
> >             CSSPaintImageGenerator::Factory* factory =
> >                   static_cast<CSSPaintImageGenerator::Factory*>
> >                  
> (Supplement<CSSPaintImageGenerator::Factory>::from(instance(),
> > supplementName()));
> >             return factory->create(name, document, observer);
> >         }
> >     };
> > 
> >     class Factory {
> >         virtual CSSPaintImageGenerator* create(name, document, observer);
> >     }
> > };
> > 
> > And hook up in ModulesInitializer like:
> > 
> > void ModulesInitializer::initialize()
> > {
> >     CSSPaintImageGenerator::SupplementableHost.instance().provideSupplement(
> >         CSSPaintImageGeneratorImpl::Factory.instance(),
> > CSSPaintImageGenerator::SupplementableHost.supplementName());
> > }
> > 
> 
> Ah, thanks -- I now understand it would be more nasty than the current static
> variable hack...
> 
> Final question: Can we use ChromeClient to bypass the dependency from core/ to
> modules/?
> 
>   frame()->page()->chromeClient()->youCanCallWhateverMethodFromWebLayer();
> 
> ChromeClient is (one of the) standard ways to bypass the dependency from core/
> to modules/.

So at CSSPaintValue create time we don't have access to the frame/page/etc. In
order to do this we'd need to pass in the LayoutObject for all the public
methods that core/ will do access on this class. For example for the
invalidationProperties:

public CSSPaintValue : /* etc */ {
public:
  const Vector<CSSPropertyID>& nativeInvalidationProperties(/* Need to pass in
LayoutObject/Page here? */);
};

Which is a bit confusing as to why you'd need a layoutobject/page/etc.

The nativeInvalidationProperties() method is probably going to be called at
ComputedStyle#needsPaintInvalidationObject which doesn't have access to these
things.

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintValue.cpp (right):

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintValue.cpp:54: for (const auto& curr :
clients()) {
On 2016/04/12 00:44:20, haraken wrote:
> 
> for (LayoutObject* client : clients().keys())

Done.

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (right):

https://codereview.chromium.org/1866623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:97: if
(m_invalidConstructor)
On 2016/04/12 00:44:20, haraken wrote:
> 
> I'd replace m_invalidConstructor with m_didCallConstructor, which just checks
if
> the constructor has been called or not.
> 
> Just to confirm: You don't need to call the constructor multiple times, right?
> i.e., what happens if a user script updates the constructor function after
> instantiating the CSSPaintDefinition object?

Yup we just call the constructor once. In the spec we'll explicitly say that we
grab the constructor function before using it. This is similar to the
web-components model of grabbing all the callbacks upon registration.

See: https://github.com/w3c/webcomponents/issues/417

Changed to m_didCallConstructor.

[haraken, 2016-04-12 04:02:43]

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp (right):

https://codereview.chromium.org/1866623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPaintImageGenerator.cpp:19:
s_createFunction = createFunction;
On 2016/04/12 03:08:09, ikilpatrick wrote:
> On 2016/04/12 00:44:20, haraken wrote:
> > On 2016/04/11 22:34:23, ikilpatrick wrote:
> > > On 2016/04/11 02:18:14, haraken wrote:
> > > > 
> > > > It's a bit nasty to bypass the core/ => modules/ dependency using a
global
> > > > variable. Alternately, can we use Supplement<WorkerClient>?
> > > > 
> > > > See how IndexedDBClient addresses the dependency problem:
> > > > https://codereview.chromium.org/1865213006/
> > > 
> > > I just wanted to check if this is what you meant before doing this:
> > > 
> > > Create a Supplementable inside CSSPaintImageGenerator i.e.
> > > 
> > > class CSSPaintImageGenerator : /* etc */ {
> > > public:
> > >     class SupplementableHost : public Supplementable<SupplementableHost> {
> > >         static SupplentableHost& instance()
> > >         {
> > >             DEFINE_STATIC_LOCAL(/* etc */);
> > >             return instance;
> > >         }
> > >         static const char* supplementName() { return
> > > "CSSPaintImageGenerator::Factory"; }
> > >         static CSSPaintImageGenerator* create(name, document, observer)
> > >         {
> > >             CSSPaintImageGenerator::Factory* factory =
> > >                   static_cast<CSSPaintImageGenerator::Factory*>
> > >                  
> > (Supplement<CSSPaintImageGenerator::Factory>::from(instance(),
> > > supplementName()));
> > >             return factory->create(name, document, observer);
> > >         }
> > >     };
> > > 
> > >     class Factory {
> > >         virtual CSSPaintImageGenerator* create(name, document, observer);
> > >     }
> > > };
> > > 
> > > And hook up in ModulesInitializer like:
> > > 
> > > void ModulesInitializer::initialize()
> > > {
> > >    
CSSPaintImageGenerator::SupplementableHost.instance().provideSupplement(
> > >         CSSPaintImageGeneratorImpl::Factory.instance(),
> > > CSSPaintImageGenerator::SupplementableHost.supplementName());
> > > }
> > > 
> > 
> > Ah, thanks -- I now understand it would be more nasty than the current
static
> > variable hack...
> > 
> > Final question: Can we use ChromeClient to bypass the dependency from core/
to
> > modules/?
> > 
> >   frame()->page()->chromeClient()->youCanCallWhateverMethodFromWebLayer();
> > 
> > ChromeClient is (one of the) standard ways to bypass the dependency from
core/
> > to modules/.
> 
> So at CSSPaintValue create time we don't have access to the frame/page/etc. In
> order to do this we'd need to pass in the LayoutObject for all the public
> methods that core/ will do access on this class. For example for the
> invalidationProperties:
> 
> public CSSPaintValue : /* etc */ {
> public:
>   const Vector<CSSPropertyID>& nativeInvalidationProperties(/* Need to pass in
> LayoutObject/Page here? */);
> };
> 
> Which is a bit confusing as to why you'd need a layoutobject/page/etc.
> 
> The nativeInvalidationProperties() method is probably going to be called at
> ComputedStyle#needsPaintInvalidationObject which doesn't have access to these
> things.

Thanks, I think our conclusion is that we need a better, more generalized
mechanism to bypass the core/ => modules/ dependency.

LGTM.

[Timothy Loh, 2016-04-21 03:43:50]

lgtm

[chrishtr, 2016-04-25 23:07:19]

Some nits below while reading the documentation. (Thanks for adding it.)

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/README.md (right):

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:17: instance is lazily
creating upon first use. If the constructor throws for some reason the
s/creating/created/

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:18: constructor is marked
as invalid and will always produce invalid images.
Is this per spec? I guess there are arguments both ways.

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:25: `Image`s from. This is
done via the `CSSPaintImageGenerator#paint` method. Each `CSSPaintValue`
remove 'from'

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:31: When created the
generator will access paint worklet and lookup it's corresponding
access its paint worklet

[chrishtr, 2016-04-26 01:13:42]

lgtm

Looks good after nits. Thanks for the very thorough patch and well-written
tests, and for
your patience waiting for the review.

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/csspaint/geometry-background-image-tiled.html
(right):

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/geometry-background-image-tiled.html:22:
el.style.width = '200px';
Nit: It would be clearer if you made it 100px x 100px for all three examples.

[ikilpatrick, 2016-04-26 16:57:41]

Thanks for reviewing!

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/csspaint/geometry-background-image-tiled.html
(right):

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/csspaint/geometry-background-image-tiled.html:22:
el.style.width = '200px';
On 2016/04/26 01:13:42, chrishtr wrote:
> Nit: It would be clearer if you made it 100px x 100px for all three examples.

Done.

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/README.md (right):

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:17: instance is lazily
creating upon first use. If the constructor throws for some reason the
On 2016/04/25 23:07:19, chrishtr wrote:
> s/creating/created/

Done.

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:18: constructor is marked
as invalid and will always produce invalid images.
On 2016/04/25 23:07:19, chrishtr wrote:
> Is this per spec? I guess there are arguments both ways.

Going to bring this behaviour up at the next F2F. But yeah, valid arguments both
ways; leaning towards this to prevent constructor trashing when something is
obviously bad.

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:25: `Image`s from. This is
done via the `CSSPaintImageGenerator#paint` method. Each `CSSPaintValue`
On 2016/04/25 23:07:19, chrishtr wrote:
> remove 'from'

Done.

https://codereview.chromium.org/1866623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/README.md:31: When created the
generator will access paint worklet and lookup it's corresponding
On 2016/04/25 23:07:19, chrishtr wrote:
> access its paint worklet

Done.

[ikilpatrick, 2016-04-26 18:20:43]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-04-26 18:20:44]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
haraken@chromium.org, timloh@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1866623002/#ps220001
(title: "addressed comments + rebase.")

[commit-bot: I haz the power, 2016-04-26 18:21:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1866623002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1866623002/220001

[commit-bot: I haz the power, 2016-04-26 19:11:09]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-26 19:13:07]

Description was changed from

==========
Hook up CSSPaintValue::image to CSS Paint API callback.

This introduces the plumbing required for custom paint.

PaintGeneratedImage - Holds onto a SkPicture for painting. This is
produced as the result of custom paint.

CSSPaintImageGeneratorImpl - Generates a PaintGeneratedImage based on a
LayoutObject and size. Invokes the custom paint callback inside
CSSPaintDefinition.

BUG=578252
==========

to

==========
Hook up CSSPaintValue::image to CSS Paint API callback.

This introduces the plumbing required for custom paint.

PaintGeneratedImage - Holds onto a SkPicture for painting. This is
produced as the result of custom paint.

CSSPaintImageGeneratorImpl - Generates a PaintGeneratedImage based on a
LayoutObject and size. Invokes the custom paint callback inside
CSSPaintDefinition.

BUG=578252

Committed: https://crrev.com/065941e38a7ec787b7c1e13e50128816e1d422eb
Cr-Commit-Position: refs/heads/master@{#389838}
==========

[commit-bot: I haz the power, 2016-04-26 19:13:09]

Patchset 12 (id:??) landed as
https://crrev.com/065941e38a7ec787b7c1e13e50128816e1d422eb
Cr-Commit-Position: refs/heads/master@{#389838}