Summary: | WebBackForwardList should separate "has no current index" from the integer value of the current index | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ahmad.saleem792, darin, eric, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Brady Eidson
2012-05-24 13:02:08 PDT
Created attachment 143934 [details]
Current WIP
Not ready for review. Just attached for posterity.
That WIP went above and beyond what I suggested we should do with this bug. I'll attach a more limited patch that just transitions to using the bool flag and will do a more comprehensive cleanup of the mechanism in https://bugs.webkit.org/show_bug.cgi?id=87513 Created attachment 144103 [details]
Patch v1
Comment on attachment 144103 [details] Patch v1 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. (In reply to comment #4) > > 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. I have a *much* more thorough patch than this one I plan to do in https://bugs.webkit.org/show_bug.cgi?id=87513 Adding the bool and doing the m_current rename seemed like a separate enough task to split it off separately. That said, I'll make many of the tweaks you suggested here before landing, and make sure everything is addressed in bug 87513 Landed in http://trac.webkit.org/changeset/118560 The real fireworks are on their way. Attachment 144103 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.
https://github.com/WebKit/WebKit/commit/78f980d4eb70fdca0a2d1800d40806c0226a53e5 <- Landed and didn't backed out. |