Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446093004:
[Web Payments] Initial plumb of Payment Request dialog (Closed)
|
Created:
4 years, 1 month ago by Kevin Bailey Modified:
4 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Web Payments] Initial plumb of Payment Request dialog
Very crude plumbing of web payments Payment Request dialog box.
Does not handle all messages, and dialog box is nearly empty.
BUG=659648
Committed: https://crrev.com/7f6421b1628a15833196ff7cf1fefbe5fcd69558
Cr-Commit-Position: refs/heads/master@{#433235}
Patch Set 1 #Patch Set 2 : Replaced my flag with existing one #
Total comments: 32
Patch Set 3 : Responses #
Total comments: 1
Patch Set 4 : Remove mystery strings #
Total comments: 26
Patch Set 5 : Better object life-cycle management #Patch Set 6 : Minor responses #Patch Set 7 : Rebase, and see if empty library was making Windows unhappy #Patch Set 8 : Try again to make Windows happy #Patch Set 9 : Make Android build happy #
Total comments: 15
Patch Set 10 : Handling Cancel #Patch Set 11 : Minor responses #
Total comments: 24
Patch Set 12 : Responses #Patch Set 13 : Missed a response #Patch Set 14 : More responses #
Total comments: 2
Patch Set 15 : Restore intermediate variable #Patch Set 16 : Rebase #Patch Set 17 : Fix bad rebase #Patch Set 18 : Handle new member function #Messages
Total messages: 56 (27 generated)
Description was changed from ========== [Web Payments] Initial plumb of Payment Request dialog Very crude plumbing of web payments Payment Request dialog box. Does not handle all messages, and dialog box is nearly empty. BUG=659648 ========== to ========== [Web Payments] Initial plumb of Payment Request dialog Very crude plumbing of web payments Payment Request dialog box. Does not handle all messages, and dialog box is nearly empty. BUG=659648 ==========
krb@chromium.org changed reviewers: + rouslan@chromium.org
rouslan@chromium.org changed reviewers: + anthonyvd@chromium.org
krb, you're on the right track. Thank you for your efforts! :-D (anthonyvd fyi) https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2990: if (AreExperimentalWebPlatformFeaturesEnabled()) { nit: I think there's no need to explicitly check the flag. If the flag is disabled, then this handler will never be invoked. I guess you could argue that we shave off some memory and startup time by checking for the flag. Not sure what you reasoning was. Either way, this one is up to you to decide. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:1: static_library("payments") { License header please. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Ditto https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:34: // TODO: Give the dialog |this| to delete it when done. Did you plan to address these TODOs in this CL? https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. "(c)" is no longer recommended in license header. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:19: mojo::InterfaceRequest<blink::mojom::PaymentRequest> request); // blink::mojom::PaymentRequest implementation: (Groups of methods marked with "override" are usually adorned with a single-line comment that points out the interface that they are implementing. That way, in absence of a modern IDE, a reader would know where to look for the documentation on these methods.) https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:29: ~PaymentRequestImpl() override; Style nit: The destructor should follow the constructor. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:32: // content::RenderFrameHost* render_frame_host_; Remove? https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:33: content::WebContents* web_contents_; Do you happen to know the ownership relationship of web_contents_ an this PaymentRequestImpl object? I want to say "web_contents_ owns this object", but not sure. This kind of information would be useful in a comment. If you're not using this variable anywhere, better just remove it to avoid trouble ;-) https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:34: mojo::Binding<blink::mojom::PaymentRequest> binding_; DISALLOW_COPY_AND_ASSIGN(PaymentRequestImpl); (Always helps with memory management snafoos.) https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.cc (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Ditto https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:9: : label_(new views::Label(base::ASCIIToUTF16("Payments dialog"))) { i18n https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:20: ps.set_width(ps.width() + 200); constant? https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Ditto https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:24: ~PaymentRequestDialog() override; Next to constructor, please. https://codereview.chromium.org/2446093004/diff/20001/components/payments/BUI... File components/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/20001/components/payments/BUI... components/payments/BUILD.gn:19: # Place-holder. Does this patch not compile the newly added files?
https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2990: if (AreExperimentalWebPlatformFeaturesEnabled()) { On 2016/11/04 21:58:12, rouslan wrote: > nit: I think there's no need to explicitly check the flag. If the flag is > disabled, then this handler will never be invoked. I guess you could argue that > we shave off some memory and startup time by checking for the flag. Not sure > what you reasoning was. Either way, this one is up to you to decide. Mostly I was unsure about how reachable this service was, and didn't want it listening if unneeded. This change is supposed to be invisible without the flag, and yes, it shouldn't use up any extra memory if the user didn't request it. In short, I don't see the harm if conditional, but some harm if not. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:1: static_library("payments") { On 2016/11/04 21:58:12, rouslan wrote: > License header please. Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/04 21:58:12, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:34: // TODO: Give the dialog |this| to delete it when done. On 2016/11/04 21:58:13, rouslan wrote: > Did you plan to address these TODOs in this CL? No, not yet. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/04 21:58:13, rouslan wrote: > "(c)" is no longer recommended in license header. Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:19: mojo::InterfaceRequest<blink::mojom::PaymentRequest> request); On 2016/11/04 21:58:13, rouslan wrote: > // blink::mojom::PaymentRequest implementation: > > (Groups of methods marked with "override" are usually adorned with a single-line > comment that points out the interface that they are implementing. That way, in > absence of a modern IDE, a reader would know where to look for the documentation > on these methods.) Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:29: ~PaymentRequestImpl() override; On 2016/11/04 21:58:13, rouslan wrote: > Style nit: The destructor should follow the constructor. Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:32: // content::RenderFrameHost* render_frame_host_; On 2016/11/04 21:58:14, rouslan wrote: > Remove? Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:33: content::WebContents* web_contents_; On 2016/11/04 21:58:13, rouslan wrote: > Do you happen to know the ownership relationship of web_contents_ an this > PaymentRequestImpl object? I want to say "web_contents_ owns this object", but > not sure. This kind of information would be useful in a comment. If you're not > using this variable anywhere, better just remove it to avoid trouble ;-) I was going to deal with ownership later, thus the TODO. It's used in ::Init(). https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:34: mojo::Binding<blink::mojom::PaymentRequest> binding_; On 2016/11/04 21:58:13, rouslan wrote: > DISALLOW_COPY_AND_ASSIGN(PaymentRequestImpl); > > (Always helps with memory management snafoos.) Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.cc (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/04 21:58:14, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:9: : label_(new views::Label(base::ASCIIToUTF16("Payments dialog"))) { On 2016/11/04 21:58:14, rouslan wrote: > i18n This label is already removed in the new branch, moved to the title. I'll add a constant there. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:20: ps.set_width(ps.width() + 200); On 2016/11/04 21:58:14, rouslan wrote: > constant? It will go away, to be replaced by asking each element their preferred size. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/04 21:58:15, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2446093004/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:24: ~PaymentRequestDialog() override; On 2016/11/04 21:58:15, rouslan wrote: > Next to constructor, please. Done. https://codereview.chromium.org/2446093004/diff/20001/components/payments/BUI... File components/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/20001/components/payments/BUI... components/payments/BUILD.gn:19: # Place-holder. On 2016/11/04 21:58:15, rouslan wrote: > Does this patch not compile the newly added files? The new files are over in chrome/browser/payments (and are compiled and tested, of course) so the question confuses me. The purpose of this placeholder is so the other directories can depend on a single stable target, and we can change it without bothering other owners unnecessarily.
LGTM % comment https://codereview.chromium.org/2446093004/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2446093004/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:15528: + </message> Seems like a bad merge. These strings should be gone now.
krb@chromium.org changed reviewers: + thestig@chromium.org
Lei, Could you take a quick owner look? thx, krb
https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:7: "payment_request_impl.cc", Add the .h files too. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:11: "//skia", I don't see any Skia usage. Probably needs to depend on components/payments at least. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:17: new PaymentRequestImpl(render_frame_host, web_contents, std::move(request)); Does anything run this code yet? LSAN may complain if it leaks. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:21: content::RenderFrameHost* unused_render_frame_host, Is that because the RFH is used on Android? https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:24: // TODO: consider a weak ptr too Well, when is this going to delete itself? https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:10: #include "content/public/browser/render_frame_host.h" Forward declare? (Chromium style diverges from google3 style) https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:27: void Show() override {} Not sure if the compiler will like the impl for virtual methods in the header. Run some try jobs? https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.cc (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:22: ps.set_width(ps.width() + 200); ps.Enlarge(200, 200); https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:8: #include "ui/gfx/canvas.h" Not used? https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:14: // TODO: This should be more specific to the stage of payment collection. TODO(who) ? https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:22: // We might be able to get rid of this later. TODO(someone): ... ? https://codereview.chromium.org/2446093004/diff/60001/components/payments/BUI... File components/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/60001/components/payments/BUI... components/payments/BUILD.gn:29: # Place-holder. I assume there is a plan to put files here in the near future?
Rouslan, I was hoping to free the PaymentRequestImpl in this CL. I expected that calling OnError() would travel back to the client and close the connection, but I'm not seeing it. Any thoughts in this regard would be appreciated. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:7: "payment_request_impl.cc", On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > Add the .h files too. Done. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:11: "//skia", On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > I don't see any Skia usage. Probably needs to depend on components/payments at > least. It whines about SkTypes.h. I added payments. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:17: new PaymentRequestImpl(render_frame_host, web_contents, std::move(request)); On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > Does anything run this code yet? LSAN may complain if it leaks. It's behind a flag, but nevertheless I tried improving the memory management. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:21: content::RenderFrameHost* unused_render_frame_host, On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > Is that because the RFH is used on Android? I removed it. It's based on WebContents now. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:24: // TODO: consider a weak ptr too On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > Well, when is this going to delete itself? At this point, when the Mojo connection goes away. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:10: #include "content/public/browser/render_frame_host.h" On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > Forward declare? (Chromium style diverges from google3 style) It doesn't seem to provide any benefit in this leaf include file, but here you go. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.h:27: void Show() override {} On 2016/11/09 18:08:56, Lei Zhang (slow) wrote: > Not sure if the compiler will like the impl for virtual methods in the header. > Run some try jobs? I haven't seen any complain. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.cc (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.cc:22: ps.set_width(ps.width() + 200); On 2016/11/09 18:08:56, Lei Zhang (slow) wrote: > ps.Enlarge(200, 200); Done, thanks. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:8: #include "ui/gfx/canvas.h" On 2016/11/09 18:08:56, Lei Zhang (slow) wrote: > Not used? Gone. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:14: // TODO: This should be more specific to the stage of payment collection. On 2016/11/09 18:08:56, Lei Zhang (slow) wrote: > TODO(who) ? Removed comment. https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/ui/payment_request_dialog.h:22: // We might be able to get rid of this later. On 2016/11/09 18:08:56, Lei Zhang (slow) wrote: > TODO(someone): ... ? Removed comment. https://codereview.chromium.org/2446093004/diff/60001/components/payments/BUI... File components/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/60001/components/payments/BUI... components/payments/BUILD.gn:29: # Place-holder. On 2016/11/09 18:08:56, Lei Zhang (slow) wrote: > I assume there is a plan to put files here in the near future? Yes. I could just point chrome/browser/BUILD.gn at :payment_request for now and change it later, but it seems silly to unnecessarily bother an OWNER, especially when I'd prefer a name like 'payments' to refer to the whole payments library. [Update] Windows' linker complained about it, so I'll change this later.
On 2016/11/15 14:36:33, Kevin Bailey wrote: > I was hoping to free the PaymentRequestImpl in this CL. > I expected that calling OnError() would travel back to the > client and close the connection, but I'm not seeing it. > Any thoughts in this regard would be appreciated. You need to add a mojo connection error handler in PaymentRequestImpl. PaymentRequestClient::OnError() will signal the renderer to close the Mojo connection. At this point, the mojo connection error handler in PaymentRequestImpl should hide the UI and delete itself.
Connection error handler in Blink: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... In Java: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:11: "//skia", On 2016/11/15 14:36:32, Kevin Bailey wrote: > On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > > I don't see any Skia usage. Probably needs to depend on components/payments at > > least. > > It whines about SkTypes.h. I added payments. You probably want to depend on //ui/views? https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:25: if (set_.find(web_contents) == set_.end()) { You can use base::ContainsKey() if that feels more readable. https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:27: new payments::PaymentRequestImpl(web_contents, std::move(request))); base::MakeUnique? https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:31: } else { No need for else after a return. https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:44: base::Lock mu_; This is going to protect against thread-safe access? What threads call (Un)AssignPaymentRequest() ? https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/ui/payment_request_dialog.h:20: ui::ModalType GetModalType() const override; style: groups the overrides together: // views::DialogDelegateView: foo Foo() override; bar Bar() override;
This patch allows the user to click Cancel, and re-open the dialog any number of times, while preventing more than one dialog at a time, however, there are still many things missing: Closing the dialog while traveling up the stack Handling Abort() (and Complete()) Handling Abort() called before Init() Handling simultaneous closes from both ends of the stack etc. Not sure if you want to deal with these cases in this CL.
https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:44: base::Lock mu_; On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > This is going to protect against thread-safe access? What threads call > (Un)AssignPaymentRequest() ? This comes directly from our conversation about multiple frames trying to open a dialog.
On 2016/11/15 19:26:35, Kevin Bailey wrote: > https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... > File chrome/browser/payments/payment_request_impl.cc (right): > > https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... > chrome/browser/payments/payment_request_impl.cc:44: base::Lock mu_; > On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > > This is going to protect against thread-safe access? What threads call > > (Un)AssignPaymentRequest() ? > > This comes directly from our conversation about multiple frames trying to open a > dialog. Only one thread should ever call the factory. The lock should not be necessary.
https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:44: base::Lock mu_; On 2016/11/15 19:26:35, Kevin Bailey wrote: > On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > > This is going to protect against thread-safe access? What threads call > > (Un)AssignPaymentRequest() ? > > This comes directly from our conversation about multiple frames trying to open a > dialog. When multiple frames try to communicate with the browser, AFAIK the browser handles everything on the UI thread, so there's no racing. WebContents and many other content/ classes all live on the UI thread too.
https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... File chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2446093004/diff/60001/chrome/browser/payments... chrome/browser/payments/BUILD.gn:11: "//skia", On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > On 2016/11/15 14:36:32, Kevin Bailey wrote: > > On 2016/11/09 18:08:55, Lei Zhang (slow) wrote: > > > I don't see any Skia usage. Probably needs to depend on components/payments > at > > > least. > > > > It whines about SkTypes.h. I added payments. > > You probably want to depend on //ui/views? If one dependency is as good as another (per our discussion). :) https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:25: if (set_.find(web_contents) == set_.end()) { On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > You can use base::ContainsKey() if that feels more readable. Not for me, but happy to follow Rouslan's preference. https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:27: new payments::PaymentRequestImpl(web_contents, std::move(request))); On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > base::MakeUnique? Sorry, obsolete. https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:31: } else { On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > No need for else after a return. No harm in making the "either or" behavior more obvious either. Done. https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:44: base::Lock mu_; On 2016/11/15 19:30:57, Lei Zhang (not reviewing code) wrote: > On 2016/11/15 19:26:35, Kevin Bailey wrote: > > On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > > > This is going to protect against thread-safe access? What threads call > > > (Un)AssignPaymentRequest() ? > > > > This comes directly from our conversation about multiple frames trying to open > a > > dialog. > > When multiple frames try to communicate with the browser, AFAIK the browser > handles everything on the UI thread, so there's no racing. WebContents and many > other content/ classes all live on the UI thread too. I got rid of the lock, but if you're saying that there may be multiple simultaneous WebContents (that a set<> is needed here), then another off-line discussion may be in order. https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/ui/payment_request_dialog.h:20: ui::ModalType GetModalType() const override; On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > style: groups the overrides together: > > // views::DialogDelegateView: > foo Foo() override; > bar Bar() override; I separated them to indicate that they all came from different places (my fault for not specifying where.) I don't see how (in this case) saying that the overrides come from the one and only base class informs the reader of anything. This way rather hides their origin.
https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:25: if (set_.find(web_contents) == set_.end()) { I prefer what you have written.
https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:44: base::Lock mu_; All WebContents objects live in the single browser process in the single UI thread. Locks are only useful for the multi-process and multi-thread situations.
https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... File chrome/browser/payments/ui/payment_request_dialog.h (right): https://codereview.chromium.org/2446093004/diff/160001/chrome/browser/payment... chrome/browser/payments/ui/payment_request_dialog.h:20: ui::ModalType GetModalType() const override; On 2016/11/15 20:33:55, Kevin Bailey wrote: > On 2016/11/15 19:20:47, Lei Zhang (not reviewing code) wrote: > > style: groups the overrides together: > > > > // views::DialogDelegateView: > > foo Foo() override; > > bar Bar() override; > > I separated them to indicate that they all came from different > places (my fault for not specifying where.) I don't see how > (in this case) saying that the overrides come from the one and > only base class informs the reader of anything. This way rather > hides their origin. Oh, I didn't the realize the DialogDelegateView hierachy is so complicated. You can certainly separate it out again, but add comments to label what's overriding what.
https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2992: #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_WIN) You can do #elif since this is related to the above block. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3001: #endif And add a blank line to separate from the unrelated thing below. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:10: #include "base/synchronization/lock.h" No longer needed. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:39: std::set<content::WebContents*> set_; If you make this a map of: content::WebContents* to std::unique_ptr<PaymentRequestImpl>, then you don't need refcounting, and there's clear ownership. You'll have to call UnassignPaymentRequest() from onError(). https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:27: friend class base::RefCounted<PaymentRequestImpl>; Most often, existing code just put these in the private block below, rather than having public/private/public/private. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:40: void onError(); OnError() ? Is this suppose to be part of the mojom thing? https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/ui/payment_request_dialog.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/ui/payment_request_dialog.cc:14: label_(new views::Label(base::ASCIIToUTF16("Payments dialog"))) { I assume the string will be internationalized later.
https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2992: #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_WIN) On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > You can do #elif since this is related to the above block. Done. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3001: #endif On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > And add a blank line to separate from the unrelated thing below. Done. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:10: #include "base/synchronization/lock.h" On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > No longer needed. Done. Odd, git cl lint doesn't complain. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:39: std::set<content::WebContents*> set_; On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > If you make this a map of: content::WebContents* to > std::unique_ptr<PaymentRequestImpl>, then you don't need refcounting, and > there's clear ownership. You'll have to call UnassignPaymentRequest() from > onError(). Funny that you mention it, but it was a map<> and I changed it because the binding, for Mojo below, seems to prefer ref-counting, I suppose to prevent referencing a deleted object. I assumed this is what people preferred. Not true? https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:27: friend class base::RefCounted<PaymentRequestImpl>; On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > Most often, existing code just put these in the private block below, rather than > having public/private/public/private. I moved it here because Rouslan suggested that the destructor should be after the constructor. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:40: void onError(); On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > OnError() ? Is this suppose to be part of the mojom thing? I capitalized it in case that's what you're hinting at. Otherwise, not sure what you're asking. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/ui/payment_request_dialog.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/ui/payment_request_dialog.cc:14: label_(new views::Label(base::ASCIIToUTF16("Payments dialog"))) { On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > I assume the string will be internationalized later. It's just a placeholder. I vaguely recall that the dialog title will actually be per purchase, like the store name. (I don't have the mocks in front of me.)
I would also suggest sprinkling in DCHECK_CURRENTLY_ON(content::BrowserThread::UI) to make sure your code is actually running on the right thread. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:10: #include "base/synchronization/lock.h" On 2016/11/16 14:13:47, Kevin Bailey wrote: > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > No longer needed. > > Done. Odd, git cl lint doesn't complain. git cl lint is not a "find what #includes you don't need" tool. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:39: std::set<content::WebContents*> set_; On 2016/11/16 14:13:47, Kevin Bailey wrote: > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > If you make this a map of: content::WebContents* to > > std::unique_ptr<PaymentRequestImpl>, then you don't need refcounting, and > > there's clear ownership. You'll have to call UnassignPaymentRequest() from > > onError(). > > Funny that you mention it, but it was a map<> and I changed it > because the binding, for Mojo below, seems to prefer ref-counting, > I suppose to prevent referencing a deleted object. I assumed this > is what people preferred. Not true? I haven't played with Mojo in a while. Ok, how about the same map suggestion, but with a scoped_refptr instead of a unique_ptr? Basically avoid the need for manual AddRef/Release calls. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:27: friend class base::RefCounted<PaymentRequestImpl>; On 2016/11/16 14:13:47, Kevin Bailey wrote: > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > Most often, existing code just put these in the private block below, rather > than > > having public/private/public/private. > > I moved it here because Rouslan suggested that the destructor should > be after the constructor. At the time of that suggestion, the destructor was public, but now you made it private, so it does not make sense style-wise to keep them together anymore. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:40: void onError(); On 2016/11/16 14:13:47, Kevin Bailey wrote: > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > OnError() ? Is this suppose to be part of the mojom thing? > > I capitalized it in case that's what you're hinting at. > Otherwise, not sure what you're asking. There's no override keyword, so I wasn't sure if it's actually related to the other methods.
https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:39: std::set<content::WebContents*> set_; On 2016/11/16 18:39:30, Lei Zhang (not reviewing code) wrote: > On 2016/11/16 14:13:47, Kevin Bailey wrote: > > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > > If you make this a map of: content::WebContents* to > > > std::unique_ptr<PaymentRequestImpl>, then you don't need refcounting, and > > > there's clear ownership. You'll have to call UnassignPaymentRequest() from > > > onError(). > > > > Funny that you mention it, but it was a map<> and I changed it > > because the binding, for Mojo below, seems to prefer ref-counting, > > I suppose to prevent referencing a deleted object. I assumed this > > is what people preferred. Not true? > > I haven't played with Mojo in a while. Ok, how about the same map suggestion, > but with a scoped_refptr instead of a unique_ptr? Basically avoid the need for > manual AddRef/Release calls. FWIW, most of the set_connection_error_handler() calls found in Code Search passes pointers wrapped in base::Unretained(). I don't know Mojo well enough to tell you why. Maybe ask the Mojo experts if a refcounted object is really needed here?
The single-threaded factory was an obvious place for a DCHECK. The rest were guesses based on speculation of shared data access. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:10: #include "base/synchronization/lock.h" On 2016/11/16 18:39:30, Lei Zhang (Mostly OOO) wrote: > On 2016/11/16 14:13:47, Kevin Bailey wrote: > > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > > No longer needed. > > > > Done. Odd, git cl lint doesn't complain. > > git cl lint is not a "find what #includes you don't need" tool. As I stated. My point was, since it does do 'iwyu', maybe it should. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:39: std::set<content::WebContents*> set_; On 2016/11/16 18:39:30, Lei Zhang (Mostly OOO) wrote: > On 2016/11/16 14:13:47, Kevin Bailey wrote: > > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > > If you make this a map of: content::WebContents* to > > > std::unique_ptr<PaymentRequestImpl>, then you don't need refcounting, and > > > there's clear ownership. You'll have to call UnassignPaymentRequest() from > > > onError(). > > > > Funny that you mention it, but it was a map<> and I changed it > > because the binding, for Mojo below, seems to prefer ref-counting, > > I suppose to prevent referencing a deleted object. I assumed this > > is what people preferred. Not true? > > I haven't played with Mojo in a while. Ok, how about the same map suggestion, > but with a scoped_refptr instead of a unique_ptr? Basically avoid the need for > manual AddRef/Release calls. Works for me, since it prevents one without the other; just realize that it only swaps e.g. Release() for something else. BTW, I realize that "what people preferred" was an ambiguous phrase. I meant, "the right way" as opposed to "the popular way". The function takes a base::Closure(), so it's really the binding setting the policy for ownership, not Mojo. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.h (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:27: friend class base::RefCounted<PaymentRequestImpl>; On 2016/11/16 18:39:30, Lei Zhang (Mostly OOO) wrote: > On 2016/11/16 14:13:47, Kevin Bailey wrote: > > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > > Most often, existing code just put these in the private block below, rather > > than > > > having public/private/public/private. > > > > I moved it here because Rouslan suggested that the destructor should > > be after the constructor. > > At the time of that suggestion, the destructor was public, but now you made it > private, so it does not make sense style-wise to keep them together anymore. I understand. Done. I assume we still want the destructor after the constructor in the .cc file, to easily see that everything was undone. https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.h:40: void onError(); On 2016/11/16 18:39:30, Lei Zhang (Mostly OOO) wrote: > On 2016/11/16 14:13:47, Kevin Bailey wrote: > > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > > OnError() ? Is this suppose to be part of the mojom thing? > > > > I capitalized it in case that's what you're hinting at. > > Otherwise, not sure what you're asking. > > There's no override keyword, so I wasn't sure if it's actually related to the > other methods. I didn't space it away either. Fixed.
lgtm https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/200001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:39: std::set<content::WebContents*> set_; On 2016/11/16 22:34:09, Kevin Bailey wrote: > On 2016/11/16 18:39:30, Lei Zhang (Mostly OOO) wrote: > > On 2016/11/16 14:13:47, Kevin Bailey wrote: > > > On 2016/11/16 00:59:33, Lei Zhang (not reviewing code) wrote: > > > > If you make this a map of: content::WebContents* to > > > > std::unique_ptr<PaymentRequestImpl>, then you don't need refcounting, and > > > > there's clear ownership. You'll have to call UnassignPaymentRequest() from > > > > onError(). > > > > > > Funny that you mention it, but it was a map<> and I changed it > > > because the binding, for Mojo below, seems to prefer ref-counting, > > > I suppose to prevent referencing a deleted object. I assumed this > > > is what people preferred. Not true? > > > > I haven't played with Mojo in a while. Ok, how about the same map suggestion, > > but with a scoped_refptr instead of a unique_ptr? Basically avoid the need for > > manual AddRef/Release calls. > > Works for me, since it prevents one without the other; just realize > that it only swaps e.g. Release() for something else. > > BTW, I realize that "what people preferred" was an ambiguous phrase. > I meant, "the right way" as opposed to "the popular way". I don't know what "the right way" is, so I would suggest asking mojo/OWNERS if refcounting is necessary. > The function takes a base::Closure(), so it's really the binding > setting the policy for ownership, not Mojo. I meant |binding_| is a Mojo thing in general. https://codereview.chromium.org/2446093004/diff/260001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/260001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:26: map_[web_contents] = scoped_refptr<payments::PaymentRequestImpl>( You may be able to assign directly without explicitly creating a scoped_refptr.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2446093004/diff/260001/chrome/browser/payment... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2446093004/diff/260001/chrome/browser/payment... chrome/browser/payments/payment_request_impl.cc:26: map_[web_contents] = scoped_refptr<payments::PaymentRequestImpl>( On 2016/11/16 23:02:26, Lei Zhang (Mostly OOO) wrote: > You may be able to assign directly without explicitly creating a scoped_refptr. It's worse than that. It's incorrect to get rid of the intermediate variable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2446093004/#ps280001 (title: "Restore intermediate variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/chrome_content_browser_client.cc:
While running git apply --index -p1;
error: patch failed: chrome/browser/chrome_content_browser_client.cc:2978
error: chrome/browser/chrome_content_browser_client.cc: patch does not apply
Patch: chrome/browser/chrome_content_browser_client.cc
Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc
b/chrome/browser/chrome_content_browser_client.cc
index
ce9507def3a49c33c13c5bdf4f876c55c6833eea..9fac515371e508a6c019a5aac3fe28badb41feb8
100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -57,6 +57,7 @@
#include "chrome/browser/notifications/platform_notification_service_impl.h"
#include "chrome/browser/page_load_metrics/metrics_navigation_throttle.h"
#include "chrome/browser/password_manager/chrome_password_manager_client.h"
+#include "chrome/browser/payments/payment_request_impl.h"
#include "chrome/browser/permissions/permission_context_base.h"
#include "chrome/browser/platform_util.h"
#include "chrome/browser/prerender/prerender_final_status.h"
@@ -1411,6 +1412,15 @@ bool IsAutoReloadVisibleOnlyEnabled() {
return true;
}
+#if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_WIN)
+bool AreExperimentalWebPlatformFeaturesEnabled() {
+ const base::CommandLine& browser_command_line =
+ *base::CommandLine::ForCurrentProcess();
+ return browser_command_line.HasSwitch(
+ switches::kEnableExperimentalWebPlatformFeatures);
+}
+#endif
+
void MaybeAppendBlinkSettingsSwitchForFieldTrial(
const base::CommandLine& browser_command_line,
base::CommandLine* command_line) {
@@ -2978,6 +2988,15 @@ void
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces(
base::Bind(&ForwardShareServiceRequest,
web_contents->GetJavaInterfaces()->GetWeakPtr()));
}
+#elif defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_WIN)
+ if (AreExperimentalWebPlatformFeaturesEnabled()) {
+ content::WebContents* web_contents =
+ content::WebContents::FromRenderFrameHost(render_frame_host);
+ if (web_contents) {
+ registry->AddInterface(
+ base::Bind(CreatePaymentRequestHandler, web_contents));
+ }
+ }
#endif
#if defined(ENABLE_MEDIA_ROUTER)
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2446093004/#ps340001 (title: "Handle new member function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Web Payments] Initial plumb of Payment Request dialog Very crude plumbing of web payments Payment Request dialog box. Does not handle all messages, and dialog box is nearly empty. BUG=659648 ========== to ========== [Web Payments] Initial plumb of Payment Request dialog Very crude plumbing of web payments Payment Request dialog box. Does not handle all messages, and dialog box is nearly empty. BUG=659648 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [Web Payments] Initial plumb of Payment Request dialog Very crude plumbing of web payments Payment Request dialog box. Does not handle all messages, and dialog box is nearly empty. BUG=659648 ========== to ========== [Web Payments] Initial plumb of Payment Request dialog Very crude plumbing of web payments Payment Request dialog box. Does not handle all messages, and dialog box is nearly empty. BUG=659648 Committed: https://crrev.com/7f6421b1628a15833196ff7cf1fefbe5fcd69558 Cr-Commit-Position: refs/heads/master@{#433235} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/7f6421b1628a15833196ff7cf1fefbe5fcd69558 Cr-Commit-Position: refs/heads/master@{#433235} |
