Adding a previews IO-thread blacklist

components/previews/previews_black_list.h

+// 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.
+
+#ifndef COMPONENTS_PREVIEWS_PREVIEWS_BLACK_LIST_H_
+#define COMPONENTS_PREVIEWS_PREVIEWS_BLACK_LIST_H_
+
+#include <stdint.h>
+
+#include <memory>
+#include <queue>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
+#include "components/previews/previews_opt_out_store.h"
+
+namespace base {
+class Time;
+}
+
+namespace previews {
+class PreviewsBlackListItem;
+
+// Manages the state of black listed domains for the previews experiment. Loads
+// the stored black list from |opt_out_store| and manages an in memory black
+// list on the IO thread. Updates to the black list are stored in memory and
+// pushed to the store. Asynchronous modifications are stored in a queue and
+// executed in order. Reading from the black list is always synchronous, and if
+// the black list is not currently loaded (e.g., at startup, after clearing
+// browsing history), domains are reported as black listed.
+class PreviewsBlackList {
+ public:
+  // |opt_out_store| is the backing store to retrieve and store black list
+  // information, and can be null.

[tbansal1, 2016/09/14 17:00:03]

May be explain how the |opt_out_store| is used? It is not clear what happens
when it is null vs. when it is not.

[RyanSturm, 2016/09/14 18:36:42]

Done.

+  explicit PreviewsBlackList(
+      std::unique_ptr<PreviewsOptOutStore> opt_out_store);
+  ~PreviewsBlackList();
+
+  typedef base::Closure QueueClosure;
+  typedef base::Callback<void(bool)> BlackListedCallback;
+
+  // Asynchronously adds a new navigation to to the in-memory black list and
+  // backing store. |opt_out| is whether the uesr opted out of the preview or
+  // navigated away from the page by another way.
+  void AddPreviewNavigation(const std::string& host_name,

[tbansal1, 2016/09/14 17:00:03]

Is it possible to pass GURL? So that, the conversion from GURL -> Host ad
vice-versa is internal to the black list code.

[RyanSturm, 2016/09/14 18:36:42]

Done.

+                            bool opt_out,
+                            PreviewsType type);
+
+  // Synchronously determines if |host_name| should be allowed to show previews.
+  // If the black list has loaded yet, this will always return false.
+  bool IsLoadedAndAllowed(const std::string& host_name);

[tbansal1, 2016/09/14 17:00:03]

Why does this function not take PreviewsType as input? is the blacklist indexed
by type?

[RyanSturm, 2016/09/14 18:36:42]

The store will track PreviewsType, but PreviewsType is not part of the decision
right now. The goal of this is to allow the database to be created and have all
the information we need in all versions.

I'll add a PreviewsType to this, but not use it in  decision making to make the
API seem clearer.

[tbansal1, 2016/09/14 20:17:50]

okay, please add PreviewType so that callers do not have to make any
modifications once you really start using this.

[RyanSturm, 2016/09/14 21:01:03]

Done.

+
+ private:
+  // Synchronously adds a new navigation to the in-memory black list. Adds a new
+  // navigation to backing store. |opt_out| is whether the uesr opted out of
+  // the preview or navigated away from the page by another way.
+  void AddPreviewNavigationSync(const std::string& host_name,
+                                bool opt_out,
+                                PreviewsType type);
+
+  // Returns the PreviewsBlackListItem representing |host_name|. If there is no
+  // item for |host_name|, one will be created iff |create_if_needed| is true.
+  PreviewsBlackListItem* GetBlackListItem(const std::string& host_name,
+                                          bool create_if_needed);
+
+  // Callback passed to the backing store when loading black list information.
+  // Moves the returned map into the in-memory black list and runs any
+  // outstanding tasks.
+  void LoadBlackListDone(std::unique_ptr<BlackListItemMap> black_list_item_map);
+
+  // Called while waiting for the black list to be loaded from the backing
+  // store.
+  // Enqueues a task to run when when loading black list information has
+  // completed. Maintains the order that tasks were called in.
+  void QueuePendingTask(QueueClosure callback);
+
+  // Map maintaining the in-memory black list.
+  std::unique_ptr<BlackListItemMap> black_list_item_map_;
+
+  // Whether the black list is done being loaded from the backing store.
+  bool loaded_;
+
+  // The backing store of the black list information.
+  std::unique_ptr<PreviewsOptOutStore> opt_out_store_;
+
+  // Callbacks to be run after loading information from the backing store has
+  // completed.
+  std::queue<QueueClosure> pending_callbacks_;

[tbansal1, 2016/09/14 17:00:03]

Is the queue really needed? Is it possible to just drop the events and record
UMA for that? If it happens too frequent, we can possibly do something.

[RyanSturm, 2016/09/14 18:36:42]

I'd rather have it. It's possible, but unlikely that it is really needed. It
keeps the in-memory map and backing store in sync, so I'd prefer to keep it.

[tbansal1, 2016/09/14 20:17:50]

You can keep them in sync by simply dropping events until the map is loaded. 
This basically means loss of recording of an opt-out in the worst case (may be
also add UMA to track that).

[RyanSturm, 2016/09/14 21:01:03]

Acknowledged. Seems like we might as well not drop them since the implementation
is straight forward. If you push again, I'll remove it.

+
+  base::ThreadChecker thread_checker_;
+
+  base::WeakPtrFactory<PreviewsBlackList> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PreviewsBlackList);
+};
+
+}  // namespace previews
+
+#endif  // COMPONENTS_PREVIEWS_PREVIEWS_BLACK_LIST_ITEM_H_