Query API: Introduces an OfflinePageModelQuery object.

components/offline_pages/offline_page_model_query.h

Index: components/offline_pages/offline_page_model_query.h
diff --git a/components/offline_pages/offline_page_model_query.h b/components/offline_pages/offline_page_model_query.h
new file mode 100644
index 0000000000000000000000000000000000000000..187ad64a4003ce7454e8c7a60a91880ff1eb6e1a
--- /dev/null
+++ b/components/offline_pages/offline_page_model_query.h
@@ -0,0 +1,137 @@
+// 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_OFFLINE_PAGES_OFFLINE_PAGE_MODEL_QUERY_H_
+#define COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_MODEL_QUERY_H_
+
+#include <set>
+#include <vector>
+
+#include "base/memory/ptr_util.h"
+#include "components/offline_pages/client_policy_controller.h"
+#include "components/offline_pages/offline_page_item.h"
+#include "components/offline_pages/offline_page_types.h"
+
+namespace offline_pages {
+
+// Can be used by OfflinePageModel instances to direct a query of the model.
+class OfflinePageModelQuery {
+ public:
+  enum class Requirement {

[fgorski, 2016/10/26 18:05:42]

this almost wants to be called constraint.
What do you think?

Also, could you please document it?

[dewittj, 2016/10/27 22:49:18]

Could be, I like requirement but if you feel strongly I will change it.

Docs done.

+    UNSET,
+    INCLUDE_MATCHING,
+    EXCLUDE_MATCHING,
+  };
+
+  OfflinePageModelQuery();
+  virtual ~OfflinePageModelQuery();
+
+  bool GetRestrictedToNamespaces(std::set<std::string>* namespaces_out);
+
+  Requirement GetRestrictedToOfflineIds(std::set<int64_t>* ids_out);

[fgorski, 2016/10/26 18:05:42]

This pattern would work for a status variable, but it doesn't seem like the
right thing with this types.

Could this be either a pair or both values returned through out params?

I know this is convenient for the if statement, but it does not feel right.

[dewittj, 2016/10/27 22:49:18]

Done.

+  Requirement GetRestrictedToClientIds(std::set<ClientId>* ids_out);
+  Requirement GetRestrictedToUrls(std::set<GURL>* ids_out);
+  bool GetAllowExpired();
+
+  // This is the workhorse function that is used by the in-memory offline page
+  // model, given a page it will find out whether that page matches the query.
+  bool Matches(const OfflinePageItem& page);

[fgorski, 2016/10/26 18:05:42]

Now that I think about that. Everything between destructor and this method feels
odd. Why do we need getters on the query if we have a Match?

[dewittj, 2016/10/27 22:49:18]

Match is not going to be usable when we move to a SQL-Query based model.  We
already are using the offline ID restriction to optimize the executequery
method.

+
+ private:
+  friend class OfflinePageModelQueryBuilder;
+
+  bool allow_expired_ = false;

[fgorski, 2016/10/26 18:05:42]

constructor should take care of that I believe.

[dewittj, 2016/10/27 22:49:18]

This is in the C++11 Allowed Features section of
https://chromium-cpp.appspot.com/

[fgorski, 2016/10/28 02:40:39]

Acknowledged.

+
+  std::unique_ptr<std::set<std::string>> restricted_to_namespaces_;

[fgorski, 2016/10/26 18:05:42]

I wonder if we can apply this to eliminate last_n restriction.

[dewittj, 2016/10/27 22:49:18]

I'm not sure what you mean, can you elaborate?

[fgorski, 2016/10/28 02:40:39]

Nerer mind, I realized that the policy is written in a very special way...

+
+  std::pair<Requirement, std::set<int64_t>> offline_ids_;
+  std::pair<Requirement, std::set<ClientId>> client_ids_;
+  std::pair<Requirement, std::set<GURL>> urls_;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageModelQuery);
+};
+
+// Used to create an offline page model query.  QueryBuilders without
+// modifications create queries that allow all pages that are not expired.
+// Can restrict results by policies provided by |ClientPolicyController|, or by
+// individual features of pages. Each restriction comes with a |Requirement|
+// that can be used to specify whether the input restriction should match all

[fgorski, 2016/10/26 18:05:42]

perhaps: should include or exclude matching pages?

[dewittj, 2016/10/27 22:49:18]

Done.

+// result pages or should match zero result pages.
+class OfflinePageModelQueryBuilder {
+ public:
+  using Requirement = OfflinePageModelQuery::Requirement;
+
+  OfflinePageModelQueryBuilder();
+  ~OfflinePageModelQueryBuilder();
+
+  // Sets the offline page IDs that are valid for this request.  If called
+  // multiple times, overwrites previous offline page ID restrictions.
+  OfflinePageModelQueryBuilder& SetOfflinePageIds(
+      Requirement requirement,
+      const std::vector<int64_t>& ids);
+
+  // Sets the client IDs that are valid for this request.  If called // multiple

[fgorski, 2016/10/26 18:05:42]

remove "// " before multiple.

[dewittj, 2016/10/27 22:49:18]

Done.

+  // times, overwrites previous client ID restrictions.
+  OfflinePageModelQueryBuilder& SetClientIds(Requirement requirement,
+                                             const std::vector<ClientId>& ids);
+
+  // Sets the URLs that are valid for this request.  If called multiple times,
+  // overwrites previous URL restrictions.
+  OfflinePageModelQueryBuilder& SetUrls(Requirement requirement,
+                                        const std::vector<GURL>& urls);
+
+  // Only include namespaces whose namespaces satisfy

[fgorski, 2016/10/26 18:05:42]

If I am not mistaken you meant:
Only include *pages* whose namespaces satisfy?

This applies to documentation below.

[dewittj, 2016/10/27 22:49:18]

Done.

+  // ClientPolicyController::IsSupportedByDownload(|namespace|) ==
+  //     |supported_by_download|
+  // Multiple calls overwrite previous ones.
+  OfflinePageModelQueryBuilder& RequireSupportedByDownload(
+      Requirement supported_by_download);
+
+  // Only include namespaces whose namespaces satisfy
+  // ClientPolicyController::IsShownAsRecentlyVisitedSite(|namespace|) ==
+  //     |recently_visited|
+  // Multiple calls overwrite previous ones.
+  OfflinePageModelQueryBuilder& RequireShownAsRecentlyVisitedSite(
+      Requirement recently_visited);
+
+  // Only include namespaces whose namespaces satisfy
+  // ClientPolicyController::IsRestrictedToOriginalTab(|namespace|) ==
+  //     |original_tab|
+  // Multiple calls overwrite previous ones.
+  OfflinePageModelQueryBuilder& RequireRestrictedToOriginalTab(
+      Requirement original_tab);
+
+  // Resets whether we return expired pages.  If called multiple times the bit
+  // is overwritten and |allow_expired| from the last call is saved.
+  OfflinePageModelQueryBuilder& AllowExpiredPages(bool allow_expired);
+
+  // Returns the built-up query based on the above APIs.  This resets the

[fgorski, 2016/10/26 18:05:42]

Builds the query using the provided policy |controller|.
Resets the internal state of the builder.  |controller| should not be nullptr.

[dewittj, 2016/10/27 22:49:18]

Done.

+  // internal state.
+  std::unique_ptr<OfflinePageModelQuery> Build(
+      ClientPolicyController* controller);
+
+ private:
+  // Intersects the allowed namespaces in query_ with |namespaces|.  If
+  // |inverted| is true, intersects the allowed namespaces with all namespaces
+  // except those provided in |namespaces|.
+  void IntersectWithNamespaces(ClientPolicyController* controller,
+                               const std::vector<std::string>& namespaces,
+                               Requirement match_requirement);
+
+  std::pair<Requirement, std::vector<int64_t>> offline_ids_;
+  std::pair<Requirement, std::vector<ClientId>> client_ids_;
+  std::pair<Requirement, std::vector<GURL>> urls_;
+
+  Requirement supported_by_download_ = Requirement::UNSET;
+  Requirement shown_as_recently_visited_site_ = Requirement::UNSET;
+  Requirement restricted_to_original_tab_ = Requirement::UNSET;
+
+  bool allow_expired_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageModelQueryBuilder);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_MODEL_QUERY_H_