Bug 144739 – Fix sizes crash and add invalid value tests.

• Component: New Bugs
• Status: RESOLVED FIXED

[Yoav Weiss]

Fix sizes crash and add invalid value tests.

[Yoav Weiss]

Created attachment 252585 [details]
Patch

[Yoav Weiss]

Created attachment 252586 [details]
Patch

[Darin Adler]

Comment on attachment 252586 [details]
Patch

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

I’m OK with this patch, but I think it’s unnecessarily awkward.

> Source/WebCore/css/SourceSizeList.cpp:59
> -static unsigned computeLength(CSSValue* value, RenderStyle& style, RenderView* view)
> +static bool computeLength(CSSValue* value, RenderStyle& style, RenderView* view, unsigned& sourceSizeLength)

Seems a little inelegant to do the checking in the same function that computes lengths, given that we do this work in a loop and we only need the check once. I suggest putting the checking into a separate function. Maybe we should even have parseSizesAttribute use two separate loops for CSSPrimitiveValue and CSSCalcValue rather than constantly branching inside the loop.

> Source/WebCore/css/SourceSizeList.cpp:86
> +            if (!computeLength(sourceSize.length.get(), style, view, sourceSizeLength))
> +                break;

I think it’s strange to check if the value is a length inside this loop instead of checking it outside the loop. The break here is illogical until you think it through.

> Source/WebCore/css/SourceSizeList.cpp:93
> -    return computeLength(CSSPrimitiveValue::create(100, CSSPrimitiveValue::CSS_VW).ptr(), style, view);
> +    if (computeLength(CSSPrimitiveValue::create(100, CSSPrimitiveValue::CSS_VW).ptr(), style, view, sourceSizeLength))
> +        return sourceSizeLength;
> +    ASSERT_NOT_REACHED();
> +    return 0;

Combining the checking with the computation makes this ugly too. That ASSERT_NOT_REACHED is self-inflicted.

Also seems a shame that we have to allocation memory just to reuse code. Could we refactor this so we don’t have to heap-allocated a reference counted object and then delete it just to do the length computation?

[Yoav Weiss]

Created attachment 252614 [details]
Patch

[Yoav Weiss]

Comment on attachment 252614 [details]
Patch

I've refactored the code so that it would be more straight forward.

The length check has to happen only after we know we have a CSSPrimitiveLength on our hands, to it has to happen in the loop, but only happens on the first matching expression (in case it is in fact a CSSPrimitiveValue). We also need to maintain a single loop for both lengths and calcs, since the algorithm is a first-match.

Regarding the creation of CSSPrimitiveValue just to calculate length, that is a shame. I'll try to refactor out the computeLength logic of CSSPrimitiveValue to be static, so that it can be used here without creating a new object. I'll do that as a separate issue though, since I suspect it may be a large patch.

[WebKit Commit Bot]

Comment on attachment 252614 [details]
Patch

Clearing flags on attachment: 252614

Committed r183948: <http://trac.webkit.org/changeset/183948>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 252614 [details]
Patch

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

> Source/WebCore/css/SourceSizeList.cpp:61
> +    return CSSPrimitiveValue::create(100, CSSPrimitiveValue::CSS_VW)->computeLength<unsigned>(CSSToLengthConversionData(&style, &style, view));

This needs a FIXME. We want to reimplement this so it does it without allocating a CSSPrimitiveValue object in the future.

[Yoav Weiss]

Added a fixme at https://bugs.webkit.org/show_bug.cgi?id=144861