Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with the newly introduced RenderFrameHost
in order to know the (sub)frame that created the payment request.

This CL also adds WebContentsStatics which should contain public static
methods for WebContents.

BUG=620173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001 (http://crrev.com/2681933002#ps100001)

Review-Url: https://codereview.chromium.org/2692023002
Cr-Commit-Position: refs/heads/master@{#457464}
Committed: https://chromium.googlesource.com/chromium/src/+/6e62a5770c75bce60af3a8f91866e2ea7dcbf3bf

[rwlbuis, 2017-02-15 22:12:25]

Description was changed from

==========
mojo


RenderFrameHost WIP

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001
(http://crrev.com/2681933002#ps100001)
==========

to

==========
Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with the newly introduced RenderFrameHost
in order to know the (sub)frame that created the payment request.

BUG=620173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001
(http://crrev.com/2681933002#ps100001)
==========

[rwlbuis, 2017-03-07 18:35:08]

Patchset #5 (id:80001) has been deleted

[rwlbuis, 2017-03-07 18:49:28]

Patchset #5 (id:100001) has been deleted

[rwlbuis, 2017-03-09 15:25:06]

Patchset #6 (id:140001) has been deleted

[rwlbuis, 2017-03-09 15:44:53]

Description was changed from

==========
Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with the newly introduced RenderFrameHost
in order to know the (sub)frame that created the payment request.

BUG=620173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001
(http://crrev.com/2681933002#ps100001)
==========

to

==========
Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with the newly introduced RenderFrameHost
in order to know the (sub)frame that created the payment request.

This CL also adds WebContentsStatics which should contain public static
methods for WebContents.

BUG=620173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001
(http://crrev.com/2681933002#ps100001)
==========

[rwlbuis, 2017-03-09 19:02:46]

rob.buis@samsung.com changed reviewers:
+ boliu@chromium.org, rouslan@chromium.org

[rwlbuis, 2017-03-09 19:02:47]

PTAL.

I think implementing sending of iframe origin (main reason why we do these CLs)
can be done later as it only adds to the size of this patch.

[please use gerrit instead, 2017-03-09 19:40:11]

payments LGTM

Thank you!

[rwlbuis, 2017-03-09 20:12:27]

Patchset #8 (id:200001) has been deleted

[boliu, 2017-03-10 19:02:09]

boliu@chromium.org changed reviewers:
+ rockot@chromium.org

[boliu, 2017-03-10 19:02:11]

+rockot to look over the mojo things. This directly mirrors mojo implementation
on WebContents, so I don't think there are any issues, but doesn't hurt to check
:p

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java
(right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:24:
RenderFrameHostDelegate getRenderFrameHostDelegate();
again, RenderFrameHostDelegate should not be in content_public

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java
(right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java:5:
package org.chromium.content_public.browser;
this should not be part of the public interface

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsStatics.java
(right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsStatics.java:14:
public static WebContents fromRenderFrameHost(RenderFrameHost rfh) {
this class is free to import non-public content classes and free to just cast
RFH to RFHImpl, so there is no need for RFHDelegate to be public

https://codereview.chromium.org/2692023002/diff/180001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:236: // scoped to this WebContents.
This provides access to interfaces implemented
fix comment

[rwlbuis, 2017-03-10 21:40:42]

PTAL.

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java
(right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:24:
RenderFrameHostDelegate getRenderFrameHostDelegate();
On 2017/03/10 19:02:10, boliu wrote:
> again, RenderFrameHostDelegate should not be in content_public

Done.

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java
(right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java:5:
package org.chromium.content_public.browser;
On 2017/03/10 19:02:10, boliu wrote:
> this should not be part of the public interface

Done.

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsStatics.java
(right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsStatics.java:14:
public static WebContents fromRenderFrameHost(RenderFrameHost rfh) {
On 2017/03/10 19:02:10, boliu wrote:
> this class is free to import non-public content classes and free to just cast
> RFH to RFHImpl, so there is no need for RFHDelegate to be public

Ah, I did not realize that was okay :) Fixed.

https://codereview.chromium.org/2692023002/diff/180001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2692023002/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:236: // scoped to this WebContents.
This provides access to interfaces implemented
On 2017/03/10 19:02:11, boliu wrote:
> fix comment

Done.

[boliu, 2017-03-11 00:49:06]

content lgtm % rockot review

Are there tests for the payment stuff in chrome? Or are there already tests that
don't need to be updated for this CL?

[rwlbuis, 2017-03-11 01:13:18]

On 2017/03/11 00:49:06, boliu wrote:
> content lgtm % rockot review

Thanks!

> Are there tests for the payment stuff in chrome? Or are there already tests
that
> don't need to be updated for this CL?

I think there are plenty of tests for this. One example is the many java tests
with this test base:
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...

There are C++ tests too. I am sure Rouslan can point them out if you want. I
believe payment tests not regressing/manual testing showing up no problems
is proof enough and we do not need new tests as we are simply changing one
internal mojo parameter (WebContents -> RFH), end result should be the same.

My manual testing showed up no problems on some payment request demo sites, so I
have confidence the new mojo lines are doing their job.

[please use gerrit instead, 2017-03-11 01:20:28]

On 2017/03/11 01:13:18, rwlbuis wrote:
> On 2017/03/11 00:49:06, boliu wrote:
> > content lgtm % rockot review
> 
> Thanks!
> 
> > Are there tests for the payment stuff in chrome? Or are there already tests
> that
> > don't need to be updated for this CL?
> 
> I think there are plenty of tests for this. One example is the many java tests
> with this test base:
>
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...
> 
> There are C++ tests too. I am sure Rouslan can point them out if you want. I
> believe payment tests not regressing/manual testing showing up no problems
> is proof enough and we do not need new tests as we are simply changing one
> internal mojo parameter (WebContents -> RFH), end result should be the same.
> 
> My manual testing showed up no problems on some payment request demo sites, so
I
> have confidence the new mojo lines are doing their job.

Agreed that we have enough existing test coverage for this.

[Ken Rockot(use gerrit already), 2017-03-13 17:24:25]

rockot@chromium.org changed reviewers:
+ sammc@chromium.org

[Ken Rockot(use gerrit already), 2017-03-13 17:24:25]

l-g-t-m but +sammc for a sanity check first. I don't recall if there was any
reason not to add a Java registrar for RFH previously, or if we just didn't have
a need for one.

[Sam McNally, 2017-03-13 23:35:10]

LGTM

On 2017/03/13 17:24:25, Ken Rockot wrote:
> l-g-t-m but +sammc for a sanity check first. I don't recall if there was any
> reason not to add a Java registrar for RFH previously, or if we just didn't
have
> a need for one.

At the time there wasn't a Java RFH.

[rwlbuis, 2017-03-13 23:39:09]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2017-03-13 23:39:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2692023002/#ps240001
(title: "Rebase after https://codereview.chromium.org/2708243004")

[commit-bot: I haz the power, 2017-03-13 23:39:57]

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-03-13 23:50:19]

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

[commit-bot: I haz the power, 2017-03-13 23:50:21]

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...)

[rwlbuis, 2017-03-14 02:09:47]

@jochen PTAL these 2 files:
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeInterfaceRegistrar.java
content/public/browser/render_frame_host.h

[rwlbuis, 2017-03-14 10:32:03]

rob.buis@samsung.com changed reviewers:
+ jochen@chromium.org

[rwlbuis, 2017-03-14 17:47:20]

rob.buis@samsung.com changed reviewers:
+ jam@chromium.org
- jochen@chromium.org

[rwlbuis, 2017-03-14 17:47:21]

@jam PTAL at this public content change:
content/public/browser/render_frame_host.h

[rwlbuis, 2017-03-14 19:08:26]

On 2017/03/14 17:47:21, rwlbuis wrote:
> @jam PTAL at this public content change:
> content/public/browser/render_frame_host.h
Oh and this one:
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeInterfaceRegistrar.java

[rwlbuis, 2017-03-15 02:12:01]

rob.buis@samsung.com changed reviewers:
+ jochen@chromium.org

[rwlbuis, 2017-03-15 02:12:02]

Adding jochen back, it seems I removed him too soon :)

[jochen (gone - plz use gerrit), 2017-03-15 15:49:31]

On 2017/03/15 at 02:12:02, rob.buis wrote:
> Adding jochen back, it seems I removed him too soon :)

Please ask an owner that is closer to ChromeInterfaceRegistrar.java to approve
that part.

I'm deferring to jam@ for the content/ changes

[rwlbuis, 2017-03-15 15:56:23]

On 2017/03/15 15:49:31, jochen wrote:
> On 2017/03/15 at 02:12:02, rob.buis wrote:
> > Adding jochen back, it seems I removed him too soon :)
> 
> Please ask an owner that is closer to ChromeInterfaceRegistrar.java to approve
> that part.
> 
> I'm deferring to jam@ for the content/ changes

Ok thanks, I'll try to find someone.

[rwlbuis, 2017-03-15 15:58:39]

rob.buis@samsung.com changed reviewers:
+ tedchoc@chromium.org
- jochen@chromium.org

[rwlbuis, 2017-03-15 15:58:40]

Adding tedchoc for:
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeInterfaceRegistrar.java

[jam, 2017-03-15 17:12:11]

https://codereview.chromium.org/2692023002/diff/240001/content/browser/androi...
File content/browser/android/java_interfaces_impl.cc (right):

https://codereview.chromium.org/2692023002/diff/240001/content/browser/androi...
content/browser/android/java_interfaces_impl.cc:70:
render_frame_host->GetJavaRenderFrameHost().obj());
since this new method is only called from inside content, put it on
RenderFrameHostImpl

[rwlbuis, 2017-03-15 17:26:24]

Patchset #10 (id:260001) has been deleted

[rwlbuis, 2017-03-15 17:52:53]

PTAL.

https://codereview.chromium.org/2692023002/diff/240001/content/browser/androi...
File content/browser/android/java_interfaces_impl.cc (right):

https://codereview.chromium.org/2692023002/diff/240001/content/browser/androi...
content/browser/android/java_interfaces_impl.cc:70:
render_frame_host->GetJavaRenderFrameHost().obj());
On 2017/03/15 17:12:11, jam wrote:
> since this new method is only called from inside content, put it on
> RenderFrameHostImpl

Done.

[Ted C, 2017-03-15 23:43:29]

On 2017/03/15 15:58:40, rwlbuis wrote:
> Adding tedchoc for:
>
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeInterfaceRegistrar.java

lgtm - ChromeInterfaceRegistrar.java

[jam, 2017-03-16 15:39:58]

lgtm

[please use gerrit instead, 2017-03-16 15:43:14]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-03-16 15:43:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, boliu@chromium.org, sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2692023002/#ps280001
(title: "Address review comment")

[commit-bot: I haz the power, 2017-03-16 15:43:48]

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-03-16 17:12:10]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1489678994803420,
"parent_rev": "22bef933a34b9938f90d834759e49a8e9dd2ad05", "commit_rev":
"6e62a5770c75bce60af3a8f91866e2ea7dcbf3bf"}

[commit-bot: I haz the power, 2017-03-16 17:12:51]

Description was changed from

==========
Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with the newly introduced RenderFrameHost
in order to know the (sub)frame that created the payment request.

This CL also adds WebContentsStatics which should contain public static
methods for WebContents.

BUG=620173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001
(http://crrev.com/2681933002#ps100001)
==========

to

==========
Make PaymentRequestImpl work with RenderFrameHost

Make PaymentRequestImpl work with the newly introduced RenderFrameHost
in order to know the (sub)frame that created the payment request.

This CL also adds WebContentsStatics which should contain public static
methods for WebContents.

BUG=620173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

patch from issue 2681933002 at patchset 100001
(http://crrev.com/2681933002#ps100001)

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

[commit-bot: I haz the power, 2017-03-16 17:12:53]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/6e62a5770c75bce60af3a8f91866...