Save favicon during reading list distillation

ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm

+// Copyright 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 "base/memory/ptr_util.h"
+#include "components/favicon/ios/web_favicon_driver.h"
+#include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
+#include "ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h"

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

This include should go first, then a line-break then the rest of
includes/imports.

[gambard, 2016/12/20 08:54:22]

Done.

+#import "ios/testing/wait_util.h"
+#include "ios/web/public/test/test_web_thread_bundle.h"
+#include "ios/web/public/web_state/web_state_observer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/gtest_mac.h"

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

Do you need this include?

[gambard, 2016/12/20 08:54:23]

Done.

+#include "testing/platform_test.h"
+
+namespace dom_distiller {
+
+// Test class.
+class FaviconWebStateDispatcherTest : public PlatformTest {
+ public:
+  FaviconWebStateDispatcherTest() : web_state_destroyed_(false) {
+    TestChromeBrowserState::Builder builder;
+    browser_state_ = builder.Build();
+  }
+
+  web::BrowserState* GetBrowserState() { return browser_state_.get(); }
+
+  bool IsWebStateDestroyed() { return web_state_destroyed_; }
+  void WebStateDestroyed() { web_state_destroyed_ = true; }
+
+ private:
+  web::TestWebThreadBundle thread_bundle_;

[Eugene But (OOO till 7-30), 2016/12/19 17:30:07]

Can you subclass from web::WebTest which already has TestChromeBrowserState and
web::TestWebThreadBundle?

[gambard, 2016/12/20 08:54:22]

No the BrowserState is not initialized enough.

[Eugene But (OOO till 7-30), 2016/12/20 17:36:18]

Can we initialize browser_state_ in WebTest correctly and then subclass?
web::WebTest was created for code reuse and we should take advantage of that.

+  std::unique_ptr<TestChromeBrowserState> browser_state_;
+  bool web_state_destroyed_;
+};
+
+// Observer for the test.
+class FaviconWebStateDispatcherObserverTest : public web::WebStateObserver {

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

Should this be TestFaviconWebStateDispatcherObserver? Test suffix normally used
for test fixtures (not stubs).

[gambard, 2016/12/20 08:54:23]

Done.

+ public:
+  FaviconWebStateDispatcherObserverTest(web::WebState* web_state,
+                                        FaviconWebStateDispatcherTest* owner)
+      : web::WebStateObserver(web_state), owner_(owner) {}
+
+  // WebStateObserver implementation:
+  void WebStateDestroyed() override { owner_->WebStateDestroyed(); };
+
+ private:
+  FaviconWebStateDispatcherTest* owner_;  // weak, owns this object.
+};
+
+// Tests that RequestWebState returns a WebState with a FaviconDriver attached.
+TEST_F(FaviconWebStateDispatcherTest, RequestWebState) {
+  FaviconWebStateDispatcherImpl* dispatcher =

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

This looks like a leak. Do you want to use stack dispatcher like this?:

FaviconWebStateDispatcherImpl dispatcher(GetBrowserState());

[gambard, 2016/12/20 08:54:23]

Done.

+      new FaviconWebStateDispatcherImpl(GetBrowserState());
+  web::WebState* webState = dispatcher->RequestWebState();

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

s/webState/web_state

[gambard, 2016/12/20 08:54:23]

Done.

+
+  favicon::WebFaviconDriver* driver =
+      favicon::WebFaviconDriver::FromWebState(webState);

[Eugene But (OOO till 7-30), 2016/12/19 17:30:07]

Do you want to test that history and bookmarks services were attached as well?

[gambard, 2016/12/20 08:54:23]

History and Bookmarks are attached to the favicon driver, so no.

+  EXPECT_NE(driver, nullptr);
+}
+
+// Tests that the WebState returned will be destroyed after a delay.
+TEST_F(FaviconWebStateDispatcherTest, ReturnWebState) {
+  FaviconWebStateDispatcherImpl* dispatcher =
+      new FaviconWebStateDispatcherImpl(GetBrowserState(), 0);
+  web::WebState* webState = dispatcher->RequestWebState();

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

s/webState/web_state

[gambard, 2016/12/20 08:54:23]

Done.

+
+  std::unique_ptr<FaviconWebStateDispatcherObserverTest> observer =
+      base::MakeUnique<FaviconWebStateDispatcherObserverTest>(webState, this);
+
+  ConditionBlock condition = ^{

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

Optional nit: consider inlining this block.

[gambard, 2016/12/20 08:54:23]

Acknowledged.

+    return IsWebStateDestroyed();
+  };
+
+  dispatcher->ReturnWebState(webState);
+
+  ASSERT_TRUE(testing::WaitUntilConditionOrTimeout(0.5, condition));

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

How come that 0.5 seconds is enough for WebState destruction?
kDefaultDelayFavicon is 10 seconds.

[gambard, 2016/12/20 08:54:22]

There is a new constructor for the dispatcher where you can specify the delay
and override the default (unittest cannot wait for 10s). Here the delay is 0.

+}
+}

[Eugene But (OOO till 7-30), 2016/12/19 17:30:06]

// namespace dom_distiller

[gambard, 2016/12/20 08:54:23]

Done.