Replace hasAttribute with the check for actual value

Replace hasAttribute with the check for actual value

This change replaces the use of hasAttribute() by a
better and correct approach, to check if the actual
value is greater than or equal to zero. This patch also
includes a Ref Test which demonstrates the setting of attribute
within a 'use' tag to test the changed code path.

BUG=235256
R=pdr@chromium.org

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

[mrunal, 2013-06-19 23:28:34]

Please take a look.

[Stephen Chennney, 2013-06-20 17:57:12]

The test need to be re-written with the following features:
1. No text, as it tends to fail even on ref-tests
2. No border - it's extraneous
3. The correct result should be all green. The incorrect result should have red.
You can change the symbol to just have one green and three red rects, s.t. the
red ones are clipped out by the limited width/height.
4. The ref test in this case can simply be a single green rect - no use or
symbol or anything like that.

[mrunal, 2013-06-20 21:14:44]

@Stephen - thanks for your comments. I have updated the test.

[Stephen Chennney, 2013-06-20 21:49:47]

lgtm

[commit-bot: I haz the power, 2013-06-20 21:50:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/17287012/4001

[commit-bot: I haz the power, 2013-06-20 23:35:02]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-21 17:48:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/17287012/4001

[commit-bot: I haz the power, 2013-06-21 19:04:38]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[mrunal, 2013-06-21 21:35:41]

Looks like the Layout Test failures were genuine. I have updated the patch to
check if it's only '> 0' rather than '>= 0'. I tested all the failing layout
tests and it seems to have passed after this change. Will submit this to CQ
again.

[commit-bot: I haz the power, 2013-06-21 21:36:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/17287012/26001

[commit-bot: I haz the power, 2013-06-22 00:38:44]

Change committed as 152911

[pdr., 2013-06-22 18:07:33]

On 2013/06/21 21:35:41, mrunal wrote:
> Looks like the Layout Test failures were genuine. I have updated the patch to
> check if it's only '> 0' rather than '>= 0'. I tested all the failing layout
> tests and it seems to have passed after this change. Will submit this to CQ
> again.

Please wait for a second LGTM if there are code changes involved.


This change lists R=pdr@chromium.org, but I have some reservations.
What happens if you animate/set the value from 10 to 0? I think we'll skip
setting the width/height to 0.

[mrunal, 2013-06-24 21:37:55]

> Please wait for a second LGTM if there are code changes involved.
> 
> 
> This change lists mailto:R=pdr@chromium.org, but I have some reservations.
> What happens if you animate/set the value from 10 to 0? I think we'll skip
> setting the width/height to 0.

Sorry pdr, I assumed once we have a LGTM from a reviewer it's ok to upload minor
changes later. My bad.

Your concern is valid. I thought about it now and I don't see any other way to
check if the attribute was set. I tried using useElement->heightIsValid() but it
always returns false. Is there another flag to indicate the attribute has been
set? Problem over here is we cannot distinguish between if the zero is the
initial value or the value set later.

[pdr., 2013-06-25 15:38:29]

On 2013/06/24 21:37:55, mrunal wrote:
> > Please wait for a second LGTM if there are code changes involved.
> > 
> > 
> > This change lists mailto:R=pdr@chromium.org, but I have some reservations.
> > What happens if you animate/set the value from 10 to 0? I think we'll skip
> > setting the width/height to 0.
> 
> Sorry pdr, I assumed once we have a LGTM from a reviewer it's ok to upload
minor
> changes later. My bad.

Not to worry at all; we have the codereview tool to catch this :) Let not roll
out the patch, but do you mind doing a second patch to fix this one?

> 
> Your concern is valid. I thought about it now and I don't see any other way to
> check if the attribute was set. I tried using useElement->heightIsValid() but
it
> always returns false. Is there another flag to indicate the attribute has been
> set? Problem over here is we cannot distinguish between if the zero is the
> initial value or the value set later.

What about just removing the check entirely and always setting the viewport
size?

[mrunal, 2013-06-25 21:15:56]

> Not to worry at all; we have the codereview tool to catch this :) Let not roll
> out the patch, but do you mind doing a second patch to fix this one?
Do I have to revert the patch or will you? If I have to, how do I do it?
 
> > Your concern is valid. I thought about it now and I don't see any other way
to
> > check if the attribute was set. I tried using useElement->heightIsValid()
but
> it
> > always returns false. Is there another flag to indicate the attribute has
been
> > set? Problem over here is we cannot distinguish between if the zero is the
> > initial value or the value set later.
> 
> What about just removing the check entirely and always setting the viewport
> size?
I don't think that solves the problem either, especially if the width and height
values are not specified on use attribute at all.
In that case it will use default 0 value for width and height.