Return created channels synchronously

Return created channels synchronously

This CL's focus is to return the channels synchronously. A next CL will introduce a close() api to message ports so resources can be freed when not used, and then will change the way we post messages in background thread.
At the receive side, we need to deal with messages received when a port is being transferred. this is for a next cl.

The channel ports are created in a different thread. This CL introduces an internal "pending" state that is not visible to users to hide the fact that channels are not ready yet.

BUG=393291
Committed: https://crrev.com/2d72bc0ea342dd43b5ba9593efb585c3c8bf8473
Cr-Commit-Position: refs/heads/master@{#316243}

[sgurun-gerrit only, 2015-02-12 20:46:48]

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

[sgurun-gerrit only, 2015-02-12 20:47:00]

On 2015/02/12 20:46:48, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:hush@chromium.org

ptal, thanks

[hush (inactive), 2015-02-13 03:25:08]

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1796: *
@param msgPorts The sent message ports, if any. Pass null if there is no
msgPorts --> sentPorts

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1802: if
(mPostMessageSender == null) {
Just to make it clear here:

mPostMessageSender is only responsible for sending messages to frames. Sending
messages from port 1 to port 2 are done by the ports themselves, because the
ports have their own postMessageSenders (even though AwContents might have
created a message channel that consists of port 1 and port 2)

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1813:
String sourceOrigin, String targetOrigin, int[] sentPorts) {
so the subtle difference between postMessageToFrame is we're sending the port
ids instead of messageport objects in this method.
Could you rename "sentPorts" to "sentPortIds"?

This method basically "postMessageForReal", right?

Anyways, please add a description for this method.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(left):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:127:
p.close();
why is this removed?

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:45:
* Observer for MessageChannel events.
Do you plan to support observing more events other than creation?

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/MessagePort.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:25: * A
message port should be closed by the using app when it is not needed any more.
This will free
"closed by the app"?

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:30: *
port cannot send messages and any received messages will be queued. This state
is not visible
The app cannot send messages from the transferred port. And any received
messages will be queued while the port is being transferred.
I guess that's what you mean.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:7:
import java.util.ArrayDeque;
please add a new line after this.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:85:
mShouldQueue = true;
I feel it is weird that a method that looks like its reading an internal state
actually changes the internal state.
mShouldQueue is the same as mMessageQueue.size() == 0 right? Maybe we could get
rid of this state.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:108:
public void postMessage(String frameName, String message, String sourceOrigin,
name it postMessageIfPossible?

https://codereview.chromium.org/920873003/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:263:
channel[0].postMessage("1", null);
This is testing you can postMessage from a closed port.
a similar test would be channel[1].postMessage("1", null) to verify you can't
postMessage to a closed port.

[sgurun-gerrit only, 2015-02-13 05:11:59]

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1796: *
@param msgPorts The sent message ports, if any. Pass null if there is no
On 2015/02/13 03:25:08, hush wrote:
> msgPorts --> sentPorts

Done.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1802: if
(mPostMessageSender == null) {
On 2015/02/13 03:25:08, hush wrote:
> Just to make it clear here:
> 
> mPostMessageSender is only responsible for sending messages to frames. Sending
> messages from port 1 to port 2 are done by the ports themselves, because the
> ports have their own postMessageSenders (even though AwContents might have
> created a message channel that consists of port 1 and port 2)

yes, exactly. Messages can be posted either from a webview directly to a frame,
(or main frame) or using a message port of a message channel.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1813:
String sourceOrigin, String targetOrigin, int[] sentPorts) {
On 2015/02/13 03:25:08, hush wrote:
> so the subtle difference between postMessageToFrame is we're sending the port
> ids instead of messageport objects in this method.
> Could you rename "sentPorts" to "sentPortIds"?
> 
done
> This method basically "postMessageForReal", right?
> 
yes.
> Anyways, please add a description for this method.
done (at the public interface)/

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(left):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:127:
p.close();
On 2015/02/13 03:25:08, hush wrote:
> why is this removed?

I will implement the close() functionality in the next CL. Basically, I am
thinking we may not even need to close it here, because it is already in
transferred state, and any data structure allocated will be released here (and
in messageportservice) during transfer, but leaving to next CL for real.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:45:
* Observer for MessageChannel events.
On 2015/02/13 03:25:08, hush wrote:
> Do you plan to support observing more events other than creation?

not at this time.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/MessagePort.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:25: * A
message port should be closed by the using app when it is not needed any more.
This will free
On 2015/02/13 03:25:08, hush wrote:
> "closed by the app"?

Done.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:30: *
port cannot send messages and any received messages will be queued. This state
is not visible
On 2015/02/13 03:25:08, hush wrote:
> The app cannot send messages from the transferred port. And any received
> messages will be queued while the port is being transferred.
> I guess that's what you mean.

clarified.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:7:
import java.util.ArrayDeque;
On 2015/02/13 03:25:08, hush wrote:
> please add a new line after this.

Done.

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:85:
mShouldQueue = true;
On 2015/02/13 03:25:08, hush wrote:
> I feel it is weird that a method that looks like its reading an internal state
> actually changes the internal state.

this is a private method so I did not worry too much about it. I will see if it
looks better to move to another location.

> mShouldQueue is the same as mMessageQueue.size() == 0 right? Maybe we could
get
> rid of this state.

sure, good point. (however it is mMessageQueue.size() > 0

https://codereview.chromium.org/920873003/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/PostMessageSender.java:108:
public void postMessage(String frameName, String message, String sourceOrigin,
On 2015/02/13 03:25:08, hush wrote:
> name it postMessageIfPossible?

I think the name is compatible with HTML5 MessagePort though. Also it posts the
message one way or another (the message is not dropped:)

https://codereview.chromium.org/920873003/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/920873003/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:263:
channel[0].postMessage("1", null);
On 2015/02/13 03:25:08, hush wrote:
> This is testing you can postMessage from a closed port.
> a similar test would be channel[1].postMessage("1", null) to verify you can't
> postMessage to a closed port.

If I understand correctly, you are asking what happens if a message is posted to
a channel, where the target port is closed. In this case, the message gets lost,
no exception is thrown.

[sgurun-gerrit only, 2015-02-13 05:12:22]

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

[sgurun-gerrit only, 2015-02-13 05:12:53]

Mikhail, can you also review as a second eye? thanks!

[mnaganov (inactive), 2015-02-13 10:33:31]

lgtm

[sgurun-gerrit only, 2015-02-13 14:42:23]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-02-13 14:43:33]

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

[commit-bot: I haz the power, 2015-02-13 15:19:02]

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

[commit-bot: I haz the power, 2015-02-13 15:19:03]

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

[sgurun-gerrit only, 2015-02-13 17:25:43]

New patchsets have been uploaded after l-g-t-m from mnaganov@chromium.org

[sgurun-gerrit only, 2015-02-13 17:50:25]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-02-13 17:51:42]

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

[commit-bot: I haz the power, 2015-02-13 18:11:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-13 18:13:04]

Patchset 3 (id:??) landed as
https://crrev.com/2d72bc0ea342dd43b5ba9593efb585c3c8bf8473
Cr-Commit-Position: refs/heads/master@{#316243}