Fetch extension blacklist states from SafeBrowsing server

chrome/browser/extensions/blacklist_state_fetcher.h

+// Copyright 2013 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 CHROME_BROWSER_EXTENSIONS_BLACKLIST_STATE_FETCHER_H_
+#define CHROME_BROWSER_EXTENSIONS_BLACKLIST_STATE_FETCHER_H_

[mattm, 2013/11/25 21:48:55]

kinda seems like the fetcher should live in the safe_browsing directory

[Oleg Eterevsky, 2013/11/27 14:07:38]

I thought about it and it seems strange to have blacklist and blacklist_fetcher
in different directories.

[mattm, 2013/12/03 02:55:45]

Well, it's also weird that BlacklistStateFetcher depends on the safebrowsing
config and context and connects to safebrowsing servers, but isn't under the
safebrowsing directory. Also the proto is in chrome/common/safe_browsing but
only used from extensions.
I think it makes sense to keep the stuff that interacts with safebrowsing
services in safe_browsing/, that doesn't mean their users need to be in the same
directory. This is how existing cases work (ex: DownloadProtectionService &
DownloadFeedbackService live in safe_browsing but are used from downloads/)

+
+#include <set>
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/extensions/blacklist.h"

[mattm, 2013/12/03 02:55:45]

This is just for BlacklistState? This is somewhat of a circular dep, cleaner to
remove and use the enum from the proto instead.

[Oleg Eterevsky, 2013/12/04 10:45:11]

As I said, I don't like the idea of some arbitrary extensions code depending on
crx_info.pb.h. I see two choices (besides leaving it as it is):
- Convert from proto enum to Blacklist enum in Blacklist instead of
BlacklistStateFetcher.
- Make a separate header just for this enum to avoid circular dependence. 

Also, strictly speaking, it's a circular dep since I only include blacklist.h in
blacklist_fetcher.cc, not blacklist_fether.h.

[mattm, 2013/12/10 11:09:40]

I guess either is fine.

[Oleg Eterevsky, 2013/12/10 17:10:44]

Done.

+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
+
+namespace extensions {
+
+class BlacklistStateFetcher : public net::URLFetcherDelegate {
+ public:
+  BlacklistStateFetcher();
+
+  ~BlacklistStateFetcher();
+
+  typedef base::Callback<void(Blacklist::BlacklistState)> RequestCallback;
+
+  virtual void Request(const std::string& id, const RequestCallback& callback);
+
+  void SetSafeBrowsingConfig(const SafeBrowsingProtocolConfig& config);

[mattm, 2013/11/25 21:48:55]

Seems cleaner to just pass the config into the constructor

[Oleg Eterevsky, 2013/11/27 14:07:38]

The config is either passed here (usually in case of unit tests), or taken from
g_browser_process->safe_browsing_service().

[mattm, 2013/12/03 02:55:45]

Right, but with the constructor the unittests can pass in their own test
versions when they create the fetcher, and in the real chrome, whoever creates
the BlacklistStateFetcher can get the values from SafeBrowsingService to pass
in.  It is cleaner to have things passed in the constructor rather than having
otherwise independent classes like this calling into globals in order to
initialize themselves.

[Oleg Eterevsky, 2013/12/04 10:45:11]

Ok, makes sense, in case this moves to SafeBrowsing. Just to check that I
understand correctly your idea:

- BlacklistFetcher is created by and managed by SafeBrowsing service,
- Blacklist gets it from g_browser_process->safe_browsing_service()

Right?

+
+ protected:
+  // net::URLFetcherDelegate interface.
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
+
+ private:
+  GURL RequestUrl() const;
+
+  SafeBrowsingProtocolConfig safe_browsing_config_;
+  bool safe_browsing_config_initialized_;
+
+  // ID for URLFetchers for testing.
+  int url_fetcher_id_;
+
+  // Extension id by URLFetcher.
+  std::map<const net::URLFetcher*, std::string> requests_;
+
+  typedef std::multimap<std::string, RequestCallback> CallbackMultiMap;
+
+  // Callbacks by extension ID.
+  CallbackMultiMap callbacks_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlacklistStateFetcher);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_BLACKLIST_STATE_FETCHER_H_
+