Fix SVG animations which check viewBox attribute

Fix SVG animations which check viewBox attribute

Earlier hasAttribute() was used to check the viewBox has been set
or not. hasAttribute() should not be used, as it can return false,
if the attribute is not set but the value is animating.

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

[payal.pandey, 2015-04-17 10:02:30]

payal.pandey@samsung.com changed reviewers:
+ schenney@chromium.org

[payal.pandey, 2015-04-17 10:03:42]

Please have a look on the CL and review the same.

[fs, 2015-04-17 11:01:11]

On 2015/04/17 10:03:42, payal.pandey wrote:
> Please have a look on the CL and review the same.

There's already "work" ongoing for this in
https://codereview.chromium.org/1082723002/

[payal.pandey, 2015-04-17 11:12:08]

On 2015/04/17 11:01:11, fs wrote:
> On 2015/04/17 10:03:42, payal.pandey wrote:
> > Please have a look on the CL and review the same.
> 
> There's already "work" ongoing for this in
> https://codereview.chromium.org/1082723002/


Okay no problem.. 
 
I wondered, if i could work on other fixes where hasAttribute is being used..
like... as given below:

core/svg/SVGFEConvolveMatrixElement.cpp
 if (!hasAttribute(SVGNames::orderAttr)

[payal.pandey, 2015-04-17 12:41:10]

Modified the CL for use of hasAttribute in SVGViewSpec.h.
Please have a look on this CL.

[payal.pandey, 2015-04-17 22:25:04]

payal.pandey@samsung.com changed reviewers:
+ fs@opera.com, pdr@chromium.org
- schenney@chromium.org

[payal.pandey, 2015-04-17 22:25:48]

Modified the CL for use of hasAttribute in SVGViewSpec.h.
Please have a look on this CL.

[pdr., 2015-04-18 02:52:25]

On 2015/04/17 at 22:25:48, payal.pandey wrote:
> Modified the CL for use of hasAttribute in SVGViewSpec.h.
> Please have a look on this CL.

Please create a test for every change before asking for reviews. See the other
tests added in crbug.com/235256 for examples.

[payal.pandey, 2015-04-20 12:19:03]

Updated with testcase for CL.
Please review the CL.

[fs, 2015-04-20 12:26:02]

On 2015/04/20 12:19:03, payal.pandey wrote:
> Updated with testcase for CL.
> Please review the CL.

It doesn't look like the test is testing the fix - does the test really fail
without the fix and pass with the fix?

[payal.pandey, 2015-04-22 09:35:16]

Thanks for the feedback and information shared
actually helped alot.
Upadated with testcase, which actually represents
that when the viewbox attribute is not set,
then hasAttribute() return false,
but value is still amimating with <set>.

Please have a look on this CL.

[fs, 2015-04-22 10:56:51]

https://codereview.chromium.org/1051023006/diff/60001/LayoutTests/svg/animati...
File LayoutTests/svg/animations/viewspec-animated-viewbox-expected.html (right):

https://codereview.chromium.org/1051023006/diff/60001/LayoutTests/svg/animati...
LayoutTests/svg/animations/viewspec-animated-viewbox-expected.html:5: Test for
crbug.com/249578: viewSpec's ViewBoxParams.<br/>
Ditto.

https://codereview.chromium.org/1051023006/diff/60001/LayoutTests/svg/animati...
File LayoutTests/svg/animations/viewspec-animated-viewbox.html (right):

https://codereview.chromium.org/1051023006/diff/60001/LayoutTests/svg/animati...
LayoutTests/svg/animations/viewspec-animated-viewbox.html:5: Test for
crbug.com/249578: viewSpec's ViewBoxParams.<br/>
Wrong bug ref?

https://codereview.chromium.org/1051023006/diff/60001/Source/core/svg/SVGView...
File Source/core/svg/SVGViewSpec.h (right):

https://codereview.chromium.org/1051023006/diff/60001/Source/core/svg/SVGView...
Source/core/svg/SVGViewSpec.h:81:
viewBox()->baseValue()->setValue(inheritFromElement->viewBox()->currentValue()->value());
This will just snapshot the value, which likely mean that viewBox animations
combined with fragment view-specs won't work great. That seems to be a
pre-existing condition though, so the not the fault of this CL.

[payal.pandey, 2015-04-22 11:07:47]

Incorporated the review comments.
Please have a look again.

[fs, 2015-04-22 11:14:37]

lgtm

[payal.pandey, 2015-04-22 11:15:12]

The CQ bit was checked by payal.pandey@samsung.com

[commit-bot: I haz the power, 2015-04-22 11:15:24]

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

[payal.pandey, 2015-04-22 11:15:32]

On 2015/04/22 11:14:37, fs wrote:
> lgtm

[payal.pandey, 2015-04-22 11:15:40]

On 2015/04/22 11:15:32, payal.pandey wrote:
> On 2015/04/22 11:14:37, fs wrote:
> > lgtm

[payal.pandey, 2015-04-22 11:16:13]

On 2015/04/22 11:15:40, payal.pandey wrote:
> On 2015/04/22 11:15:32, payal.pandey wrote:
> > On 2015/04/22 11:14:37, fs wrote:
> > > lgtm

Thanks for LGTM.

[commit-bot: I haz the power, 2015-04-22 14:24:21]

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

[aboxhall, 2015-04-22 17:21:11]

On 2015/04/22 14:24:21, I haz the power (commit-bot) wrote:
> Committed patchset #5 (id:80001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=194217

The test added in this patch has been failing consistently:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...

[aboxhall, 2015-04-22 17:22:47]

On 2015/04/22 17:21:11, aboxhall wrote:
> On 2015/04/22 14:24:21, I haz the power (commit-bot) wrote:
> > Committed patchset #5 (id:80001) as
> > https://src.chromium.org/viewvc/blink?view=rev&revision=194217
> 
> The test added in this patch has been failing consistently:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...

Ah - silly me, it's not consistent, but flaky. Will mark test as flaky.

[pdr., 2015-04-24 00:10:56]

On 2015/04/22 at 17:22:47, aboxhall wrote:
> On 2015/04/22 17:21:11, aboxhall wrote:
> > On 2015/04/22 14:24:21, I haz the power (commit-bot) wrote:
> > > Committed patchset #5 (id:80001) as
> > > https://src.chromium.org/viewvc/blink?view=rev&revision=194217
> > 
> > The test added in this patch has been failing consistently:
> >
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...
> 
> Ah - silly me, it's not consistent, but flaky. Will mark test as flaky.

@payal.pandey, see: https://code.google.com/p/chromium/issues/detail?id=479823