[Webkit-unassigned] [Bug 87418] WebBackForwardList should separate "has no current index" from the integer value of the current index
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 25 13:17:22 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=87418
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #144103|review? |review+
Flag| |
--- Comment #4 from Darin Adler <darin at apple.com> 2012-05-25 13:16:26 PST ---
(From update of attachment 144103)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list