[webkit-reviews] review granted: [Bug 87418] WebBackForwardList should separate "has no current index" from the integer value of the current index : [Attachment 144103] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 87418: WebBackForwardList should separate "has no current index" from the
integer value of the current index
https://bugs.webkit.org/show_bug.cgi?id=87418

Attachment 144103: Patch v1
https://bugs.webkit.org/attachment.cgi?id=144103&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=144103&action=review


> Source/WebKit2/UIProcess/WebBackForwardList.cpp:139
> +    if (m_hasCurrentIndex)
> +	   return m_entries[m_currentIndex].get();
>      return 0;

I think this would read better with a ? : than with an if statement. Or the
early return should be the "return 0" if you want to stick with an if.

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:148
> +    if (m_currentIndex && m_hasCurrentIndex)
> +	   return m_entries[m_currentIndex - 1].get();
>      return 0;

Same comment as above. I think the early return should be the error case or we
could use a ? : expression.

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:157
> +    if (m_entries.size() && m_hasCurrentIndex && m_currentIndex <
m_entries.size() - 1)
> +	   return m_entries[m_currentIndex + 1].get();
>      return 0;

And again.

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:260
> +	   m_currentIndex = 0;

No need for this line of code:

1) The code above already sets m_currentIndex to 0.
2) If we set m_hasCurrentIndex to false, it doesn’t matter what m_currentIndex
is set to.

> Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:141
> +    uint32_t currentIndex = currentCFIndex == -1 ? 0 :
static_cast<unsigned>(currentCFIndex);

Mix of unsigned and uint32_t on this line.

> Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:142
> +    bool hasCurrentIndex = currentCFIndex > -1;

We should do this first, and then we could use it when initializing
currentIndex.

> Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:190
>      // Perform a sanity check: in case we're out of range, we reset.
> -    if (m_current != NoCurrentItemIndex && m_current >= newEntries.size())
> -	   m_current = NoCurrentItemIndex;
> +    if (m_hasCurrentIndex && m_currentIndex >= newEntries.size())
> +	   m_hasCurrentIndex = false;

We don’t need this sanity check because it’s redundant with checks done above.
I thought you were going to remove it and replace it with an assertion in this
patch.