Rendering suppression extension tokens shouldn't be 0, should handle overflow

[Tim Horton]

Apparently 0 is not a good value to try to insert into a HashSet.

From http://trac.webkit.org/changeset/150388

[Radar WebKit Bug Importer]

<rdar://problem/14004474>

[Tim Horton]

Created attachment 203086 [details]
patch

[WebKit Commit Bot]

Attachment 203086 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4199:  Missing space before ( in while(  [whitespace/parens] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 203086 [details]
patch

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.

[Darin Adler]

Comment on attachment 203086 [details]
patch

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

>>> 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.

You could just leave out the "!token" part if the only reason you were ruling out zero was to avoid the bad hash set value.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4200
> +        token++;

This ought to be:

    token = ++m_maximumRenderingSuppressionToken;

Don’t you think?

[Andy Estes]

Comment on attachment 203086 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4198
> +    unsigned token = ++m_maximumRenderingSuppressionToken;

You could make this 'unsigned token = m_maximumRenderingSuppressionToken + 1', since you'll update m_maximumRenderingSuppressionToken a few lines later. No need to update it twice.

[Tim Horton]

(In reply to comment #6)
> (From update of attachment 203086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4198
> > +    unsigned token = ++m_maximumRenderingSuppressionToken;
> 
> You could make this 'unsigned token = m_maximumRenderingSuppressionToken + 1', since you'll update m_maximumRenderingSuppressionToken a few lines later. No need to update it twice.

Agreed.

(In reply to comment #5)
> You could just leave out the "!token" part if the only reason you were ruling out zero was to avoid the bad hash set value.

That is the only reason, so yeah, I'll leave it out.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4200
> > +        token++;
> 
> This ought to be:
> 
>     token = ++m_maximumRenderingSuppressionToken;
> 
> Don’t you think?

As Andy notes, I update m_maximumRenderingSuppressionToken a few lines below.

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 203086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review
> 
> r=me assuming you deal with the 0xFFFFFFFF problem.

Thanks!

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

Me too!

> 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.

I did not know that!

[Tim Horton]

http://trac.webkit.org/changeset/150838