Bug 68057 – Eliminate Length::undefinedLength = -1 and replace with Undefined LengthType.

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Eliminate Length::undefinedLength = -1 and replace with Undefined LengthType.

[Luke Macpherson]

Created attachment 107288 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 107288 [details]
Patch

Attachment 107288 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9651458

[Luke Macpherson]

Created attachment 107430 [details]
Patch

[Darin Adler]

Comment on attachment 107430 [details]
Patch

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.

[Luke Macpherson]

Comment on attachment 107430 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107430&action=review

>> 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.

The reason this is not the case is that most code does not actually check whether the value returned by length is -1 or not, so 99% of the time Length(-1,Fixed) is still treated as a valid length.

>> 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.

My intent was to change the cases that treat -1 as invalid to use Lenght(Undefined), and leave the cases that treat Length(-1, Fixed) as an actual length alone.
The only case I'm not confident about is in RenderListMarker::updateMargins() which sometimes sets MarginStart to Length(-1, Fixed). In this case the tests indicate that this -1 should be treated as a length, even though the intent is not clear from the code. In this case I have left it as a valid Length of -1 so that the tests continue to pass.

>> Source/WebCore/platform/Length.h:83
>>      }
> 
> 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.

Done.

>> Source/WebCore/platform/Length.h:123
>> +    // 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.

Comment updated.

>> Source/WebCore/platform/Length.h:134
>> +                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.

Done.

>> Source/WebCore/platform/Length.h:152
>>                  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.

Done.

>> Source/WebCore/platform/Length.h:174
>>          return m_isFloat ? !m_floatValue : !m_intValue;
> 
> Doesn’t this need to assert !isUndefined()?

Done.

>> Source/WebCore/rendering/RenderImage.cpp:489
>>              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.

The existing case treated Length(-1, Fixed) as a valid value, and it is still treated as such, so there is no behavioral change here.

>> Source/WebCore/rendering/RenderImage.cpp:506
>>              return false;
> 
> Ditto.

As above.

>> Source/WebCore/rendering/RenderListBox.cpp:197
>> +    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.

Agreed, done.

[Luke Macpherson]

Created attachment 107461 [details]
Patch

[Darin Adler]

Comment on attachment 107430 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107430&action=review

>>> Source/WebCore/rendering/RenderImage.cpp:489
>>>              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.
> 
> The existing case treated Length(-1, Fixed) as a valid value, and it is still treated as such, so there is no behavioral change here.

I don’t think you understood which cases I was talking about.

There is a behavior change if this function gets a length that comes from the function in CSSStyleApplyProperty.cpp that this patch changes or from the result of the Length::initialMaxSize function.

If neither of those should ever happen, then I suggest that the undefined case should do an ASSERT_NOT_REACHED instead of just returning false.

If either of those things can happen, then we can probably demonstrate with a test that there is a behavior change.

[Luke Macpherson]

Created attachment 107561 [details]
Patch

[Luke Macpherson]

Comment on attachment 107430 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107430&action=review

>>>> Source/WebCore/rendering/RenderImage.cpp:489
>>>>              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.
>> 
>> The existing case treated Length(-1, Fixed) as a valid value, and it is still treated as such, so there is no behavioral change here.
> 
> I don’t think you understood which cases I was talking about.
> 
> There is a behavior change if this function gets a length that comes from the function in CSSStyleApplyProperty.cpp that this patch changes or from the result of the Length::initialMaxSize function.
> 
> If neither of those should ever happen, then I suggest that the undefined case should do an ASSERT_NOT_REACHED instead of just returning false.
> 
> If either of those things can happen, then we can probably demonstrate with a test that there is a behavior change.

Ok, I've added an ASSERT_NOT_REACHED there.

[Darin Adler]

Comment on attachment 107561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107561&action=review

> Source/WebCore/platform/Length.h:124
> +    // Note: May only be called for Fixed, Percent and Auto lengths.
> +    //       Other types will ASSERT in order to catch invalid length calculations.

We normally don’t line up subsequent lines with spaces like this.

> Source/WebCore/platform/Length.h:139
> +        return 0;

It would also be good to have an ASSERT_NOT_REACHED outside the switch statement.

> Source/WebCore/platform/Length.h:160
> +        return 0;

It would also be good to have an ASSERT_NOT_REACHED outside the switch statement.

> Source/WebCore/platform/Length.h:178
> +        return 0;

It would also be good to have an ASSERT_NOT_REACHED outside the switch statement.

[Luke Macpherson]

Comment on attachment 107561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107561&action=review

>> Source/WebCore/platform/Length.h:124
>> +    //       Other types will ASSERT in order to catch invalid length calculations.
> 
> We normally don’t line up subsequent lines with spaces like this.

Done.

>> Source/WebCore/platform/Length.h:139
>> +        return 0;
> 
> It would also be good to have an ASSERT_NOT_REACHED outside the switch statement.

Done.

>> Source/WebCore/platform/Length.h:160
>> +        return 0;
> 
> It would also be good to have an ASSERT_NOT_REACHED outside the switch statement.

Done.

>> Source/WebCore/platform/Length.h:178
>> +        return 0;
> 
> It would also be good to have an ASSERT_NOT_REACHED outside the switch statement.

Done.

[Luke Macpherson]

Created attachment 107580 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107580 [details]
Patch

Clearing flags on attachment: 107580

Committed r95502: <http://trac.webkit.org/changeset/95502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.