Mojo: Armed Watchers

Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
and avoiding any possibility of EDK deadlocks caused by cancellations,
during watch notification.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595, 700171
Review-Url: https://codereview.chromium.org/2725133002
Cr-Commit-Position: refs/heads/master@{#457269}
Committed: https://chromium.googlesource.com/chromium/src/+/9eadabae1de0cb22725450596fe324f03dd63b92

[Ken Rockot(use gerrit already), 2017-03-02 08:57:39]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 08:57:55]

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

[Ken Rockot(use gerrit already), 2017-03-02 09:20:07]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-02 09:20:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-02 09:20:23]

Dry run: 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-02 10:45:19]

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

[commit-bot: I haz the power, 2017-03-02 10:45:22]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2017-03-02 17:48:57]

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

[Ken Rockot(use gerrit already), 2017-03-02 17:48:58]

PTAL - Looks like I have some red to fix, but the CL is pretty large so I wanted
to send it out. Please look at EDK changes,  public cpp watcher changes, and
bindings changes. Thanks!

[Ken Rockot(use gerrit already), 2017-03-02 19:09:14]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 19:09:40]

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

[Ken Rockot(use gerrit already), 2017-03-02 19:26:40]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 19:27:12]

Dry run: 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-02 21:06:35]

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

[commit-bot: I haz the power, 2017-03-02 21:06:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yzshen1, 2017-03-03 00:03:50]

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/fu...
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/fu...
mojo/public/c/system/functions.h:162: // |handle|: The handle to watcher. Must
be an open message pipe or data pipe
watchter -> watch?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/ty...
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:185: // |MojoWatchNotificationFlags|: Passed to a
callback invoked by an armed
nit: strictly speaking the callback may be called while the watcher is unarmed.

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

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:193: CancelWait();
Will this cause problem while we are in the middle of a sync watch? For example:
1) Endpoint a uses Connector::SyncWatch() to wait for an incoming sync response.
2) The same thread is re-entered by another endpoint b.
3) Endpoint b calls EnableNestedDispatch() on a.
4) It will destroy the sync_watcher_ of a, which will cause the sync watch to
quit.

WDYT?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:221: for (;;) {
nit: Does it make sense to merge ReadAllAvailableMessages() into this method or
merge this loop into ReadAllAvailableMessages()?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/destruction_tracker.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/destruction_tracker.h:5: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_DELETION_TRACKER_H_
Please make it consistent with the file name.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/destruction_tracker.h:34: explicit operator bool()
const { return was_destroyed_; }
can we use !tracker_ to indicate destructed?

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

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:166: bool* was_deleted_flag_ = nullptr;
Is the reason to use this instead of DestructionTracker that we don't need to
worry about reentrancy here?

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:147: arm();
Will it make things simpler if we use mojo::Watcher instead of the raw C API?

[Ken Rockot(use gerrit already), 2017-03-03 00:32:09]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-03 00:32:18]

Patchset #4 (id:80001) has been deleted

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

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

[Ken Rockot(use gerrit already), 2017-03-03 00:37:05]

Thanks! In addition to the inline comments, I've also reverted the redundant
NotifyState in MessagePipeDispatcher and removed the unnecessary use of
AtomicFlag from EDK's Watcher.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/fu...
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/fu...
mojo/public/c/system/functions.h:162: // |handle|: The handle to watcher. Must
be an open message pipe or data pipe
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> watchter -> watch?

done

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/ty...
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:185: // |MojoWatchNotificationFlags|: Passed to a
callback invoked by an armed
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> nit: strictly speaking the callback may be called while the watcher is
unarmed.

done

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

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:193: CancelWait();
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Will this cause problem while we are in the middle of a sync watch? For
example:
> 1) Endpoint a uses Connector::SyncWatch() to wait for an incoming sync
response.
> 2) The same thread is re-entered by another endpoint b.
> 3) Endpoint b calls EnableNestedDispatch() on a.
> 4) It will destroy the sync_watcher_ of a, which will cause the sync watch to
quit.
> 
> WDYT?

Good point. Now I just reset handle_watcher_ before WaitToReadMore.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:221: for (;;) {
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> nit: Does it make sense to merge ReadAllAvailableMessages() into this method
or merge this loop into ReadAllAvailableMessages()?

Done

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/destruction_tracker.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/destruction_tracker.h:5: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_DELETION_TRACKER_H_
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Please make it consistent with the file name.

N/A - delete this code and used WeakPtrs instead per
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/lTXF4ejhbnQ

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/destruction_tracker.h:34: explicit operator bool()
const { return was_destroyed_; }
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> can we use !tracker_ to indicate destructed?

N/A

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

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.h:166: bool* was_deleted_flag_ = nullptr;
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Is the reason to use this instead of DestructionTracker that we don't need to
worry about reentrancy here?

N/A - using WeakPtr now.

But for the record, the reason was that I didn't think it made sense to put
DestructionTracker in an arbitrary public API (it's not a feature of bindings or
system really), so I decided to hide it in bindings where I know we need
reentrancy. Alas, it's irrelevant now :)

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:147: arm();
On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Will it make things simpler if we use mojo::Watcher instead of the raw C API?

Yes, most certainly, but I assume it will result in an argument over Blink
depending on mojo/public/cpp/system, so I'd rather hold off on that.

[commit-bot: I haz the power, 2017-03-03 01:43:14]

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

[commit-bot: I haz the power, 2017-03-03 01:43:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Rockot(use gerrit already), 2017-03-03 19:08:14]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 19:08:38]

Dry run: 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-03 20:13:06]

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

[commit-bot: I haz the power, 2017-03-03 20:13:07]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2017-03-04 21:48:17]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 21:48:27]

Dry run: 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-04 22:43:48]

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

[commit-bot: I haz the power, 2017-03-04 22:43:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

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

Description was changed from

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior. All existing Watcher users
use this behavior except for the bindings Connector.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

to

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Make watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior. All existing Watcher users
use this behavior except for the bindings Connector.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

[Ken Rockot(use gerrit already), 2017-03-07 22:03:17]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 22:04:22]

Dry run: 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-07 23:16:05]

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

[commit-bot: I haz the power, 2017-03-07 23:16:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2017-03-07 23:26:39]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 23:27:39]

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

[Ken Rockot(use gerrit already), 2017-03-07 23:28:53]

Description was changed from

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Make watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior. All existing Watcher users
use this behavior except for the bindings Connector.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

to

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
with the trade-off of making it slightly easier for users to do
something stupid.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

[Ken Rockot(use gerrit already), 2017-03-07 23:30:34]

OK, I believe this is really ready for review now. Still an issue with ios's use
of Watcher which I'm looking into, but all //mojo changes are in a good state.
PTAL!

[Ken Rockot(use gerrit already), 2017-03-07 23:38:00]

Just adding some inline comments to preemptively explain some parts of the CL,
hope it helps :)

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/messag...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/messag...
mojo/edk/system/message_pipe_dispatcher.cc:314:
watchers_.NotifyState(GetHandleSignalsStateNoLock());
I've had to add these state notifications to ReadMessage again after all. The
reason is that for WatcherDispatcher::Arm to be efficient, it must always have
up-to-date information about a watched handle's signal state. Before we only
needed to notify when state changed positively, but now we need to notify any
time the state changes, positive or negative.

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:44: // Pump potentially multiple finalizer
lists in sequence in order to avoid
BTW this change to handling nested finalizers is not necessary for this patch. I
can always land it separately if you prefer, but I liked the idea of avoiding
recursive destructor invocations.

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/reques...
File mojo/edk/system/request_context.h (left):

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/reques...
mojo/edk/system/request_context.h:60: void
AddWatchCancelFinalizer(scoped_refptr<Watcher> watcher);
This mechanism only existed to support cancelling watches from within
notification callbacks. Before we forced mutual exclusion and so this was used
to defer cancellation. That was quite difficult to understand and now that we
have manual arming I don't believe it's necessary to require that exclusion.

[commit-bot: I haz the power, 2017-03-07 23:39:05]

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

[commit-bot: I haz the power, 2017-03-07 23:39:07]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2017-03-08 00:50:35]

Sorry, please ignore again for now. I have some more changes to make :>

[Ken Rockot(use gerrit already), 2017-03-08 23:07:33]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-08 23:07:59]

OK, really ready for review this time.

Sorry for the large CL. I felt a rename of mojo::Watcher was appropriate given
that it's now a limited subset of watcher functionality.

Most of the core work is still in EDK code.

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

Dry run: 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-08 23:12:53]

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

[commit-bot: I haz the power, 2017-03-08 23:12:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ken Rockot(use gerrit already), 2017-03-08 23:24:56]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 23:25:44]

Dry run: 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-08 23:38:20]

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

[commit-bot: I haz the power, 2017-03-08 23:38:22]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2017-03-08 23:54:44]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 23:55:27]

Dry run: 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 00:20:57]

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

[commit-bot: I haz the power, 2017-03-09 00:20:58]

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

[Ken Rockot(use gerrit already), 2017-03-09 00:29:37]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 00:30:18]

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

[Ken Rockot(use gerrit already), 2017-03-09 00:35:47]

On 2017/03/08 at 23:07:59, Ken Rockot wrote:
> OK, really ready for review this time.
> 
> Sorry for the large CL. I felt a rename of mojo::Watcher was appropriate given
that it's now a limited subset of watcher functionality.
> 
> Most of the core work is still in EDK code.

Also worth noting that my previous comments regarding RequestContext are invalid
and can be ignored.

Because it's undesirable to make watch cancellation illegal from within
notification callbacks, I decided that the safest thing to do is to ensure that
a MOJO_RESULT_CANCELLED notification is always sent for any watch regardless of
how it's cancelled.

This is a bit safer than the status quo, which sends a MOJO_RESULT_CANCELLED
notification only when the watched handle is closed. Otherwise users assume that
once MojoCancelWatch is called, no more notifications can fire. That's the
guarantee made by the API today, but it requires explicit cancellation to be
exclusive to notification and thus the only way to cancel watch from within a
notification handler is to close the handle (MojoCancelWatch would deadlock).

Guaranteeing a final MOJO_RESULT_CANCELLED for all watch contexts allows users
to safely treat the context value as an owned, thread-safe object pointer which
can be reliably deleted on MOJO_RESULT_CANCELLED. You can see how it's used this
way in mojo::SimpleWatcher and content::MessagePort, where it's used as a
scoped_refptr instance.

Some of the burden of sorting out cross-thread behavior from the notification
callback shifted onto the user, but this gives them more freedom to decide how
to implement synchronization, and as mojo::SimpleWatcher demonstrates, we can
still provide helpful wrappers to this end.

[Ken Rockot(use gerrit already), 2017-03-09 01:19:24]

Description was changed from

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
with the trade-off of making it slightly easier for users to do
something stupid.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

to

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
with the trade-off of making it slightly easier for users to do
something stupid.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

[Ken Rockot(use gerrit already), 2017-03-09 01:21:49]

Description was changed from

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
with the trade-off of making it slightly easier for users to do
something stupid.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

to

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
and avoiding any possibility of EDK deadlocks caused by cancellations,
during watch notification.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

[commit-bot: I haz the power, 2017-03-09 01:59:33]

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

[commit-bot: I haz the power, 2017-03-09 01:59:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2017-03-09 02:23:48]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 02:24:20]

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

[Ken Rockot(use gerrit already), 2017-03-09 02:35:06]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 02:35:29]

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

[Ken Rockot(use gerrit already), 2017-03-09 02:35:32]

Patchset #4 (id:100001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:35:45]

Patchset #4 (id:50058) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:35:56]

Patchset #4 (id:130001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:36:07]

Patchset #6 (id:190001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:36:22]

Patchset #4 (id:150001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:36:35]

Patchset #6 (id:230001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:36:43]

Patchset #6 (id:250001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-09 02:36:50]

Patchset #8 (id:310001) has been deleted

[commit-bot: I haz the power, 2017-03-09 02:56:46]

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

[commit-bot: I haz the power, 2017-03-09 02:56:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2017-03-09 04:54:17]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 04:54:34]

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

[Ken Rockot(use gerrit already), 2017-03-09 04:54:45]

Patchset #8 (id:330001) has been deleted

[commit-bot: I haz the power, 2017-03-09 07:31:15]

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

[commit-bot: I haz the power, 2017-03-09 07:31:16]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2017-03-09 07:40:19]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-09 07:40:34]

Patchset #8 (id:350001) has been deleted

[commit-bot: I haz the power, 2017-03-09 07:40:35]

Dry run: 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 09:24:35]

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

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

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-03-09 19:08:11]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 19:08:47]

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

[Ken Rockot(use gerrit already), 2017-03-09 19:21:20]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 19:22:05]

Dry run: 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 21:09:07]

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

[commit-bot: I haz the power, 2017-03-09 21:09:08]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-03-11 00:44:58]

Some more comments.

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_respo...
content/child/url_response_body_consumer.cc:60: FROM_HERE,
base::Bind(&URLResponseBodyConsumer::OnReadable, AsWeakPtr(),
We may not need weak ptr support for this class any more.

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests...
File
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java
(right):

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests...
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java:74:
* @param read_pipe A MessagePipeHandle to read from when onResult triggers
success.
nit: read_pipe -> readPipe

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:484: if (!in_two_phase_read_ &&
bytes_available_ && !suppress_readable_signal_)
nit: there is no need to check |bytes_available_| because the previous line has
done that.

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watche...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watche...
mojo/edk/system/watcher_dispatcher.cc:352: for (auto& handles :
watched_handles_) {
nit: handles -> handle?

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/d...
File mojo/public/c/system/data_pipe.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/d...
mojo/public/c/system/data_pipe.h:89: //   |MOJO_READ_DATA_FLAG_CLEAR_SIGNAL| -
If reading fails due to a lack of
An idea for your consideration:
One thing is that this CLEAR_SIGNAL changes the handle state that may also
affects others.

For example, pipe_user_a does read with ALL_OR_NONE | CLEAR_SIGNAL. And later it
gives the pipe to pipe_user_b. Pipe_user_b doesn't do ALL_OR_NONE, and it
decides to wait until READABLE instead of directly trying a read. In that case,
if no new data is available, pipe_user_b will never do a read, because the pipe
appears unreadable to him. 

Is it possible to define a new kind of signal? For example, NEW_DATA_READABLE.
It will signal whenever new data arrives. Anyone who wants to do ALL_OR_NONE
read should watch for NEW_DATA_READABLE instead.

WDYT?

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/w...
File mojo/public/c/system/watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/w...
mojo/public/c/system/watcher.h:58: // watch different signals) as long as a
different |context| is provided for
Are there use cases where we need to watch a handle multiple times in the  same
watcher? I am thinking if we only allow a handle being added to a watcher once,
that may save some bookkeeping work in the implementation.

WDYT? Thanks!

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/w...
mojo/public/c/system/watcher.h:121: // lastter case, all registered contexts no
the watcher are implicitly cancelled
no -> on

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/connector.cc:313: MojoResult rv =
handle_watcher_->Arm(&ready_result);
Please consider using a different name, it hides |rv| of the outer scope.

[Ken Rockot(use gerrit already), 2017-03-12 21:43:40]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 21:43:51]

Dry run: 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-12 21:55:44]

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

[commit-bot: I haz the power, 2017-03-12 21:55:46]

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

[Ken Rockot(use gerrit already), 2017-03-12 22:19:59]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 22:20:07]

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

[Ken Rockot(use gerrit already), 2017-03-12 22:24:13]

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_respo...
content/child/url_response_body_consumer.cc:60: FROM_HERE,
base::Bind(&URLResponseBodyConsumer::OnReadable, AsWeakPtr(),
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> We may not need weak ptr support for this class any more.

I think that's true, but I'd rather leave this unchanged for this CL if you're
OK with that

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests...
File
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java
(right):

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests...
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java:74:
* @param read_pipe A MessagePipeHandle to read from when onResult triggers
success.
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> nit: read_pipe -> readPipe

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:484: if (!in_two_phase_read_ &&
bytes_available_ && !suppress_readable_signal_)
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> nit: there is no need to check |bytes_available_| because the previous line
has done that.

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watche...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watche...
mojo/edk/system/watcher_dispatcher.cc:352: for (auto& handles :
watched_handles_) {
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> nit: handles -> handle?

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/d...
File mojo/public/c/system/data_pipe.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/d...
mojo/public/c/system/data_pipe.h:89: //   |MOJO_READ_DATA_FLAG_CLEAR_SIGNAL| -
If reading fails due to a lack of
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> An idea for your consideration:
> One thing is that this CLEAR_SIGNAL changes the handle state that may also
affects others.
> 
> For example, pipe_user_a does read with ALL_OR_NONE | CLEAR_SIGNAL. And later
it gives the pipe to pipe_user_b. Pipe_user_b doesn't do ALL_OR_NONE, and it
decides to wait until READABLE instead of directly trying a read. In that case,
if no new data is available, pipe_user_b will never do a read, because the pipe
appears unreadable to him. 
> 
> Is it possible to define a new kind of signal? For example, NEW_DATA_READABLE.
It will signal whenever new data arrives. Anyone who wants to do ALL_OR_NONE
read should watch for NEW_DATA_READABLE instead.
> 
> WDYT?

Argued with myself for a while over this. I think it's a good solution. Done (in
a separate precursor CL)

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/w...
File mojo/public/c/system/watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/w...
mojo/public/c/system/watcher.h:58: // watch different signals) as long as a
different |context| is provided for
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> Are there use cases where we need to watch a handle multiple times in the 
same watcher? I am thinking if we only allow a handle being added to a watcher
once, that may save some bookkeeping work in the implementation.
> 
> WDYT? Thanks!

Good point. I was trying to keep the API as flexible as possible but I don't
think this complexity needs to be there. A user can always use a single watch to
watch multiple signals, and demultiplex notifications based on signals_state.

Done.

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/w...
mojo/public/c/system/watcher.h:121: // lastter case, all registered contexts no
the watcher are implicitly cancelled
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> no -> on

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/connector.cc:313: MojoResult rv =
handle_watcher_->Arm(&ready_result);
On 2017/03/11 at 00:44:58, yzshen1 wrote:
> Please consider using a different name, it hides |rv| of the outer scope.

Done

[commit-bot: I haz the power, 2017-03-12 23:05:48]

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

[commit-bot: I haz the power, 2017-03-12 23:05:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ken Rockot(use gerrit already), 2017-03-13 06:10:37]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 06:10:45]

Dry run: 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-13 08:10:33]

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

[commit-bot: I haz the power, 2017-03-13 08:10:35]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-03-13 16:35:51]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 16:36:06]

Dry run: 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-13 18:31:40]

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

[commit-bot: I haz the power, 2017-03-13 18:31:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2017-03-13 20:21:18]

LGTM with a few nits.

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system...
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.h:25: // This provides a convnient
thread-bound watcher implementation to safely watch
convnient -> convenient

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.h:94: // |MOJO_RESULT_CANCELLED| if the
SimpleWatcher is currently armed. Use
I haven't understood this. Why is CANCELLED not dispatched if it is armed?

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch.h
File mojo/edk/system/watch.h (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch....
mojo/edk/system/watch.h:42: // dispatcher notifies the WatcherDispatcher of a
state change a state change.
nit: "a state change" occurs twice.

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watche...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watche...
mojo/edk/system/watcher_dispatcher.cc:120: if (watches_.count(context) ||
watched_handles_.count(dispatcher.get()))
[just to double check] IIUC, we want |context| to be unique because that is the
only ID information when the callback is called. (The handle itself is not
passed into the callback.) right?

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system...
File mojo/public/cpp/system/simple_watcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.cc:59: delete context_ref;
Does it make sense to use Context* as the |context_value|, which has one ref
owned by the registered watch, and we release that ref on cancellation?

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.cc:185: // unset by then. This prevents
the cancellation notification from reaching
Is the last sentence a comment for line 178 instead?

[Ken Rockot(use gerrit already), 2017-03-13 21:59:30]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-13 22:00:11]

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system...
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.h:25: // This provides a convnient
thread-bound watcher implementation to safely watch
On 2017/03/13 at 20:21:18, yzshen1 wrote:
> convnient -> convenient

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.h:94: // |MOJO_RESULT_CANCELLED| if the
SimpleWatcher is currently armed. Use
On 2017/03/13 at 20:21:18, yzshen1 wrote:
> I haven't understood this. Why is CANCELLED not dispatched if it is armed?

Sorry, wording is bad. CANCELLED is always dispatched. What I meant by this is
that, unlike CANCELLED results, non-CANCELLED results can only be dispatched
when armed. I've tried to improve the wording.

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch.h
File mojo/edk/system/watch.h (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch....
mojo/edk/system/watch.h:42: // dispatcher notifies the WatcherDispatcher of a
state change a state change.
On 2017/03/13 at 20:21:18, yzshen1 wrote:
> nit: "a state change" occurs twice.

Fixed

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watche...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watche...
mojo/edk/system/watcher_dispatcher.cc:120: if (watches_.count(context) ||
watched_handles_.count(dispatcher.get()))
On 2017/03/13 at 20:21:18, yzshen1 wrote:
> [just to double check] IIUC, we want |context| to be unique because that is
the only ID information when the callback is called. (The handle itself is not
passed into the callback.) right?

Correct. Unlike the WaitSet API, I didn't want watchers to return handle values,
because on cancellation the handle value is no longer valid (and in theory may
even refer to a valid handle that has different meaning if the value has been
reused by the system).

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system...
File mojo/public/cpp/system/simple_watcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.cc:59: delete context_ref;
On 2017/03/13 at 20:21:18, yzshen1 wrote:
> Does it make sense to use Context* as the |context_value|, which has one ref
owned by the registered watch, and we release that ref on cancellation?

Sure, that's effectively equivalent to this, but I guess it does seem a little
cleaner. Done.

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.cc:185: // unset by then. This prevents
the cancellation notification from reaching
On 2017/03/13 at 20:21:18, yzshen1 wrote:
> Is the last sentence a comment for line 178 instead?

Ah good catch. It used to be accurate here, but I added
DisableCancellationNotifications() later after encountering issues described in
the TODO there. Moved the comment.

[commit-bot: I haz the power, 2017-03-13 22:00:16]

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

[Ken Rockot(use gerrit already), 2017-03-13 22:12:26]

rockot@chromium.org changed reviewers:
+ csharrison@chromium.org, eugenebut@chromium.org, haraken@chromium.org,
jam@chromium.org, miu@chromium.org, xhwang@chromium.org, yhirano@chromium.org

[Ken Rockot(use gerrit already), 2017-03-13 22:12:27]

Note to any reviewers of code using data pipes: The changes here are a stop-gap
to adapt your Watchers to the new SimpleWatcher API, which is not entirely
appropriate for data pipe handles, but is functional enough to do the right
thing and not break your code. I will be following up with a CL that introduces
a new kind of Watcher specifically for data pipes and it will make this code
less ugly.

Please take a look!

+miu: chrome/browser/media
+yhirano (and +csharrison): c/b/loader and c/child loading stuff
+jam: content MessagePort
+eugenebut: ios/
+xhwang: media/
+haraken: Blink MessagePort

Thanks all

[Ken Rockot(use gerrit already), 2017-03-13 22:13:56]

On 2017/03/13 at 22:12:27, Ken Rockot wrote:
> Note to any reviewers of code using data pipes: The changes here are a
stop-gap to adapt your Watchers to the new SimpleWatcher API, which is not
entirely appropriate for data pipe handles, but is functional enough to do the
right thing and not break your code. I will be following up with a CL that
introduces a new kind of Watcher specifically for data pipes and it will make
this code less ugly.
> 
> Please take a look!
> 
> +miu: chrome/browser/media
> +yhirano (and +csharrison): c/b/loader and c/child loading stuff
> +jam: content MessagePort
> +eugenebut: ios/
> +xhwang: media/
> +haraken: Blink MessagePort

That should be Blink MojoWatcher* :)

> 
> Thanks all

[Ken Rockot(use gerrit already), 2017-03-13 22:19:49]

Description was changed from

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
and avoiding any possibility of EDK deadlocks caused by cancellations,
during watch notification.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595
==========

to

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
and avoiding any possibility of EDK deadlocks caused by cancellations,
during watch notification.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595,700171
==========

[jam, 2017-03-13 23:56:21]

rs lgtm

[xhwang, 2017-03-14 00:21:27]

media/ lgtm and looking forward to the new DataPipe watchers :) Thanks!

[commit-bot: I haz the power, 2017-03-14 00:21:47]

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

[commit-bot: I haz the power, 2017-03-14 00:21:49]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-03-14 05:41:55]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {
Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?

[haraken, 2017-03-14 09:51:01]

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:65:
std::unique_ptr<CrossThreadPersistent<MojoWatcher>> m_context;

This looks strange in a coupe of ways.

The only thing we need to make sure is that MojoWatcher's wrapper does not get
collected while the watcher is running. If MojoWatcher's wrapper is alive, the
MojoWatcher is kept alive as well.

To achieve that, you just need to return true from hasPendingActivity() while
the watcher is running. i.e., one boolean flag would be sufficient, I guess.

[Ken Rockot(use gerrit already), 2017-03-14 14:16:05]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-14 14:16:14]

Patchset #16 (id:530001) has been deleted

[commit-bot: I haz the power, 2017-03-14 14:16:25]

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

[Ken Rockot(use gerrit already), 2017-03-14 14:17:40]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {
On 2017/03/14 at 05:41:55, yhirano wrote:
> Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?

No, that implies we're in a two-phase read, which ends with EndReadDataRaw. We
always ArmOrNotify after that.

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:65:
std::unique_ptr<CrossThreadPersistent<MojoWatcher>> m_context;
On 2017/03/14 at 09:51:01, haraken wrote:
> This looks strange in a coupe of ways.
> 
> The only thing we need to make sure is that MojoWatcher's wrapper does not get
collected while the watcher is running. If MojoWatcher's wrapper is alive, the
MojoWatcher is kept alive as well.
> 
> To achieve that, you just need to return true from hasPendingActivity() while
the watcher is running. i.e., one boolean flag would be sufficient, I guess.

I see, thanks for the clarification. Actually we don't even need a bool, we can
use the validity of m_handle as a signal still. I've made it so that explicit
cancel() does not reset this value, but instead only resets the watcher handle
itself. This achieves the desired behavior.

[haraken, 2017-03-14 15:10:32]

WebKit LGTM

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:90: MojoResult rv =

Nit: rv => result

Blink prefers a fully qualified name.

[Ken Rockot(use gerrit already), 2017-03-14 15:30:02]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:30:45]

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

[yhirano, 2017-03-14 15:32:10]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {
On 2017/03/14 14:17:40, Ken Rockot wrote:
> On 2017/03/14 at 05:41:55, yhirano wrote:
> > Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?
> 
> No, that implies we're in a two-phase read, which ends with EndReadDataRaw. We
> always ArmOrNotify after that.

Sorry, I don't understand: My understanding is "we're in a two-phase read, which
ends with EndReadDataRaw." The EndReadDataRaw is on L88, and this ArmOrNotify
will continue posting tasks until Reclaim is called.

[Ken Rockot(use gerrit already), 2017-03-14 15:51:08]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:51:47]

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

[Ken Rockot(use gerrit already), 2017-03-14 15:52:21]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_resp...
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {
On 2017/03/14 at 15:32:10, yhirano wrote:
> On 2017/03/14 14:17:40, Ken Rockot wrote:
> > On 2017/03/14 at 05:41:55, yhirano wrote:
> > > Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?
> > 
> > No, that implies we're in a two-phase read, which ends with EndReadDataRaw.
We
> > always ArmOrNotify after that.
> 
> Sorry, I don't understand: My understanding is "we're in a two-phase read,
which ends with EndReadDataRaw." The EndReadDataRaw is on L88, and this
ArmOrNotify will continue posting tasks until Reclaim is called.

Ah, sorry, I misunderstood your comment and overlooked this code. I thought you
were suggesting that we did need ArmOrNotify when the result is
MOJO_RESULT_BUSY. You're right to suggest that we don't. Fixed.

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:90: MojoResult rv =
On 2017/03/14 at 15:10:32, haraken wrote:
> Nit: rv => result
> 
> Blink prefers a fully qualified name.

Done

[commit-bot: I haz the power, 2017-03-14 17:42:54]

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

[commit-bot: I haz the power, 2017-03-14 17:42:56]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-03-15 12:07:31]

lgtm

[mmenke, 2017-03-15 14:11:29]

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

[mmenke, 2017-03-15 14:11:31]

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.h:43: // Selects how this SimpleWatcher is
to be armed.
Please add a definition of "arm", either here or above the class declaration, or
above the Arm() method.

[Ken Rockot(use gerrit already), 2017-03-15 17:04:27]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 17:05:03]

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

[Ken Rockot(use gerrit already), 2017-03-15 17:05:05]

Patchset #19 (id:610001) has been deleted

[commit-bot: I haz the power, 2017-03-15 17:08:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ken Rockot(use gerrit already), 2017-03-15 17:23:45]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 17:24:06]

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

[Ken Rockot(use gerrit already), 2017-03-15 17:30:45]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 17:31:30]

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

[Ken Rockot(use gerrit already), 2017-03-15 17:33:10]

Patchset #19 (id:630001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-15 17:33:28]

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
mojo/public/cpp/system/simple_watcher.h:43: // Selects how this SimpleWatcher is
to be armed.
On 2017/03/15 at 14:11:31, mmenke wrote:
> Please add a definition of "arm", either here or above the class declaration,
or above the Arm() method.

Done. I've tried to flesh out the documentation a bit more here.

[mmenke, 2017-03-15 17:44:07]

On 2017/03/15 17:33:28, Ken Rockot wrote:
>
https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
> File mojo/public/cpp/system/simple_watcher.h (right):
> 
>
https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
> mojo/public/cpp/system/simple_watcher.h:43: // Selects how this SimpleWatcher
is
> to be armed.
> On 2017/03/15 at 14:11:31, mmenke wrote:
> > Please add a definition of "arm", either here or above the class
declaration,
> or above the Arm() method.
> 
> Done. I've tried to flesh out the documentation a bit more here.

Thanks, much appreciated!

[Ken Rockot(use gerrit already), 2017-03-15 17:49:39]

On 2017/03/15 at 17:44:07, mmenke wrote:
> On 2017/03/15 17:33:28, Ken Rockot wrote:
> >
https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
> > File mojo/public/cpp/system/simple_watcher.h (right):
> > 
> >
https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system...
> > mojo/public/cpp/system/simple_watcher.h:43: // Selects how this
SimpleWatcher is
> > to be armed.
> > On 2017/03/15 at 14:11:31, mmenke wrote:
> > > Please add a definition of "arm", either here or above the class
declaration,
> > or above the Arm() method.
> > 
> > Done. I've tried to flesh out the documentation a bit more here.
> 
> Thanks, much appreciated!

I notice you're also an owner of content/browser/loader - any chance could
review the changes there in lieu of csharrison@? :)

[Eugene But (OOO till 7-30), 2017-03-15 17:56:35]

ios lgtm

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_m...
File ios/web/webui/web_ui_mojo_inttest.mm (right):

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_m...
ios/web/webui/web_ui_mojo_inttest.mm:169: // TODO(rockot): Introduce the full
watcher API to JS and get rid
On ios we use TODO(crbug.com/###): format, because otherwise we endup with bunch
of TODOs for people how left the team. Do you want a bug for this TODO?

[commit-bot: I haz the power, 2017-03-15 19:13:37]

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

[commit-bot: I haz the power, 2017-03-15 19:13:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-03-15 21:55:03]

c/b/l LGTM

[Ken Rockot(use gerrit already), 2017-03-15 22:04:32]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 22:05:19]

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

[Ken Rockot(use gerrit already), 2017-03-15 22:59:34]

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_m...
File ios/web/webui/web_ui_mojo_inttest.mm (right):

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_m...
ios/web/webui/web_ui_mojo_inttest.mm:169: // TODO(rockot): Introduce the full
watcher API to JS and get rid
On 2017/03/15 at 17:56:34, Eugene But wrote:
> On ios we use TODO(crbug.com/###): format, because otherwise we endup with
bunch of TODOs for people how left the team. Do you want a bug for this TODO?

Seems reasonable. I've filed a bug and updated the TODO.

[miu, 2017-03-15 23:34:01]

chrome/browser/media/* and media/remoting/* lgtm

[Ken Rockot(use gerrit already), 2017-03-15 23:35:37]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2017-03-15 23:35:41]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2017-03-15 23:35:42]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
jam@chromium.org, xhwang@chromium.org, haraken@chromium.org,
yhirano@chromium.org, eugenebut@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2725133002/#ps670001
(title: ".")

[commit-bot: I haz the power, 2017-03-15 23:36:15]

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-15 23:57:52]

CQ is committing da patch.

Bot data: {"patchset_id": 670001, "attempt_start_ts": 1489620941677400,
"parent_rev": "2ca03688be282be7b59aadc46489823e9ebcf3bd", "commit_rev":
"9eadabae1de0cb22725450596fe324f03dd63b92"}

[commit-bot: I haz the power, 2017-03-15 23:58:43]

Description was changed from

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
and avoiding any possibility of EDK deadlocks caused by cancellations,
during watch notification.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595,700171
==========

to

==========
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Makes watchers a first-class non-transferable handle type in Mojo,
allows multiple distinct events to be watched by a single watcher,
and requires watchers to be explicitly armed before a notification
will fire. Also now allows for cancellation (and watcher closure)
from within notification callbacks, simplifying cancellation logic
and avoiding any possibility of EDK deadlocks caused by cancellations,
during watch notification.

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming. Also renames it to SimpleWatcher to
adequately convey that this is only a simplified helper class that
does not utilize the full power of watchers.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior in many cases.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification, and is also now used for Watchers which
watch a data pipe handle.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595,700171

Review-Url: https://codereview.chromium.org/2725133002
Cr-Commit-Position: refs/heads/master@{#457269}
Committed:
https://chromium.googlesource.com/chromium/src/+/9eadabae1de0cb22725450596fe3...
==========

[commit-bot: I haz the power, 2017-03-15 23:58:46]

Committed patchset #20 (id:670001) as
https://chromium.googlesource.com/chromium/src/+/9eadabae1de0cb22725450596fe3...

[Guido Urdaneta, 2017-03-16 15:04:32]

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

The reason for reverting is: FindIt considers this as culprit of breaking  Linux
Chromium OS ASan LSan Tests (1) (stats) bot
See
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

aura_unittests aura_unittests

failed 1
Run on OS: 'Ubuntu-14.04'
failures:
UserActivityForwarderTest.ForwardActivityToDetector


Will reland if the revert doesn't fix the bot.
.

[Ken Rockot(use gerrit already), 2017-03-16 15:16:56]

You won't be able to revert without reverting other CLs too, unfortunately.
The leak is test only and a fix is out for review.

On Mar 16, 2017 8:04 AM, <guidou@chromium.org> wrote:

> A revert of this CL (patchset #20 id:670001) has been created in
> https://codereview.chromium.org/2750373002/ by guidou@chromium.org.
>
> The reason for reverting is: FindIt considers this as culprit of breaking
> Linux
> Chromium OS ASan LSan Tests (1) (stats) bot
> See
> https://build.chromium.org/p/chromium.memory/builders/
> Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/20033
>
> aura_unittests aura_unittests
>
> failed 1
> Run on OS: 'Ubuntu-14.04'
> failures:
> UserActivityForwarderTest.ForwardActivityToDetector
>
>
> Will reland if the revert doesn't fix the bot.
> .
>
> https://codereview.chromium.org/2725133002/
>

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

[Ken Rockot(use gerrit already), 2017-03-16 15:16:56]

You won't be able to revert without reverting other CLs too, unfortunately.
The leak is test only and a fix is out for review.

On Mar 16, 2017 8:04 AM, <guidou@chromium.org> wrote:

> A revert of this CL (patchset #20 id:670001) has been created in
> https://codereview.chromium.org/2750373002/ by guidou@chromium.org.
>
> The reason for reverting is: FindIt considers this as culprit of breaking
> Linux
> Chromium OS ASan LSan Tests (1) (stats) bot
> See
> https://build.chromium.org/p/chromium.memory/builders/
> Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/20033
>
> aura_unittests aura_unittests
>
> failed 1
> Run on OS: 'Ubuntu-14.04'
> failures:
> UserActivityForwarderTest.ForwardActivityToDetector
>
>
> Will reland if the revert doesn't fix the bot.
> .
>
> https://codereview.chromium.org/2725133002/
>

-- 
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.

[Guido Urdaneta, 2017-03-16 15:17:02]

Cancelled the revert since rockot@ has a fix:
https://codereview.chromium.org/2754863002