Remove isSupportedAttribute in svg

Spring cleaning: remove isSupportedAttribute.

Inspired by Darin Adler's WebKit patch:
https://bugs.webkit.org/show_bug.cgi?id=143194

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

[Erik Dahlström (inactive), 2015-04-10 13:06:06]

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

[Erik Dahlström (inactive), 2015-04-10 13:39:50]

PTAL.

No big surprises in the performance tryrun:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-04...

[fs, 2015-04-10 14:25:24]

On 2015/04/10 13:39:50, Erik Dahlström wrote:
> No big surprises in the performance tryrun:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-04...

Some numbers on binary size/footprint would've been interesting too.

Could've been split up into the "simple and non-controversial" (like most/all of
SVGFE*) and the slightly more difficult (the rest) for a slightly nicer review
experience.

Ought to credit the original somehow too.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimationElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimationElement.cpp:210: SVGTests::parseAttribute(name,
value);
The SVGTests constructor adds all its attributes to the property map, so this
would've been handled by the property-map lookup in SVGElement::parseAttribute.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGCirc...
File Source/core/svg/SVGCircleElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGCirc...
Source/core/svg/SVGCircleElement.cpp:97: 
Drop this blank line.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGComp...
File Source/core/svg/SVGComponentTransferFunctionElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGComp...
Source/core/svg/SVGComponentTransferFunctionElement.cpp:82:
invalidateFilterPrimitiveParent(this);
Will now invalidate the filter on every change - pretty unlikely to get such
updates here in the first place though I suppose...

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGElli...
File Source/core/svg/SVGEllipseElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGElli...
Source/core/svg/SVGEllipseElement.cpp:100: if (lengthAttrs) {
Join these two blocks, and then put the InvalidationGuard in that scope.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGFELi...
File Source/core/svg/SVGFELightElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGFELi...
Source/core/svg/SVGFELightElement.cpp:100: SVGElement::InvalidationGuard
invalidationGuard(this);
Move this down to just before isSVGFEDiffuseLightingElement (where things start
to go down for real.)

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGFELi...
Source/core/svg/SVGFELightElement.cpp:117: }
Should probably move the ASSERT_NOT_REACHED() here now.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGImag...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGImag...
Source/core/svg/SVGImageElement.cpp:116: if (isLengthAttribute) {
I think we should either move this down to just before the other
isLengthAttribute block, or move that one up - which is best will boil down to
the frequency of update. Feels the length attrs should be more common, so I
guess that means moving the later block earlier.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGLine...
File Source/core/svg/SVGLineElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGLine...
Source/core/svg/SVGLineElement.cpp:63: if (isLengthAttribute) {
Merge blocks.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGText...
File Source/core/svg/SVGTextPositioningElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGText...
Source/core/svg/SVGTextPositioningElement.cpp:77: 
Drop.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:558: } else if (attrName ==
SVGNames::beginAttr || attrName == SVGNames::beginAttr) {
One of these should be endAttr.

[Erik Dahlström (inactive), 2015-04-10 15:33:44]

On the subject of footprint, I did a quick comparison. This patch shaves 21K off
content_shell (release).

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimationElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimationElement.cpp:210: SVGTests::parseAttribute(name,
value);
On 2015/04/10 14:25:23, fs wrote:
> The SVGTests constructor adds all its attributes to the property map, so this
> would've been handled by the property-map lookup in
SVGElement::parseAttribute.

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGCirc...
File Source/core/svg/SVGCircleElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGCirc...
Source/core/svg/SVGCircleElement.cpp:97: 
On 2015/04/10 14:25:23, fs wrote:
> Drop this blank line.

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGComp...
File Source/core/svg/SVGComponentTransferFunctionElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGComp...
Source/core/svg/SVGComponentTransferFunctionElement.cpp:82:
invalidateFilterPrimitiveParent(this);
On 2015/04/10 14:25:23, fs wrote:
> Will now invalidate the filter on every change - pretty unlikely to get such
> updates here in the first place though I suppose...

This was not on purpose, will fix.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGElli...
File Source/core/svg/SVGEllipseElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGElli...
Source/core/svg/SVGEllipseElement.cpp:100: if (lengthAttrs) {
On 2015/04/10 14:25:24, fs wrote:
> Join these two blocks, and then put the InvalidationGuard in that scope.

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGFELi...
File Source/core/svg/SVGFELightElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGFELi...
Source/core/svg/SVGFELightElement.cpp:100: SVGElement::InvalidationGuard
invalidationGuard(this);
On 2015/04/10 14:25:24, fs wrote:
> Move this down to just before isSVGFEDiffuseLightingElement (where things
start
> to go down for real.)

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGFELi...
Source/core/svg/SVGFELightElement.cpp:117: }
On 2015/04/10 14:25:24, fs wrote:
> Should probably move the ASSERT_NOT_REACHED() here now.

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGImag...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGImag...
Source/core/svg/SVGImageElement.cpp:116: if (isLengthAttribute) {
On 2015/04/10 14:25:24, fs wrote:
> I think we should either move this down to just before the other
> isLengthAttribute block, or move that one up - which is best will boil down to
> the frequency of update. Feels the length attrs should be more common, so I
> guess that means moving the later block earlier.

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGText...
File Source/core/svg/SVGTextPositioningElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/SVGText...
Source/core/svg/SVGTextPositioningElement.cpp:77: 
On 2015/04/10 14:25:24, fs wrote:
> Drop.

Done.

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/40001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:558: } else if (attrName ==
SVGNames::beginAttr || attrName == SVGNames::beginAttr) {
On 2015/04/10 14:25:24, fs wrote:
> One of these should be endAttr.

%)

Done.

[fs, 2015-04-10 16:09:10]

On 2015/04/10 15:33:44, Erik Dahlström wrote:
> On the subject of footprint, I did a quick comparison. This patch shaves 21K
off
> content_shell (release).

That's pretty decent =).

LGTM, but hopefully pdr has time to thrash through this as well... (lot's of
moved InvalidationGuards)

[pdr., 2015-04-10 18:39:21]

LGTM

https://codereview.chromium.org/1074813002/diff/60001/Source/core/svg/SVGImag...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/60001/Source/core/svg/SVGImag...
Source/core/svg/SVGImageElement.cpp:120: updateRelativeLengthsInformation();
Does this path need to be covered by the invalidation guard (here, and
elsewhere?) The code is inconsistent whether it should be covered with the
invalidation guard.

[Erik Dahlström (inactive), 2015-04-13 08:11:17]

https://codereview.chromium.org/1074813002/diff/60001/Source/core/svg/SVGImag...
File Source/core/svg/SVGImageElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/60001/Source/core/svg/SVGImag...
Source/core/svg/SVGImageElement.cpp:120: updateRelativeLengthsInformation();
On 2015/04/10 18:39:21, pdr wrote:
> Does this path need to be covered by the invalidation guard (here, and
> elsewhere?) The code is inconsistent whether it should be covered with the
> invalidation guard.

Hmm, yes I think it should be covered by that. I had another look at the other
places like this and they looked fine to me, tried to make things a bit more
consistent in the latest patchset.

https://codereview.chromium.org/1074813002/diff/80001/Source/core/svg/SVGSVGE...
File Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1074813002/diff/80001/Source/core/svg/SVGSVGE...
Source/core/svg/SVGSVGElement.cpp:324: SVGElement::InvalidationGuard
invalidationGuard(this);
Note that all the above branches sets updateRelativeLengthsOrViewBox=true, so
we'll hit this codepath for all of the attributes that are handled here.

[pdr., 2015-04-13 18:11:33]

On 2015/04/13 at 08:11:17, ed wrote:
>
https://codereview.chromium.org/1074813002/diff/60001/Source/core/svg/SVGImag...
> File Source/core/svg/SVGImageElement.cpp (right):
> 
>
https://codereview.chromium.org/1074813002/diff/60001/Source/core/svg/SVGImag...
> Source/core/svg/SVGImageElement.cpp:120: updateRelativeLengthsInformation();
> On 2015/04/10 18:39:21, pdr wrote:
> > Does this path need to be covered by the invalidation guard (here, and
> > elsewhere?) The code is inconsistent whether it should be covered with the
> > invalidation guard.
> 
> Hmm, yes I think it should be covered by that. I had another look at the other
places like this and they looked fine to me, tried to make things a bit more
consistent in the latest patchset.
> 
>
https://codereview.chromium.org/1074813002/diff/80001/Source/core/svg/SVGSVGE...
> File Source/core/svg/SVGSVGElement.cpp (right):
> 
>
https://codereview.chromium.org/1074813002/diff/80001/Source/core/svg/SVGSVGE...
> Source/core/svg/SVGSVGElement.cpp:324: SVGElement::InvalidationGuard
invalidationGuard(this);
> Note that all the above branches sets updateRelativeLengthsOrViewBox=true, so
we'll hit this codepath for all of the attributes that are handled here.

LGTM2

[Erik Dahlström (inactive), 2015-04-13 18:32:20]

The CQ bit was checked by ed@opera.com

[Erik Dahlström (inactive), 2015-04-13 18:32:20]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
fs@opera.com
Link to the patchset: https://codereview.chromium.org/1074813002/#ps80001
(title: "invalidation guard tweaks")

[commit-bot: I haz the power, 2015-04-13 18:32:45]

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

[commit-bot: I haz the power, 2015-04-13 20:18:33]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193650