[Web Payments] Add data attribution string to dialog

[Web Payments] Add data attribution string to dialog

BUG=721126
Review-Url: https://codereview.chromium.org/2876663002
Cr-Commit-Position: refs/heads/master@{#471018}
Committed: https://chromium.googlesource.com/chromium/src/+/6a43b93049d96b053514530d99754a24aeaaa4fd

[anthonyvd, 2017-05-10 23:59:27]

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

[anthonyvd, 2017-05-11 00:00:03]

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

[anthonyvd, 2017-05-11 00:00:04]

Hi Math, this change is based on your patch at
https://codereview.chromium.org/2864283003. Can you PTAL?

[Mathieu, 2017-05-11 00:28:04]

Looks great, some comments!

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:550:
Profile::FromBrowserContext(state()->GetBrowserContext()), "");
could we expose BrowserContext from the PaymentRequestDialogView perhaps? 

request()->web_contents()->GetBrowserContext()

Would avoid going to the state, which has become a grab bag (most of it my
fault!)

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:932: if
(first_transaction_completed) {
would benefit from a top-level comment to explain this if-else if-else business

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:944: //
Settings link.
remove

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:951:
base::string16 begin_tag = base::UTF8ToUTF16("BEGIN_LINK");
// Activate the "Settings" link in the data source string.

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
File components/payments/content/BUILD.gn (right):

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
components/payments/content/BUILD.gn:9: "payment_prefs.cc",
payments_prefs

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
components/payments/content/payment_response_helper.cc:190:
payment_request_delegate_->MarkFirstTransactionAsCompleted();
probably fine to write to the pref directly in
PaymentRequest::OnPaymentResponseAvailable without the need for the delegate?

you have access to web_contents()->GetBrowserContext() there

[Mathieu, 2017-05-11 00:28:48]

You also forgot to add payments_prefs files

[anthonyvd, 2017-05-11 00:56:46]

Comments addressed. PTAL!

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:550:
Profile::FromBrowserContext(state()->GetBrowserContext()), "");
On 2017/05/11 at 00:28:04, Mathieu wrote:
> could we expose BrowserContext from the PaymentRequestDialogView perhaps? 
> 
> request()->web_contents()->GetBrowserContext()
> 
> Would avoid going to the state, which has become a grab bag (most of it my
fault!)

Good point, somehow I missed the fact that the dialog could go through |request|
to get the web contents. This also makes it possible to have
PaymentRequestDialogView::GetProfile instead of getting the profile from the
BrowserContext everywhere since PaymentRequestDialogView is in chrome/browser.

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:932: if
(first_transaction_completed) {
On 2017/05/11 at 00:28:04, Mathieu wrote:
> would benefit from a top-level comment to explain this if-else if-else
business

Done.

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:944: //
Settings link.
On 2017/05/11 at 00:28:04, Mathieu wrote:
> remove

Done.

https://codereview.chromium.org/2876663002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:951:
base::string16 begin_tag = base::UTF8ToUTF16("BEGIN_LINK");
On 2017/05/11 at 00:28:04, Mathieu wrote:
> // Activate the "Settings" link in the data source string.

Done.

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
components/payments/content/payment_response_helper.cc:190:
payment_request_delegate_->MarkFirstTransactionAsCompleted();
On 2017/05/11 at 00:28:04, Mathieu wrote:
> probably fine to write to the pref directly in
PaymentRequest::OnPaymentResponseAvailable without the need for the delegate?
> 
> you have access to web_contents()->GetBrowserContext() there

Unfortunately, the PrefService hangs off Profile, which is in chrome/browser so
I can't get to prefs directly from here.

[anthonyvd, 2017-05-11 00:58:47]

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

[commit-bot: I haz the power, 2017-05-11 01:00:17]

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-05-11 01:06:36]

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

[commit-bot: I haz the power, 2017-05-11 01:06:37]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Mathieu, 2017-05-11 01:08:37]

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
components/payments/content/payment_response_helper.cc:190:
payment_request_delegate_->MarkFirstTransactionAsCompleted();
On 2017/05/11 00:56:46, anthonyvd wrote:
> On 2017/05/11 at 00:28:04, Mathieu wrote:
> > probably fine to write to the pref directly in
> PaymentRequest::OnPaymentResponseAvailable without the need for the delegate?
> > 
> > you have access to web_contents()->GetBrowserContext() there
> 
> Unfortunately, the PrefService hangs off Profile, which is in chrome/browser
so
> I can't get to prefs directly from here.

PaymentRequest::OnPaymentResponseAvailable could still do it if it had
delegate_->GetPrefService, which is a slightly more reusable pattern!

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

https://codereview.chromium.org/2876663002/diff/20001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:45: void
MarkFirstTransactionAsCompleted() override;
Would prefer GetPrefService here, which we could reuse later.

https://codereview.chromium.org/2876663002/diff/20001/components/payments/con...
File components/payments/content/BUILD.gn (right):

https://codereview.chromium.org/2876663002/diff/20001/components/payments/con...
components/payments/content/BUILD.gn:9: "payment_prefs.cc",
should probably be in core/, sorry I didn't say earlier

[Mathieu, 2017-05-11 01:20:26]

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
components/payments/content/payment_response_helper.cc:190:
payment_request_delegate_->MarkFirstTransactionAsCompleted();
On 2017/05/11 01:08:37, Mathieu wrote:
> On 2017/05/11 00:56:46, anthonyvd wrote:
> > On 2017/05/11 at 00:28:04, Mathieu wrote:
> > > probably fine to write to the pref directly in
> > PaymentRequest::OnPaymentResponseAvailable without the need for the
delegate?
> > > 
> > > you have access to web_contents()->GetBrowserContext() there
> > 
> > Unfortunately, the PrefService hangs off Profile, which is in chrome/browser
> so
> > I can't get to prefs directly from here.
> 
> PaymentRequest::OnPaymentResponseAvailable could still do it if it had
> delegate_->GetPrefService, which is a slightly more reusable pattern!

Actually another review reminds me, should probably be PaymentRequest::Complete

[anthonyvd, 2017-05-11 13:24:27]

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

[anthonyvd, 2017-05-11 13:24:42]

Comments addressed, PTAL.

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2876663002/diff/1/components/payments/content...
components/payments/content/payment_response_helper.cc:190:
payment_request_delegate_->MarkFirstTransactionAsCompleted();
On 2017/05/11 at 01:20:25, Mathieu wrote:
> On 2017/05/11 01:08:37, Mathieu wrote:
> > On 2017/05/11 00:56:46, anthonyvd wrote:
> > > On 2017/05/11 at 00:28:04, Mathieu wrote:
> > > > probably fine to write to the pref directly in
> > > PaymentRequest::OnPaymentResponseAvailable without the need for the
delegate?
> > > > 
> > > > you have access to web_contents()->GetBrowserContext() there
> > > 
> > > Unfortunately, the PrefService hangs off Profile, which is in
chrome/browser
> > so
> > > I can't get to prefs directly from here.
> > 
> > PaymentRequest::OnPaymentResponseAvailable could still do it if it had
> > delegate_->GetPrefService, which is a slightly more reusable pattern!
> 
> Actually another review reminds me, should probably be
PaymentRequest::Complete

Done.

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

https://codereview.chromium.org/2876663002/diff/20001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:45: void
MarkFirstTransactionAsCompleted() override;
On 2017/05/11 at 01:08:37, Mathieu wrote:
> Would prefer GetPrefService here, which we could reuse later.

Nice, done.

https://codereview.chromium.org/2876663002/diff/20001/components/payments/con...
File components/payments/content/BUILD.gn (right):

https://codereview.chromium.org/2876663002/diff/20001/components/payments/con...
components/payments/content/BUILD.gn:9: "payment_prefs.cc",
On 2017/05/11 at 01:08:37, Mathieu wrote:
> should probably be in core/, sorry I didn't say earlier

Done.

[commit-bot: I haz the power, 2017-05-11 13:24:56]

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-05-11 13:29:33]

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

[commit-bot: I haz the power, 2017-05-11 13:29:34]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[anthonyvd, 2017-05-11 13:35:57]

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

[commit-bot: I haz the power, 2017-05-11 13:36:31]

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-05-11 13:45:44]

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

[commit-bot: I haz the power, 2017-05-11 13:45:45]

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

[Mathieu, 2017-05-11 13:47:56]

Thanks for your patience, this lgtm!

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
File components/payments/core/payment_prefs.h (right):

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
components/payments/core/payment_prefs.h:5: #ifndef
COMPONENTS_PAYMENTS_CONTENT_PAYMENT_PREFS_H_
fix

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
components/payments/core/payment_prefs.h:14: extern const char
kPaymentsFirstTransactionCompleted[];
Let's add a comment plz

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
File components/payments/core/payment_request_delegate.h (right):

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
components/payments/core/payment_request_delegate.h:84: virtual PrefService*
GetPrefService() = 0;
does this just work without fwd-declaring PrefService?

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

https://codereview.chromium.org/2876663002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:22: #include
"content/public/browser/web_contents.h"
should include pref_service.h somewhere here I think?

https://codereview.chromium.org/2876663002/diff/60001/components/payments/cor...
File components/payments/core/DEPS (right):

https://codereview.chromium.org/2876663002/diff/60001/components/payments/cor...
components/payments/core/DEPS:8: "+components/prefs",
not used?

[anthonyvd, 2017-05-11 14:50:02]

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

[commit-bot: I haz the power, 2017-05-11 14:50:45]

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-05-11 17:11:48]

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

[commit-bot: I haz the power, 2017-05-11 17:11:48]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-05-11 17:28:56]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-05-11 17:28:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2876663002/#ps80001
(title: "Address comments, fix tests compile")

[commit-bot: I haz the power, 2017-05-11 17:30:02]

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-05-11 17:43:31]

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

[commit-bot: I haz the power, 2017-05-11 17:43:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[anthonyvd, 2017-05-11 17:46:27]

anthonyvd@chromium.org changed reviewers:
+ gab@chromium.org

[anthonyvd, 2017-05-11 17:46:28]

Hi gab@! 

Can you please do an OWNERS review of chrome/browser/prefs/browser_prefs.cc and
approve the DEPS changes adding '+components/pref_registry' and
'+components/prefs'?

Thanks!

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
File components/payments/core/payment_prefs.h (right):

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
components/payments/core/payment_prefs.h:5: #ifndef
COMPONENTS_PAYMENTS_CONTENT_PAYMENT_PREFS_H_
On 2017/05/11 at 13:47:55, Mathieu wrote:
> fix

Done.

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
components/payments/core/payment_prefs.h:14: extern const char
kPaymentsFirstTransactionCompleted[];
On 2017/05/11 at 13:47:55, Mathieu wrote:
> Let's add a comment plz

Done.

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
File components/payments/core/payment_request_delegate.h (right):

https://codereview.chromium.org/2876663002/diff/40001/components/payments/cor...
components/payments/core/payment_request_delegate.h:84: virtual PrefService*
GetPrefService() = 0;
On 2017/05/11 at 13:47:55, Mathieu wrote:
> does this just work without fwd-declaring PrefService?

It does but probably only by pure luck. Added the fwd-declare.

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

https://codereview.chromium.org/2876663002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:22: #include
"content/public/browser/web_contents.h"
On 2017/05/11 at 13:47:55, Mathieu wrote:
> should include pref_service.h somewhere here I think?

No need since we're not using it. All that matters here is for the compiler to
know the size of the type (which is easy for a pointer!)

https://codereview.chromium.org/2876663002/diff/60001/components/payments/cor...
File components/payments/core/DEPS (right):

https://codereview.chromium.org/2876663002/diff/60001/components/payments/cor...
components/payments/core/DEPS:8: "+components/prefs",
On 2017/05/11 at 13:47:55, Mathieu wrote:
> not used?

Done.

[gab, 2017-05-11 18:26:36]

On 2017/05/11 17:46:28, anthonyvd wrote:
> Hi gab@! 
> 
> Can you please do an OWNERS review of chrome/browser/prefs/browser_prefs.cc
and
> approve the DEPS changes adding '+components/pref_registry' and
> '+components/prefs'?

lgtm

[anthonyvd, 2017-05-11 18:27:07]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:28:17]

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-05-11 18:39:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494527227223540,
"parent_rev": "721d64b58f6e051c58ac46c0a8542fa8ee7de193", "commit_rev":
"6a43b93049d96b053514530d99754a24aeaaa4fd"}

[commit-bot: I haz the power, 2017-05-11 18:39:40]

Description was changed from

==========
[Web Payments] Add data attribution string to dialog

BUG=721126
==========

to

==========
[Web Payments] Add data attribution string to dialog

BUG=721126

Review-Url: https://codereview.chromium.org/2876663002
Cr-Commit-Position: refs/heads/master@{#471018}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a43b93049d96b053514530d9975...
==========

[commit-bot: I haz the power, 2017-05-11 18:39:42]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6a43b93049d96b053514530d9975...