Move toClipPath to SVGGeometryElement.

This CL is that remove toClipPath on SVGGraphicsElement.

BUG=494445
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197629

[hyunjunekim2, 2015-06-06 05:55:50]

hyunjune.kim@samsung.com changed reviewers:
+ pdr@chromium.org

[hyunjunekim2, 2015-06-06 05:57:03]

pdr, About issue 494445, Could you give me advice for PS2?

[pdr., 2015-06-09 05:15:57]

pdr@chromium.org changed reviewers:
+ fmalita@chromium.org, fs@opera.com

[pdr., 2015-06-09 05:15:59]

https://codereview.chromium.org/1154853007/diff/20001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGResourceClipper.cpp (right):

https://codereview.chromium.org/1154853007/diff/20001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGResourceClipper.cpp:87: SVGGeometryElement*
styled = toSVGGeometryElement(childElement);
Because this element can be an SVGUseElement, we'll need to leave this as
toSVGGraphicsElement. We can rename styled to something better though, such as
childGraphicsElement.

https://codereview.chromium.org/1154853007/diff/20001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/1154853007/diff/20001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.h:47: SVGGeometryElement*
layoutObjectClipChild(Path&) const;
Sadness... the original code is worse than I expected and has real bugs. This
may require many patches and I understand if you'd like to work on something
else. If you're up for it, I'll sketch out a good first patch below:

SVGClipPainter::applyClippingToContext has two "modes":
1) A "path-only" approach (tryPathOnlyClipping) which calls
SVGUseElement::toClipPath
2) A mask approach (drawClipMaskContent) which ends up calling
SVGUseElement::layoutObjectClipChild
Our goal is to unify these so they use the same code. The following might be a
good design:

const SVGGraphicsElement* SVGUseElement::targetGraphicsElementForClipping();
You should write a comment in the header file describing what we are returning
and why. You may want to reference the spec, particularly this part: "If a <use>
element is a child of a clipPath element, it must directly reference <path>,
<text> or basic shapes elements." You probably want to actually check for the
allowed references too... something like isSVGTextElement() || isSVGTSpanElement
|| isSVGGeometryElement. You may also want to mention how we will return null if
an allowed element is not available.

Lets leave SVGUseElement::toClipPath for now, but make it non-virtual. Also
change the implementation to call our new targetGraphicsElementForClipping
function, but make sure to check isSVGGeometryElement because path-only clipping
doesn't work for text.

[hyunjunekim2, 2015-06-10 14:00:42]

Thank you for your advice.

https://codereview.chromium.org/1154853007/diff/20001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGResourceClipper.cpp (right):

https://codereview.chromium.org/1154853007/diff/20001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGResourceClipper.cpp:87: SVGGeometryElement*
styled = toSVGGeometryElement(childElement);
On 2015/06/09 05:15:58, pdr wrote:
> Because this element can be an SVGUseElement, we'll need to leave this as
> toSVGGraphicsElement. We can rename styled to something better though, such as
> childGraphicsElement.

Ok, I understand.

https://codereview.chromium.org/1154853007/diff/20001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/1154853007/diff/20001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.h:47: SVGGeometryElement*
layoutObjectClipChild(Path&) const;
I will try again for your advice.

[hyunjunekim2, 2015-06-15 14:38:38]

pdr,
Could you check PS5 whether this way is right and give me a advice?
And I made a function named |targetGraphicsElementForClipping|.
Thank you.

[pdr., 2015-06-17 04:42:13]

This is looking pretty good. Please update the change description to describe
the changes you're making. This will also need a rebase as fmalita just updated
the path ops svg clipping code.

https://codereview.chromium.org/1154853007/diff/80001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/1154853007/diff/80001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:472: if (isSVGTextElement(element) ||
isSVGTSpanElement(element) || element.isSVGGeometryElement())
Lets add a link to the spec here.
// Spec: "If a <use> element is a child of a clipPath element, it must directly
// reference <path>, <text> or basic shapes elements. Indirect references are an
// error and the clipPath element must be ignored."
// http://dev.w3.org/fxtf/css-masking-1/#the-clip-path

Can you also move the isDirectReference check and reportError call here instead
of in toClipPath?

https://codereview.chromium.org/1154853007/diff/80001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:479: LayoutObject*
SVGUseElement::layoutObjectClipChild() const
This function is pretty simple now, can you move this logic into
LayoutSVGResourceClipper::createContentPicture and remove
SVGUseElement::layoutObjectClipChild?

https://codereview.chromium.org/1154853007/diff/80001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.h (right):

https://codereview.chromium.org/1154853007/diff/80001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.h:47: SVGGraphicsElement*
targetGraphicsElementForClipping() const;
Please add a comment describing this.

Something like:
// Return the element that should be used for clipping, or null if a valid clip
element is not directly referenced.

[hyunjunekim2, 2015-06-19 14:35:31]

pdr,
Could you check PS14? Thank you.

[fs, 2015-06-22 10:57:44]

https://codereview.chromium.org/1154853007/diff/240001/Source/core/layout/svg...
File Source/core/layout/svg/LayoutSVGResourceClipper.cpp (right):

https://codereview.chromium.org/1154853007/diff/240001/Source/core/layout/svg...
Source/core/layout/svg/LayoutSVGResourceClipper.cpp:90: SVGGraphicsElement*
styled = toSVGGraphicsElement(childElement);
Could drop this variable, and just use |childElement| below instead.

https://codereview.chromium.org/1154853007/diff/240001/Source/core/layout/svg...
Source/core/layout/svg/LayoutSVGResourceClipper.cpp:192: layoutObject = nullptr;
This is a dead store (layoutObject will go out of scope at the 'continue'
statement.)

https://codereview.chromium.org/1154853007/diff/240001/Source/core/svg/SVGUse...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/1154853007/diff/240001/Source/core/svg/SVGUse...
Source/core/svg/SVGUseElement.cpp:443: void SVGUseElement::toClipPath(Path&
path)
const qualify the method?

https://codereview.chromium.org/1154853007/diff/240001/Source/core/svg/SVGUse...
Source/core/svg/SVGUseElement.cpp:469: if (element.isSVGGraphicsElement()) {
Suggest you turn this into the negation to flatten the control-flow a bit:

if (!element.isSVGGraphicsElement())
    return nullptr;

https://codereview.chromium.org/1154853007/diff/240001/Source/core/svg/SVGUse...
Source/core/svg/SVGUseElement.cpp:475: if (isSVGClipPathElement(element))
There's nothing special with <clipPath> in this case, so remove this.

https://codereview.chromium.org/1154853007/diff/240001/Source/core/svg/SVGUse...
Source/core/svg/SVGUseElement.cpp:478:
document().accessSVGExtensions().reportError("Not allowed to use indirect
reference in <clip-path>");
Add return after this (or the method will still return the element.)

[hyunjunekim2, 2015-06-22 15:28:57]

pdr, fs
Could you check PS15? Thank you.

[fs, 2015-06-22 15:47:38]

The following two are still not addressed, otherwise LGTM

On 2015/06/22 10:57:44, fs wrote:
...
>
https://codereview.chromium.org/1154853007/diff/240001/Source/core/layout/svg...
> Source/core/layout/svg/LayoutSVGResourceClipper.cpp:90: SVGGraphicsElement*
> styled = toSVGGraphicsElement(childElement);
> Could drop this variable, and just use |childElement| below instead.
...
>
https://codereview.chromium.org/1154853007/diff/240001/Source/core/svg/SVGUse...
> Source/core/svg/SVGUseElement.cpp:443: void SVGUseElement::toClipPath(Path&
> path)
> const qualify the method?

[hyunjunekim2, 2015-06-22 23:26:17]

pdr, fs
Could you check PS16 and give me advice? Thank you. :)

[pdr., 2015-06-23 05:53:33]

On 2015/06/22 at 23:26:17, hyunjune.kim wrote:
> pdr, fs
> Could you check PS16 and give me advice? Thank you. :)

LGTM

[pdr., 2015-06-23 05:53:42]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-06-23 05:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1154853007/#ps280001
(title: "Draft 14")

[commit-bot: I haz the power, 2015-06-23 05:53:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154853007/280001

[commit-bot: I haz the power, 2015-06-23 06:56:36]

Committed patchset #16 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197629