[webkit-reviews] review denied: [Bug 233382] Support rendering url(), CSS basic shapes other than path(), and coord-box for offset-path : [Attachment 455707] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 233382: Support rendering url(), CSS basic shapes other than path(), and
coord-box for offset-path
https://bugs.webkit.org/show_bug.cgi?id=233382

Attachment 455707: Patch

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




--- Comment #12 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 455707
  --> https://bugs.webkit.org/attachment.cgi?id=455707
Patch

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

> Source/WebCore/rendering/PathOperation.h:71
> +    static Ref<ReferencePathOperation> create(const String&, const String&,
const RefPtr<SVGElement>);

You need to name the string parameters in the header so the reader knows what
they are.

> Source/WebCore/rendering/PathOperation.h:85
> +    ReferencePathOperation(const String&, const String&, const
RefPtr<SVGElement>);

Ditto

> Source/WebCore/rendering/RenderLayer.cpp:1345
> +	   if (is<RenderBox>(renderer())) {
> +	       auto cssBox =
transformBoxToCSSBoxType(renderer().style().transformBox());
> +	       if (auto pathOperation = renderer().style().offsetPath()) {
> +		   if (pathOperation->type() == PathOperation::Box)
> +		       cssBox =
downcast<BoxPathOperation>(pathOperation)->referenceBox();
> +	       }
> +
> +	       referenceBox =
snapRectToDevicePixels(downcast<RenderBox>(renderer()).referenceBox(cssBox),
renderer().document().deviceScaleFactor());
> +	   }

This doesn't seem right. The presence of an offset path shouldn't affect
transform rendering. This needs a testcase.

> Source/WebCore/rendering/style/RenderStyle.cpp:1528
> +    switch (operation.type())
> +    {

Brace on the previous line.

> Source/WebCore/rendering/style/RenderStyle.cpp:1532
> +	   case PathOperation::Reference:
> +	   {

Ditto

> Source/WebCore/rendering/style/RenderStyle.cpp:1538
> +	   case PathOperation::Box:
> +	   {

Ditto

> Source/WebCore/rendering/style/RenderStyle.cpp:1542
> +	   case PathOperation::Ray:
> +	   {

Ditto