Bug 87513 – WebBackForwardList needs an overhaul to consistently and clearly handle error conditions.

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

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

[Brady Eidson]

Created attachment 144154 [details]
Patch v1

[WebKit Review Bot]

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.

[Brady Eidson]

Created attachment 144159 [details]
Patch v2 - Now with more style

[Brady Eidson]

Bump.

[Darin Adler]

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.

[Brady Eidson]

(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!

[Brady Eidson]

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