Bug 87513

Summary: WebBackForwardList needs an overhaul to consistently and clearly handle error conditions.
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: 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 Flags
Patch v1
none
Patch v2 - Now with more style darin: review+

Description Brady Eidson 2012-05-25 11:00:28 PDT
WebBackForwardList needs an overhaul to consistently and clearly handle error conditions.

This will be a followup to https://bugs.webkit.org/show_bug.cgi?id=87418
Comment 1 Brady Eidson 2012-05-25 15:22:32 PDT
Created attachment 144154 [details]
Patch v1
Comment 2 WebKit Review Bot 2012-05-25 15:24:30 PDT
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.
Comment 3 Brady Eidson 2012-05-25 15:42:18 PDT
Created attachment 144159 [details]
Patch v2 - Now with more style
Comment 4 Brady Eidson 2012-06-26 13:40:02 PDT
Bump.
Comment 5 Darin Adler 2012-06-26 16:21:17 PDT
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.
Comment 6 Brady Eidson 2012-06-27 09:01:21 PDT
(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!
Comment 7 Brady Eidson 2012-06-27 11:29:15 PDT
http://trac.webkit.org/changeset/121354