Issue 61753005: Remove SVGLocatable and SVGTransformable

Issue 61753005: Remove SVGLocatable and SVGTransformable (Closed)

Created:
7 years, 1 month ago by rwlbuis

Modified:
7 years, 1 month ago

Reviewers:
pdr., Stephen Chennney, fmalita_google_do_not_use

CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, Mike Lawther (Google), dstockwell, Timothy Loh, f(malita), jchaffraix+rendering, darktears, pdr, Stephen Chennney, Steve Block, dino_apple.com, Eric Willigers

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Remove SVGLocatable and SVGTransformable These interfaces are removed from SVG2 spec. Move interfaces to SVGGraphicsElement and parsing utilities to SVGParserUtilities. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161755

Patch Set 1 #

Total comments: 16

Patch Set 2 : Fix nits #

Total comments: 2

Patch Set 3 : Patch for landing #

Patch Set 4 : Fix farthestVieportElement #

Created: 7 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+316 lines, -506 lines)			Patch
M	Source/core/core.gypi	View	1	2 chunks	+0 lines, -3 lines	0 comments	Download
M	Source/core/rendering/svg/RenderSVGModelObject.cpp	View	1 2	4 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/core/rendering/svg/RenderSVGResourceContainer.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/svg/RenderSVGShape.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGAnimateTransformElement.cpp	View		2 chunks	+3 lines, -2 lines	0 comments	Download
M	Source/core/svg/SVGElement.h	View	1	2 chunks	+5 lines, -2 lines	0 comments	Download
M	Source/core/svg/SVGElement.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGGradientElement.cpp	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/svg/SVGGraphicsElement.h	View	1 2	1 chunk	+9 lines, -7 lines	0 comments	Download
M	Source/core/svg/SVGGraphicsElement.cpp	View	1 2 3	2 chunks	+61 lines, -4 lines	0 comments	Download
D	Source/core/svg/SVGLocatable.h	View		1 chunk	+0 lines, -63 lines	0 comments	Download
D	Source/core/svg/SVGLocatable.cpp	View		1 chunk	+0 lines, -103 lines	0 comments	Download
M	Source/core/svg/SVGParserUtilities.h	View		3 chunks	+15 lines, -0 lines	0 comments	Download
M	Source/core/svg/SVGParserUtilities.cpp	View		2 chunks	+205 lines, -0 lines	0 comments	Download
M	Source/core/svg/SVGPatternElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGPatternElement.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGSVGElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGSVGElement.cpp	View	1 2	3 chunks	+3 lines, -5 lines	0 comments	Download
M	Source/core/svg/SVGTransformList.cpp	View		2 chunks	+3 lines, -3 lines	0 comments	Download
D	Source/core/svg/SVGTransformable.h	View		1 chunk	+0 lines, -60 lines	0 comments	Download
D	Source/core/svg/SVGTransformable.cpp	View		1 chunk	+0 lines, -240 lines	0 comments	Download
M	Source/core/svg/SVGViewSpec.cpp	View		2 chunks	+1 line, -2 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

pdr.

Fantastic change! Mostly just nits but lets do one more round. https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGElement.cpp File Source/core/svg/SVGElement.cpp (right): ...

7 years, 1 month ago (2013-11-08 23:50:48 UTC) #2

rwlbuis

Try to fix the nits. https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGElement.cpp File Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGElement.cpp#newcode320 Source/core/svg/SVGElement.cpp:320: AffineTransform SVGElement::localCoordinateSpaceTransform(SVGElement::CTMScope) const On ...

7 years, 1 month ago (2013-11-10 20:35:44 UTC) #3

pdr.

LGTM with three requests. https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGGraphicsElement.cpp File Source/core/svg/SVGGraphicsElement.cpp (right): https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGGraphicsElement.cpp#newcode213 Source/core/svg/SVGGraphicsElement.cpp:213: || node->hasTagName(SVGNames::imageTag)); On 2013/11/10 20:35:45, ...

7 years, 1 month ago (2013-11-11 00:02:08 UTC) #4

LGTM with three requests.

https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGGraphicsEl...
File Source/core/svg/SVGGraphicsElement.cpp (right):

https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGGraphicsEl...
Source/core/svg/SVGGraphicsElement.cpp:213: ||
node->hasTagName(SVGNames::imageTag));
On 2013/11/10 20:35:45, rwlbuis wrote:
> On 2013/11/08 23:50:48, pdr wrote:
> > This was probably a latent bug. Images don't have viewports.
> > 
> > Should marker and pattern be included in this list as well?
> 
> I am not sure, this list is stated here:
> 
> http://www.w3.org/TR/SVG11/coords.html#EstablishingANewViewport

Ah, you are correct. It looks like viewports and viewBoxes are slightly
different.

https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGGraphicsEl...
File Source/core/svg/SVGGraphicsElement.h (right):

https://codereview.chromium.org/61753005/diff/1/Source/core/svg/SVGGraphicsEl...
Source/core/svg/SVGGraphicsElement.h:41: AffineTransform
getTransformToElement(SVGElement*, ExceptionState&, StyleUpdateStrategy =
AllowStyleUpdate);
On 2013/11/10 20:35:45, rwlbuis wrote:
> On 2013/11/08 23:50:48, pdr wrote:
> > Is the StyleUpdateStrategy parameter needed?
> 
> I think so.

No, it's not needed. Just remove the updateStrategy parameter and replace it
with AllowStyleUpdate in the function.

https://codereview.chromium.org/61753005/diff/90001/Source/core/svg/SVGGraphi...
File Source/core/svg/SVGGraphicsElement.h (right):

https://codereview.chromium.org/61753005/diff/90001/Source/core/svg/SVGGraphi...
Source/core/svg/SVGGraphicsElement.h:44: static SVGElement*
nearestViewportElement(const SVGElement*);
I still don't think this is necessary.

There are now four callers of this:
1) SVGGraphicsElement::nearestViewportElement() - replace with the static
version
2) RenderSVGModelObject::getElementCTM() - the only callers of this pass in an
SVGGraphicsElement. Lets just replace the generic SVGElement* parameters with
SVGGraphicsElement*.
3) SVGSVGElement::setupInitialView uses nearestViewportElement but then
immediately checks for isSVGElement. This can just be refactored to look for the
nearest SVG element.
4) SVGGraphicsElement::computeCTM() - replace SVGElement* parameter with
SVGGraphicsElement*.

https://codereview.chromium.org/61753005/diff/90001/Source/core/svg/SVGGraphi...
Source/core/svg/SVGGraphicsElement.h:46: virtual AffineTransform
localCoordinateSpaceTransform(SVGElement::CTMScope) const { return
animatedLocalTransform(); }
Please add OVERRIDE

virtual AffineTransform localCoordinateSpaceTransform(SVGElement::CTMScope)
const OVERRIDE { ... etc