Add Java wrapper for RenderFrameHost

Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

Review-Url: https://codereview.chromium.org/2681933002
Cr-Original-Commit-Position: refs/heads/master@{#455635}
Committed: https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff07740984b2389f43
Review-Url: https://codereview.chromium.org/2681933002
Cr-Commit-Position: refs/heads/master@{#456152}
Committed: https://chromium.googlesource.com/chromium/src/+/300b08741f54d52bbdf1c6e400f54dcf64b0da34

[rwlbuis, 2017-02-07 21:49:36]

Description was changed from

==========
RenderFrameHost WIP

BUG=
==========

to

==========
RenderFrameHost WIP

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

[rwlbuis, 2017-02-08 15:51:37]

Patchset #3 (id:40001) has been deleted

[rwlbuis, 2017-02-08 16:35:58]

Patchset #3 (id:60001) has been deleted

[rwlbuis, 2017-02-08 20:57:06]

Patchset #3 (id:80001) has been deleted

[rwlbuis, 2017-02-09 19:25:22]

Patchset #3 (id:100001) has been deleted

[rwlbuis, 2017-02-09 20:47:07]

Patchset #3 (id:120001) has been deleted

[rwlbuis, 2017-02-10 15:20:06]

Patchset #3 (id:140001) has been deleted

[rwlbuis, 2017-02-10 15:39:35]

Patchset #3 (id:160001) has been deleted

[rwlbuis, 2017-02-10 22:21:26]

Patchset #4 (id:200001) has been deleted

[rwlbuis, 2017-02-13 22:32:27]

Patchset #4 (id:220001) has been deleted

[rwlbuis, 2017-02-13 23:39:28]

Patchset #4 (id:240001) has been deleted

[rwlbuis, 2017-02-14 17:12:00]

Description was changed from

==========
RenderFrameHost WIP

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

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a GetMainFrame
method to WebContents.

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

[rwlbuis, 2017-02-14 17:13:17]

Description was changed from

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a GetMainFrame
method to WebContents.

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

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost.
It also adds a getMainFrame method to WebContents.
This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

[rwlbuis, 2017-02-14 17:13:33]

Description was changed from

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost.
It also adds a getMainFrame method to WebContents.
This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.
This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

[rwlbuis, 2017-02-14 17:13:49]

Description was changed from

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.
This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

[rwlbuis, 2017-02-14 17:18:46]

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

[rwlbuis, 2017-02-14 17:18:47]

PTAL.

Same approach as was done for WebContents is implemented here.

Two issues from my side which likely need solving:
1. I circumvented the problem that frame_host should not include web_contents
stuff using the DEPS file. I do not see another solution myself right now?
2. I did not really get too much into the issue of RenderFrameHost deletion
handling and the associated WebContents changing. AFAICS getMainFrame is always
safe to do, asking WebContents associated with a given RenderFrameHost maybe
not. Would like some pointers in this area?

[please use gerrit instead, 2017-02-14 18:31:09]

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

[please use gerrit instead, 2017-02-14 18:31:10]

Ted, who would be a good reviewer for the JNI changes?

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:70: env,
render_frame_host_->GetLastCommittedURL().spec());
Can you register a listener for render_frame_host_ destruction? That way you can
sent your own render_frame_host_ to nullptr when that happens.

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:24: // and
reflow please

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:633: RenderFrameHostAndroid*
GetRenderFrameHostAndroid();
Let's remove GetRenderFrameHostAndroid() and use render_frame_host_android_
directly, because it is used only inside of the render_frame_host_impl.cc.

https://codereview.chromium.org/2681933002/diff/280001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/280001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:22:
private WebContents mWebContents;
final

[Ted C, 2017-02-14 18:43:35]

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

[Ted C, 2017-02-14 18:43:36]

+boliu

I'm not necessarily against adding this, but would be good to know how this will
be used.

Also, we should aim at keeping the API the same as the public RenderFrameHost if
we go with this route.  So having GetWebContents seems wrong to me (it needs the
delegate which is from renderer_frame_host_impl and would not be something the
public API would have access to, at least not through those means).  getURL
should also be getLastCommittedUrl again for consistency.

But as I stated above, can you show in payments how this would be useful to
solve?  It might be the machinery being added is more than needed to solve your
usecase.

[please use gerrit instead, 2017-02-14 18:52:55]

On 2017/02/14 18:43:36, Ted C wrote: 
> I'm not necessarily against adding this, but would be good to know how this
will
> be used.

We'd like to hang PaymentRequest mojo services from the RenderFrameHost. This
will let PaumentRequestImpl know the exact sub-frame that invoked the web
payment API, which is required to pass the origin of the iframe to the payment
app.

[Ted C, 2017-02-14 18:59:05]

On 2017/02/14 18:52:55, rouslan wrote:
> On 2017/02/14 18:43:36, Ted C wrote: 
> > I'm not necessarily against adding this, but would be good to know how this
> will
> > be used.
> 
> We'd like to hang PaymentRequest mojo services from the RenderFrameHost. This
> will let PaumentRequestImpl know the exact sub-frame that invoked the web
> payment API, which is required to pass the origin of the iframe to the payment
> app.

Sorry for not being more explicit, I'm looking for code that show it
being used to verify this approach works.

[rwlbuis, 2017-02-14 19:08:15]

On 2017/02/14 18:59:05, Ted C wrote:
> On 2017/02/14 18:52:55, rouslan wrote:
> > On 2017/02/14 18:43:36, Ted C wrote: 
> > We'd like to hang PaymentRequest mojo services from the RenderFrameHost.
This
> > will let PaumentRequestImpl know the exact sub-frame that invoked the web
> > payment API, which is required to pass the origin of the iframe to the
payment
> > app.
> 
> Sorry for not being more explicit, I'm looking for code that show it
> being used to verify this approach works.

I have a rough WIP patch here that uses the new wrappers (two patches got
squashed
but I guess only the java changes need looking at, you can probably skip the
mojo stuff as well):
https://codereview.chromium.org/2692023002/

I verified that I get the correct url from the passed frame in PaymentRequesImpl
(RenderFrameHost.getUrl) and the payment sheet showed up.
From the test bots runs it seems it causes one failure which I still need to
look at.

[rwlbuis, 2017-02-14 19:26:27]

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:70: env,
render_frame_host_->GetLastCommittedURL().spec());
On 2017/02/14 18:31:10, rouslan wrote:
> Can you register a listener for render_frame_host_ destruction? That way you
can
> sent your own render_frame_host_ to nullptr when that happens.

Good idea, but I noticed just using base::SupportsUserData like WebContentsImpl
does will take care of cleanup, so I made that change.

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:24: // and
On 2017/02/14 18:31:10, rouslan wrote:
> reflow please

Done.

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2681933002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:633: RenderFrameHostAndroid*
GetRenderFrameHostAndroid();
On 2017/02/14 18:31:10, rouslan wrote:
> Let's remove GetRenderFrameHostAndroid() and use render_frame_host_android_
> directly, because it is used only inside of the render_frame_host_impl.cc.

Done.

https://codereview.chromium.org/2681933002/diff/280001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/280001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:22:
private WebContents mWebContents;
On 2017/02/14 18:31:10, rouslan wrote:
> final

Done.

[rwlbuis, 2017-02-14 21:35:40]

On 2017/02/14 18:43:36, Ted C wrote:

> Also, we should aim at keeping the API the same as the public RenderFrameHost
if
> we go with this route.  So having GetWebContents seems wrong to me (it needs
the
> delegate which is from renderer_frame_host_impl and would not be something the
> public API would have access to, at least not through those means). 

I like the rule you mentioned (keep API the same), however for getting the
WebContents
from RenderFrameHost it is not ideal:
- only solution I could see is exposing the public and static
WebContents::FromRenderFrameHost
  to Java. However for Java < 1.8 there is no support for static methods in
interfaces (seems
  we use the interfaces everywhere and not the Impl classes, at least I think
tests are not
  supposed to use the Impl classes).
- since RenderFrameHostDelegate is not public I guess it can't be made into a
Java wrapper.

Willing to try any idea though, let me know.

> getURL should also be getLastCommittedUrl again for consistency.

Done.

[boliu, 2017-02-15 02:12:51]

Sorry for the delay.. High level feedback

For changes to public (content_public for java) I would like to see actual code
that make use of that code. For this, maybe you can start with code that just
constructs the java RFH and have a test that make sue of it. Then add other
things in CLs that actually need them

Ownership of java RFH should not follow WebContents. For WebContents, it's
created and owned by content embedder. That's not the case for RFH. Eg,
RFH.destroy makes no sense to me.

Layering. RFH is conceptually one layer below WebContents, so can't include
WebContents. So follow the same pattern as the native classes, have
WebContents.fromRFH, rather than RFH.getWebContents.


Re static methods, define a parallel abstract class, eg RenderFrameHostStatics.

fwiw, we had a discussion around rules for the java content API, although I
haven’t bothered continuing the discussion further. But in case you are
interested:
https://groups.google.com/a/chromium.org/d/msg/slimming-clank/7hT8xIOqYcc/awm...
The parallel static-only abstract class idea came from there.

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
File content/browser/frame_host/DEPS (right):

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
content/browser/frame_host/DEPS:24: "+content/browser/web_contents",
this is a layering violation, as ted already pointed out

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:28: if
(!render_frame_host_android)
java side has this check already?

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:46:
base::android::ScopedJavaGlobalRef<jobject> obj_;
native strong ref is probably not ok due to webview, which relies on gc to
delete things, use JavaObjectWeakGlobalRef instead

(don't follow WebContents in this case, WebContents is wrong..)

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

https://codereview.chromium.org/2681933002/diff/340001/content/public/browser...
content/public/browser/render_frame_host.h:51: public base::SupportsUserData {
can this be on the impl class?

[rwlbuis, 2017-02-15 22:08:49]

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
File content/browser/frame_host/DEPS (right):

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
content/browser/frame_host/DEPS:24: "+content/browser/web_contents",
On 2017/02/15 02:12:51, boliu wrote:
> this is a layering violation, as ted already pointed out

Done.

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:28: if
(!render_frame_host_android)
On 2017/02/15 02:12:51, boliu wrote:
> java side has this check already?

Possibly, I don't think we need this code though so problem gone :)

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:46:
base::android::ScopedJavaGlobalRef<jobject> obj_;
On 2017/02/15 02:12:51, boliu wrote:
> native strong ref is probably not ok due to webview, which relies on gc to
> delete things, use JavaObjectWeakGlobalRef instead
> 
> (don't follow WebContents in this case, WebContents is wrong..)

Done.

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

https://codereview.chromium.org/2681933002/diff/340001/content/public/browser...
content/public/browser/render_frame_host.h:51: public base::SupportsUserData {
On 2017/02/15 02:12:51, boliu wrote:
> can this be on the impl class?

Done.

[rwlbuis, 2017-02-15 22:16:42]

On 2017/02/15 02:12:51, boliu wrote:
> Sorry for the delay.. High level feedback

Thanks for the feedback!

> For changes to public (content_public for java) I would like to see actual
code
> that make use of that code. For this, maybe you can start with code that just
> constructs the java RFH and have a test that make sue of it. Then add other
> things in CLs that actually need them

Actual code using this CL will be:
http://codereview.chromium.org/2692023002/

> Ownership of java RFH should not follow WebContents. For WebContents, it's
> created and owned by content embedder. That's not the case for RFH. Eg,
> RFH.destroy makes no sense to me.

Ok, I removed the destroy.

> Layering. RFH is conceptually one layer below WebContents, so can't include
> WebContents. So follow the same pattern as the native classes, have
> WebContents.fromRFH, rather than RFH.getWebContents.

Makes sense, done.

> Re static methods, define a parallel abstract class, eg
RenderFrameHostStatics.
> 
> fwiw, we had a discussion around rules for the java content API, although I
> haven’t bothered continuing the discussion further. But in case you are
> interested:
>
https://groups.google.com/a/chromium.org/d/msg/slimming-clank/7hT8xIOqYcc/awm...
> The parallel static-only abstract class idea came from there.

I preferred a solution where RenderFrameHost is not an interface but an abstract
base class.
However happy to add RenderFrameHostStatics class if that is preferred.
For both solutions though I think I need to make WebContentsImpl public. Is
there a way around that?

PTAL.

[boliu, 2017-02-16 01:00:59]

Ownership is tricky. Now that native only holds a weak reference to java RFH,
nothing is actually preventing it from being garbage collected. I think the
right thing is to have java WebContentsImpl hold onto it.
WebContents.getMainFrame will lazily create RFH. Probably can set up the tree
structure later when subframes are needed. Should work, in my head at least..

On 2017/02/15 22:16:42, rwlbuis wrote:
> I preferred a solution where RenderFrameHost is not an interface but an
abstract
> base class.
> However happy to add RenderFrameHostStatics class if that is preferred.
> For both solutions though I think I need to make WebContentsImpl public. Is
> there a way around that?

I think you can remove the static method altogether, this CL anyway

https://codereview.chromium.org/2681933002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:60: return NULL;
nit: nullptr everywhere

https://codereview.chromium.org/2681933002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2681933002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:235:
ScopedJavaLocalRef<jobject> FromRenderFrameHost(
this is a really convoluted way to implement this..

maybe we should have a RenderFrameHostDelegate (which I guess at this point is
empty) in java, then it has to be passed to RFH constructor, and java
WebContents can just cast it back

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:49:
/* package */ public class WebContentsImpl implements WebContents {
this doesn't need to be public, after RFH stops importing this

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

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:17:
public static WebContents getWebContents(RenderFrameHost rfh) {
this is still a layering violation

also why does this exist at all?

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:26:
public abstract String getLastCommittedURL();
remove this in this CL, nothing uses it

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

https://codereview.chromium.org/2681933002/diff/400001/content/public/browser...
content/public/browser/render_frame_host.h:238: CONTENT_EXPORT static
RenderFrameHost* FromJavaRenderFrameHost(
this should be on RenderFrameHostAndroid

[rwlbuis, 2017-02-16 22:08:37]

Patchset #12 (id:420001) has been deleted

[rwlbuis, 2017-02-28 17:45:24]

Patchset #13 (id:460001) has been deleted

[rwlbuis, 2017-02-28 21:26:08]

Patchset #13 (id:480001) has been deleted

[rwlbuis, 2017-03-01 21:53:58]

@boliu apologies, I was on vacation and then was occupied by other stuff.

>Ownership is tricky. Now that native only holds a weak reference to java RFH,
>nothing is actually preventing it from being garbage collected. I think the
>right thing is to have java WebContentsImpl hold onto it.
>WebContents.getMainFrame will lazily create RFH. Probably can set up the tree
>structure later when subframes are needed. Should work, in my head at least..

I tried to implement this the way I understand it, as well as the other
suggestions (RenderFrameHostDelegate etc.) :)

PTAL.

https://codereview.chromium.org/2681933002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:60: return NULL;
On 2017/02/16 01:00:58, boliu wrote:
> nit: nullptr everywhere

Acknowledged.

https://codereview.chromium.org/2681933002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2681933002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:235:
ScopedJavaLocalRef<jobject> FromRenderFrameHost(
On 2017/02/16 01:00:58, boliu wrote:
> this is a really convoluted way to implement this..
> 
> maybe we should have a RenderFrameHostDelegate (which I guess at this point is
> empty) in java, then it has to be passed to RFH constructor, and java
> WebContents can just cast it back

Ok I added RenderFrameHostDelegate.

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:49:
/* package */ public class WebContentsImpl implements WebContents {
On 2017/02/16 01:00:58, boliu wrote:
> this doesn't need to be public, after RFH stops importing this

Done.

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

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:17:
public static WebContents getWebContents(RenderFrameHost rfh) {
On 2017/02/16 01:00:58, boliu wrote:
> this is still a layering violation
> 
> also why does this exist at all?

It is being used in the test I added in the CL. But I removed it since the RFH
delegate idea seems better.

https://codereview.chromium.org/2681933002/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:26:
public abstract String getLastCommittedURL();
On 2017/02/16 01:00:58, boliu wrote:
> remove this in this CL, nothing uses it

This is actually an important method for Payment Request. I added it in the test
as well.

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

https://codereview.chromium.org/2681933002/diff/400001/content/public/browser...
content/public/browser/render_frame_host.h:238: CONTENT_EXPORT static
RenderFrameHost* FromJavaRenderFrameHost(
On 2017/02/16 01:00:58, boliu wrote:
> this should be on RenderFrameHostAndroid

Ok, turned out we do not need it.

[boliu, 2017-03-03 23:55:30]

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:36: RenderFrameHostImpl*
render_frame_host() const { return render_frame_host_; }
unused

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:46: RenderFrameHostImpl*
render_frame_host_;
RenderFrameHostImpl* const

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:49:
base::WeakPtrFactory<RenderFrameHostAndroid> weak_factory_;
unused

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:299:
GetJavaRenderFrameHostDelegate() = 0;
you can just default implement this to return nullptr

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:134: const char
kRenderFrameHostAndroidKey[] = "render_frame_host_android";
put this in anonymous namespace below

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:21:
RenderFrameHostDelegate mDelegate;
final, also comment that this may be null

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:47:
return nativeGetLastCommittedURL(mNativeRenderFrameHostAndroid);
need to check for 0

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

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java:15:
WebContents getAsWebContents();
mentioning WebContents here is still a layering violation. WebContentsImpl
should know enough to know when it's safe to cast this back down to
WebContentsImpl. this interface literally has no methods right now.

Also this should not be part of content_public. you can add
WebContents.fromRenderFrameHost instead if needed

[rwlbuis, 2017-03-06 20:58:58]

PTAL. Of course let me know if I got you incorrectly regarding the proposed
design.

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:36: RenderFrameHostImpl*
render_frame_host() const { return render_frame_host_; }
On 2017/03/03 23:55:29, boliu wrote:
> unused

Done.

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:46: RenderFrameHostImpl*
render_frame_host_;
On 2017/03/03 23:55:29, boliu wrote:
> RenderFrameHostImpl* const

Done.

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:49:
base::WeakPtrFactory<RenderFrameHostAndroid> weak_factory_;
On 2017/03/03 23:55:29, boliu wrote:
> unused

Done.

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:299:
GetJavaRenderFrameHostDelegate() = 0;
On 2017/03/03 23:55:29, boliu wrote:
> you can just default implement this to return nullptr

Done.

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2681933002/diff/510001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:134: const char
kRenderFrameHostAndroidKey[] = "render_frame_host_android";
On 2017/03/03 23:55:29, boliu wrote:
> put this in anonymous namespace below

Done.

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:21:
RenderFrameHostDelegate mDelegate;
On 2017/03/03 23:55:29, boliu wrote:
> final, also comment that this may be null

Done.

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:47:
return nativeGetLastCommittedURL(mNativeRenderFrameHostAndroid);
On 2017/03/03 23:55:29, boliu wrote:
> need to check for 0

Done.

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

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java:15:
WebContents getAsWebContents();
On 2017/03/03 23:55:30, boliu wrote:
> mentioning WebContents here is still a layering violation. WebContentsImpl
> should know enough to know when it's safe to cast this back down to
> WebContentsImpl. this interface literally has no methods right now.

I wanted to mirror the C++ structure here, but indeed I did that incorrectly.

> Also this should not be part of content_public. you can add
> WebContents.fromRenderFrameHost instead if needed

I moved RenderFrameHostDelegate out of content_public and introduced
WebContentsImpl.fromRenderFrameHost. Since fromRenderFrameHost needs to
be static IMO it does not fit well in WebContents.

[boliu, 2017-03-06 23:12:38]

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

https://codereview.chromium.org/2681933002/diff/510001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHostDelegate.java:15:
WebContents getAsWebContents();
On 2017/03/06 20:58:58, rwlbuis wrote:
> On 2017/03/03 23:55:30, boliu wrote:
> > mentioning WebContents here is still a layering violation. WebContentsImpl
> > should know enough to know when it's safe to cast this back down to
> > WebContentsImpl. this interface literally has no methods right now.
> 
> I wanted to mirror the C++ structure here, but indeed I did that incorrectly.
> 
> > Also this should not be part of content_public. you can add
> > WebContents.fromRenderFrameHost instead if needed
> 
> I moved RenderFrameHostDelegate out of content_public and introduced
> WebContentsImpl.fromRenderFrameHost. Since fromRenderFrameHost needs to
> be static IMO it does not fit well in WebContents.

That's fine for this CL. But if you ever need it to be public, should add a
WebContentsStatics class in content_public and add the static methods belonging
to WebContents there.

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:17: #include
"base/memory/weak_ptr.h"
remove

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:28: class CONTENT_EXPORT
RenderFrameHostAndroid
missed this last time, probably doesn't need to be exported since nothing
outside of content should be including this file? then remove the header too

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:36: RenderFrameHostImpl*
render_frame_host() const { return render_frame_host_; }
unused as in remove it

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:300: return nullptr;
can't inline this, clang bot will complain

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostDelegate.java
(right):

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostDelegate.java:17:
WebContents getAsWebContents();
this is still here?

WebContentsImpl should be casting (maybe with an instanceof check)

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:16:
// TODO(tedchoc): Remove the package restriction once this class moves to a
non-public content
remove this comment and the package part. WebContentsImpl needs to import this,
so this just can't be package period

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:160:
public static WebContents fromRenderFrameHost(RenderFrameHost rfh) {
nit: static methods at the top

[rwlbuis, 2017-03-07 19:44:56]

>> I moved RenderFrameHostDelegate out of content_public and introduced
>> WebContentsImpl.fromRenderFrameHost. Since fromRenderFrameHost needs to
>> be static IMO it does not fit well in WebContents.

>That's fine for this CL. But if you ever need it to be public, should add a
>WebContentsStatics class in content_public and add the static methods belonging
>to WebContents there.

Yes makes sense. I finally understand the layering violation now.

PTAL.

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:17: #include
"base/memory/weak_ptr.h"
On 2017/03/06 23:12:38, boliu wrote:
> remove

Done.

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:28: class CONTENT_EXPORT
RenderFrameHostAndroid
On 2017/03/06 23:12:38, boliu wrote:
> missed this last time, probably doesn't need to be exported since nothing
> outside of content should be including this file? then remove the header too

Done.

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:36: RenderFrameHostImpl*
render_frame_host() const { return render_frame_host_; }
On 2017/03/06 23:12:38, boliu wrote:
> unused as in remove it

Done.

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2681933002/diff/550001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:300: return nullptr;
On 2017/03/06 23:12:38, boliu wrote:
> can't inline this, clang bot will complain

Done.

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostDelegate.java
(right):

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostDelegate.java:17:
WebContents getAsWebContents();
On 2017/03/06 23:12:38, boliu wrote:
> this is still here?
> 
> WebContentsImpl should be casting (maybe with an instanceof check)

Done. Sorry I did not realize it was that simple! :)

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:16:
// TODO(tedchoc): Remove the package restriction once this class moves to a
non-public content
On 2017/03/06 23:12:38, boliu wrote:
> remove this comment and the package part. WebContentsImpl needs to import
this,
> so this just can't be package period

Done.

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2681933002/diff/550001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:160:
public static WebContents fromRenderFrameHost(RenderFrameHost rfh) {
On 2017/03/06 23:12:38, boliu wrote:
> nit: static methods at the top

Done.

[boliu, 2017-03-08 00:39:32]

almost there, a few small things, and dealing correctly with weakref

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:30:
ScopedJavaLocalRef<jobject> obj = Java_RenderFrameHostImpl_create(
ahh, missed this earlier..

So right now, there's no strong ref to the java RFH between the constructor here
and GetJavaObject. So in theory, it can be gc-ed between these two calls, so
that GetJavaObject will immediately return null.

Easy fix, lazy create java side in GetJavaObject. Use
JavaObjectWeakGlobalRef::is_uninitialized check instead of actually
dereferencing weakref though.

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:5: #ifndef
CONTENT_BROWSER_FRAME_HOST_ANDROID_H_
this need to match the full path, so

CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_ANDROID_H_

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:140: const char
kRenderFrameHostAndroidKey[] = "render_frame_host_android";
keys are generally declared as void* taking address of itself.

const void* const kFooKey = &kFooKey;

then it's guaranteed to be unique. feel free to write a separate CL to fix
web_contents_impl

https://codereview.chromium.org/2681933002/diff/570001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:429:
base::android::ScopedJavaLocalRef<jobject> GetJavaRenderFrameHostDelegate()
move this to the RenderFrameHostDelegate section

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

https://codereview.chromium.org/2681933002/diff/570001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:13:
* Get the last committed URL of the frame
nit: period at the end

[do_not_use, 2017-03-08 18:28:26]

rwlbuis@gmail.com changed reviewers:
+ rwlbuis@gmail.com

[do_not_use, 2017-03-08 18:28:27]

PTAL.

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:30:
ScopedJavaLocalRef<jobject> obj = Java_RenderFrameHostImpl_create(
On 2017/03/08 00:39:32, boliu wrote:
> ahh, missed this earlier..
> 
> So right now, there's no strong ref to the java RFH between the constructor
here
> and GetJavaObject. So in theory, it can be gc-ed between these two calls, so
> that GetJavaObject will immediately return null.
> 
> Easy fix, lazy create java side in GetJavaObject. Use
> JavaObjectWeakGlobalRef::is_uninitialized check instead of actually
> dereferencing weakref though.

Done, hope I did it correctly.

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.h (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.h:5: #ifndef
CONTENT_BROWSER_FRAME_HOST_ANDROID_H_
On 2017/03/08 00:39:32, boliu wrote:
> this need to match the full path, so
> 
> CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_ANDROID_H_

Done.

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:140: const char
kRenderFrameHostAndroidKey[] = "render_frame_host_android";
On 2017/03/08 00:39:32, boliu wrote:
> keys are generally declared as void* taking address of itself.
> 
> const void* const kFooKey = &kFooKey;
> 
> then it's guaranteed to be unique. feel free to write a separate CL to fix
> web_contents_impl

Done.

https://codereview.chromium.org/2681933002/diff/570001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2681933002/diff/570001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:429:
base::android::ScopedJavaLocalRef<jobject> GetJavaRenderFrameHostDelegate()
On 2017/03/08 00:39:32, boliu wrote:
> move this to the RenderFrameHostDelegate section

Done. Same in the cc.

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

https://codereview.chromium.org/2681933002/diff/570001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:13:
* Get the last committed URL of the frame
On 2017/03/08 00:39:32, boliu wrote:
> nit: period at the end

Done.

[rwlbuis, 2017-03-08 18:42:13]

rob.buis@samsung.com changed reviewers:
- rwlbuis@gmail.com

[boliu, 2017-03-08 21:43:37]

https://codereview.chromium.org/2681933002/diff/590001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/590001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:42:
ScopedJavaLocalRef<jobject> obj = Java_RenderFrameHostImpl_create(
need to declare this out of the if scope, and return from this strong ref rather
than from weak.get.

[rwlbuis, 2017-03-08 22:20:48]

PTAL.

https://codereview.chromium.org/2681933002/diff/590001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/590001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:42:
ScopedJavaLocalRef<jobject> obj = Java_RenderFrameHostImpl_create(
On 2017/03/08 21:43:37, boliu wrote:
> need to declare this out of the if scope, and return from this strong ref
rather
> than from weak.get.

I am not sure I got it 100% but please have a look at my latest patch set :)

[boliu, 2017-03-08 22:27:16]

https://codereview.chromium.org/2681933002/diff/610001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/610001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:41:
ScopedJavaLocalRef<jobject> obj;
this code is correct, but can still make it easier to read

if you put the return in the if-statement, then this doesn't need to be declared
outside of if. also rename this var "local_ref" to avoid confusion

[rwlbuis, 2017-03-08 22:51:14]

PTAL.

https://codereview.chromium.org/2681933002/diff/610001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_android.cc (right):

https://codereview.chromium.org/2681933002/diff/610001/content/browser/frame_...
content/browser/frame_host/render_frame_host_android.cc:41:
ScopedJavaLocalRef<jobject> obj;
On 2017/03/08 22:27:16, boliu wrote:
> this code is correct, but can still make it easier to read
> 
> if you put the return in the if-statement, then this doesn't need to be
declared
> outside of if. also rename this var "local_ref" to avoid confusion

Done.

[boliu, 2017-03-08 22:53:09]

lgtm

[rwlbuis, 2017-03-08 23:01:47]

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

[commit-bot: I haz the power, 2017-03-08 23:02:43]

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

[please use gerrit instead, 2017-03-08 23:12:43]

lgtm

[commit-bot: I haz the power, 2017-03-09 00:06:50]

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

[commit-bot: I haz the power, 2017-03-09 00:06:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rwlbuis, 2017-03-09 00:36:20]

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

[commit-bot: I haz the power, 2017-03-09 00:37:01]

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-09 01:56:26]

CQ is committing da patch.

Bot data: {"patchset_id": 630001, "attempt_start_ts": 1489019780159510,
"parent_rev": "62643d9574be80279c2d94868216a8cf9706c11c", "commit_rev":
"67dd9f2795389cc3776481ff07740984b2389f43"}

[commit-bot: I haz the power, 2017-03-09 01:57:28]

Description was changed from

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

Review-Url: https://codereview.chromium.org/2681933002
Cr-Commit-Position: refs/heads/master@{#455635}
Committed:
https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff0774...
==========

[commit-bot: I haz the power, 2017-03-09 01:57:30]

Committed patchset #20 (id:630001) as
https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff0774...

[jam, 2017-03-09 22:46:20]

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

[jam, 2017-03-09 22:46:22]

testWebContentsMainFrame fails with --enable-browser-side-navigation

Can you fix this please?

[jam, 2017-03-09 22:53:16]

A revert of this CL (patchset #20 id:630001) has been created in
https://codereview.chromium.org/2741043004/ by jam@chromium.org.

The reason for reverting is: The new test fails with PlzNavigate:
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

I spoke with Bo and he's ok with reverting this for now since nothing depends on
it. Please test with PlzNavigate; we're working on getting main waterfall
testers in the next day or two. In the meantime, since PlzNavigate is so close
to launching we don't want regressions. Apologies that there are no public bots
yet..

[boliu, 2017-03-09 22:54:34]

https://codereview.chromium.org/2681933002/diff/630001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java
(right):

https://codereview.chromium.org/2681933002/diff/630001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java:333:
assertEquals("RenderFrameHost has incorrect last committed URL", "about:blank",
failure is:

junit.framework.ComparisonFailure: RenderFrameHost has incorrect last committed
URL expected:<[about:blank]> but was:<[]>

should be ok to just comment this out for now

[rwlbuis, 2017-03-09 23:12:23]

On 2017/03/09 22:53:16, jam wrote:
> A revert of this CL (patchset #20 id:630001) has been created in
> https://codereview.chromium.org/2741043004/ by mailto:jam@chromium.org.
> 
> The reason for reverting is: The new test fails with PlzNavigate:
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...
> 
> I spoke with Bo and he's ok with reverting this for now since nothing depends
on
> it. Please test with PlzNavigate; we're working on getting main waterfall
> testers in the next day or two. In the meantime, since PlzNavigate is so close
> to launching we don't want regressions. Apologies that there are no public
bots
> yet..

Ok understood, thanks for the revert.

[rwlbuis, 2017-03-09 23:33:45]

On 2017/03/09 22:54:34, boliu wrote:
>
https://codereview.chromium.org/2681933002/diff/630001/content/public/android...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java
> (right):
> 
>
https://codereview.chromium.org/2681933002/diff/630001/content/public/android...
>
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java:333:
> assertEquals("RenderFrameHost has incorrect last committed URL",
"about:blank",
> failure is:
> 
> junit.framework.ComparisonFailure: RenderFrameHost has incorrect last
committed
> URL expected:<[about:blank]> but was:<[]>
> 
> should be ok to just comment this out for now

Sounds good, I made such a patch:
https://codereview.chromium.org/2745673002/

[rwlbuis, 2017-03-09 23:40:52]

On 2017/03/09 22:54:34, boliu wrote:
>
https://codereview.chromium.org/2681933002/diff/630001/content/public/android...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java
> (right):
> 
>
https://codereview.chromium.org/2681933002/diff/630001/content/public/android...
>
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java:333:
> assertEquals("RenderFrameHost has incorrect last committed URL",
"about:blank",
> failure is:
> 
> junit.framework.ComparisonFailure: RenderFrameHost has incorrect last
committed
> URL expected:<[about:blank]> but was:<[]>
> 
> should be ok to just comment this out for now

I made this patch in patch set #21, i.e. it is patch set #20 with the
assertEquals disabled and a FIXME.

[rwlbuis, 2017-03-09 23:59:12]

Reopening this again.

[rwlbuis, 2017-03-09 23:59:54]

Description was changed from

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

Review-Url: https://codereview.chromium.org/2681933002
Cr-Commit-Position: refs/heads/master@{#455635}
Committed:
https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff0774...
==========

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

Review-Url: https://codereview.chromium.org/2681933002
Cr-Commit-Position: refs/heads/master@{#455635}
Committed:
https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff0774...
==========

[boliu, 2017-03-10 00:03:42]

https://codereview.chromium.org/2681933002/diff/650001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java
(right):

https://codereview.chromium.org/2681933002/diff/650001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java:333:
// FIXME: make this work with PlzNavigate.
so.. how about actually loading some test page, and check the actual url?

also did you test this locally with the command line switch added?

[rwlbuis, 2017-03-10 19:08:54]

PTAL.

https://codereview.chromium.org/2681933002/diff/650001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java
(right):

https://codereview.chromium.org/2681933002/diff/650001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java:333:
// FIXME: make this work with PlzNavigate.
On 2017/03/10 00:03:42, boliu wrote:
> so.. how about actually loading some test page, and check the actual url?
> 
> also did you test this locally with the command line switch added?

I updated the test to load a data uri and this one works with PlzNavigate.

[boliu, 2017-03-10 19:23:15]

lgtm

[rwlbuis, 2017-03-10 19:25:08]

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

[rwlbuis, 2017-03-10 19:25:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2681933002/#ps670001
(title: "Make test work with PlzNavigate")

[commit-bot: I haz the power, 2017-03-10 19:25:52]

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-10 20:44:03]

CQ is committing da patch.

Bot data: {"patchset_id": 670001, "attempt_start_ts": 1489173908874850,
"parent_rev": "c44be6cc56995d84b6ae5c5916791bbe5ea7c947", "commit_rev":
"300b08741f54d52bbdf1c6e400f54dcf64b0da34"}

[commit-bot: I haz the power, 2017-03-10 20:44:47]

Description was changed from

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

Review-Url: https://codereview.chromium.org/2681933002
Cr-Commit-Position: refs/heads/master@{#455635}
Committed:
https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff0774...
==========

to

==========
Add Java wrapper for RenderFrameHost

This patch adds a Java wrapper for RenderFrameHost. It also adds a
getMainFrame method to WebContents.

This will be useful for Payment Request which needs to know
the exact (sub)frame issued the payment request.

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

Review-Url: https://codereview.chromium.org/2681933002
Cr-Original-Commit-Position: refs/heads/master@{#455635}
Committed:
https://chromium.googlesource.com/chromium/src/+/67dd9f2795389cc3776481ff0774...
Review-Url: https://codereview.chromium.org/2681933002
Cr-Commit-Position: refs/heads/master@{#456152}
Committed:
https://chromium.googlesource.com/chromium/src/+/300b08741f54d52bbdf1c6e400f5...
==========

[commit-bot: I haz the power, 2017-03-10 20:44:49]

Committed patchset #22 (id:670001) as
https://chromium.googlesource.com/chromium/src/+/300b08741f54d52bbdf1c6e400f5...