[webkit-reviews] review granted: [Bug 87513] WebBackForwardList needs an overhaul to consistently and clearly handle error conditions. : [Attachment 144159] Patch v2 - Now with more style

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 87513: WebBackForwardList needs an overhaul to consistently and clearly
handle error conditions.
https://bugs.webkit.org/show_bug.cgi?id=87513

Attachment 144159: Patch v2 - Now with more style
https://bugs.webkit.org/attachment.cgi?id=144159&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.