[Payment Request] Change the lifetime management of PaymentRequestImpl

[Payment Request] Change the lifetime management of PaymentRequestImpl

Was previously 1 PaymentRequestImpl per WebContents. Now there is a
PaymentRequestWebContentsManager (1 per WebContents) that owns all the
PaymentRequestImpl's, whose raw pointers are shared with the delegate
and the mojo bindings.

Also, moving PaymentRequestImpl from chrome/ to components/ which creates
the need for a PaymentRequestDelegate (initially managing the Chrome
dialog).

BUG=678302
Review-Url: https://codereview.chromium.org/2611253004
Cr-Commit-Position: refs/heads/master@{#442335}
Committed: https://chromium.googlesource.com/chromium/src/+/f709499d132d767ce77caf04134cd231160835ac

[Mathieu, 2017-01-06 14:27:35]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 14:27:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 14:32:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 14:32:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-01-06 21:15:06]

Description was changed from

==========
[Payment Request] Change the lifetime management of PaymentRequestImpl

Was previously 1 PaymentRequestImpl per WebContents. Now there is a
PaymentRequestWebContentsManager (1 per WebContents) that owns all the
PaymentRequestImpl's, whose raw pointers are shared with the delegate
and the mojo bindings.

Also, moving PaymentRequestImpl from chrome/ to components/ which creates
the need for a PaymentRequestDelegate (initially managing the Chrome
dialog).

BUG=678302
==========

to

==========
[Payment Request] Change the lifetime management of PaymentRequestImpl

Was previously 1 PaymentRequestImpl per WebContents. Now there is a
PaymentRequestWebContentsManager (1 per WebContents) that owns all the
PaymentRequestImpl's, whose raw pointers are shared with the delegate
and the mojo bindings.

Also, moving PaymentRequestImpl from chrome/ to components/ which creates
the need for a PaymentRequestDelegate (initially managing the Chrome
dialog).

BUG=678302
==========

[Mathieu, 2017-01-06 21:15:07]

mathp@chromium.org changed reviewers:
+ rouslan@chromium.org

[Mathieu, 2017-01-06 21:17:54]

Hi Rouslan, PTAL. 

Future CLs will tackle browser tests more (moving html files from android and
exercising them). For now I have confirmed that we maintain the same behavior as
before + we can have multiple PR per web_contents.

Also I made the name PaymentRequestWebContentsManager specific because the goal
is not to have much logic in there, ever. It's purely a container class for PRs.

Let me know what you think

[Mathieu, 2017-01-06 21:20:29]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 21:20:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 21:28:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 21:28:01]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Mathieu, 2017-01-06 21:33:58]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 21:34:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 21:41:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 21:41:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[please use gerrit instead, 2017-01-06 21:49:10]

Excellent stuff!

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:8: #include
"components/payments/payment_request_impl.h"
May not need to include payment_request_impl.h. Forward declaration of
PaymentRequestImpl class in the corresponding header file was sufficient, I
think.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:17: };
Need to override the destructor from PaymentRequestDelegate.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:17: };
 private:
  DISALLOW_COPY_AND_ASSIGN(ChromePaymentRequestDelegate);

(from "base/macros.h")

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/payment_request_factory.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.cc:19: DCHECK(web_contents);
#include "base/logging.h"

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.cc:26: web_contents,
std::move(delegate), std::move(request));
Would a single statement be easier to understand?

PaymentRequestWebContentsManager::GetOrCreateForWebContents(web_contents)->CreatePaymentRequestImpl(web_contents,
MakeUnique(new ChromePaymentRequestDelegate), std::move(request));

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/payment_request_factory.h (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/site_per_process_payments_browsertest.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/site_per_process_payments_browsertest.cc:103:
EXPECT_EQ(5U, payment_requests.size());
Would you mind adding a different browser test file? site_per_process_* files
are intended only for the OOPIF feature:

http://www.chromium.org/developers/design-documents/oop-iframes

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_delegate.h (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_delegate.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_delegate.h:14: virtual void
ShowPaymentRequestDialog(PaymentRequestImpl* impl) = 0;
Need a virtual destructor with empty body:

"To make sure all implementations of the interface can be destroyed correctly,
the interface must also declare a virtual destructor (in an exception to the
first rule, this should not be pure). See Stroustrup, The C++ Programming
Language, 3rd edition, section 12.4 for details."

https://google.github.io/styleguide/cppguide.html#Interfaces

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_impl.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_impl.cc:59: // Kill ourselves.
DestroyRequest(this) is self-explanatory, IMHO.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_impl.h (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_impl.h:51: PaymentRequestWebContentsManager*
manager_;  // Weakly owned.
Did you mean to say that |manager_| owns PaymentRequestImpl?

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_web_contents_manager.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.cc:21:
DCHECK(web_contents);
#include "base/logging.h"

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.cc:34:
std::make_pair(new_request.get(), std::move(new_request)));
Different compilers evaluate the order of parameters differently. Some do it
left to right, others do it right to left. In the latter case,
std::move(new_request) will evaluate first, at which point new_request will not
longer be a valid pointer, so new_request.get() will return nullptr.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_web_contents_manager.h (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.h:59: };
DISALLOW_COPY_AND_ASSIGN(PaymentRequestWebContentsManager);

(from "base/macros.h")

[Mathieu, 2017-01-07 05:02:52]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 05:02:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-01-07 05:03:08]

PTAL

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/01/06 21:49:09, rouslan wrote:
> 2017

Done.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:8: #include
"components/payments/payment_request_impl.h"
On 2017/01/06 21:49:09, rouslan wrote:
> May not need to include payment_request_impl.h. Forward declaration of
> PaymentRequestImpl class in the corresponding header file was sufficient, I
> think.

Done.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/payment_request_factory.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/06 21:49:09, rouslan wrote:
> 2017

Done.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.cc:19: DCHECK(web_contents);
On 2017/01/06 21:49:09, rouslan wrote:
> #include "base/logging.h"

Done.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.cc:26: web_contents,
std::move(delegate), std::move(request));
On 2017/01/06 21:49:09, rouslan wrote:
> Would a single statement be easier to understand?
> 
>
PaymentRequestWebContentsManager::GetOrCreateForWebContents(web_contents)->CreatePaymentRequestImpl(web_contents,
> MakeUnique(new ChromePaymentRequestDelegate), std::move(request));

Done. good suggestion!

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/payment_request_factory.h (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/payment_request_factory.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/06 21:49:09, rouslan wrote:
> 2017

Done.

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
File chrome/browser/payments/site_per_process_payments_browsertest.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/chrome/browser/payments...
chrome/browser/payments/site_per_process_payments_browsertest.cc:103:
EXPECT_EQ(5U, payment_requests.size());
On 2017/01/06 21:49:09, rouslan wrote:
> Would you mind adding a different browser test file? site_per_process_* files
> are intended only for the OOPIF feature:
> 
> http://www.chromium.org/developers/design-documents/oop-iframes

Done.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_delegate.h (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_delegate.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/01/06 21:49:09, rouslan wrote:
> 2017

Done.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_delegate.h:14: virtual void
ShowPaymentRequestDialog(PaymentRequestImpl* impl) = 0;
On 2017/01/06 21:49:09, rouslan wrote:
> Need a virtual destructor with empty body:
> 
> "To make sure all implementations of the interface can be destroyed correctly,
> the interface must also declare a virtual destructor (in an exception to the
> first rule, this should not be pure). See Stroustrup, The C++ Programming
> Language, 3rd edition, section 12.4 for details."
> 
> https://google.github.io/styleguide/cppguide.html#Interfaces

Done.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_impl.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/01/06 21:49:09, rouslan wrote:
> 2017

Well it was moved so I'll keep 2016.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_impl.cc:59: // Kill ourselves.
On 2017/01/06 21:49:09, rouslan wrote:
> DestroyRequest(this) is self-explanatory, IMHO.

Done.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_impl.h (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_impl.h:51: PaymentRequestWebContentsManager*
manager_;  // Weakly owned.
On 2017/01/06 21:49:09, rouslan wrote:
> Did you mean to say that |manager_| owns PaymentRequestImpl?

Done, clarified comment.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_web_contents_manager.cc (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/01/06 21:49:10, rouslan wrote:
> 2017

Done.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.cc:21:
DCHECK(web_contents);
On 2017/01/06 21:49:10, rouslan wrote:
> #include "base/logging.h"

Done.

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.cc:34:
std::make_pair(new_request.get(), std::move(new_request)));
On 2017/01/06 21:49:10, rouslan wrote:
> Different compilers evaluate the order of parameters differently. Some do it
> left to right, others do it right to left. In the latter case,
> std::move(new_request) will evaluate first, at which point new_request will
not
> longer be a valid pointer, so new_request.get() will return nullptr.

Oh wow, OK. I'm caching the raw pointer now, that should do it?

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
File components/payments/payment_request_web_contents_manager.h (right):

https://codereview.chromium.org/2611253004/diff/40001/components/payments/pay...
components/payments/payment_request_web_contents_manager.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/01/06 21:49:10, rouslan wrote:
> 2017

Done.

[commit-bot: I haz the power, 2017-01-07 05:09:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 05:09:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[please use gerrit instead, 2017-01-09 14:50:13]

LGTM % comments

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:17: };
Need to override the destructor from the parent and also disallow copying and
assignment of this object.

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
File chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc
(right):

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc:38:
content::SetupCrossSiteRedirector(https_server_.get());
No need for cross-site redirector when not testing cross-site iframes.

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc:68:
GURL url = https_server_->GetURL("127.0.0.1",
Hostnames are important only for cross-site iframe testing. Here you can omit
"127.0.0.1":

GURL url = https_server_->GetURL("/payment_request_multiple_requests.html");

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
File chrome/browser/payments/site_per_process_payments_browsertest.cc (right):

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/site_per_process_payments_browsertest.cc:36:
switches::kEnableExperimentalWebPlatformFeatures);
thx! :-)

[sdefresne, 2017-01-09 16:38:46]

sdefresne@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2017-01-09 16:38:47]

https://codereview.chromium.org/2611253004/diff/80001/components/payments/BUI...
File components/payments/BUILD.gn (right):

https://codereview.chromium.org/2611253004/diff/80001/components/payments/BUI...
components/payments/BUILD.gn:39: "//content/public/browser",
If components/payments/BUILD.gn is loaded on iOS then you'll get error when
running "gn check" as this will cause "//content/public/browser" to be loaded on
iOS even if no iOS target depends on this one (this is because every target
visible from a BUILD.gn file is instantiated and buildable via ninja even if
there is no direct dependency from //).

The quick solution to fix this is to put this target behind if (!is_ios):

if (!is_ios) {
  static_library(....) {
  }
}

But the correct solution is instead to change payments component to be a
layered-component (see
https://www.chromium.org/developers/design-documents/layered-components-design)
as DEPS are not fine grained enough.

That is move all files currently in component/payments to
component/payments/core and move the new files to components/payments/content.

[Mathieu, 2017-01-09 16:53:15]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 16:53:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-01-09 16:53:38]

mathp@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org

[Mathieu, 2017-01-09 16:54:20]

Thanks Sylvain! Implemented the fix and filed a bug for the layered component.

+avi for adding //content to DEPS
+sky for adding //mojo to DEPS

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:17: };
On 2017/01/09 14:50:13, rouslan wrote:
> Need to override the destructor from the parent and also disallow copying and
> assignment of this object.

Done.

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
File chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc
(right):

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/09 14:50:13, rouslan wrote:
> 2017

Doh! I should be used to it around February

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc:38:
content::SetupCrossSiteRedirector(https_server_.get());
On 2017/01/09 14:50:13, rouslan wrote:
> No need for cross-site redirector when not testing cross-site iframes.

Done.

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/payment_request_web_contents_manager_browsertest.cc:68:
GURL url = https_server_->GetURL("127.0.0.1",
On 2017/01/09 14:50:13, rouslan wrote:
> Hostnames are important only for cross-site iframe testing. Here you can omit
> "127.0.0.1":
> 
> GURL url = https_server_->GetURL("/payment_request_multiple_requests.html");

Done.

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
File chrome/browser/payments/site_per_process_payments_browsertest.cc (right):

https://codereview.chromium.org/2611253004/diff/80001/chrome/browser/payments...
chrome/browser/payments/site_per_process_payments_browsertest.cc:36:
switches::kEnableExperimentalWebPlatformFeatures);
On 2017/01/09 14:50:13, rouslan wrote:
> thx! :-)

Acknowledged.

[Mathieu, 2017-01-09 16:56:04]

mathp@chromium.org changed reviewers:
- sdefresne@chromium.org

[Avi (use Gerrit), 2017-01-09 17:20:14]

LGTM for the dependency though have the appropriate love for our iOS peeps.

[sdefresne, 2017-01-09 17:35:39]

sdefresne@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2017-01-09 17:35:39]

lgtm

https://codereview.chromium.org/2611253004/diff/100001/components/payments/DEPS
File components/payments/DEPS (right):

https://codereview.chromium.org/2611253004/diff/100001/components/payments/DE...
components/payments/DEPS:2: "+content/public/browser",
Please add a TODO to move this to components/payments/content with a reference
to the bug.

[Mathieu, 2017-01-09 17:43:35]

Thanks

https://codereview.chromium.org/2611253004/diff/100001/components/payments/DEPS
File components/payments/DEPS (right):

https://codereview.chromium.org/2611253004/diff/100001/components/payments/DE...
components/payments/DEPS:2: "+content/public/browser",
On 2017/01/09 17:35:39, sdefresne wrote:
> Please add a TODO to move this to components/payments/content with a reference
> to the bug.

Done.

[sky, 2017-01-09 18:55:32]

mojo dep LGTM

https://codereview.chromium.org/2611253004/diff/120001/components/payments/pa...
File components/payments/payment_request_impl.h (right):

https://codereview.chromium.org/2611253004/diff/120001/components/payments/pa...
components/payments/payment_request_impl.h:22: class PaymentRequestImpl :
payments::mojom::PaymentRequest {
One of the reasons we standardized on putting all mojoms in the mojom namespace
is so that you don't have to name classes like this with Impl. Use
PaymentRequest.

[Mathieu, 2017-01-09 19:17:42]

Thanks!

https://codereview.chromium.org/2611253004/diff/120001/components/payments/pa...
File components/payments/payment_request_impl.h (right):

https://codereview.chromium.org/2611253004/diff/120001/components/payments/pa...
components/payments/payment_request_impl.h:22: class PaymentRequestImpl :
payments::mojom::PaymentRequest {
On 2017/01/09 18:55:32, sky wrote:
> One of the reasons we standardized on putting all mojoms in the mojom
namespace
> is so that you don't have to name classes like this with Impl. Use
> PaymentRequest.

Thanks, I've changed this.

[Mathieu, 2017-01-09 19:17:47]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-01-09 19:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, sdefresne@chromium.org, avi@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2611253004/#ps140001
(title: "PaymentRequestImpl -> PaymentRequest")

[commit-bot: I haz the power, 2017-01-09 19:18:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-01-09 19:19:05]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2017-01-09 19:21:20]

Patchset #8 (id:140001) has been deleted

[Mathieu, 2017-01-09 19:21:26]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-01-09 19:21:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, avi@chromium.org, rouslan@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2611253004/#ps160001
(title: "PaymentRequestImpl -> PaymentRequest")

[commit-bot: I haz the power, 2017-01-09 19:22:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 20:48:40]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483989686165260,
"parent_rev": "9e47a74b21cb8b32e79eb8f41945afcc35d8a825", "commit_rev":
"f709499d132d767ce77caf04134cd231160835ac"}

[commit-bot: I haz the power, 2017-01-09 20:49:08]

Description was changed from

==========
[Payment Request] Change the lifetime management of PaymentRequestImpl

Was previously 1 PaymentRequestImpl per WebContents. Now there is a
PaymentRequestWebContentsManager (1 per WebContents) that owns all the
PaymentRequestImpl's, whose raw pointers are shared with the delegate
and the mojo bindings.

Also, moving PaymentRequestImpl from chrome/ to components/ which creates
the need for a PaymentRequestDelegate (initially managing the Chrome
dialog).

BUG=678302
==========

to

==========
[Payment Request] Change the lifetime management of PaymentRequestImpl

Was previously 1 PaymentRequestImpl per WebContents. Now there is a
PaymentRequestWebContentsManager (1 per WebContents) that owns all the
PaymentRequestImpl's, whose raw pointers are shared with the delegate
and the mojo bindings.

Also, moving PaymentRequestImpl from chrome/ to components/ which creates
the need for a PaymentRequestDelegate (initially managing the Chrome
dialog).

BUG=678302

Review-Url: https://codereview.chromium.org/2611253004
Cr-Commit-Position: refs/heads/master@{#442335}
Committed:
https://chromium.googlesource.com/chromium/src/+/f709499d132d767ce77caf04134c...
==========

[commit-bot: I haz the power, 2017-01-09 20:49:10]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f709499d132d767ce77caf04134c...