[webkit-reviews] review granted: [Bug 131606] Replace width*height with calls to area : [Attachment 229287] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 131606: Replace width*height with calls to area
https://bugs.webkit.org/show_bug.cgi?id=131606

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

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


I worry that these different area functions have different overflow behaviors.
They all have the same name, but different behavior.

The only ones I am really comfortable with are the ones that return floating
point, because there I know what to expect from overflow. The integer ones
worry me.

Otherwise this patch seems fantastic!

> Source/WebCore/platform/graphics/LayoutRect.h:252
> +    return rect.width() * rect.height();

Seems a shame to convert to a float only after a possible overflow.

> Source/WebCore/platform/graphics/LayoutSize.h:207
> +    return size.width() * size.height();

Seems a shame to convert to a float only after a possible overflow.

> Source/WebCore/platform/graphics/Region.cpp:119
> +    for (size_t i = 0; i < size; ++i)
> +	   totalArea += area(rects[i]);

Should use C++11 for loop instead of size and such.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:532
>      if (templateSize.width() > rect.width() || templateSize.height() >
rect.height()
> -	   || (templateSize.width() * templateSize.height() >
m_sourceRect.width() * m_sourceRect.height())) {
> +	   || area(templateSize) > area(m_sourceRect)) {

Should combine onto a single line now that it’s short enough.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:560
>      if (templateSize.width() > hRect.width() || templateSize.height() >
hRect.height()
> -	   || (templateSize.width() * templateSize.height() > hRect.width() *
hRect.height())) {
> +	   || area(templateSize) > area(hRect)) {

Should combine onto a single line now that it’s short enough.