Summary: | WebBackForwardList needs an overhaul to consistently and clearly handle error conditions. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Brady Eidson
2012-05-25 11:00:28 PDT
Created attachment 144154 [details]
Patch v1
Attachment 144154 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:192: Extra space before ) in if [whitespace/parens] [5]
Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:255: Extra space before ) in if [whitespace/parens] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 144159 [details]
Patch v2 - Now with more style
Bump. Comment on attachment 144159 [details] Patch v2 - Now with more style View in context: https://bugs.webkit.org/attachment.cgi?id=144159&action=review Looks OK. Did we really have to add a V1? > Source/WebKit2/UIProcess/WebBackForwardList.cpp:266 > + // And if that's the case we should also not have any entries. > + ASSERT(m_entries.isEmpty()); This assertion is pointless. The code above already returned if m_entries.size() was <= 1 so this assertion can never be hit. So if we want to assert something, we should assert currentItem before this impossible if statement. > Source/WebKit2/UIProcess/WebBackForwardList.cpp:268 > + // But just in case it does happen in practice we should get back in to a consistent state now. Typo: Should be "into" not "in to". > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:112 > + ASSERT(currentIndex == -1 || (currentIndex > -1 && currentIndex < CFArrayGetCount(entries.get()))); I would have written >= 0 rather than > -1). > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:113 > + if (currentIndex < -1 || (currentIndex > -1 && currentIndex >= CFArrayGetCount(entries.get()))) { Either you should write this exactly like the assertion, or more simply like this: if (currentIndex < -1 || currentIndex >= CFArrayGetCount(entries.get())) The check for currentIndex > -1 does not add anything. (In reply to comment #5) > (From update of attachment 144159 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144159&action=review > > Looks OK. Did we really have to add a V1? I like how much stricter we get to be in the V1 code because we get to differentiate the "has no current item index" case. All other comments accounted for before landing. Thanks! |