Make supportsMarkers non virtual

Make supportsMarkers non virtual

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

[rwlbuis, 2014-03-19 14:35:02]

It seems to make more sense to test whether the element can handle markers in
one place, hence this refactor :)

[f(malita), 2014-03-19 14:58:55]

https://codereview.chromium.org/203193008/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResources.cpp (right):

https://codereview.chromium.org/203193008/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResources.cpp:240: if (element->supportsMarkers()
&& style->hasMarkers()) {
Let's invert the order here - I suspect style->hasMarkers() is cheaper now.

https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:273: return s_tagList.contains(localName());
We're replacing a virtual call with a hashset lookup during shape layout. Not
sure whether this is noticeable, but we should double check - can you run the
SVG perf tests locally and make sure we're not introducing regressions?

https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.h:58: bool supportsMarkers() const;
This is now just a tag check and the only clients are
RenderSVGShape::shouldGenerateMarkerPositions() and
SVGResources::buildResources(). I don't think we need to expose it in SVGElement
anymore, it can be a SVGRenderSupport helper.

[rwlbuis, 2014-03-19 15:28:04]

On 2014/03/19 14:58:55, Florin Malita wrote:
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/rendering/sv...
> File Source/core/rendering/svg/SVGResources.cpp (right):
> 
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/SVGResources.cpp:240: if (element->supportsMarkers()
> && style->hasMarkers()) {
> Let's invert the order here - I suspect style->hasMarkers() is cheaper now.

Makes sense, done in patch set 3.

>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> Source/core/svg/SVGElement.cpp:273: return s_tagList.contains(localName());
> We're replacing a virtual call with a hashset lookup during shape layout. Not
> sure whether this is noticeable, but we should double check - can you run the
> SVG perf tests locally and make sure we're not introducing regressions?

Will run some tests later.

>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> File Source/core/svg/SVGElement.h (right):
> 
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> Source/core/svg/SVGElement.h:58: bool supportsMarkers() const;
> This is now just a tag check and the only clients are
> RenderSVGShape::shouldGenerateMarkerPositions() and
> SVGResources::buildResources(). I don't think we need to expose it in
SVGElement
> anymore, it can be a SVGRenderSupport helper.

Makes sense, done in patch set 3.

[rwlbuis, 2014-03-19 16:05:30]

>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> > File Source/core/svg/SVGElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> > Source/core/svg/SVGElement.cpp:273: return s_tagList.contains(localName());
> > We're replacing a virtual call with a hashset lookup during shape layout.
Not
> > sure whether this is noticeable, but we should double check - can you run
the
> > SVG perf tests locally and make sure we're not introducing regressions?
> 
> Will run some tests later.

I tried and on my linux desktop saw no difference on the overall SVG perf tests
nor running the HereGear.html test (only one which uses markers).

[f(malita), 2014-03-19 16:07:51]

On 2014/03/19 16:05:30, rwlbuis wrote:
> >
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> > > File Source/core/svg/SVGElement.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/203193008/diff/20001/Source/core/svg/SVGEleme...
> > > Source/core/svg/SVGElement.cpp:273: return
s_tagList.contains(localName());
> > > We're replacing a virtual call with a hashset lookup during shape layout.
> Not
> > > sure whether this is noticeable, but we should double check - can you run
> the
> > > SVG perf tests locally and make sure we're not introducing regressions?
> > 
> > Will run some tests later.
> 
> I tried and on my linux desktop saw no difference on the overall SVG perf
tests
> nor running the HereGear.html test (only one which uses markers).

Thanks, LGTM.

[rwlbuis, 2014-03-19 16:12:09]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-03-19 16:12:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/203193008/40001

[commit-bot: I haz the power, 2014-03-19 16:12:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 16:12:25]

Failed to apply patch for Source/core/rendering/svg/SVGResources.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/svg/SVGResources.cpp
  Hunk #3 FAILED at 250.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/rendering/svg/SVGResources.cpp.rej

Patch:       Source/core/rendering/svg/SVGResources.cpp
Index: Source/core/rendering/svg/SVGResources.cpp
diff --git a/Source/core/rendering/svg/SVGResources.cpp
b/Source/core/rendering/svg/SVGResources.cpp
index
f59ad854bb75e8a014276dfaed8e044e4a491694..1c3b6aae16ae6ee84d83a92ac27674ba4ce9aa86
100644
--- a/Source/core/rendering/svg/SVGResources.cpp
+++ b/Source/core/rendering/svg/SVGResources.cpp
@@ -91,7 +91,7 @@ static HashSet<AtomicString>& clipperFilterMaskerTags()
     return s_tagList;
 }
 
-static HashSet<AtomicString>& markerTags()
+bool SVGResources::supportsMarkers(const SVGElement& element)
 {
     DEFINE_STATIC_LOCAL(HashSet<AtomicString>, s_tagList, ());
     if (s_tagList.isEmpty()) {
@@ -101,7 +101,7 @@ static HashSet<AtomicString>& markerTags()
         s_tagList.add(SVGNames::polylineTag.localName());
     }
 
-    return s_tagList;
+    return s_tagList.contains(element.localName());
 }
 
 static HashSet<AtomicString>& fillAndStrokeTags()
@@ -250,7 +250,7 @@ PassOwnPtr<SVGResources> SVGResources::buildResources(const
RenderObject* object
         }
     }
 
-    if (markerTags().contains(tagName) && style->hasMarkers()) {
+    if (style->hasMarkers() && supportsMarkers(*element)) {
         AtomicString markerStartId(style->markerStartResource());
         if
(!ensureResources(resources)->setMarkerStart(getRenderSVGResourceById<RenderSVGResourceMarker>(document,
markerStartId)))
             registerPendingResource(extensions, markerStartId, element);

[rwlbuis, 2014-03-19 16:56:34]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-03-19 16:56:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/203193008/60001

[commit-bot: I haz the power, 2014-03-19 18:15:46]

Change committed as 169568