Use std::optional<size_t> for a WebBackForwardList's current index
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Oct 2018 14:27:38 +0000 (14:27 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Oct 2018 14:27:38 +0000 (14:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190448

Reviewed by Chris Dumez.

Also remove m_capacity, which was immutable.

* UIProcess/WebBackForwardList.cpp:
(WebKit::WebBackForwardList::WebBackForwardList):
(WebKit::WebBackForwardList::~WebBackForwardList):
(WebKit::WebBackForwardList::pageClosed):
(WebKit::WebBackForwardList::addItem):
(WebKit::WebBackForwardList::goToItem):
(WebKit::WebBackForwardList::currentItem const):
(WebKit::WebBackForwardList::backItem const):
(WebKit::WebBackForwardList::forwardItem const):
(WebKit::WebBackForwardList::itemAtIndex const):
(WebKit::WebBackForwardList::backListCount const):
(WebKit::WebBackForwardList::forwardListCount const):
(WebKit::WebBackForwardList::backListAsAPIArrayWithLimit const):
(WebKit::WebBackForwardList::forwardListAsAPIArrayWithLimit const):
(WebKit::WebBackForwardList::removeAllItems):
(WebKit::WebBackForwardList::clear):
(WebKit::WebBackForwardList::backForwardListState const):
(WebKit::WebBackForwardList::restoreFromState):
(WebKit::WebBackForwardList::loggingString):
* UIProcess/WebBackForwardList.h:
(WebKit::WebBackForwardList::currentIndex const): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@237100 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebBackForwardList.cpp
Source/WebKit/UIProcess/WebBackForwardList.h

index 851d738..f610a60 100644 (file)
@@ -1,3 +1,34 @@
+2018-10-15  Alex Christensen  <achristensen@webkit.org>
+
+        Use std::optional<size_t> for a WebBackForwardList's current index
+        https://bugs.webkit.org/show_bug.cgi?id=190448
+
+        Reviewed by Chris Dumez.
+
+        Also remove m_capacity, which was immutable.
+
+        * UIProcess/WebBackForwardList.cpp:
+        (WebKit::WebBackForwardList::WebBackForwardList):
+        (WebKit::WebBackForwardList::~WebBackForwardList):
+        (WebKit::WebBackForwardList::pageClosed):
+        (WebKit::WebBackForwardList::addItem):
+        (WebKit::WebBackForwardList::goToItem):
+        (WebKit::WebBackForwardList::currentItem const):
+        (WebKit::WebBackForwardList::backItem const):
+        (WebKit::WebBackForwardList::forwardItem const):
+        (WebKit::WebBackForwardList::itemAtIndex const):
+        (WebKit::WebBackForwardList::backListCount const):
+        (WebKit::WebBackForwardList::forwardListCount const):
+        (WebKit::WebBackForwardList::backListAsAPIArrayWithLimit const):
+        (WebKit::WebBackForwardList::forwardListAsAPIArrayWithLimit const):
+        (WebKit::WebBackForwardList::removeAllItems):
+        (WebKit::WebBackForwardList::clear):
+        (WebKit::WebBackForwardList::backForwardListState const):
+        (WebKit::WebBackForwardList::restoreFromState):
+        (WebKit::WebBackForwardList::loggingString):
+        * UIProcess/WebBackForwardList.h:
+        (WebKit::WebBackForwardList::currentIndex const): Deleted.
+
 2018-10-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 
         [JSC] Remove Option::useAsyncIterator
index 6031291..47a5398 100644 (file)
@@ -42,9 +42,6 @@ static const unsigned DefaultCapacity = 100;
 
 WebBackForwardList::WebBackForwardList(WebPageProxy& page)
     : m_page(&page)
-    , m_hasCurrentIndex(false)
-    , m_currentIndex(0)
-    , m_capacity(DefaultCapacity)
 {
     LOG(BackForward, "(Back/Forward) Created WebBackForwardList %p", this);
 }
@@ -54,7 +51,7 @@ WebBackForwardList::~WebBackForwardList()
     LOG(BackForward, "(Back/Forward) Destroying WebBackForwardList %p", this);
 
     // A WebBackForwardList should never be destroyed unless it's associated page has been closed or is invalid.
-    ASSERT((!m_page && !m_hasCurrentIndex) || !m_page->isValid());
+    ASSERT((!m_page && !m_currentIndex) || !m_page->isValid());
 }
 
 WebBackForwardListItem* WebBackForwardList::itemForID(const BackForwardItemIdentifier& identifier)
@@ -85,23 +82,23 @@ void WebBackForwardList::pageClosed()
 
     m_page = nullptr;
     m_entries.clear();
-    m_hasCurrentIndex = false;
+    m_currentIndex = std::nullopt;
 }
 
 void WebBackForwardList::addItem(Ref<WebBackForwardListItem>&& newItem)
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    if (!m_capacity || !m_page)
+    if (!m_page)
         return;
 
     Vector<Ref<WebBackForwardListItem>> removedItems;
     
-    if (m_hasCurrentIndex) {
+    if (m_currentIndex) {
         m_page->recordAutomaticNavigationSnapshot();
 
         // Toss everything in the forward list.
-        unsigned targetSize = m_currentIndex + 1;
+        unsigned targetSize = *m_currentIndex + 1;
         removedItems.reserveCapacity(m_entries.size() - targetSize);
         while (m_entries.size() > targetSize) {
             didRemoveItem(m_entries.last());
@@ -111,15 +108,15 @@ void WebBackForwardList::addItem(Ref<WebBackForwardListItem>&& newItem)
 
         // Toss the first item if the list is getting too big, as long as we're not using it
         // (or even if we are, if we only want 1 entry).
-        if (m_entries.size() == m_capacity && (m_currentIndex || m_capacity == 1)) {
+        if (m_entries.size() >= DefaultCapacity && (*m_currentIndex)) {
             didRemoveItem(m_entries[0]);
             removedItems.append(WTFMove(m_entries[0]));
             m_entries.remove(0);
 
             if (m_entries.isEmpty())
-                m_hasCurrentIndex = false;
+                m_currentIndex = std::nullopt;
             else
-                m_currentIndex--;
+                --*m_currentIndex;
         }
     } else {
         // If we have no current item index we should also not have any entries.
@@ -136,42 +133,41 @@ void WebBackForwardList::addItem(Ref<WebBackForwardListItem>&& newItem)
 
     bool shouldKeepCurrentItem = true;
 
-    if (!m_hasCurrentIndex) {
+    if (!m_currentIndex) {
         ASSERT(m_entries.isEmpty());
         m_currentIndex = 0;
-        m_hasCurrentIndex = true;
     } else {
-        shouldKeepCurrentItem = m_page->shouldKeepCurrentBackForwardListItemInList(m_entries[m_currentIndex]);
+        shouldKeepCurrentItem = m_page->shouldKeepCurrentBackForwardListItemInList(m_entries[*m_currentIndex]);
         if (shouldKeepCurrentItem)
-            m_currentIndex++;
+            ++*m_currentIndex;
     }
 
     auto* newItemPtr = newItem.ptr();
     if (!shouldKeepCurrentItem) {
         // m_current should never be pointing past the end of the entries Vector.
         // If it is, something has gone wrong and we should not try to swap in the new item.
-        ASSERT(m_currentIndex < m_entries.size());
+        ASSERT(*m_currentIndex < m_entries.size());
 
-        removedItems.append(m_entries[m_currentIndex].copyRef());
-        m_entries[m_currentIndex] = WTFMove(newItem);
+        removedItems.append(m_entries[*m_currentIndex].copyRef());
+        m_entries[*m_currentIndex] = WTFMove(newItem);
     } else {
         // m_current should never be pointing more than 1 past the end of the entries Vector.
         // If it is, something has gone wrong and we should not try to insert the new item.
-        ASSERT(m_currentIndex <= m_entries.size());
+        ASSERT(*m_currentIndex <= m_entries.size());
 
-        if (m_currentIndex <= m_entries.size())
-            m_entries.insert(m_currentIndex, WTFMove(newItem));
+        if (*m_currentIndex <= m_entries.size())
+            m_entries.insert(*m_currentIndex, WTFMove(newItem));
     }
 
-    LOG(BackForward, "(Back/Forward) WebBackForwardList %p added an item. Current size %zu, current index %zu, threw away %zu items", this, m_entries.size(), m_currentIndex, removedItems.size());
+    LOG(BackForward, "(Back/Forward) WebBackForwardList %p added an item. Current size %zu, current index %zu, threw away %zu items", this, m_entries.size(), *m_currentIndex, removedItems.size());
     m_page->didChangeBackForwardList(newItemPtr, WTFMove(removedItems));
 }
 
 void WebBackForwardList::goToItem(WebBackForwardListItem& item)
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    if (!m_entries.size() || !m_page || !m_hasCurrentIndex)
+    if (!m_entries.size() || !m_page || !m_currentIndex)
         return;
 
     size_t targetIndex = notFound;
@@ -188,7 +184,7 @@ void WebBackForwardList::goToItem(WebBackForwardListItem& item)
         return;
     }
 
-    if (targetIndex < m_currentIndex) {
+    if (targetIndex < *m_currentIndex) {
         unsigned delta = m_entries.size() - targetIndex - 1;
         String deltaValue = delta > 10 ? "over10"_s : String::number(delta);
         m_page->logDiagnosticMessage(WebCore::DiagnosticLoggingKeys::backNavigationDeltaKey(), deltaValue, ShouldSample::No);
@@ -196,18 +192,18 @@ void WebBackForwardList::goToItem(WebBackForwardListItem& item)
 
     // If we're going to an item different from the current item, ask the client if the current
     // item should remain in the list.
-    auto& currentItem = m_entries[m_currentIndex];
+    auto& currentItem = m_entries[*m_currentIndex];
     bool shouldKeepCurrentItem = true;
     if (currentItem.ptr() != &item) {
         m_page->recordAutomaticNavigationSnapshot();
-        shouldKeepCurrentItem = m_page->shouldKeepCurrentBackForwardListItemInList(m_entries[m_currentIndex]);
+        shouldKeepCurrentItem = m_page->shouldKeepCurrentBackForwardListItemInList(m_entries[*m_currentIndex]);
     }
 
     // If the client said to remove the current item, remove it and then update the target index.
     Vector<Ref<WebBackForwardListItem>> removedItems;
     if (!shouldKeepCurrentItem) {
         removedItems.append(currentItem.copyRef());
-        m_entries.remove(m_currentIndex);
+        m_entries.remove(*m_currentIndex);
         targetIndex = notFound;
         for (size_t i = 0; i < m_entries.size(); ++i) {
             if (m_entries[i].ptr() == &item) {
@@ -226,30 +222,30 @@ void WebBackForwardList::goToItem(WebBackForwardListItem& item)
 
 WebBackForwardListItem* WebBackForwardList::currentItem() const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    return m_page && m_hasCurrentIndex ? m_entries[m_currentIndex].ptr() : nullptr;
+    return m_page && m_currentIndex ? m_entries[*m_currentIndex].ptr() : nullptr;
 }
 
 WebBackForwardListItem* WebBackForwardList::backItem() const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    return m_page && m_hasCurrentIndex && m_currentIndex ? m_entries[m_currentIndex - 1].ptr() : nullptr;
+    return m_page && m_currentIndex && *m_currentIndex ? m_entries[*m_currentIndex - 1].ptr() : nullptr;
 }
 
 WebBackForwardListItem* WebBackForwardList::forwardItem() const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    return m_page && m_hasCurrentIndex && m_entries.size() && m_currentIndex < m_entries.size() - 1 ? m_entries[m_currentIndex + 1].ptr() : nullptr;
+    return m_page && m_currentIndex && m_entries.size() && *m_currentIndex < m_entries.size() - 1 ? m_entries[*m_currentIndex + 1].ptr() : nullptr;
 }
 
 WebBackForwardListItem* WebBackForwardList::itemAtIndex(int index) const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    if (!m_hasCurrentIndex || !m_page)
+    if (!m_currentIndex || !m_page)
         return nullptr;
     
     // Do range checks without doing math on index to avoid overflow.
@@ -259,21 +255,21 @@ WebBackForwardListItem* WebBackForwardList::itemAtIndex(int index) const
     if (index > forwardListCount())
         return nullptr;
         
-    return m_entries[index + m_currentIndex].ptr();
+    return m_entries[index + *m_currentIndex].ptr();
 }
 
 int WebBackForwardList::backListCount() const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    return m_page && m_hasCurrentIndex ? m_currentIndex : 0;
+    return m_page && m_currentIndex ? *m_currentIndex : 0;
 }
 
 int WebBackForwardList::forwardListCount() const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    return m_page && m_hasCurrentIndex ? m_entries.size() - (m_currentIndex + 1) : 0;
+    return m_page && m_currentIndex ? m_entries.size() - (*m_currentIndex + 1) : 0;
 }
 
 Ref<API::Array> WebBackForwardList::backList() const
@@ -288,9 +284,9 @@ Ref<API::Array> WebBackForwardList::forwardList() const
 
 Ref<API::Array> WebBackForwardList::backListAsAPIArrayWithLimit(unsigned limit) const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    if (!m_page || !m_hasCurrentIndex)
+    if (!m_page || !m_currentIndex)
         return API::Array::create();
 
     unsigned backListSize = static_cast<unsigned>(backListCount());
@@ -310,9 +306,9 @@ Ref<API::Array> WebBackForwardList::backListAsAPIArrayWithLimit(unsigned limit)
 
 Ref<API::Array> WebBackForwardList::forwardListAsAPIArrayWithLimit(unsigned limit) const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
-    if (!m_page || !m_hasCurrentIndex)
+    if (!m_page || !m_currentIndex)
         return API::Array::create();
 
     unsigned size = std::min(static_cast<unsigned>(forwardListCount()), limit);
@@ -322,9 +318,9 @@ Ref<API::Array> WebBackForwardList::forwardListAsAPIArrayWithLimit(unsigned limi
     Vector<RefPtr<API::Object>> vector;
     vector.reserveInitialCapacity(size);
 
-    unsigned last = m_currentIndex + size;
+    size_t last = *m_currentIndex + size;
     ASSERT(last < m_entries.size());
-    for (unsigned i = m_currentIndex + 1; i <= last; ++i)
+    for (size_t i = *m_currentIndex + 1; i <= last; ++i)
         vector.uncheckedAppend(m_entries[i].ptr());
 
     return API::Array::create(WTFMove(vector));
@@ -332,7 +328,7 @@ Ref<API::Array> WebBackForwardList::forwardListAsAPIArrayWithLimit(unsigned limi
 
 void WebBackForwardList::removeAllItems()
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
     LOG(BackForward, "(Back/Forward) WebBackForwardList %p removeAllItems (has %zu of them)", this, m_entries.size());
 
@@ -344,13 +340,13 @@ void WebBackForwardList::removeAllItems()
     }
 
     m_entries.clear();
-    m_hasCurrentIndex = false;
+    m_currentIndex = std::nullopt;
     m_page->didChangeBackForwardList(nullptr, WTFMove(removedItems));
 }
 
 void WebBackForwardList::clear()
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
     LOG(BackForward, "(Back/Forward) WebBackForwardList %p clear (has %zu of them)", this, m_entries.size());
 
@@ -363,7 +359,7 @@ void WebBackForwardList::clear()
 
     if (!currentItem) {
         // We should only ever have no current item if we also have no current item index.
-        ASSERT(!m_hasCurrentIndex);
+        ASSERT(!m_currentIndex);
 
         // But just in case it does happen in practice we should get back into a consistent state now.
         for (size_t i = 0; i < size; ++i) {
@@ -372,7 +368,7 @@ void WebBackForwardList::clear()
         }
 
         m_entries.clear();
-        m_hasCurrentIndex = false;
+        m_currentIndex = std::nullopt;
         m_page->didChangeBackForwardList(nullptr, WTFMove(removedItems));
 
         return;
@@ -385,7 +381,7 @@ void WebBackForwardList::clear()
 
     removedItems.reserveCapacity(size - 1);
     for (size_t i = 0; i < size; ++i) {
-        if (i != m_currentIndex && m_hasCurrentIndex)
+        if (m_currentIndex && i != *m_currentIndex)
             removedItems.append(WTFMove(m_entries[i]));
     }
 
@@ -395,17 +391,17 @@ void WebBackForwardList::clear()
     if (currentItem)
         m_entries.append(currentItem.releaseNonNull());
     else
-        m_hasCurrentIndex = false;
+        m_currentIndex = std::nullopt;
     m_page->didChangeBackForwardList(nullptr, WTFMove(removedItems));
 }
 
 BackForwardListState WebBackForwardList::backForwardListState(WTF::Function<bool (WebBackForwardListItem&)>&& filter) const
 {
-    ASSERT(!m_hasCurrentIndex || m_currentIndex < m_entries.size());
+    ASSERT(!m_currentIndex || *m_currentIndex < m_entries.size());
 
     BackForwardListState backForwardListState;
-    if (m_hasCurrentIndex)
-        backForwardListState.currentIndex = m_currentIndex;
+    if (m_currentIndex)
+        backForwardListState.currentIndex = *m_currentIndex;
 
     for (size_t i = 0; i < m_entries.size(); ++i) {
         auto& entry = m_entries[i];
@@ -441,8 +437,7 @@ void WebBackForwardList::restoreFromState(BackForwardListState backForwardListSt
         backForwardListItemState.identifier = { Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() };
         items.uncheckedAppend(WebBackForwardListItem::create(WTFMove(backForwardListItemState), m_page->pageID()));
     }
-    m_hasCurrentIndex = !!backForwardListState.currentIndex;
-    m_currentIndex = backForwardListState.currentIndex.value_or(0);
+    m_currentIndex = backForwardListState.currentIndex ? std::optional<size_t>(*backForwardListState.currentIndex) : std::nullopt;
     m_entries = WTFMove(items);
 
     LOG(BackForward, "(Back/Forward) WebBackForwardList %p restored from state (has %zu entries)", this, m_entries.size());
@@ -482,11 +477,11 @@ void WebBackForwardList::didRemoveItem(WebBackForwardListItem& backForwardListIt
 const char* WebBackForwardList::loggingString()
 {
     StringBuilder builder;
-    builder.append(String::format("WebBackForwardList %p - %zu entries, has current index %s (%zu)", this, m_entries.size(), m_hasCurrentIndex ? "YES" : "NO", m_hasCurrentIndex ? m_currentIndex : 0));
+    builder.append(String::format("WebBackForwardList %p - %zu entries, has current index %s (%zu)", this, m_entries.size(), m_currentIndex ? "YES" : "NO", m_currentIndex ? *m_currentIndex : 0));
 
     for (size_t i = 0; i < m_entries.size(); ++i) {
         builder.append("\n");
-        if (m_hasCurrentIndex && m_currentIndex == i)
+        if (m_currentIndex && *m_currentIndex == i)
             builder.append(" * ");
         else
             builder.append(" - ");
index 5ea5678..048029d 100644 (file)
@@ -60,7 +60,6 @@ public:
 
     const BackForwardListItemVector& entries() const { return m_entries; }
 
-    uint32_t currentIndex() const { return m_currentIndex; }
     int backListCount() const;
     int forwardListCount() const;
 
@@ -87,11 +86,7 @@ private:
 
     WebPageProxy* m_page;
     BackForwardListItemVector m_entries;
-    
-    // FIXME: m_currentIndex should be a std::optional<size_t>
-    bool m_hasCurrentIndex;
-    size_t m_currentIndex;
-    size_t m_capacity;
+    std::optional<size_t> m_currentIndex;
 };
 
 } // namespace WebKit