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

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted 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 355114: Patch

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




--- Comment #39 from Dean Jackson <dino at apple.com> ---
Comment on attachment 355114
  --> https://bugs.webkit.org/attachment.cgi?id=355114
Patch

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

Reviewing this while everyone else is on a break. They should double check when
they come back.

> Source/WebCore/ChangeLog:13
> +	   This class adds a new member to RenderStyle. To stop this from
increasing the size of RenderStyle, this patch uses #pragma pack
> +	   to pack RenderStyle, shrinking it from 88 bytes to 86.

I'm r+ing this, but Antti should confirm this is ok.

> Source/WebCore/rendering/style/RenderStyle.cpp:87
> +static_assert(sizeof(RenderStyle) <= sizeof(SameSizeAsRenderStyle),
"RenderStyle should stay small");

Don't we normally change SameSizeAsRenderStyle? I guess this would allow it to
grow in the future, as long as we never exceed that value.

> Source/WebCore/rendering/style/RenderStyle.cpp:155
> +	   auto isNew = customPaintWatchedPropertiesMap().add(this,
std::make_unique<HashSet<String>>(WTFMove(*customPaintWatchedPropertiesMap().ge
t(&other))));

Even though you are always expecting this to create a new entry, I think you
should use .ensure() anyway.

> Source/WebCore/rendering/style/RenderStyle.cpp:186
> +	   auto isNew = customPaintWatchedPropertiesMap().add(this,
std::make_unique<HashSet<String>>(WTFMove(*customPaintWatchedPropertiesMap().ge
t(&other))));

Same here.

> Source/WebCore/rendering/style/RenderStyle.cpp:275
> +	   auto isNew = customPaintWatchedPropertiesMap().add(this,
std::make_unique<HashSet<String>>(*customPaintWatchedPropertiesMap().get(&other
)));
> +	   ASSERT_UNUSED(isNew, isNew);

And here.

> Source/WebCore/rendering/style/RenderStyle.cpp:297
> +	   auto isNew = customPaintWatchedPropertiesMap().add(this,
std::make_unique<HashSet<String>>(WTFMove(*customPaintWatchedPropertiesMap().ge
t(&a))));
> +	   ASSERT_UNUSED(isNew, isNew);

Ditto.

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

And one more.