[webkit-reviews] review granted: [Bug 212190] Add more Bitmap methods. : [Attachment 399939] proposed patch.

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 212190: Add more Bitmap methods.
https://bugs.webkit.org/show_bug.cgi?id=212190

Attachment 399939: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=399939&action=review




--- Comment #5 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 399939
  --> https://bugs.webkit.org/attachment.cgi?id=399939
proposed patch.

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

r=me, with a couple of suggestions for the tests.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1001
> +    {

Is this block just the same as the one above except for s/size/smallsize/g ?
If so, is it possible to make it an helper function to avoid the repetition?

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1093
> +		   temp.set(i, (randomNumber() > 0.5));

I am not sure that a random number in a test is a great idea. It sounds like a
recipe for flakiness/non-reproducibility.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1102
> +    Bitmap<smallSize, WordType> smallTemp;

Same comment as above, maybe try merging the tests for smallSize and for size
with a helper function.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1113
> +		   smallTemp.set(i, (randomNumber() > 0.5));

see my comment above, I think it might be nicer to bake somewhere (at the top
of the file?) an array with the random-looking values, instead of drawing them
truly randomly at each execution.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1427
> +	   // 1 & 0

typo: & instead of ^

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1434
> +	   // 1 & 1

ditto.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1506
> +	   // 1 & 0

ditto.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1513
> +	   // 1 & 1

ditto.