Request Picker task

components/offline_pages/background/pick_request_task_builder.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_OFFLINE_PAGES_BACKGROUND_PICK_REQUEST_TASK_BUILDER_H_
+#define COMPONENTS_OFFLINE_PAGES_BACKGROUND_PICK_REQUEST_TASK_BUILDER_H_
+
+#include "components/offline_pages/background/device_conditions.h"
+

[fgorski, 2016/11/03 22:00:29]

nit: space not needed.

[Pete Williamson, 2016/11/04 18:53:37]

Done.

+#include "components/offline_pages/background/pick_request_task.h"
+#include "components/offline_pages/background/request_coordinator.h"
+
+namespace offline_pages {
+
+class OfflinerPolicy;
+class RequestCoordinatorEventLogger;
+class RequestNotifier;
+class RequestQueue;
+
+class PickRequestTaskBuilder {

[fgorski, 2016/11/03 22:00:29]

Given the interface presented by this class it feels more like a factory and not
a builder (in the design patterns sense). Do you think it would be reasonable to
change it?

Also, please add documentation.

[Pete Williamson, 2016/11/04 18:53:37]

Doc added.

I'm not sure I understand your request.  Do you mean just that I should change
the name of the class from "builder" to "factory", or that the interface should
somehow be different?

[fgorski, 2016/11/04 21:41:28]

Yes, I think Factory suits this case better.

Also, Please move the documentation to the .h file. I think you accidentally
added it in the .cc file.

+ public:
+  PickRequestTaskBuilder(OfflinerPolicy* policy,
+                         RequestNotifier* notifier,
+                         RequestCoordinatorEventLogger* event_logger);
+
+  ~PickRequestTaskBuilder();
+
+  std::unique_ptr<PickRequestTask> CreatePickerTask(
+      RequestQueue* queue,
+      PickRequestTask::RequestPickedCallback picked_callback,
+      PickRequestTask::RequestNotPickedCallback not_picked_callback,

[fgorski, 2016/11/03 22:00:29]

Could you use const & for both callbacks, unless you know of a reason not to.

[Pete Williamson, 2016/11/04 18:53:37]

Done.

+      DeviceConditions* conditions,
+      std::set<int64_t>& disabled_requests);
+
+ private:
+  // Unowned pointer to the Policy
+  OfflinerPolicy* policy_;
+  // Unowned pointer to the notifier
+  RequestNotifier* notifier_;
+  // Unowned pointer to the EventLogger
+  RequestCoordinatorEventLogger* event_logger_;
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_PICK_REQUEST_TASK_BUILDER_H_