Add a content API to create a message channel.

Add a content API to create a message channel.

BUG=393291
Add a content API to create a message channel. Android Webview will use this channel for communicating to JS. Also move postmessage related logic to its own class from webcontents.

Committed: https://crrev.com/4780212cc71a0297d5fd5818eff783aea661f2dc
Cr-Commit-Position: refs/heads/master@{#304873}

[sgurun-gerrit only, 2014-07-12 00:07:34]

Hello Marja,

can you please take a look at this? I will be adding tests in a next patch.

[marja, 2014-07-14 08:02:00]

+jam 

I'm not familiar enough with how this is supposed to work on Android or with
your overall design. (Tests and an overall description of the design would make
it easier to understand.)

I suppose jam@ would be a better person to review.

The patch kinda looks reasonable but it's hard to say what are all the places
that need plumbing, and what are the modification points.

[marja, 2014-07-14 08:15:35]

Humm, I noticed you have a test in your other CL though:

https://codereview.chromium.org/332693004

[sgurun-gerrit only, 2014-07-14 14:46:45]

On 2014/07/14 08:15:35, marja wrote:
> Humm, I noticed you have a test in your other CL though:
> 
> https://codereview.chromium.org/332693004

There is an internal design document, I will email it to you privately. I think
it should not be a problem to carry most of it to public domain, I can do it
tomorrow.

[mnaganov (inactive), 2014-07-14 16:06:22]

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:177:
ScopedJavaGlobalRef<jobject>* callback,
Who owns the scoped callback ref? Do we need to delete it in this method?

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.h:169: jobject obj,
nit: indentation

[jam, 2014-07-15 00:38:17]

content/browser/message_port_service.cc lgtm. for the other files in android
directories, I defer to owners there

[sgurun-gerrit only, 2014-07-15 22:19:54]

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:177:
ScopedJavaGlobalRef<jobject>* callback,
We pass it as a owned_ptr. It is deleted once calback is deleted.

On 2014/07/14 16:06:22, Mikhail Naganov (Cr) wrote:
> Who owns the scoped callback ref? Do we need to delete it in this method?

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/387113003/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.h:169: jobject obj,
On 2014/07/14 16:06:22, Mikhail Naganov (Cr) wrote:
> nit: indentation

Done.

[sgurun-gerrit only, 2014-07-15 22:23:57]

yfriedman need review for everything except
content/browser/message_port_service.cc

[sgurun-gerrit only, 2014-07-17 23:46:01]

On 2014/07/15 22:23:57, sgurun wrote:
> yfriedman need review for everything except
> content/browser/message_port_service.cc

@yaron, ping.

[sgurun-gerrit only, 2014-07-18 00:22:31]

On 2014/07/17 23:46:01, sgurun wrote:
> On 2014/07/15 22:23:57, sgurun wrote:
> > yfriedman need review for everything except
> > content/browser/message_port_service.cc
> 
> @yaron, ping.

If you have the same concerns here that content_view_core_impl is getting larger
with aw specific code, as an alternative, I can try to create a public API for
message_port_service, if its owners accept.

[Yaron, 2014-07-18 21:54:00]

Yes, I have the same concerns about unnecessary bloating CVC for aw-specific
functionality. In this case, I think even independent of aw, it looks like
MessagePortService is a standalone element so it's cleaner to work with directly
than piling on top of CVC

https://codereview.chromium.org/387113003/diff/60001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/387113003/diff/60001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1459: int port1;
These are stack allocated and are passing to another thread and back and the
callback will execute after this frame. I think instead you can new them and use
base::Owned on the reply to give the callback ownership.

https://codereview.chromium.org/387113003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/387113003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1404:
public void createMessageChannel(ValueCallback<MessageChannel> callback) {
Please add a test for this API

[sgurun-gerrit only, 2014-10-28 00:41:10]

sgurun@chromium.org changed reviewers:
+ sky@chromium.org, tedchoc@chromium.org

[sgurun-gerrit only, 2014-10-28 00:41:10]

sgurun@chromium.org changed required reviewers:
+ sky@chromium.org, tedchoc@chromium.org

[sgurun-gerrit only, 2014-10-28 00:41:11]

Hello,

Since code & owners have changed while the CL was idle, I changed the reviewers:

sky@ please review content/browser/message_port_service.cc
tedchoc@ please review everything else
all other reviewers, FYI

Thanks,

https://codereview.chromium.org/387113003/diff/60001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/387113003/diff/60001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1459: int port1;
On 2014/07/18 21:54:00, Yaron wrote:
> These are stack allocated and are passing to another thread and back and the
> callback will execute after this frame. I think instead you can new them and
use
> base::Owned on the reply to give the callback ownership.

Done.

https://codereview.chromium.org/387113003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/387113003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1404:
public void createMessageChannel(ValueCallback<MessageChannel> callback) {
On 2014/07/18 21:54:00, Yaron wrote:
> Please add a test for this API

Done.

[sky, 2014-10-28 02:04:29]

sky@chromium.org changed required reviewers:
- sky@chromium.org

[sky, 2014-10-28 02:04:29]

I'm not a good reviewer for this one.

-sky

[sky, 2014-10-28 02:04:40]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sgurun-gerrit only, 2014-10-28 19:22:04]

sgurun@chromium.org changed reviewers:
+ nasko@chromium.org

[sgurun-gerrit only, 2014-10-28 19:22:04]

sgurun@chromium.org changed required reviewers:
+ nasko@chromium.org

[sgurun-gerrit only, 2014-10-28 19:22:35]

On 2014/10/28 02:04:29, sky wrote:
> I'm not a good reviewer for this one.
> 
> -sky

@nasko: please review content/browser/message_port_service.cc

Thanks!

[nasko, 2014-10-28 22:39:57]

You already have LGTM from jam@ for content/browser/message_port_service.cc, so
no need for one from me.

[Ted C, 2014-10-28 22:44:06]

On 2014/10/28 00:41:11, sgurun wrote:
> Hello,
> 
> Since code & owners have changed while the CL was idle, I changed the
reviewers:
> 
> sky@ please review content/browser/message_port_service.cc
> tedchoc@ please review everything else
> all other reviewers, FYI
> 
> Thanks,
> 
>
https://codereview.chromium.org/387113003/diff/60001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/387113003/diff/60001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:1459: int port1;
> On 2014/07/18 21:54:00, Yaron wrote:
> > These are stack allocated and are passing to another thread and back and the
> > callback will execute after this frame. I think instead you can new them and
> use
> > base::Owned on the reply to give the callback ownership.
> 
> Done.
> 
>
https://codereview.chromium.org/387113003/diff/60001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/387113003/diff/60001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1404:
> public void createMessageChannel(ValueCallback<MessageChannel> callback) {
> On 2014/07/18 21:54:00, Yaron wrote:
> > Please add a test for this API
> 
> Done.

What parts of the content API are not public that are preventing you from
putting
this entirely within the aw package?

And if this must remain in the shared content/public directory, then please do
not add them to WebContents directly.  Add a separate class that manages this
behavior for you.

WebContents.java/web_contents_android should have API signatures that are
as close as possible to web_contents.h.  For additional functionality, they
should
live in a separate class (for example here you would simply pass in a
WebContents.java).

[sgurun-gerrit only, 2014-10-28 23:28:22]

> 
> What parts of the content API are not public that are preventing you from
> putting
> this entirely within the aw package?

MessagePortService and its friends (messageportmessagefilter) are not public.

> 
> And if this must remain in the shared content/public directory, then please do
> not add them to WebContents directly.  Add a separate class that manages this
> behavior for you.
> 
> WebContents.java/web_contents_android should have API signatures that are
> as close as possible to web_contents.h.  For additional functionality, they
> should
> live in a separate class (for example here you would simply pass in a
> WebContents.java).
Ok I will try to split this up.

[sgurun-gerrit only, 2014-11-04 00:58:02]

sgurun@chromium.org changed reviewers:
+ jochen@chromium.org

[sgurun-gerrit only, 2014-11-04 00:58:03]

sgurun@chromium.org changed required reviewers:
- nasko@chromium.org

[sgurun-gerrit only, 2014-11-04 00:58:27]

On 2014/10/28 23:28:22, sgurun wrote:
> > 
> > What parts of the content API are not public that are preventing you from
> > putting
> > this entirely within the aw package?
> 
> MessagePortService and its friends (messageportmessagefilter) are not public.
> 
> > 
> > And if this must remain in the shared content/public directory, then please
do
> > not add them to WebContents directly.  Add a separate class that manages
this
> > behavior for you.
> > 
> > WebContents.java/web_contents_android should have API signatures that are
> > as close as possible to web_contents.h.  For additional functionality, they
> > should
> > live in a separate class (for example here you would simply pass in a
> > WebContents.java).
> Ok I will try to split this up.

tedchoc@: Please review content/public/android and content/browser/web_contents
jochen@ Please review content/browser/message_port_provider*    (since Nasko
seems on leave now)
mnaganov: Please review aw/*

[sgurun-gerrit only, 2014-11-04 01:01:55]

On 2014/11/04 00:58:27, sgurun wrote:
> On 2014/10/28 23:28:22, sgurun wrote:
> > > 
> > > What parts of the content API are not public that are preventing you from
> > > putting
> > > this entirely within the aw package?
> > 
> > MessagePortService and its friends (messageportmessagefilter) are not
public.
> > 
> > > 
> > > And if this must remain in the shared content/public directory, then
please
> do
> > > not add them to WebContents directly.  Add a separate class that manages
> this
> > > behavior for you.
> > > 
> > > WebContents.java/web_contents_android should have API signatures that are
> > > as close as possible to web_contents.h.  For additional functionality,
they
> > > should
> > > live in a separate class (for example here you would simply pass in a
> > > WebContents.java).
> > Ok I will try to split this up.
> 
> tedchoc@: Please review content/public/android and
content/browser/web_contents
> jochen@ Please review content/browser/message_port_provider*    (since Nasko
> seems on leave now)
> mnaganov: Please review aw/*

jochen@ Please also review content/public/browser/message_port_provider.h

[mnaganov (inactive), 2014-11-04 02:34:36]

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:54:
if (!mReady) mLock.wait(TIMEOUT);
Please use ScalableTimeout.scaleTimeout to make the timeout scalable.

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:99:
// Wait until js object is available
This approach to waiting doesn't look reliable. In the Java Bridge tests we use
TestCallbackHelperContainer.OnPageFinishedHelper.

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:121:
// we could do a more through verification.
typo: thorough

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:126:
final int[] msgPorts = new int[1];
This array is not used.

[Ted C, 2014-11-04 18:31:43]

On 2014/11/04 02:34:36, mnaganov (cr) wrote:
>
https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
> (right):
> 
>
https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:54:
> if (!mReady) mLock.wait(TIMEOUT);
> Please use ScalableTimeout.scaleTimeout to make the timeout scalable.
> 
>
https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:99:
> // Wait until js object is available
> This approach to waiting doesn't look reliable. In the Java Bridge tests we
use
> TestCallbackHelperContainer.OnPageFinishedHelper.
> 
>
https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:121:
> // we could do a more through verification.
> typo: thorough
> 
>
https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:126:
> final int[] msgPorts = new int[1];
> This array is not used.

content/android - lgtm

[sgurun-gerrit only, 2014-11-05 01:03:02]

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:54:
if (!mReady) mLock.wait(TIMEOUT);
On 2014/11/04 02:34:36, mnaganov (cr) wrote:
> Please use ScalableTimeout.scaleTimeout to make the timeout scalable.

Done.

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:99:
// Wait until js object is available
On 2014/11/04 02:34:36, mnaganov (cr) wrote:
> This approach to waiting doesn't look reliable. In the Java Bridge tests we
use
> TestCallbackHelperContainer.OnPageFinishedHelper.

Done.

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:121:
// we could do a more through verification.
On 2014/11/04 02:34:36, mnaganov (cr) wrote:
> typo: thorough

Done.

https://codereview.chromium.org/387113003/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:126:
final int[] msgPorts = new int[1];
On 2014/11/04 02:34:36, mnaganov (cr) wrote:
> This array is not used.

Done.

[mnaganov (inactive), 2014-11-05 01:31:55]

android_webview/ LGTM

[sgurun-gerrit only, 2014-11-17 22:37:50]

On 2014/11/05 01:31:55, mnaganov (cr) wrote:
> android_webview/ LGTM

jochen@ ping,

[jochen (gone - plz use gerrit), 2014-11-18 13:29:39]

jochen@chromium.org changed required reviewers:
- tedchoc@chromium.org

[jochen (gone - plz use gerrit), 2014-11-18 13:29:41]

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:14: #include
"content/public/browser/web_contents.h"
files in content/ should use the _impl.h versions

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:14: #include
"content/public/browser/web_contents.h"
files in content/ should use the _impl.h versions

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:16: using base::string16;
please don't add using for base:: types

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:54: int* port1, int* port2) {
is this clang-formated?

https://codereview.chromium.org/387113003/diff/120001/content/public/browser/...
File content/public/browser/message_port_provider.h (right):

https://codereview.chromium.org/387113003/diff/120001/content/public/browser/...
content/public/browser/message_port_provider.h:23: static void
PostMessageToFrame(WebContents* web_contents,
please document what these methods do and how you're supposed to call them

https://codereview.chromium.org/387113003/diff/120001/content/public/browser/...
content/public/browser/message_port_provider.h:24: base::string16*
source_origin,
why aren't these const refs?

[sgurun-gerrit only, 2014-11-18 22:51:55]

PTAL, thanks,

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:14: #include
"content/public/browser/web_contents.h"
On 2014/11/18 13:29:41, jochen (slow) wrote:
> files in content/ should use the _impl.h versions

Done.

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:14: #include
"content/public/browser/web_contents.h"
On 2014/11/18 13:29:41, jochen (slow) wrote:
> files in content/ should use the _impl.h versions

Done.

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:16: using base::string16;
On 2014/11/18 13:29:41, jochen (slow) wrote:
> please don't add using for base:: types

Done.

https://codereview.chromium.org/387113003/diff/120001/content/browser/message...
content/browser/message_port_provider.cc:54: int* port1, int* port2) {
On 2014/11/18 13:29:41, jochen (slow) wrote:
> is this clang-formated?

Done.

https://codereview.chromium.org/387113003/diff/120001/content/public/browser/...
File content/public/browser/message_port_provider.h (right):

https://codereview.chromium.org/387113003/diff/120001/content/public/browser/...
content/public/browser/message_port_provider.h:23: static void
PostMessageToFrame(WebContents* web_contents,
On 2014/11/18 13:29:41, jochen (slow) wrote:
> please document what these methods do and how you're supposed to call them

Done.

https://codereview.chromium.org/387113003/diff/120001/content/public/browser/...
content/public/browser/message_port_provider.h:24: base::string16*
source_origin,
On 2014/11/18 13:29:41, jochen (slow) wrote:
> why aren't these const refs?

Done.

[jochen (gone - plz use gerrit), 2014-11-19 08:49:20]

lgtm

https://codereview.chromium.org/387113003/diff/140001/content/public/browser/...
File content/public/browser/message_port_provider.h (right):

https://codereview.chromium.org/387113003/diff/140001/content/public/browser/...
content/public/browser/message_port_provider.h:33: // Creates a message channel
and provide the ids of the message ports that are
nit empty line above this line

[sgurun-gerrit only, 2014-11-19 19:15:48]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2014-11-19 19:16:21]

thanks all!

https://codereview.chromium.org/387113003/diff/140001/content/public/browser/...
File content/public/browser/message_port_provider.h (right):

https://codereview.chromium.org/387113003/diff/140001/content/public/browser/...
content/public/browser/message_port_provider.h:33: // Creates a message channel
and provide the ids of the message ports that are
On 2014/11/19 08:49:20, jochen (slow) wrote:
> nit empty line above this line

Done.

[commit-bot: I haz the power, 2014-11-19 19:17:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/387113003/160001

[commit-bot: I haz the power, 2014-11-19 20:17:05]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-11-19 20:18:07]

Patchset 9 (id:??) landed as
https://crrev.com/4780212cc71a0297d5fd5818eff783aea661f2dc
Cr-Commit-Position: refs/heads/master@{#304873}