[chrome.dial] Adds chrome.dial.fetchDeviceDecription API.

chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc

+// Copyright (c) 2016 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 <memory>
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "chrome/browser/extensions/api/dial/device_description_fetcher.h"
+#include "chrome/browser/extensions/api/dial/dial_device_data.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_fetcher.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace extensions {
+namespace api {
+namespace dial {
+
+class DeviceDescriptionFetcherTest : public testing::Test {
+ public:
+  DeviceDescriptionFetcherTest() : url_("http://127.0.0.1/description.xml") {}
+
+  void TearDown() override { EXPECT_TRUE(callback_was_run_); }

[Wez, 2017/01/06 01:24:03]

Rather than maintain a separate boolean, would it be adequate to test that both
the cb_ members are invalid/null, since they are OnceCallbacks, so get cleared
when Run()?

+
+  void ExpectSuccess(const GURL& expected_app_url,
+                     const std::string& expected_description) {
+    success_cb_ = base::BindOnce(&DeviceDescriptionFetcherTest::AssertSuccess,
+                                 base::Unretained(this), expected_app_url,
+                                 expected_description);
+  }
+
+  void ExpectFailure(const std::string& expected_message) {
+    expect_success_ = false;
+    error_cb_ = base::BindOnce(&DeviceDescriptionFetcherTest::AssertFailure,
+                               base::Unretained(this), expected_message);
+  }
+
+  net::TestURLFetcher* StartRequest() {
+    fetcher_ = base::MakeUnique<DeviceDescriptionFetcher>(
+        url_, &profile_, std::move(success_cb_), std::move(error_cb_));
+    fetcher_->Start();
+    return factory_.GetFetcherByID(DeviceDescriptionFetcher::kURLFetcherID);
+  }
+
+ protected:
+  const content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  const net::TestURLFetcherFactory factory_;
+  const GURL url_;
+  base::OnceCallback<void(const DialDeviceDescriptionData&)> success_cb_;
+  base::OnceCallback<void(const std::string&)> error_cb_;
+  std::unique_ptr<DeviceDescriptionFetcher> fetcher_;
+  bool callback_was_run_ = false;
+  bool expect_success_ = true;
+
+ private:
+  void AssertSuccess(const GURL& expected_app_url,
+                     const std::string& expected_description,
+                     const DialDeviceDescriptionData& description) {

[Wez, 2017/01/06 01:24:03]

I'd suggest renaming these to OnSuccess/SuccessCallback/HandleSuccess or similar
- the name AssertFoo implies that the method verifies that condition Foo is the
case, whereas these are actually called when Foo is the case, to verify that
that was what was expected (i.e. the other way around).

[mark a. foltz, 2017/01/09 21:38:07]

Done.

+    EXPECT_TRUE(expect_success_);

[Wez, 2017/01/06 01:24:03]

Will this expectation ever fire? If we ExpectFailure then we only set an
error_cb_, so presumably the success callback is null/invalid in that case, and
will crash the test if invoked?

You could instead always set both, and provided ExpectedSuccess and
UnexpectedSuccess, etc, handlers here.

[mark a. foltz, 2017/01/09 21:38:07]

Removed expectations.

+    callback_was_run_ = true;
+    EXPECT_EQ(expected_app_url, description.app_url);
+    EXPECT_EQ(expected_description, description.device_description);
+  }
+
+  void AssertFailure(const std::string& expected_message,
+                     const std::string& message) {
+    EXPECT_FALSE(expect_success_);
+    callback_was_run_ = true;
+    EXPECT_TRUE(message.find(expected_message) == 0);
+  }
+};

[Wez, 2017/01/06 01:24:03]

nit: DISALLOW_COPY_AND_ASSIGN, or ~Foo() etc = delete

[mark a. foltz, 2017/01/09 21:38:07]

Done.

+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchSuccessful) {
+  ExpectSuccess(GURL("http://127.0.0.1/apps"), "<xml>description</xml>");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  headers->AddHeader("Application-URL: http://127.0.0.1/apps");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->SetResponseString("<xml>description</xml>");
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnHttp404) {

[Wez, 2017/01/06 01:24:03]

nit: The name matches the expectation failure; can we rename or add a comment to
explain what situation this tests? e.g. is this the expectation for the case of
being unable to connect to the device?

[mark a. foltz, 2017/01/09 21:38:07]

Renamed.

+  ExpectFailure("HTTP 404:");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_NOT_FOUND);
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnMissingAppUrl) {
+  ExpectFailure("Missing or empty Application-URL:");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnEmptyAppUrl) {
+  ExpectFailure("Missing or empty Application-URL:");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  headers->AddHeader("Application-URL:");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnInvalidAppUrl) {
+  ExpectFailure("Invalid Application-URL:");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  headers->AddHeader("Application-URL: http://www.example.com");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnEmptyDescription) {
+  ExpectFailure("Missing or empty response");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  headers->AddHeader("Application-URL: http://127.0.0.1/apps");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->SetResponseString("");
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnBadDescription) {
+  ExpectFailure("Invalid response encoding");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  headers->AddHeader("Application-URL: http://127.0.0.1/apps");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->SetResponseString("\xfc\x9c\xbf\x80\xbf\x80");
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnResponseTooLarge) {
+  ExpectFailure("Response too large");
+  net::TestURLFetcher* test_fetcher = StartRequest();
+
+  test_fetcher->set_response_code(net::HTTP_OK);
+  scoped_refptr<net::HttpResponseHeaders> headers =
+      new net::HttpResponseHeaders("");
+  headers->AddHeader("Application-URL: http://127.0.0.1/apps");
+  test_fetcher->set_response_headers(headers);
+  test_fetcher->SetResponseString(std::string(262145, 'd'));
+  test_fetcher->delegate()->OnURLFetchComplete(test_fetcher);
+}
+
+}  // namespace dial
+}  // namespace api
+}  // namespace extensions