[webkit-reviews] review denied: [Bug 191443] CSS Painting API should pass 'this' correctly to paint callback, and repaint when properties change. : [Attachment 354829] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 191443: CSS Painting API should pass 'this' correctly to paint callback,
and repaint when properties change.
https://bugs.webkit.org/show_bug.cgi?id=191443

Attachment 354829: Patch

https://bugs.webkit.org/attachment.cgi?id=354829&action=review




--- Comment #29 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 354829
  --> https://bugs.webkit.org/attachment.cgi?id=354829
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354829&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:86
> +    bool hasCustomPaintWatchProperties;

Is it really OK to increase the size of RenderStyle? Clearly we care about its
size given the static_assert.

> Source/WebCore/rendering/style/RenderStyle.cpp:157
> +	   m_hasCustomPaintWatchProperties = true;

Is this flag just to avoid a HashMap lookup?

> Source/WebCore/rendering/style/RenderStyle.cpp:180
> +    if (other.m_hasCustomPaintWatchProperties) {

What if this->m_hasCustomPaintWatchProperties is true, shouldn't use remove
this from customPaintWatchedPropertiesMap()?

You would have potentially found this bug if you had an assertion whenever you
call customPaintWatchedPropertiesMap().add() to make sure we added new entry.

> Source/WebCore/rendering/style/RenderStyle.cpp:1052
> +	   customPaintWatchedPropertiesMap().add(this,
std::make_unique<HashSet<String>>());

We use HashMap::ensure() to make sure we avoid expensive operations such as
std::make_unique<HashSet<String>>() when the key already exists.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:49
> +    struct PaintDefinition : public CanMakeWeakPtr<PaintDefinition> {

struct PaintDefinition : public CanMakeWeakPtr<PaintDefinition> {
WTF_MAKE_FAST_ALLOCATED;
public:

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:57
> +	   PaintDefinition(const AtomicString& name, JSC::JSObject*
paintConstructor, Ref<CSSPaintCallback>&&, Vector<String>&& inputProperties,
Vector<String> inputArguments);

This usually comes *before* the members.

Vector<String> should not be passed by value.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:77
> +	   auto locker = holdLock(paintDefinitionLock());

#if !ASSERT_DISABLED
before this.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:78
> +	   ASSERT(paintDefinitionMap().isEmpty());

#endif
after this. to avoid locking when assertions are disabled.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:55
> +    ,
m_identifier(generateObjectIdentifier<WorkletGlobalScopeIdentifierType>())

What is this identifier used for? If it is only a key in the HashMap, we could
presumably store |this| as key?