Move FakeWebHistoryService to components/history

components/history/core/browser/fake_web_history_service.h

Index: components/history/core/browser/fake_web_history_service.h
diff --git a/components/history/core/browser/fake_web_history_service.h b/components/history/core/browser/fake_web_history_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..c0f8b3e815996e1fe249d6649620fc5fd9f92bbd
--- /dev/null
+++ b/components/history/core/browser/fake_web_history_service.h
@@ -0,0 +1,114 @@
+// 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.
+
+#ifndef COMPONENTS_HISTORY_CORE_BROWSER_FAKE_WEB_HISTORY_SERVICE_H_
+#define COMPONENTS_HISTORY_CORE_BROWSER_FAKE_WEB_HISTORY_SERVICE_H_
+
+#include <vector>
+
+#include "base/macros.h"
+#include "components/history/core/browser/web_history_service.h"
+
+namespace base {
+class CompletionCallback;

[engedy, 2015/11/19 16:02:16]

nit: I think this is a different type, the one you need is defined inside
WebHistoryService.

[msramek, 2015/11/30 15:17:10]

Done. I even managed two include two different wrong callbacks to .h and .cc.

+class Time;
+}
+
+namespace net {
+class URLRequestContextGetter;
+}
+
+class OAuth2TokenService;
+class SigninManagerBase;
+

[engedy, 2015/11/19 16:02:16]

nit: -newline

[msramek, 2015/11/30 15:17:10]

Done.

+
+namespace history {
+
+// A fake WebHistoryService for testing.
+//
+// Use |AddSyncedVisit| to fill the fake server-side database of synced visits.
+// Use |SetRequestOptions| to influence whether the requests should succeed
+// or fail, and with which error code.
+//
+// Call |WebHistoryService::QueryHistory| to retrieve the fake server-side

[engedy, 2015/11/19 16:02:16]

I would argue that no one would really want to call this manually.  Instead, I
would explain below for SetRequestOptions() that when the dependent classes call
this method on the fake, it will use the fake response.

[msramek, 2015/11/30 15:17:10]

Done.

+// visits.
+// TODO(msramek): Currently, the only test that uses this only counts the number

[engedy, 2015/11/19 16:02:16]

nit: Do you actually intend to work on these?  If no, I'd suggest just adding a
"Note: " to inform fellow contributors about the limitations of this class. 
Otherwise, I'd suggest actually filing a bug as well to track this.

[msramek, 2015/11/30 15:17:10]

No, as this is not needed anywhere yet. But that is the secondary meaning of the
TODO keyword :)

+// of visits and does not inspect their contents, so we represent each visit
+// with an empty dictionary. Implement actual contents if needed.
+// TODO(msramek): Other requests, such as |WebHistoryService::ExpireHistory|
+// will currently not work. Implement them if needed.
+// TODO(msramek): This class might need its own set of tests.
+class FakeWebHistoryService : public history::WebHistoryService {
+ public:
+

[engedy, 2015/11/19 16:02:16]

nit: -newline

[msramek, 2015/11/30 15:17:10]

Done.

+  class Request : public history::WebHistoryService::Request {

[engedy, 2015/11/19 16:02:16]

nit: I think this can be moved to the implementation in the cc file.

[msramek, 2015/11/30 15:17:10]

Forward declaration is not enough here. We must specify that Request inherits
from WebHistoryService::Request to check that CreateRequest (overriding
WebHistoryService::CreateRequest) has the correct signature.

[engedy, 2015/11/30 15:47:08]

You could just return the base class (i.e. WebHistoyService::Request), as
opposed to using covariant return types.

[msramek, 2015/12/01 12:27:12]

Oh, right. It's still not possible to have the entire declaration (specifically
the inheritance) in the implementation file if this is a member class. So I
removed it from FakeWebHistoryService, put into an anonymous namespace, and
renamed to FakeRequest for more clarity.

+   public:
+    Request(FakeWebHistoryService* service,
+            bool emulate_success,
+            int emulate_response_code,
+            const CompletionCallback& callback,
+            base::Time begin,
+            base::Time end,
+            int max_count);
+
+    // WebHistoryService::Request:
+    bool IsPending() override;
+    int GetResponseCode() override;
+    const std::string& GetResponseBody() override;
+    void SetPostData(const std::string& post_data) override;
+    void Start() override;
+
+   private:
+    FakeWebHistoryService* service_;
+    bool emulate_success_;
+    int emulate_response_code_;
+    const CompletionCallback& callback_;
+    base::Time begin_;
+    base::Time end_;
+    int max_count_;
+    bool is_pending_;
+    std::string response_body_;
+
+    DISALLOW_COPY_AND_ASSIGN(Request);
+  };
+
+  FakeWebHistoryService(
+      OAuth2TokenService* token_service,
+      SigninManagerBase* signin_manager,
+      const scoped_refptr<net::URLRequestContextGetter>& request_context);

[engedy, 2015/11/19 16:02:16]

nit: Same here.  Does this really work with just the forward declaration, or
does this work because something else already includes the header?

[msramek, 2015/11/30 15:17:10]

Hm, weird. I was also surprised that this would work with forward declaration,
but apparently yes, it's just included elsewhere.

+  ~FakeWebHistoryService() override;
+
+  // Sets the parameters according to which requests will be created.
+  void SetRequestOptions(bool emulate_success, int emulate_response_code);

[engedy, 2015/11/19 16:02:16]

nit: I think this name is rather confusing.  How about SetupFakeResponse or
something along those lines?  Also, could you please elaborate in the comment
about what methods in the interface will actually use the fake values?

[msramek, 2015/11/30 15:17:10]

Agreed. IIRC this is a remnant from an earlier design when this also set other
options, some of them being part of the (fake) XHR options, like time period
begin and end.

+
+  // Adds a fake visit.
+  void AddSyncedVisit(std::string url, base::Time timestamp);
+
+  // Clears all fake visits.
+  void ClearSyncedVisits();
+
+  // Counts the number of visits within a certain time range.
+  int GetNumberOfVisitsBetween(const base::Time& begin, const base::Time& end);
+
+ private:
+  base::Time GetTimeForKeyInQuery(const GURL& url, const std::string& key);

[engedy, 2015/11/19 16:02:16]

nit: Need to include <string>, and I'd include gurl.h as well unless it's
popular to fwd-declare it.

[msramek, 2015/11/30 15:17:10]

Done. Both are popular, inclusion is a bit more.

+
+  // WebHistoryService:
+  Request* CreateRequest(const GURL& url,
+                         const CompletionCallback& callback) override;
+
+  // Parameters for the fake request.
+  bool emulate_success_;
+  int emulate_response_code_;
+
+  // Fake visits storage.
+  typedef std::pair<std::string, base::Time> Visit;

[engedy, 2015/11/19 16:02:16]

I find it hard to believe that this works with just a forward declaration of
base::Time,  I suspect that something else already includes the header.  Could
you please check?

[msramek, 2015/11/30 15:17:10]

Done. Yes, it's already included.

[engedy, 2015/11/30 15:47:08]

In this case, I'd explicitly include the header here, because there is no
obvious reason why that should be the case.

[msramek, 2015/12/01 12:27:12]

Done. I actually do find it somewhat obvious that classes working with history
will include time, but I get the point.

+  std::vector<Visit> visits_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeWebHistoryService);
+};
+
+}  // namespace history
+
+#endif  // COMPONENTS_HISTORY_CORE_BROWSER_FAKE_WEB_HISTORY_SERVICE_H_