[webkit-reviews] review granted: [Bug 116886] Rendering suppression extension tokens shouldn't be 0, should handle overflow : [Attachment 203086] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 116886: Rendering suppression extension tokens shouldn't be 0, should
handle overflow
https://bugs.webkit.org/show_bug.cgi?id=116886

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

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


r=me assuming you deal with the 0xFFFFFFFF problem.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4199
>> +	while(!token || m_activeRenderingSuppressionTokens.contains(token))
> 
> Missing space before ( in while(  [whitespace/parens] [5]

First, let me say to the stylebot that I agree!

Second, there are two values that a HashSet<unsigned> can’t handle. One is 0,
and one is 0xFFFFFFFF. This code will call contains with 0xFFFFFFFF, which is a
no-no. The way to deal with that is to use the isValidValue function instead of
special casing zero.

    while (!token || !m_activeRenderingSuppressionTokens.isValidValue(token) ||
m_activeRenderingSuppressionTokens.contains(token))

There are two flaws with the code I pasted in here. One is that it’s probably
more elegant to use a class name to call a static member function rather than
using an instance to do so. The other is that it’s a little strange to have two
checks for zero, one in this code and one in the isValidValue function; not
sure the compiler will optimize one of them out.