[webkit-reviews] review denied: [Bug 227801] Implement ::backdrop pseudo element : [Attachment 435411] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Tim Nguyen (:ntim)
<ntim at apple.com>'s request for review:
Bug 227801: Implement ::backdrop pseudo element
https://bugs.webkit.org/show_bug.cgi?id=227801

Attachment 435411: Patch

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




--- Comment #9 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 435411
  --> https://bugs.webkit.org/attachment.cgi?id=435411
Patch

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

Seems pretty good but lets do another round.

> Source/WebCore/rendering/RenderElement.cpp:2378
> +RenderBlockFlow* RenderElement::backdropRenderer() const

This could return WeakPtr for safety.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:895
> +void RenderTreeBuilder::updateBackdropRenderer(RenderElement& renderer)

This could go to RenderTreeBuilder::GeneratedContent and maybe invoked from
GeneratedContent::updatePseudoElement

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:905
> +    auto style = renderer.getCachedPseudoStyle(PseudoId::Backdrop,
renderer.document().renderStyle());

You can use renderer.view().style(), so you don't bounce into DOM side
unnecessarily (it is the same thing).

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:907
> +    if (!style || style->display() == DisplayType::None)
> +	   return;

Shouldn't we destroy an existing renderer in this case? 

If so we probably should have a test.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:911
> +    auto* backdropRenderer = renderer.backdropRenderer();

This should be a WeakPtr. We have had lots of safety issues in render tree
building code.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:923
> +    RenderElement* currentParent = backdropRenderer->parent();
> +    RenderElement* newParent = renderer.parent();

WeakPtrs

> Source/WebCore/style/StyleTreeResolver.cpp:287
> -    
> -    auto& parentStyle = *elementUpdate.style;
> +
> +    auto& parentStyle = pseudoId == PseudoId::Backdrop ?
*element.document().renderStyle() : *elementUpdate.style;
>      auto* parentBoxStyle = parentBoxStyleForPseudo(elementUpdate);
> -    
> +

This is unnecessary as the code doesn't currently handle Backdrop. You could
assert against it.