Rewrite DownloadsDOMHandler

chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc

+// Copyright (c) 2012 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 "base/json/json_reader.h"
+#include "base/scoped_temp_dir.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/webui/downloads_dom_handler.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/download_persistent_store_info.h"
+#include "content/public/browser/web_contents.h"
+
+namespace {
+
+bool ListMatches(base::ListValue* left_list, const std::string& right_json) {

[James Hawkins, 2012/08/19 06:17:29]

Document method and parameters.

[benjhayden, 2012/08/19 22:20:27]

Done.

+  scoped_ptr<base::Value> right_value(base::JSONReader::Read(right_json));
+  base::ListValue* right_list = NULL;
+  CHECK(right_value->GetAsList(&right_list));

[James Hawkins, 2012/08/19 06:17:29]

s/CHECK/DCHECK/

[benjhayden, 2012/08/19 22:20:27]

Aren't DCHECKs compiled out entirely in non-debug builds? ListMatches() would
still need to GetAsList() from right_value to right_list. Same for other CHECKs
in this file.

[James Hawkins, 2012/08/22 17:39:00]

No side-effect inducing calls should be made in either CHECKs or DCHECKs for
this exact reason.  If you want to check that the call succeeded, put the return
value into a var and DCHECK that.

+  for (size_t i = 0; i < left_list->GetSize(); ++i) {
+    base::DictionaryValue* left_dict = NULL;
+    base::DictionaryValue* right_dict = NULL;
+    CHECK(left_list->GetDictionary(i, &left_dict));
+    CHECK(right_list->GetDictionary(i, &right_dict));
+    for (base::DictionaryValue::Iterator iter(*right_dict);
+          iter.HasNext(); iter.Advance()) {
+      base::Value* left_value = NULL;
+      if (left_dict->HasKey(iter.key()) &&
+          left_dict->Get(iter.key(), &left_value) &&
+          !iter.value().Equals(left_value)) {
+        LOG(WARNING) << iter.key();

[James Hawkins, 2012/08/19 06:17:29]

Remove console spam?

[benjhayden, 2012/08/19 22:20:27]

It will only print when the test is about to fail, and provide valuable
debugging information.

[James Hawkins, 2012/08/22 17:39:00]

Attempting to guess what's 'valuable' as far as logging goes leads to console
spam.  I'm not going to ask you to take it out, but keep in mind that logging
used to debug a failure (test or otherwise) should only be used during
debugging, including committing to get information from the bots then reverting
the logging once the failure is fixed.  As is this logging is taking up space
solely to debug a hypothetical failure that may or may not happen in the future.

+        return false;
+      }
+    }
+  }
+  return true;
+}
+
+class MockDownloadsDOMHandler : public DownloadsDOMHandler {

[James Hawkins, 2012/08/19 06:17:29]

Document.

[benjhayden, 2012/08/19 22:20:27]

Done.

+ public:
+  explicit MockDownloadsDOMHandler(content::DownloadManager* dlm)
+    : DownloadsDOMHandler(dlm) {
+  }
+  virtual ~MockDownloadsDOMHandler() {}
+
+  base::ListValue* downloads_list() { return downloads_list_.get(); }
+  base::ListValue* download_updated() { return download_updated_.get(); }
+
+ protected:
+  virtual content::WebContents* GetWebUIWebContents() {
+    return NULL;
+  }
+
+  virtual void CallDownloadsList(const base::ListValue& downloads) {
+    downloads_list_.reset(downloads.DeepCopy());
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE, MessageLoop::QuitClosure());
+  }
+
+  virtual void CallDownloadUpdated(const base::ListValue& download) {
+    download_updated_.reset(download.DeepCopy());
+  }
+
+ private:
+  scoped_ptr<base::ListValue> downloads_list_;
+  scoped_ptr<base::ListValue> download_updated_;
+  DISALLOW_COPY_AND_ASSIGN(MockDownloadsDOMHandler);
+};
+
+}  // anonymous namespace

[James Hawkins, 2012/08/19 06:17:29]

s/anonymous//

[benjhayden, 2012/08/19 22:20:27]

Done.

+
+class DownloadsDOMHandlerTest : public InProcessBrowserTest {
+ public:
+  DownloadsDOMHandlerTest() {}
+
+  virtual ~DownloadsDOMHandlerTest() {}
+
+  virtual void SetUpOnMainThread() OVERRIDE {
+    CHECK(downloads_directory_.CreateUniqueTempDir());
+    browser()->profile()->GetPrefs()->SetFilePath(
+        prefs::kDownloadDefaultDirectory,
+        downloads_directory_.path());
+    CHECK(test_server()->Start());
+  }
+
+  content::DownloadManager* download_manager() {
+    return content::BrowserContext::GetDownloadManager(browser()->profile());
+  }
+
+ private:
+  ScopedTempDir downloads_directory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadsDOMHandlerTest);
+};
+
+IN_PROC_BROWSER_TEST_F(DownloadsDOMHandlerTest,

[James Hawkins, 2012/08/19 06:17:29]

Document what the test is testing.

[benjhayden, 2012/08/19 22:20:27]

Done.

+    DownloadsDOMHandlerTest_Created) {
+  MockDownloadsDOMHandler mddh(download_manager());
+  GURL url = test_server()->GetURL("files/downloads/image.jpg");
+  base::Time current(base::Time::Now());
+  content::DownloadPersistentStoreInfo population_entries[] = {
+    content::DownloadPersistentStoreInfo(
+        FilePath(FILE_PATH_LITERAL("/path/to/file")),
+        url,
+        GURL(""),

[James Hawkins, 2012/08/19 06:17:29]

Optional nit: Consolidate parameters to save rows.

[benjhayden, 2012/08/19 22:20:27]

I think that it's more readable this way.

+        current - base::TimeDelta::FromMinutes(5),
+        current,
+        128,
+        128,
+        content::DownloadItem::COMPLETE,
+        1,
+        false),
+  };
+  std::vector<content::DownloadPersistentStoreInfo> entries(
+      population_entries, population_entries + arraysize(population_entries));
+  download_manager()->OnPersistentStoreQueryComplete(&entries);
+  content::RunMessageLoop();
+  EXPECT_EQ(1, static_cast<int>(mddh.downloads_list()->GetSize()));

[James Hawkins, 2012/08/19 06:17:29]

ASSERT_EQ

[benjhayden, 2012/08/19 22:20:27]

Done.

+  EXPECT_EQ(1, static_cast<int>(mddh.download_updated()->GetSize()));
+  EXPECT_TRUE(ListMatches(
+      mddh.downloads_list(),
+      "[{\"file_externally_removed\": false,"
+      "  \"file_name\": \"file\","
+      "  \"id\": 0,"
+      "  \"otr\": false,"
+      "  \"since_string\": \"Today\","
+      "  \"state\": \"COMPLETE\","
+      "  \"total\": 128}]"));
+  EXPECT_TRUE(ListMatches(
+      mddh.download_updated(),
+      "[{\"file_externally_removed\": true,"
+      "  \"id\": 0}]"));
+
+  mddh.HandleClearAll(NULL);
+  content::RunMessageLoop();
+  EXPECT_EQ(0, static_cast<int>(mddh.downloads_list()->GetSize()));
+}
+
+// TODO(benjhayden): Test the extension downloads filter for both
+// mddh.downloads_list() and mddh.download_updated().
+
+// TODO(benjhayden): Test incognito, both downloads_list() and that on-record
+// calls can't access off-record items.
+
+// TODO(benjhayden): Test that bad download ids incoming from the javascript are
+// dropped on the floor.
+
+// TODO(benjhayden): Test that IsTemporary() downloads are not shown.
+
+// TODO(benjhayden): Test that RemoveObserver is called on all download items,
+// including items that crossed IsTemporary() and back.