[Webkit-unassigned] [Bug 144739] Fix sizes crash and add invalid value tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144739

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #252586|review?                     |review+
              Flags|                            |

--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 252586
  --> https://bugs.webkit.org/attachment.cgi?id=252586
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150507/ac43784c/attachment.html>