Implement the plumbing to handle messages in a separate handler

Implement the plumbing to handle messages in a separate handler

Provide the mechanism to allow applications to provide their own handler
for receiving postmessages on a thread separate than UI thread.

BUG=393291
Committed: https://crrev.com/65b9392697befefd009dbe73d75dfe66894d814c
Cr-Commit-Position: refs/heads/master@{#318589}

[sgurun-gerrit only, 2015-02-27 22:27:50]

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

[sgurun-gerrit only, 2015-02-27 22:27:59]

On 2015/02/27 22:27:50, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:hush@chromium.org

PTAL, thanks

[sgurun-gerrit only, 2015-02-27 22:28:00]

On 2015/02/27 22:27:50, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:hush@chromium.org

PTAL, thanks

[hush (inactive), 2015-02-28 01:28:35]

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

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:140:
private void onPostMessage(int portId, String message, int[] ports) {
The method name feels unclear to me... (not sure if it is for sending messages
or receiving messages judging from its name)

This is called when Blink posts a message to the message port whose id is
|portId| right?
How about something explicit like onPortReceivesMessage?

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

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:95:
private MessageHandler mHandler;
I'm confused by the difference between "mHandler" and "mWebEventHandler" until I
read the design doc.
So this mHandler is the handler passed in by the user to do something when a
MessagePort receives a WebMessage?

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:120:
synchronized (mLock) {
setting the boolean is by default atomic, so you don't need to synchronize?

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:145:
public void setWebEventHandler(WebEventHandler webEventHandler, Handler handler)
{
so we only use the looper for the handler. It is worth mentioning in the public
API that if the developer implements the handleMessage themselves, it will be
ignored.

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:155:
public void onPostMessage(String message) {
same comment about the name in AwMessagePortService... 
this method is basically trying to post the real handler onto the handler thread
(either UI or background thread specified by the user)

How about changing the name to something that describe this behavior so that it
doesn't confuse with onMessage below?

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:164: //
Maybe called on a different thread then UI thread.
Maybe->"May be"
then->"than"

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:168:
Log.w(TAG, "Port [" + mPortId + "] received message in closed state");
do you plan to leave these logs in production?

[sgurun-gerrit only, 2015-02-28 02:15:56]

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

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:140:
private void onPostMessage(int portId, String message, int[] ports) {
On 2015/02/28 01:28:35, hush wrote:
> The method name feels unclear to me... (not sure if it is for sending messages
> or receiving messages judging from its name)
> 
> This is called when Blink posts a message to the message port whose id is
> |portId| right?
> How about something explicit like onPortReceivesMessage?

renamed to onReceivedMessage

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

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:95:
private MessageHandler mHandler;
Yes. Handler is an android OS handler that would be passed by the embedder app
(if desired) to receive messages in a separate thread.

On 2015/02/28 01:28:35, hush wrote:
> I'm confused by the difference between "mHandler" and "mWebEventHandler" until
I
> read the design doc.
> So this mHandler is the handler passed in by the user to do something when a
> MessagePort receives a WebMessage?

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:120:
synchronized (mLock) {
On 2015/02/28 01:28:35, hush wrote:
> setting the boolean is by default atomic, so you don't need to synchronize?

I think this is safer. we could use volatile but I think debugging less is
beneficial for everybody, and there is not any real performance gains since this
code is not on data path.

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:145:
public void setWebEventHandler(WebEventHandler webEventHandler, Handler handler)
{
On 2015/02/28 01:28:35, hush wrote:
> so we only use the looper for the handler. It is worth mentioning in the
public
> API that if the developer implements the handleMessage themselves, it will be
> ignored.

no I think it would be confusing. They would see nothing about the
implementation.

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:155:
public void onPostMessage(String message) {
On 2015/02/28 01:28:35, hush wrote:
> same comment about the name in AwMessagePortService... 
> this method is basically trying to post the real handler onto the handler
thread
> (either UI or background thread specified by the user)

is it posting *to* the event handler.

> 
> How about changing the name to something that describe this behavior so that
it
> doesn't confuse with onMessage below?
onReceivedMessage is probably better. renamed.

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:164: //
Maybe called on a different thread then UI thread.
On 2015/02/28 01:28:35, hush wrote:
> Maybe->"May be"
> then->"than"

Done.

https://codereview.chromium.org/961393002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:168:
Log.w(TAG, "Port [" + mPortId + "] received message in closed state");
On 2015/02/28 01:28:35, hush wrote:
> do you plan to leave these logs in production?

yes. these are real error/warning conditions that show programming error of the
developer.

[hush (inactive), 2015-02-28 02:28:21]

lgtm

[hush (inactive), 2015-02-28 02:34:47]

On 2015/02/28 02:28:21, hush wrote:
> lgtm

except some better names for onMessage and onReceivedMessage on MessagePort
class

[sgurun-gerrit only, 2015-02-28 02:44:42]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-02-28 02:45:10]

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

[commit-bot: I haz the power, 2015-02-28 03:40:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-28 03:42:55]

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