Fix SVG animations for preserveAspectRatio attribute

Fix SVG animations for preserveAspectRatio attribute

hasAttribute return false when the attribute is not set
and value still animates

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

[b.siddharth, 2015-04-22 12:51:36]

b.siddharth@samsung.com changed reviewers:
+ eae@chromium.org

[b.siddharth, 2015-04-22 12:51:37]

Please have a look on this CL

[eae, 2015-04-22 13:09:11]

eae@chromium.org changed reviewers:
+ pdr@chromium.org

[eae, 2015-04-22 13:09:11]

pdr would be a better reviewer for this

[b.siddharth, 2015-04-23 05:25:01]

On 2015/04/22 13:09:11, eae wrote:
> pdr would be a better reviewer for this

Please have a look, Thanks.

[b.siddharth, 2015-04-23 06:40:03]

b.siddharth@samsung.com changed reviewers:
+ fs@opera.com

[b.siddharth, 2015-04-23 06:40:04]

Please check & review, Thanks

[b.siddharth, 2015-04-23 06:40:07]

Please check & review, Thanks

[fs, 2015-04-23 08:30:32]

The very similar test for viewBox got marked as flaky after landing, so please
try and make sure that doesn't happen here. My suspicion would be that it's the
start of the animation that races with the application of the fragment viewspec.

https://codereview.chromium.org/1098273002/diff/1/Source/core/svg/SVGViewSpec.h
File Source/core/svg/SVGViewSpec.h (right):

https://codereview.chromium.org/1098273002/diff/1/Source/core/svg/SVGViewSpec...
Source/core/svg/SVGViewSpec.h:83: if
(inheritFromElement->preserveAspectRatio()->currentValue()->align() !=
SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_UNKNOWN &&
inheritFromElement->preserveAspectRatio()->currentValue()->meetOrSlice() !=
SVGPreserveAspectRatio::SVG_MEETORSLICE_UNKNOWN) {
I believe what you want here is
inheritFromElement->preserveAspectRatio()->isSpecified().

[pdr., 2015-04-24 00:03:42]

On 2015/04/23 at 08:30:32, fs wrote:
> The very similar test for viewBox got marked as flaky after landing, so please
try and make sure that doesn't happen here. My suspicion would be that it's the
start of the animation that races with the application of the fragment viewspec.

b.siddharth, Just FYI, you can test this by running the test like so:
pdr@yourcomputer:chromium/src/third_party/WebKit$
./Tools/Scripts/run-webkit-tests [test name] --iterations=100

The test should pass 100% of the time.

[b.siddharth, 2015-04-24 12:46:09]

Incorporated review comments.
Please have a look, Thanks.

[fs, 2015-04-24 12:51:21]

Code change looks ok.

https://codereview.chromium.org/1098273002/diff/20001/LayoutTests/svg/animati...
File LayoutTests/svg/animations/viewspec-checkaspectparams.html (right):

https://codereview.chromium.org/1098273002/diff/20001/LayoutTests/svg/animati...
LayoutTests/svg/animations/viewspec-checkaspectparams.html:7: <object width="30"
height="30"
data="resources/viewspec-checkaspectparams.svg#svgView(viewBox(0,0,30,30);preserveAspectRatio(none))"></object>
Won't this be the same even you remove the animation in the file?

[b.siddharth, 2015-04-27 12:10:54]

On 2015/04/24 12:51:21, fs wrote:
> Code change looks ok.
> 
>
https://codereview.chromium.org/1098273002/diff/20001/LayoutTests/svg/animati...
> File LayoutTests/svg/animations/viewspec-checkaspectparams.html (right):
> 
>
https://codereview.chromium.org/1098273002/diff/20001/LayoutTests/svg/animati...
> LayoutTests/svg/animations/viewspec-checkaspectparams.html:7: <object
width="30"
> height="30"
>
data="resources/viewspec-checkaspectparams.svg#svgView(viewBox(0,0,30,30);preserveAspectRatio(none))"></object>
> Won't this be the same even you remove the animation in the file?

No its not the same case, as when hasAttribute() is used without animation, it
return false value.
and isSpecified() return true in this case.

[fs, 2015-04-27 12:20:00]

On 2015/04/27 12:10:54, b.siddharth wrote:
> On 2015/04/24 12:51:21, fs wrote:
> > Code change looks ok.
> > 
> >
>
https://codereview.chromium.org/1098273002/diff/20001/LayoutTests/svg/animati...
> > File LayoutTests/svg/animations/viewspec-checkaspectparams.html (right):
> > 
> >
>
https://codereview.chromium.org/1098273002/diff/20001/LayoutTests/svg/animati...
> > LayoutTests/svg/animations/viewspec-checkaspectparams.html:7: <object
> width="30"
> > height="30"
> >
>
data="resources/viewspec-checkaspectparams.svg#svgView(viewBox(0,0,30,30);preserveAspectRatio(none))"></object>
> > Won't this be the same even you remove the animation in the file?
> 
> No its not the same case, as when hasAttribute() is used without animation, it
> return false value.
> and isSpecified() return true in this case.

I meant the visual result. In the standalone file, preserveAspectRatio does not
apply (no 'viewBox'), and regardless the value is overridden with 'none' (and a
'viewBox'). There's also that the size is the same, and the viewBox will have no
actual effect (1:1 mapping).

[b.siddharth, 2015-04-28 10:46:17]

Modified the testcase for visual result.
Please have a look.

[fs, 2015-04-28 15:05:34]

lgtm

[fs, 2015-04-28 15:05:34]

lgtm

lgtm

[fs, 2015-04-28 15:06:50]

On 2015/04/28 15:05:34, fs wrote:
> lgtm
> 
> lgtm

Darn, now I exceeded my daily quota...

[b.siddharth, 2015-04-29 05:20:44]

The CQ bit was checked by b.siddharth@samsung.com

[commit-bot: I haz the power, 2015-04-29 05:20:59]

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

[commit-bot: I haz the power, 2015-04-29 07:32:59]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194647

[Xianzhu, 2015-05-07 23:18:56]

Tried to open the test file in content_shell (ToT). The test and the ref show
differently. The test shows a 256x256 green box at the bottom-left corner in a
500x500 space of the  'object' element, while the ref shows a 500x500 green box
filling the 500x500 space of the 'object' element. Is this expected?

https://codereview.chromium.org/1126313004/ changes the timing of pixel dump
(may delay one loop), and now the test's pixel dump mismatches ref's. I'm
wondering if this means the test is broken, or we just need to update the ref.

[fs, 2015-05-08 08:25:21]

On 2015/05/07 23:18:56, Xianzhu wrote:
> Tried to open the test file in content_shell (ToT). The test and the ref show
> differently. The test shows a 256x256 green box at the bottom-left corner in a
> 500x500 space of the  'object' element, while the ref shows a 500x500 green
box
> filling the 500x500 space of the 'object' element. Is this expected?

It's indeed borked... =/ (how did it make it through CQ? Must have been an
interesting flake...)

It should never be 'bottom-left' though (only 'centered' or 'top-left', where
the latter is the expected).
 
> https://codereview.chromium.org/1126313004/ changes the timing of pixel dump
> (may delay one loop), and now the test's pixel dump mismatches ref's. I'm
> wondering if this means the test is broken, or we just need to update the ref.

I'll rewrite the ref to be more obviously correct.

[fs, 2015-05-08 08:32:05]

On 2015/05/08 08:25:21, fs wrote:
> On 2015/05/07 23:18:56, Xianzhu wrote:
> > Tried to open the test file in content_shell (ToT). The test and the ref
show
> > differently. The test shows a 256x256 green box at the bottom-left corner in
a
> > 500x500 space of the  'object' element, while the ref shows a 500x500 green
> box
> > filling the 500x500 space of the 'object' element. Is this expected?
> 
> It's indeed borked... =/ (how did it make it through CQ? Must have been an
> interesting flake...)
> 
> It should never be 'bottom-left' though (only 'centered' or 'top-left', where
> the latter is the expected).
>  
> > https://codereview.chromium.org/1126313004/ changes the timing of pixel dump
> > (may delay one loop), and now the test's pixel dump mismatches ref's. I'm
> > wondering if this means the test is broken, or we just need to update the
ref.
> 
> I'll rewrite the ref to be more obviously correct.

https://codereview.chromium.org/1131233002/

[b.siddharth, 2015-05-08 12:08:27]

On 2015/05/08 08:32:05, fs wrote:
> On 2015/05/08 08:25:21, fs wrote:
> > On 2015/05/07 23:18:56, Xianzhu wrote:
> > > Tried to open the test file in content_shell (ToT). The test and the ref
> show
> > > differently. The test shows a 256x256 green box at the bottom-left corner
in
> a
> > > 500x500 space of the  'object' element, while the ref shows a 500x500
green
> > box
> > > filling the 500x500 space of the 'object' element. Is this expected?
> > 
> > It's indeed borked... =/ (how did it make it through CQ? Must have been an
> > interesting flake...)
> > 
> > It should never be 'bottom-left' though (only 'centered' or 'top-left',
where
> > the latter is the expected).
> >  
> > > https://codereview.chromium.org/1126313004/ changes the timing of pixel
dump
> > > (may delay one loop), and now the test's pixel dump mismatches ref's. I'm
> > > wondering if this means the test is broken, or we just need to update the
> ref.
> > 
> > I'll rewrite the ref to be more obviously correct.
> 
> https://codereview.chromium.org/1131233002/

[b.siddharth, 2015-05-08 12:09:07]

On 2015/05/08 12:08:27, b.siddharth wrote:
> On 2015/05/08 08:32:05, fs wrote:
> > On 2015/05/08 08:25:21, fs wrote:
> > > On 2015/05/07 23:18:56, Xianzhu wrote:
> > > > Tried to open the test file in content_shell (ToT). The test and the ref
> > show
> > > > differently. The test shows a 256x256 green box at the bottom-left
corner
> in
> > a
> > > > 500x500 space of the  'object' element, while the ref shows a 500x500
> green
> > > box
> > > > filling the 500x500 space of the 'object' element. Is this expected?
> > > 
> > > It's indeed borked... =/ (how did it make it through CQ? Must have been an
> > > interesting flake...)
> > > 
> > > It should never be 'bottom-left' though (only 'centered' or 'top-left',
> where
> > > the latter is the expected).
> > >  
> > > > https://codereview.chromium.org/1126313004/ changes the timing of pixel
> dump
> > > > (may delay one loop), and now the test's pixel dump mismatches ref's.
I'm
> > > > wondering if this means the test is broken, or we just need to update
the
> > ref.
> > > 
> > > I'll rewrite the ref to be more obviously correct.
> > 
> > https://codereview.chromium.org/1131233002/

Hello @fs: Thanks for fixing the test case.