[mojo-bindings] Use Watch API instead of MessagePumpMojo

[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests

Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}

Committed: https://crrev.com/2cdb2f677c68f06b3bc54cd40f000ab961a09bbb
Cr-Commit-Position: refs/heads/master@{#379463}

Committed: https://crrev.com/e5d711ec0a8b327dea474d5c83dec0ae5f2c545e
Cr-Commit-Position: refs/heads/master@{#379508}

[Ken Rockot(use gerrit already), 2016-03-02 23:56:33]

I ended up making the notifications PostTask directly rather than having a
centralized per-thread dispatcher, because a batching dispatcher cannot support
nested message loops (dispatching one notification may deadlock waiting for
another notification which has already been batched and is pending dispatch).

I think this is OK for now. Please take a look!

[Ken Rockot(use gerrit already), 2016-03-02 23:57:38]

I realize I forgot gyp changes - will upload in a bit :)

[Ken Rockot(use gerrit already), 2016-03-03 00:39:44]

gyp updated

[yzshen1, 2016-03-03 16:25:32]

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:279: if
(!handle_watcher_.is_watching()) {
Does it make sense to remove the WaitToReadMore() call on line 324? It seems
unnecessary.

Can we make this "if" a DCHECK instead?

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
File mojo/public/cpp/system/tests/watcher_unittest.cc (right):

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/tests/watcher_unittest.cc:20: void
RunResultHandler(std::function<void(MojoResult)> f, MojoResult result) {
std::function is currently disallowed according to
https://chromium-cpp.appspot.com/

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
File mojo/public/cpp/system/watcher.h (right):

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:34: // Starts watching |handle|. A Watcher may
only watch one handle at time, but
at time -> at a time?

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:48: // current thread until |Cancel()| is
called or the handle is closed.
I think it is nice to also mention that destroying the Watcher object also
cancels the watch.

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:54: // passed to |Start()| will never be called
again for this Watcher
nit: trailing '.', please.

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:69: // The TaskRunner this Watcher's owning
thread. This field is safe to access
nit: "of" this Watcher's owning thread?

[Ken Rockot(use gerrit already), 2016-03-03 17:10:41]

Description was changed from

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
==========

to

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
==========

[Ken Rockot(use gerrit already), 2016-03-03 17:11:18]

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:279: if
(!handle_watcher_.is_watching()) {
On 2016/03/03 at 16:25:31, yzshen1 wrote:
> Does it make sense to remove the WaitToReadMore() call on line 324? It seems
unnecessary.
> 
> Can we make this "if" a DCHECK instead?

Ah good point. Done

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
File mojo/public/cpp/system/tests/watcher_unittest.cc (right):

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/tests/watcher_unittest.cc:20: void
RunResultHandler(std::function<void(MojoResult)> f, MojoResult result) {
On 2016/03/03 at 16:25:31, yzshen1 wrote:
> std::function is currently disallowed according to
https://chromium-cpp.appspot.com/

Woops. I've adapted the code to not reference std::function

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
File mojo/public/cpp/system/watcher.h (right):

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:34: // Starts watching |handle|. A Watcher may
only watch one handle at time, but
On 2016/03/03 at 16:25:31, yzshen1 wrote:
> at time -> at a time?

done

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:48: // current thread until |Cancel()| is
called or the handle is closed.
On 2016/03/03 at 16:25:32, yzshen1 wrote:
> I think it is nice to also mention that destroying the Watcher object also
cancels the watch.

Done

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:54: // passed to |Start()| will never be called
again for this Watcher
On 2016/03/03 at 16:25:32, yzshen1 wrote:
> nit: trailing '.', please.

Done

https://codereview.chromium.org/1759783003/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:69: // The TaskRunner this Watcher's owning
thread. This field is safe to access
On 2016/03/03 at 16:25:31, yzshen1 wrote:
> nit: "of" this Watcher's owning thread?

Done

[yzshen1, 2016-03-03 18:51:10]

lgtm

[Ken Rockot(use gerrit already), 2016-03-04 07:48:35]

Description was changed from

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
==========

to

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests
==========

[Ken Rockot(use gerrit already), 2016-03-04 07:50:19]

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

[Ken Rockot(use gerrit already), 2016-03-04 07:50:19]

Also TBRing jam@, because removing the bindings dependency on handle_watcher.h
exposed a missing #include in the new url traits test module. I've added the
include here.

[yzshen1, 2016-03-04 18:58:17]

https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
File mojo/public/cpp/system/watcher.h (right):

https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
mojo/public/cpp/system/watcher.h:47: // NOTE: You MUST not destroy a Watcher
while |is_watching()| is |true|. This
nit: is_watching -> IsWatching

There must be reasons why you would like to change from "automatically cancel on
destruction" to "explicit cancellation is required". But I haven't figured
out... :)

[Ken Rockot(use gerrit already), 2016-03-04 19:02:08]

We could go back to implicit cancellation but I decided it wasn't necessary
and think it's better to have as few magic destructors as possible :)  WDYT?

On Fri, Mar 4, 2016 at 10:58 AM, <yzshen@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
> File mojo/public/cpp/system/watcher.h (right):
>
>
>
https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
> mojo/public/cpp/system/watcher.h:47: // NOTE: You MUST not destroy a
> Watcher while |is_watching()| is |true|. This
> nit: is_watching -> IsWatching
>
> There must be reasons why you would like to change from "automatically
> cancel on destruction" to "explicit cancellation is required". But I
> haven't figured out... :)
>
> https://codereview.chromium.org/1759783003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-03-04 19:17:06]

On 2016/03/04 19:02:08, Ken Rockot wrote:
> We could go back to implicit cancellation but I decided it wasn't necessary
> and think it's better to have as few magic destructors as possible :)  WDYT?

I personally feel that it is one less thing for the user to consider if we
automatically cancel.
It is like a "scoped" thing.

But I will let you decide. :)

> 
> On Fri, Mar 4, 2016 at 10:58 AM, <mailto:yzshen@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
> > File mojo/public/cpp/system/watcher.h (right):
> >
> >
> >
>
https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
> > mojo/public/cpp/system/watcher.h:47: // NOTE: You MUST not destroy a
> > Watcher while |is_watching()| is |true|. This
> > nit: is_watching -> IsWatching
> >
> > There must be reasons why you would like to change from "automatically
> > cancel on destruction" to "explicit cancellation is required". But I
> > haven't figured out... :)
> >
> > https://codereview.chromium.org/1759783003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-03-04 22:45:39]

https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
File mojo/public/cpp/system/watcher.h (right):

https://codereview.chromium.org/1759783003/diff/160001/mojo/public/cpp/system...
mojo/public/cpp/system/watcher.h:47: // NOTE: You MUST not destroy a Watcher
while |is_watching()| is |true|. This
On 2016/03/04 at 18:58:17, yzshen1 wrote:
> nit: is_watching -> IsWatching
> 

Done

> There must be reasons why you would like to change from "automatically cancel
on destruction" to "explicit cancellation is required". But I haven't figured
out... :)

I have reverted to automatically cancelling on destruction :)

[yzshen1, 2016-03-04 22:49:26]

Nice. LGTM++

[Ken Rockot(use gerrit already), 2016-03-04 23:02:27]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-03-04 23:03:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759783003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759783003/180001

[commit-bot: I haz the power, 2016-03-05 00:00:04]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-05 00:01:31]

Description was changed from

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests
==========

to

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests

Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}
==========

[commit-bot: I haz the power, 2016-03-05 00:01:32]

Patchset 10 (id:??) landed as
https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}

[Ken Rockot(use gerrit already), 2016-03-05 00:13:29]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1768443004/ by rockot@chromium.org.

The reason for reverting is: Breaks iOS GN which is apparently a tree closer
despite having no CQ coverage.

[Ken Rockot(use gerrit already), 2016-03-05 04:43:06]

content_unittests deps fixed - wasn't properly depending on the EDK on all
platforms

relanding

[Ken Rockot(use gerrit already), 2016-03-05 04:43:10]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-03-05 04:43:10]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1759783003/#ps210001
(title: "fix content_unittests EDK deps")

[commit-bot: I haz the power, 2016-03-05 04:43:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759783003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759783003/210001

[Ken Rockot(use gerrit already), 2016-03-05 04:44:48]

On 2016/03/05 at 04:43:17, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1759783003/210001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1759783003/210001

err.. components_unittests... close enough :)

[commit-bot: I haz the power, 2016-03-05 06:04:47]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-03-05 06:06:32]

Description was changed from

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests

Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}
==========

to

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests

Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}

Committed: https://crrev.com/2cdb2f677c68f06b3bc54cd40f000ab961a09bbb
Cr-Commit-Position: refs/heads/master@{#379463}
==========

[commit-bot: I haz the power, 2016-03-05 06:06:33]

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

[Ken Rockot(use gerrit already), 2016-03-06 17:23:53]

A revert of this CL (patchset #12 id:210001) has been created in
https://codereview.chromium.org/1772503002/ by rockot@chromium.org.

The reason for reverting is: one more time. mojo_app_tests flake.

[Ken Rockot(use gerrit already), 2016-03-07 00:48:05]

Relanding this again. I've had no luck reproing the mus_apptests flake outside
of one builder, and I don't know how to debug that builder without landing this
CL. If I can't figure out a fix before tree activity picks back up, I'll revert
once again.

[Ken Rockot(use gerrit already), 2016-03-07 00:48:28]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-03-07 00:48:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759783003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759783003/210001

[commit-bot: I haz the power, 2016-03-07 01:57:44]

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

[commit-bot: I haz the power, 2016-03-07 01:57:45]

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

[Ken Rockot(use gerrit already), 2016-03-07 02:04:06]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-03-07 02:04:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759783003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759783003/210001

[commit-bot: I haz the power, 2016-03-07 02:37:01]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-03-07 02:38:10]

Description was changed from

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests

Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}

Committed: https://crrev.com/2cdb2f677c68f06b3bc54cd40f000ab961a09bbb
Cr-Commit-Position: refs/heads/master@{#379463}
==========

to

==========
[mojo-bindings] Use Watch API instead of MessagePumpMojo

This removes the C++ bindings dependency on MessagePumpMojo,
consuming the new Watch API instead.

For convenience a new mojo::Watcher is added to the public
Mojo C++ API library, and this is used by Connector.

BUG=590495
R=yzshen@chromium.org
TBR=blundell@chromium.org for rename affecting components/message_port.gypi
TBR=jam@chromium.org - added a missing header to new url tests

Committed: https://crrev.com/d06373e7cd8b4ad725ed5c64c958f2de13585add
Cr-Commit-Position: refs/heads/master@{#379402}

Committed: https://crrev.com/2cdb2f677c68f06b3bc54cd40f000ab961a09bbb
Cr-Commit-Position: refs/heads/master@{#379463}

Committed: https://crrev.com/e5d711ec0a8b327dea474d5c83dec0ae5f2c545e
Cr-Commit-Position: refs/heads/master@{#379508}
==========

[commit-bot: I haz the power, 2016-03-07 02:38:11]

Patchset 12 (id:??) landed as
https://crrev.com/e5d711ec0a8b327dea474d5c83dec0ae5f2c545e
Cr-Commit-Position: refs/heads/master@{#379508}