Bug 131606 – Replace width*height with calls to area

• Status: NEW
• Component: WebKit Misc.

[Dean Jackson]

Darin mentioned in 131553 that we should use area() helper functions for the cases where we are calculating width * height. It turns out there are a lot of places where we do this.

Add some helpers for IntSize, IntRect, LayoutRect, FloatSize and FloatRect.

[Darin Adler]

I just meant a local area helper in that file, but it would OK to put some in a header instead.

[Darin Adler]

The problem in a header is that we’d have to chose a type for the area result and it’s not 100% clear what the best type would be for all the cases.

[Dean Jackson]

Created attachment 229263 [details]
Patch

[Dean Jackson]

Created attachment 229264 [details]
Patch

[Dean Jackson]

Created attachment 229269 [details]
Patch

[Dean Jackson]

Comment on attachment 229269 [details]
Patch

Failing some tests.

[Dean Jackson]

Actually it doesn't fail any more tests than without the change. The EFL build failure doesn't give enough info in the log, so I'll upload another patch.

[Dean Jackson]

Created attachment 229287 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 229287 [details]
Patch

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

> Source/WebCore/platform/graphics/IntRect.h:253
> +inline int area(const IntRect& rect)
> +{
> +    return rect.width() * rect.height();
> +}

Why isn't this a member function? Do we care about overflow?

> Source/WebCore/platform/graphics/IntSize.h:196
> +inline int area(const IntSize& size)
> +{
> +    return size.area();
> +}

Ditto.

[Darin Adler]

Comment on attachment 229287 [details]
Patch

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.

[Ahmad Saleem]

Checking via BugID on Webkit GitHub, it seems this r+ patch didn't landed.

Is anything needed from this patch? Thanks!