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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for 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 456422: Patch

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




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

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

> Source/WebCore/rendering/PathOperation.cpp:2
> + * Copyright (C) 2012, 2013 Adobe Systems Incorporated. All rights reserved.

Doesn't seem right for a new file.

> Source/WebCore/rendering/PathOperation.h:74
> +    const RefPtr<SVGElement> element() const;

This can return a SVGElement*

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

The argument can be a SVGElement*

> Source/WebCore/rendering/RenderLayer.cpp:1340
> +		   if (pathOperation->type() == PathOperation::Box) {

This should use is<BoxPathOperation>()

> Source/WebCore/rendering/style/RenderStyle.cpp:1537
> +	   case PathOperation::Box: {
> +	       return downcast<BoxPathOperation>(operation).getPath();
> +	   }

You only need braces in case statements if you declare local variables. None of
these cases do.