Summary: | Rendering suppression extension tokens shouldn't be 0, should handle overflow | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||
Component: | WebKit2 | Assignee: | Tim Horton <thorton> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | aestes, commit-queue, simon.fraser, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Tim Horton
2013-05-28 13:41:06 PDT
Created attachment 203086 [details]
patch
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.
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. 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? 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. (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. (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! |