[webkit-reviews] review denied: [Bug 68057] Eliminate Length::undefinedLength = -1 and replace with Undefined LengthType. : [Attachment 107430] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 68057: Eliminate Length::undefinedLength = -1 and replace with Undefined
LengthType.
https://bugs.webkit.org/show_bug.cgi?id=68057

Attachment 107430: Patch
https://bugs.webkit.org/attachment.cgi?id=107430&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107430&action=review


Looks like a good change to make, but I am not sure you sufficiently covered
all the code paths that might be working with an undefined value.

> Source/WebCore/ChangeLog:8
> +	   There appear to be many cases where -1 is actually a valid Length.

If this is true, then we should be able to demonstrate that we fixed a bug with
a test case.

> Source/WebCore/ChangeLog:11
> +	   No new tests / no behavioral changes.

But the comment above claims that -1 is a valid length, so it seems there must
be at least some behavior changes.

> Source/WebCore/css/CSSPrimitiveValue.cpp:405
>	   default:
> +	       ASSERT_NOT_REACHED();
>	       return -1.0;

Seems pretty strange to return -1 here. Not something new you added.

> Source/WebCore/platform/Length.h:83
>      int value() const {
> +	   ASSERT(!isUndefined());
>	   return getIntValue();
>      }

Good to add an assertion, but maybe it should be inside the getIntValue
function.

Also, if you are touching this function you should fix its incorrect brace
style.

> Source/WebCore/platform/Length.h:123
> -    // note: works only for certain types, returns undefinedLength otherwise

> +    // note: works only for certain types

This is unnecessarily vague. The old comment was too, but it would be much
better if the comment explained the policy. It's now not even legal to call
this function if the type is not one of the right ones.

> Source/WebCore/platform/Length.h:134
>	       default:
> -		   return undefinedLength;
> +		   ASSERT_NOT_REACHED();
> +		   return 0;

Ideally we'd get rid of the default case so we can take advantage of the
compiler's warning about unhandled switch cases.

> Source/WebCore/platform/Length.h:152
>	       default:
> +		   ASSERT_NOT_REACHED();
>		   return 0;

Ideally we'd get rid of the default case so we can take advantage of the
compiler's warning about unhandled switch cases.

> Source/WebCore/platform/Length.h:167
>	       default:
> -		   return static_cast<float>(undefinedLength);
> +		   ASSERT_NOT_REACHED();
> +		   return static_cast<float>(0);

Ideally we'd get rid of the default case so we can take advantage of the
compiler's warning about unhandled switch cases.

This can just be return 0, no need for the cast.

> Source/WebCore/platform/Length.h:174
>	   return m_isFloat ? !m_floatValue : !m_intValue;

Doesn’t this need to assert !isUndefined()?

> Source/WebCore/rendering/RenderImage.cpp:489
> +	   case Undefined:
>	       return false;

This seems to change behavior. Before the undefined values were always Fixed,
so they returned true from this function.

If no undefined values are ever involved here, then I suggest that the
undefined case should do an ASSERT_NOT_REACHED instead of just returning false.


If undefined values are involved here, then we can probably demonstrate with a
test that there is a behavior change.

> Source/WebCore/rendering/RenderImage.cpp:506
> +	   case Undefined:
>	       return false;

Ditto.

> Source/WebCore/rendering/RenderListBox.cpp:197
> -    if (style()->maxWidth().isFixed() && style()->maxWidth().value() !=
undefinedLength) {
> +    if (style()->maxWidth().isFixed() && style()->maxWidth().value()) {

This one seems wrong, checking value() to see if it’s zero or not. In the other
cases you correctly removed this clause entirely.