Issue 963733002: [svg2] Make 'width' and 'height' presentation attributes

Issue 963733002: [svg2] Make 'width' and 'height' presentation attributes (Closed)

Created:
5 years, 9 months ago by Erik Dahlström (inactive)

Modified:
5 years, 9 months ago

Reviewers:
pdr., fs, Timothy Loh

CC:
blink-reviews, shans, eae+blinkwatch, fs, Steve Block, rwlbuis, krit, dstockwell, Timothy Loh, jchaffraix+rendering, pdr+svgwatchlist_chromium.org, Eric Willigers, rjwright, zoltan1, Dominik Röttsches, gyuyoung.kim_webkit.org, darktears, blink-reviews-rendering, blink-reviews-animation_chromium.org, pdr+renderingwatchlist_chromium.org, kouhei+svg_chromium.org, leviw+renderwatch, Mike Lawther (Google), ed+blinkwatch_opera.com, f(malita), Stephen Chennney

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

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

[svg2] Make 'width' and 'height' presentation attributes The following elements' width and height attributes have been made into presentation attributes: * mask * rect * image * foreignObject BUG=400725 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192013

Patch Set 1 #

Patch Set 2 : drop <pattern> from the set #

Patch Set 3 : test fixes #

Total comments: 16

Patch Set 4 : codereview fixes #

Total comments: 2

Patch Set 5 : rebase, drop assert and add issue link #

Patch Set 6 : ouch :) #

Patch Set 7 : rebase + add svg-style expectation #

Patch Set 8 : rebaseline some EX tests [Mac Win] #

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+109 lines, -81 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6 7	1 chunk	+5 lines, -0 lines	0 comments	Download
M	LayoutTests/inspector/elements/styles/svg-style-expected.txt	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	LayoutTests/svg/animations/attributeTypes-expected.txt	View		4 chunks	+6 lines, -6 lines	0 comments	Download
M	LayoutTests/svg/animations/resources/attributeTypes.svg	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/svg/animations/script-tests/attributeTypes.js	View		3 chunks	+5 lines, -5 lines	0 comments	Download
M	LayoutTests/svg/custom/mask-excessive-malloc-expected.txt	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/svg/custom/pattern-excessive-malloc-expected.txt	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/css/CSSPrimitiveValue.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/css/CSSPrimitiveValue.cpp	View	1 2 3	2 chunks	+6 lines, -1 line	0 comments	Download
M	Source/core/layout/svg/LayoutSVGForeignObject.cpp	View	1 2 3	1 chunk	+6 lines, -2 lines	0 comments	Download
M	Source/core/layout/svg/LayoutSVGImage.cpp	View	1 2 3	1 chunk	+6 lines, -2 lines	0 comments	Download
M	Source/core/layout/svg/LayoutSVGRect.cpp	View	1 2	2 chunks	+5 lines, -3 lines	0 comments	Download
M	Source/core/layout/svg/SVGPathData.cpp	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	Source/core/svg/SVGImageElement.cpp	View	1 2 3 4	1 chunk	+5 lines, -7 lines	0 comments	Download
M	Source/core/svg/SVGLengthContext.cpp	View	1 2 3 4 5	3 chunks	+29 lines, -14 lines	0 comments	Download
M	Source/core/svg/SVGMaskElement.cpp	View	1 2	3 chunks	+12 lines, -9 lines	0 comments	Download
M	Source/core/svg/SVGRectElement.cpp	View	1 2 3 4	4 chunks	+12 lines, -24 lines	0 comments	Download

Messages

Total messages: 13 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

https://codereview.chromium.org/963733002/diff/40001/Source/core/css/CSSPrimitiveValue.cpp File Source/core/css/CSSPrimitiveValue.cpp (right): https://codereview.chromium.org/963733002/diff/40001/Source/core/css/CSSPrimitiveValue.cpp#newcode155 Source/core/css/CSSPrimitiveValue.cpp:155: float CSSPrimitiveValue::clampToCSSLengthRange(float value) Reuse this in computeLength too. Should ...

5 years, 9 months ago (2015-03-12 14:28:48 UTC) #3

Erik Dahlström (inactive)

5 years, 9 months ago (2015-03-12 16:14:56 UTC) #4

LGTM w/ nit You can drop the part about <pattern> in the description. Please allow ...

5 years, 9 months ago (2015-03-12 16:40:08 UTC) #5

Erik Dahlström (inactive)

Added timloh for the core/css change, feedback would be most welcome. https://codereview.chromium.org/963733002/diff/40001/Source/core/svg/SVGRectElement.cpp File Source/core/svg/SVGRectElement.cpp (right): ...

5 years, 9 months ago (2015-03-13 09:16:24 UTC) #7

Erik Dahlström (inactive)

On 2015/03/13 09:16:24, Erik Dahlström wrote: > Added timloh for the core/css change, feedback would ...

5 years, 9 months ago (2015-03-16 12:03:35 UTC) #8

On 2015/03/13 09:16:24, Erik Dahlström wrote:
> Added timloh for the core/css change, feedback would be most welcome.
> 
>
https://codereview.chromium.org/963733002/diff/40001/Source/core/svg/SVGRectE...
> File Source/core/svg/SVGRectElement.cpp (right):
> 
>
https://codereview.chromium.org/963733002/diff/40001/Source/core/svg/SVGRectE...
> Source/core/svg/SVGRectElement.cpp:124:
ASSERT(isSupportedAttribute(attrName));
> On 2015/03/12 16:40:07, fs wrote:
> > On 2015/03/12 16:14:56, Erik Dahlström wrote:
> > > On 2015/03/12 14:28:48, fs wrote:
> > > > Probably not the most enlightening assert as long as the method starts
the
> > way
> > > > it does. Maybe there's something else that could be asserted?
> > > > propertyFromAttribute(name)->type() == AnimatedLength?
> > > 
> > > I'm not sure the assert is very useful, it was an attempt at keeping
> something
> > > similar to the ASSERT_NOT_REACHED.
> > 
> > I'm fine with dropping the assert altogether. The only case it'd be useful
is
> if
> > adding a new attribute to <rect> (which seems unlikely enough...)
> 
> Dropped in patchset #5.
> 
>
https://codereview.chromium.org/963733002/diff/60001/Source/core/svg/SVGImage...
> File Source/core/svg/SVGImageElement.cpp (right):
> 
>
https://codereview.chromium.org/963733002/diff/60001/Source/core/svg/SVGImage...
> Source/core/svg/SVGImageElement.cpp:165: // have the computed style yet we
can't
> use updateImageViewport.
> On 2015/03/12 16:40:07, fs wrote:
> > Nit: There was a bug filed about this - right? Add reference to it?
> 
> Done.

Ping @timloh and @pdr, to weigh in on the length value clamping and core/css
changes.

On the red/purple trybot results, I think that the remaining LayoutTest failures
are flakes, but I'm not 100% sure about the failing
"inspector/elements/styles/svg-style.xhtml" test though, it's consistent across
the three desktop platforms, so unless there are any objections I'll probably
mark it for rebaselining.

pdr.

On 2015/03/16 at 12:03:35, ed wrote: > On 2015/03/13 09:16:24, Erik Dahlström wrote: > > ...

5 years, 9 months ago (2015-03-16 22:35:50 UTC) #9

On 2015/03/16 at 12:03:35, ed wrote:
> On 2015/03/13 09:16:24, Erik Dahlström wrote:
> > Added timloh for the core/css change, feedback would be most welcome.
> > 
> >
https://codereview.chromium.org/963733002/diff/40001/Source/core/svg/SVGRectE...
> > File Source/core/svg/SVGRectElement.cpp (right):
> > 
> >
https://codereview.chromium.org/963733002/diff/40001/Source/core/svg/SVGRectE...
> > Source/core/svg/SVGRectElement.cpp:124:
ASSERT(isSupportedAttribute(attrName));
> > On 2015/03/12 16:40:07, fs wrote:
> > > On 2015/03/12 16:14:56, Erik Dahlström wrote:
> > > > On 2015/03/12 14:28:48, fs wrote:
> > > > > Probably not the most enlightening assert as long as the method starts
the
> > > way
> > > > > it does. Maybe there's something else that could be asserted?
> > > > > propertyFromAttribute(name)->type() == AnimatedLength?
> > > > 
> > > > I'm not sure the assert is very useful, it was an attempt at keeping
> > something
> > > > similar to the ASSERT_NOT_REACHED.
> > > 
> > > I'm fine with dropping the assert altogether. The only case it'd be useful
is
> > if
> > > adding a new attribute to <rect> (which seems unlikely enough...)
> > 
> > Dropped in patchset #5.
> > 
> >
https://codereview.chromium.org/963733002/diff/60001/Source/core/svg/SVGImage...
> > File Source/core/svg/SVGImageElement.cpp (right):
> > 
> >
https://codereview.chromium.org/963733002/diff/60001/Source/core/svg/SVGImage...
> > Source/core/svg/SVGImageElement.cpp:165: // have the computed style yet we
can't
> > use updateImageViewport.
> > On 2015/03/12 16:40:07, fs wrote:
> > > Nit: There was a bug filed about this - right? Add reference to it?
> > 
> > Done.
> 
> Ping @timloh and @pdr, to weigh in on the length value clamping and core/css
changes.
> 
> On the red/purple trybot results, I think that the remaining LayoutTest
failures are flakes, but I'm not 100% sure about the failing
"inspector/elements/styles/svg-style.xhtml" test though, it's consistent across
the three desktop platforms, so unless there are any objections I'll probably
mark it for rebaselining.

LGTM here. It looks like you can just update
inspector/elements/styles/svg-style.xhtml in this patch without the
needsrebaseline stuff.

commit-bot: I haz the power

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

5 years, 9 months ago (2015-03-17 13:26:49 UTC) #12

commit-bot: I haz the power

5 years, 9 months ago (2015-03-17 14:50:45 UTC) #13

Message was sent while issue was closed.

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192013

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages