[WebPayments] Factor out sheet-specific logic in Controllers.

chrome/browser/ui/views/payments/payment_request_sheet_controller.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 CHROME_BROWSER_UI_VIEWS_PAYMENTS_PAYMENT_REQUEST_SHEET_CONTROLLER_H_
+#define CHROME_BROWSER_UI_VIEWS_PAYMENTS_PAYMENT_REQUEST_SHEET_CONTROLLER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+
+namespace views {
+class View;
+}
+
+namespace payments {
+
+class PaymentRequestDialog;
+class PaymentRequestImpl;
+
+// The base class for objects responsible for the creation and event handling in
+// views shown in the PaymentRequestDialog.
+class PaymentRequestSheetController {
+ public:
+  // Objects of this class are owned by |dialog|, so it's a weak pointer that

[please use gerrit instead, 2016/12/15 20:29:33]

s/weak pointer/non-owned pointer/

This avoids confusion with
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h

[anthonyvd, 2016/12/15 20:58:08]

Good point, done.

+  // should be valid throughout this object's lifetime.
+  // |impl| is also not owned by this and is guaranteed to outlive dialog.
+  PaymentRequestSheetController(PaymentRequestImpl* impl,
+                                PaymentRequestDialog* dialog)
+    : impl_(impl),
+      dialog_(dialog) {}
+  virtual ~PaymentRequestSheetController() {}
+
+  virtual std::unique_ptr<views::View> CreateView() = 0;
+
+  // The PaymentRequestImpl object associated with this instance of the dialog.
+  // The pointer is owned by PaymentRequestFactory and will outlive this object.

[please use gerrit instead, 2016/12/15 20:29:33]

All ownership comments should be from the point of the caller of this method.
The caller of this method has a reference to PaymentRequestSheetController and
may not know anything about PaymentRequestFactory. What the caller needs to know
is whether they should be taking ownership of this pointer. Here the answer is
"no". So the best comment style is something like this:

// Caller should not take ownership of the result.

[anthonyvd, 2016/12/15 20:58:09]

Done, see meta comment for some discussion.

+  PaymentRequestImpl* impl() { return impl_; }
+
+  // The dialog that contains and owns this object.
+  // The return value is owned by its parent view.

[please use gerrit instead, 2016/12/15 20:29:33]

Ditto

[anthonyvd, 2016/12/15 20:58:08]

Done.

+  PaymentRequestDialog* dialog() { return dialog_; }
+
+ private:
+  // Owned by PaymentRequestFactory and will outlive this object.

[please use gerrit instead, 2016/12/15 20:29:33]

Let's restrict the private member variable comments to the scope of this file.
No need to mention the factory.

// Not owned. Will outlive this.

[anthonyvd, 2016/12/15 20:58:09]

Done.

+  PaymentRequestImpl* impl_;
+
+  // Owned by its parent view.

[please use gerrit instead, 2016/12/15 20:29:33]

// Not owned. Will outlive this.

Is that correct to say?

Given this comment, your peers working on .cc file will assume that dialog_ is
never null and always valid. This will make their lives easier.

[anthonyvd, 2016/12/15 20:58:08]

It is. Done.

+  PaymentRequestDialog* dialog_;
+
+  DISALLOW_COPY_AND_ASSIGN(PaymentRequestSheetController);
+};
+
+}  // namespace payments
+
+#endif  // CHROME_BROWSER_UI_VIEWS_PAYMENTS_PAYMENT_REQUEST_SHEET_CONTROLLER_H_