Fix DrMemory test for NativeDesktopMediaListTest.UpdateThumbnail

chrome/browser/media/native_desktop_media_list_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/native_desktop_media_list.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
 #include <vector>
@@ skipping 295 matching lines @@
     }
 
     {
       testing::InSequence dummy;
       size_t source_count = screen ? window_count + 1 : window_count;
       for (size_t i = 0; i < source_count; ++i) {
         EXPECT_CALL(observer_, OnSourceAdded(model_.get(), i))
             .WillOnce(CheckListSize(model_.get(), static_cast<int>(i + 1)));
       }
       for (size_t i = 0; i < source_count - 1; ++i) {
         EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i));

[Sergey Ulanov, 2016/06/07 23:42:30]

For aura Windows add .Times(testing::AtLeast(1)) to ignore any extra
OnSourceThumbnailChanged() calls?

[GeorgeZ, 2016/06/08 03:02:24]

This part checks the first refresh. The OnSourceThumbnailChanged() will be
called for each window once and just once. testing::InSequence is also declared.
Then the message loop will be killed. There is no extra thumbnail refresh is
expected after that. Therefore .Times(testing::AtLeast(1)) may not be necessary.

[Sergey Ulanov, 2016/06/08 19:56:38]

Yeah, Times(AtLeast(1)) would not work here because of the sequence and also
because there is VerifyAndClearExpectations() below. See my next comment.

       }
       EXPECT_CALL(observer_,
                   OnSourceThumbnailChanged(model_.get(), source_count - 1))
           .WillOnce(QuitMessageLoop(message_loop()));
     }
     model_->StartUpdating(&observer_);
     message_loop()->Run();
 
     if (screen) {
       EXPECT_EQ(model_->GetSource(0).id.type, DesktopMediaID::TYPE_SCREEN);
       EXPECT_EQ(model_->GetSource(0).id.id, 0);
     }
 
     for (size_t i = 0; i < window_count; ++i) {
       size_t source_index = screen ? i + 1 : i;
       EXPECT_EQ(model_->GetSource(source_index).id.type,
                 DesktopMediaID::TYPE_WINDOW);
       EXPECT_EQ(model_->GetSource(source_index).name,
                 base::UTF8ToUTF16("Test window"));
       int index = has_view_dialog ? i + 1 : i;
       int native_id = window_list_[index].id;
       EXPECT_EQ(model_->GetSource(source_index).id.id, native_id);
 #if defined(USE_AURA)
       if (i >= aura_window_first_index)
         EXPECT_EQ(model_->GetSource(source_index).id.aura_id,
                   native_aura_id_map_[native_id]);
 #endif
     }
     testing::Mock::VerifyAndClearExpectations(&observer_);
   }

[Sergey Ulanov, 2016/06/08 19:56:38]

Add the expectations here? something like this:

for (size_t i = source_count - aura_window_count; i < source_count; ++i)
  EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), i))
     .Times(testing::Any());
}

[GeorgeZ, 2016/06/08 21:27:41]

This will lead to TEST_F(NativeDesktopMediaListTest, AddNativeWindow) failed.

AddNativeWindow leads to OnSourceThumbnailChanged(model_.get(), 3));

Here is the snapshot of error report:
EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), index))...

  Expected arg #1: is equal to 2
           Actual: 3
         Expected: to be called any number of times
           Actual: never called - satisfied and active
[  FAILED  ] NativeDesktopMediaListTest.AddNativeWindow (144 ms)


The number of windows and possible their indices may change int the unit tests.
Other than the first refresh, OnSourceThumbnailchanged() will not be checked for
unit tests that have aura windows. Therefore it is safe to not add
.Times(testing::AnyNumber()). If .Times(testing::AnyNumber()) does need to be
added, I may need add it to each TEST_F unit test.

[Sergey Ulanov, 2016/06/08 21:40:16]

Just add it for the UpdateThumbnail test? That's the only test that sets
OnSourceThumbnailchanged() expectation

[GeorgeZ, 2016/06/08 22:16:40]

Done.

 
  protected:
   // Must be listed before |model_|, so it's destroyed last.
   MockObserver observer_;
 
   // Owned by |model_|;
   FakeWindowCapturer* window_capturer_;
 
   webrtc::WindowCapturer::WindowList window_list_;
   std::vector<std::unique_ptr<views::Widget>> desktop_widgets_;
@@ skipping 115 matching lines @@
   const std::string kTestTitle = "New Title";
   window_list_[0].title = kTestTitle;
   window_capturer_->SetWindowList(window_list_);
 
   message_loop()->Run();
 
   EXPECT_EQ(model_->GetSource(1).name, base::UTF8ToUTF16(kTestTitle));
 }
 
 TEST_F(NativeDesktopMediaListTest, UpdateThumbnail) {
-  AddWindowsAndVerify(true, kDefaultWindowCount, kDefaultAuraCount, false);
+  AddWindowsAndVerify(true, kDefaultWindowCount, 0, false);
 
   EXPECT_CALL(observer_, OnSourceThumbnailChanged(model_.get(), 1))
       .WillOnce(QuitMessageLoop(message_loop()));
 
   // Update frame for the window and verify that we get notification about it.
   window_capturer_->SetNextFrameValue(1, 10);
 
   message_loop()->Run();
 }
 
 TEST_F(NativeDesktopMediaListTest, MoveWindow) {
   AddWindowsAndVerify(true, kDefaultWindowCount, kDefaultAuraCount, false);
 
   EXPECT_CALL(observer_, OnSourceMoved(model_.get(), 2, 1))
       .WillOnce(DoAll(CheckListSize(model_.get(), kDefaultWindowCount + 1),
                       QuitMessageLoop(message_loop())));
 
   // Swap the two windows.
   webrtc::WindowCapturer::Window temp = window_list_[0];
   window_list_[0] = window_list_[1];
   window_list_[1] = temp;
   window_capturer_->SetWindowList(window_list_);
 
   message_loop()->Run();
 }