Fix VectorIterator::MoveNext() behaviour

Updates the VectorIterator interface to be compatible with the clarified
IIterator API. The return value should be S_OK if we just moved past the
last item, and E_BOUNDS if the iterator was already past the end before.
See updated docs:
https://docs.microsoft.com/en-us/previous-versions/br205830(v=vs.85)

Bug: None
Change-Id: Ifac585ca1dcfbf967afc801a64c97fa17ee2ef79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2124831Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782977}

base/win/map_unittest.cc

@@ -476,12 +476,15 @@ TEST(MapTest, First) {
   EXPECT_HRESULT_SUCCEEDED(hr);
   EXPECT_EQ(20.3, value);
   hr = iterator->MoveNext(&has_current);
-  EXPECT_FALSE(SUCCEEDED(hr));
-  EXPECT_EQ(E_BOUNDS, hr);
+  EXPECT_HRESULT_SUCCEEDED(hr);
   EXPECT_FALSE(has_current);
   hr = iterator->get_Current(&current);
   EXPECT_FALSE(SUCCEEDED(hr));
   EXPECT_EQ(E_BOUNDS, hr);
+  hr = iterator->MoveNext(&has_current);
+  EXPECT_FALSE(SUCCEEDED(hr));
+  EXPECT_EQ(E_BOUNDS, hr);
+  EXPECT_FALSE(has_current);
 
   // Test invalidation.
   hr = map->First(&iterator);

base/win/vector.h

@@ -77,14 +77,24 @@ class VectorIterator
   }
 
   IFACEMETHODIMP MoveNext(boolean* has_current) override {
-    ++current_index_;
     *has_current = FALSE;
-
-    HRESULT hr = get_HasCurrent(has_current);
+    unsigned size;
+    HRESULT hr = view_->get_Size(&size);
     if (FAILED(hr))
       return hr;
 
-    return *has_current ? hr : E_BOUNDS;
+    // Check if we're already past the last item.
+    if (current_index_ >= size)
+      return E_BOUNDS;
+
+    // Move to the next item.
+    current_index_++;
+
+    // Set |has_current| to TRUE if we're still on a valid item.
+    if (current_index_ < size)
+      *has_current = TRUE;
+
+    return hr;
   }
 
   IFACEMETHODIMP GetMany(unsigned capacity,

base/win/vector_unittest.cc

@@ -698,12 +698,15 @@ TEST(VectorTest, First) {
   EXPECT_TRUE(SUCCEEDED(hr));
   EXPECT_EQ(3, current);
   hr = iterator->MoveNext(&has_current);
-  EXPECT_FALSE(SUCCEEDED(hr));
-  EXPECT_EQ(E_BOUNDS, hr);
+  EXPECT_TRUE(SUCCEEDED(hr));
   EXPECT_FALSE(has_current);
   hr = iterator->get_Current(&current);
   EXPECT_FALSE(SUCCEEDED(hr));
   EXPECT_EQ(E_BOUNDS, hr);
+  hr = iterator->MoveNext(&has_current);
+  EXPECT_FALSE(SUCCEEDED(hr));
+  EXPECT_EQ(E_BOUNDS, hr);
+  EXPECT_FALSE(has_current);
 
   hr = vec->First(&iterator);
   EXPECT_TRUE(SUCCEEDED(hr));
@@ -715,5 +718,133 @@ TEST(VectorTest, First) {
   EXPECT_THAT(copy, ElementsAre(1, 2, 3));
 }
 
+TEST(VectorTest, MoveNext_S_OK_ValidItem) {
+  auto vec = Make<Vector<int>>(g_one_two_three);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+
+  // Moving next to a valid item should return S_OK:
+  // [1, 2, 3]
+  //  |->|
+  EXPECT_EQ(S_OK, iterator->MoveNext(&has_current));
+}
+
+TEST(VectorTest, MoveNext_S_OK_FromLastItem) {
+  auto vec = Make<Vector<int>>(g_one);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+
+  // Moving next past the last item should return S_OK:
+  // [1]
+  //  |->|
+  EXPECT_EQ(S_OK, iterator->MoveNext(&has_current));
+}
+
+TEST(VectorTest, MoveNext_E_CHANGED_STATE_ValidItem) {
+  auto vec = Make<Vector<int>>(g_one_two_three);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+
+  vec->Append(4);
+
+  // Moving next after changing the vector should return E_CHANGED_STATE:
+  EXPECT_EQ(E_CHANGED_STATE, iterator->MoveNext(&has_current));
+}
+
+TEST(VectorTest, MoveNext_E_CHANGED_STATE_AfterLastItem) {
+  auto vec = Make<Vector<int>>(g_one);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+  iterator->MoveNext(&has_current);
+
+  vec->Append(4);
+
+  // Moving next after changing the vector should return E_CHANGED_STATE:
+  EXPECT_EQ(E_CHANGED_STATE, iterator->MoveNext(&has_current));
+}
+
+TEST(VectorTest, MoveNext_E_BOUNDS) {
+  auto vec = Make<Vector<int>>(g_one);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+  iterator->MoveNext(&has_current);
+
+  // Moving next when already past the last item should return E_BOUNDS:
+  // [1]
+  //     |->|
+  EXPECT_EQ(E_BOUNDS, iterator->MoveNext(&has_current));
+}
+
+TEST(VectorTest, MoveNext_HasCurrent_ValidItem) {
+  auto vec = Make<Vector<int>>(g_one_two_three);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+
+  // Moving next to a valid item should set |has_current| to true:
+  // [1, 2, 3]
+  //  |->|
+  iterator->MoveNext(&has_current);
+  EXPECT_TRUE(has_current);
+}
+
+TEST(VectorTest, MoveNext_HasCurrent_LastItem) {
+  auto vec = Make<Vector<int>>(g_one_two);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+
+  // Moving next to the last item should set |has_current| to true:
+  // [1, 2]
+  //  |->|
+  iterator->MoveNext(&has_current);
+  EXPECT_TRUE(has_current);
+}
+
+TEST(VectorTest, MoveNext_HasCurrent_FromLastItem) {
+  auto vec = Make<Vector<int>>(g_one);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+  iterator->MoveNext(&has_current);
+
+  // Moving next when already past the end should set |has_current| to false:
+  // [1]
+  //     |->|
+  iterator->MoveNext(&has_current);
+  EXPECT_FALSE(has_current);
+}
+
+TEST(VectorTest, MoveNext_HasCurrent_AfterLastItem) {
+  auto vec = Make<Vector<int>>(g_one);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+
+  // Moving next from the last item should set |has_current| to false:
+  // [1]
+  //  |->|
+  boolean has_current;
+  iterator->MoveNext(&has_current);
+  EXPECT_FALSE(has_current);
+}
+
+TEST(VectorTest, MoveNext_HasCurrent_Changed) {
+  auto vec = Make<Vector<int>>(g_one_two);
+  ComPtr<IIterator<int>> iterator;
+  vec->First(&iterator);
+  boolean has_current;
+
+  vec->Append(4);
+
+  // Moving next after changing the vector should set |has_current| to false:
+  iterator->MoveNext(&has_current);
+  EXPECT_FALSE(has_current);
+}
+
 }  // namespace win
 }  // namespace base