MD Downloads: track downloads in C++, dispatch discrete JS updates

chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc

Index: chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc
diff --git a/chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc b/chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e9ae9a9e89f9b0e2cdd477a7077154c2acefcae9
--- /dev/null
+++ b/chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc
@@ -0,0 +1,277 @@
+// Copyright 2015 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/ui/webui/md_downloads/downloads_list_tracker.h"
+
+#include <vector>
+
+#include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/stl_util.h"
+#include "base/time/time.h"
+#include "chrome/browser/download/download_item_model.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/mock_download_item.h"
+#include "content/public/test/mock_download_manager.h"
+#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_web_ui.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::DownloadItem;
+using content::MockDownloadItem;
+using DownloadVector = std::vector<DownloadItem*>;
+using testing::_;
+using testing::Return;
+
+namespace {
+
+uint64 GetId(const base::Value* value) {
+  const base::DictionaryValue* dict;
+  CHECK(value->GetAsDictionary(&dict));
+
+  int id;
+  CHECK(dict->GetInteger("id", &id));
+  CHECK_GE(id, 0);
+  return static_cast<uint64>(id);
+}
+
+std::vector<uint64> GetIds(const base::Value* value) {
+  CHECK(value);
+
+  std::vector<uint64> ids;
+
+  if (value->GetType() == base::Value::TYPE_LIST) {
+    const base::ListValue* list;
+    value->GetAsList(&list);
+
+    for (auto* list_item : *list) {

[asanka, 2015/11/24 22:08:22]

Nit: No braces

[Dan Beam, 2015/11/25 04:06:36]

"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces."

https://google.github.io/styleguide/cppguide.html#Conditionals

i've never fully understood why something in the "Conditionals" section of the
C++ style guide would affect loops, so ambiguity abounds.  will do it if it
makes you happy, though...

[asanka, 2015/11/25 20:49:06]

Hehe.

+      ids.push_back(GetId(list_item));
+    }
+  } else {
+    ids.push_back(GetId(value));
+  }
+
+  return ids;
+}
+
+int GetIndex(const base::Value* value) {
+  CHECK(value);
+  int index;
+  CHECK(value->GetAsInteger(&index));
+  return index;
+}
+
+bool ShouldShowItem(const DownloadItem& item) {
+  DownloadItemModel model(const_cast<DownloadItem*>(&item));
+  return model.ShouldShowInShelf();
+}
+
+}  // namespace
+
+// A test version of DownloadsListTracker.
+class TestDownloadsListTracker : public DownloadsListTracker {
+ public:
+  TestDownloadsListTracker(content::DownloadManager* manager,
+                           content::WebUI* web_ui)
+      : DownloadsListTracker(manager, web_ui, base::Bind(&ShouldShowItem)) {}
+  ~TestDownloadsListTracker() override {}
+
+  using DownloadsListTracker::GetItemForTesting;
+
+ protected:
+  scoped_ptr<base::DictionaryValue> CreateDownloadItemValue(
+      content::DownloadItem* item) const override {

[asanka, 2015/11/24 22:08:22]

Take a const DownloadItem& or const DownloadItem*

[Dan Beam, 2015/11/25 04:06:36]

this is overriding the method in DownloadsListTracker, which has multiple cases
that currently require a non-const DownloadItem* (DownloadItemModel(),
DownloadManager::GetDownload(), DownloadedByExtension::Get()).

i could try to make const versions or update the downstream code to do this, if
you really want, but I don't think it's worth it

+    scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
+    CHECK_LE(item->GetId(), static_cast<uint64>(INT_MAX));
+    dict->SetInteger("id", item->GetId());
+    return dict.Pass();
+  }
+};
+
+// A fixture to test DownloadsListTracker.
+class DownloadsListTrackerTest : public testing::Test {
+ public:
+  DownloadsListTrackerTest()
+      : ui_thread_(content::BrowserThread::UI, &message_loop_) {}
+  ~DownloadsListTrackerTest() override {
+    for (auto* mock_item : mock_items_)
+      testing::Mock::VerifyAndClear(mock_item);

[asanka, 2015/11/24 22:08:22]

Expectations are verified when the mock items are destroyed in the
STLDeleteElements() call below.

[Dan Beam, 2015/11/25 04:06:36]

when a base::Time() is created via mock_item->GetStartTime(), gmock whines about
the object being leaked in ~mock_item.  this fixes that.

+    STLDeleteElements(&mock_items_);
+  }
+
+  // testing::Test:
+  void SetUp() override {
+    ON_CALL(manager_, GetBrowserContext()).WillByDefault(Return(&profile_));
+    ON_CALL(manager_, GetAllDownloads(_)).WillByDefault(
+        testing::Invoke(this, &DownloadsListTrackerTest::GetAllDownloads));
+  }
+
+  MockDownloadItem* CreateMock(uint64 id, double started) {
+    MockDownloadItem* new_item = new testing::NiceMock<MockDownloadItem>();
+    mock_items_.push_back(new_item);
+
+    ON_CALL(*new_item, GetId()).WillByDefault(Return(id));
+    ON_CALL(*new_item, GetStartTime()).WillByDefault(
+        Return(base::Time::FromDoubleT(started)));
+
+    return new_item;
+  }
+
+  MockDownloadItem* CreateNextItem() {
+    return CreateMock(mock_items_.size(), mock_items_.size());

[asanka, 2015/11/24 22:08:22]

Nit: GetStartTime() is Time::FromDoubleT(static_cast<double>(mock_item_.size()))
?

Why pass through double at all if the time value is integral?

[Dan Beam, 2015/11/25 04:06:36]

Done.

[asanka, 2015/11/25 20:49:06]

Better, but something like :

  CreateMock(mock_items_.size(), base::Time::UnixEpoch() +
base::TimeDelta::FromHours(mock_items_.size()));

where CreateMock(..) takes a base::Time would make it clearer what the code is
trying to do. But I don't feel strongly about it.

[Dan Beam, 2015/11/26 01:40:08]

Done.

+  }
+
+  void CreateTracker() {
+    tracker_.reset(new TestDownloadsListTracker(manager(), web_ui()));
+  }
+
+  content::DownloadManager* manager() { return &manager_; }
+  content::TestWebUI* web_ui() { return &web_ui_; }
+  TestDownloadsListTracker* tracker() { return tracker_.get(); }
+
+ private:
+  void GetAllDownloads(DownloadVector* result) {
+    for (auto* mock_item : mock_items_)
+      result->push_back(mock_item);
+  }
+
+  // NOTE: The initialization order of these members matters.
+  base::MessageLoopForUI message_loop_;
+  content::TestBrowserThread ui_thread_;
+  TestingProfile profile_;
+
+  testing::NiceMock<content::MockDownloadManager> manager_;
+  content::TestWebUI web_ui_;
+  scoped_ptr<TestDownloadsListTracker> tracker_;
+
+  std::vector<MockDownloadItem*> mock_items_;
+};
+
+TEST_F(DownloadsListTrackerTest, SetSearchTerms) {
+  CreateTracker();
+
+  const base::ListValue empty_terms;
+  EXPECT_FALSE(tracker()->SetSearchTerms(empty_terms));
+
+  base::ListValue search_terms;
+  search_terms.AppendString("search");
+  EXPECT_TRUE(tracker()->SetSearchTerms(search_terms));
+
+  EXPECT_FALSE(tracker()->SetSearchTerms(search_terms));
+
+  EXPECT_TRUE(tracker()->SetSearchTerms(empty_terms));
+
+  // Notifying the page is left up to the handler in this case.
+  EXPECT_TRUE(web_ui()->call_data().empty());
+}
+
+TEST_F(DownloadsListTrackerTest, StartCallsInsertItems) {
+  DownloadItem* first_item = CreateNextItem();
+
+  CreateTracker();
+  ASSERT_TRUE(tracker()->GetItemForTesting(0));
+  EXPECT_TRUE(web_ui()->call_data().empty());
+
+  tracker()->Start();
+  ASSERT_FALSE(web_ui()->call_data().empty());
+
+  EXPECT_EQ("downloads.Manager.insertItems",
+            web_ui()->call_data()[0]->function_name());
+  EXPECT_EQ(0, GetIndex(web_ui()->call_data()[0]->arg1()));
+
+  std::vector<uint64> ids = GetIds(web_ui()->call_data()[0]->arg2());
+  ASSERT_FALSE(ids.empty());
+  EXPECT_EQ(first_item->GetId(), ids[0]);
+}
+
+// The page is in a loading state until it gets an insertItems call. Ensure that
+// happens even without downloads.
+TEST_F(DownloadsListTrackerTest, EmptyGetAllItemsStillCallsInsertItems) {
+  CreateTracker();
+
+  ASSERT_FALSE(tracker()->GetItemForTesting(0));
+  ASSERT_TRUE(web_ui()->call_data().empty());
+
+  tracker()->Start();
+
+  ASSERT_FALSE(web_ui()->call_data().empty());
+  EXPECT_EQ("downloads.Manager.insertItems",
+            web_ui()->call_data()[0]->function_name());
+  ASSERT_TRUE(web_ui()->call_data()[0]->arg2());
+  EXPECT_TRUE(GetIds(web_ui()->call_data()[0]->arg2()).empty());
+}
+
+TEST_F(DownloadsListTrackerTest, OnDownloadCreatedCallsInsertItems) {
+  CreateTracker();
+  tracker()->Start();
+  web_ui()->ClearTrackedCalls();
+
+  ASSERT_FALSE(tracker()->GetItemForTesting(0));
+  DownloadItem* first_item = CreateNextItem();
+  tracker()->OnDownloadCreated(manager(), first_item);
+
+  ASSERT_FALSE(web_ui()->call_data().empty());
+  EXPECT_EQ("downloads.Manager.insertItems",
+            web_ui()->call_data()[0]->function_name());
+  EXPECT_EQ(0, GetIndex(web_ui()->call_data()[0]->arg1()));
+
+  std::vector<uint64> ids = GetIds(web_ui()->call_data()[0]->arg2());
+  ASSERT_FALSE(ids.empty());
+  EXPECT_EQ(first_item->GetId(), ids[0]);
+}
+
+TEST_F(DownloadsListTrackerTest, OnDownloadRemovedCallsRemoveItem) {
+  DownloadItem* first_item = CreateNextItem();
+
+  CreateTracker();
+  tracker()->Start();
+  web_ui()->ClearTrackedCalls();
+
+  EXPECT_TRUE(tracker()->GetItemForTesting(0));
+  tracker()->OnDownloadRemoved(manager(), first_item);
+  EXPECT_FALSE(tracker()->GetItemForTesting(0));
+
+  ASSERT_FALSE(web_ui()->call_data().empty());
+
+  EXPECT_EQ("downloads.Manager.removeItem",
+            web_ui()->call_data()[0]->function_name());
+  EXPECT_EQ(0, GetIndex(web_ui()->call_data()[0]->arg1()));
+}
+
+TEST_F(DownloadsListTrackerTest, OnDownloadUpdatedCallsRemoveItem) {
+  DownloadItem* first_item = CreateNextItem();
+
+  CreateTracker();
+  tracker()->Start();
+  web_ui()->ClearTrackedCalls();
+
+  EXPECT_TRUE(tracker()->GetItemForTesting(0));
+
+  DownloadItemModel(first_item).SetShouldShowInShelf(false);
+  tracker()->OnDownloadUpdated(manager(), first_item);
+
+  EXPECT_FALSE(tracker()->GetItemForTesting(0));
+
+  ASSERT_FALSE(web_ui()->call_data().empty());
+
+  EXPECT_EQ("downloads.Manager.removeItem",
+            web_ui()->call_data()[0]->function_name());
+  EXPECT_EQ(0, GetIndex(web_ui()->call_data()[0]->arg1()));
+}
+
+TEST_F(DownloadsListTrackerTest, StartExcludesHiddenItems) {
+  DownloadItem* first_item = CreateNextItem();
+  DownloadItemModel(first_item).SetShouldShowInShelf(false);
+
+  CreateTracker();
+  tracker()->Start();
+
+  ASSERT_FALSE(web_ui()->call_data().empty());
+
+  EXPECT_EQ("downloads.Manager.insertItems",
+            web_ui()->call_data()[0]->function_name());
+  EXPECT_TRUE(GetIds(web_ui()->call_data()[0]->arg2()).empty());
+}

[asanka, 2015/11/24 22:08:22]

Tests for incognito?

[Dan Beam, 2015/11/25 04:06:36]

will get to these soon

[Dan Beam, 2015/11/28 20:59:23]

... in a follow-up CL.  there's a pretty decent chance this CL itself gets
reverted, so I want to try now.