Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(35)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 409943003: Makes HandleWatcher block until no longer waiting on pipe (Closed)

Created:
6 years, 5 months ago by sky
Modified:
6 years, 5 months ago
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org
Project:
chromium
Visibility:
Public.

Description

Makes HandleWatcher block until no longer waiting on pipe BUG=394886 TEST=covered as best as can from tests R=darin@chromium.org, jam@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285266

Patch Set 1 #

Patch Set 2 : tweaks #

Total comments: 4

Patch Set 3 : use WaitableEvent and start threads first then schedule tasks #

Total comments: 2

Patch Set 4 : fix connector and update thread restriction #

Patch Set 5 : update comment #

Patch Set 6 : fix destructor #

Patch Set 7 : remove invalid java tests #

Patch Set 8 : more java fixes #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+159 lines, -76 lines) Patch
M base/threading/thread_restrictions.h View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java View 1 2 3 4 5 6 7 3 chunks +3 lines, -53 lines 4 comments Download
M mojo/common/handle_watcher.h View 2 chunks +4 lines, -1 line 0 comments Download
M mojo/common/handle_watcher.cc View 1 2 3 4 7 chunks +24 lines, -12 lines 0 comments Download
M mojo/common/handle_watcher_unittest.cc View 1 2 2 chunks +100 lines, -0 lines 0 comments Download
M mojo/common/message_pump_mojo.cc View 2 chunks +7 lines, -4 lines 0 comments Download
M mojo/public/cpp/bindings/lib/connector.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M mojo/public/cpp/bindings/lib/connector.cc View 1 2 3 4 5 3 chunks +12 lines, -6 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
sky
6 years, 5 months ago (2014-07-22 17:32:52 UTC) #1
darin (slow to review)
https://codereview.chromium.org/409943003/diff/20001/mojo/common/handle_watcher.cc File mojo/common/handle_watcher.cc (right): https://codereview.chromium.org/409943003/diff/20001/mojo/common/handle_watcher.cc#newcode67 mojo/common/handle_watcher.cc:67: struct WaitState { nit: You could also just use ...
6 years, 5 months ago (2014-07-22 17:42:57 UTC) #2
sky
https://codereview.chromium.org/409943003/diff/20001/mojo/common/handle_watcher.cc File mojo/common/handle_watcher.cc (right): https://codereview.chromium.org/409943003/diff/20001/mojo/common/handle_watcher.cc#newcode67 mojo/common/handle_watcher.cc:67: struct WaitState { On 2014/07/22 17:42:56, darin wrote: > ...
6 years, 5 months ago (2014-07-22 18:20:50 UTC) #3
darin (slow to review)
LGTM
6 years, 5 months ago (2014-07-22 18:25:08 UTC) #4
sky
The CQ bit was checked by sky@chromium.org
6 years, 5 months ago (2014-07-22 18:29:42 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/409943003/40001
6 years, 5 months ago (2014-07-22 18:31:34 UTC) #6
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
6 years, 5 months ago (2014-07-22 22:51:39 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-22 23:04:16 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_triggered_tests/builds/36955)
6 years, 5 months ago (2014-07-22 23:04:17 UTC) #9
qsr
https://codereview.chromium.org/409943003/diff/40001/mojo/common/message_pump_mojo.cc File mojo/common/message_pump_mojo.cc (right): https://codereview.chromium.org/409943003/diff/40001/mojo/common/message_pump_mojo.cc#newcode192 mojo/common/message_pump_mojo.cc:192: if (result == MOJO_RESULT_INVALID_ARGUMENT) { I'm not sure this ...
6 years, 5 months ago (2014-07-23 08:48:24 UTC) #10
sky
https://codereview.chromium.org/409943003/diff/40001/mojo/common/message_pump_mojo.cc File mojo/common/message_pump_mojo.cc (right): https://codereview.chromium.org/409943003/diff/40001/mojo/common/message_pump_mojo.cc#newcode192 mojo/common/message_pump_mojo.cc:192: if (result == MOJO_RESULT_INVALID_ARGUMENT) { On 2014/07/23 08:48:24, qsr_OoO_until_Aug_17 ...
6 years, 5 months ago (2014-07-23 16:21:19 UTC) #11
sky
Darin, I have to make two more additional changes: . Connector::CloseMessagePipe() was not cancelling the ...
6 years, 5 months ago (2014-07-23 16:23:30 UTC) #12
darin (slow to review)
OK
6 years, 5 months ago (2014-07-23 16:28:45 UTC) #13
sky
+jam for adding mojo::common::WatcherThreadManager. Darin is ok with the change, but the comment says you ...
6 years, 5 months ago (2014-07-23 16:30:01 UTC) #14
jam
lgtm
6 years, 5 months ago (2014-07-23 17:25:38 UTC) #15
sky
The CQ bit was checked by sky@chromium.org
6 years, 5 months ago (2014-07-23 17:52:11 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/409943003/100001
6 years, 5 months ago (2014-07-23 17:54:06 UTC) #17
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
6 years, 5 months ago (2014-07-23 18:49:10 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered_tests/builds/173948)
6 years, 5 months ago (2014-07-23 19:52:20 UTC) #19
sky
Darin, I had to update the java tests. Take another look? https://codereview.chromium.org/409943003/diff/140001/mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java File mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java (left): ...
6 years, 5 months ago (2014-07-23 23:37:15 UTC) #20
darin (slow to review)
LGTM https://codereview.chromium.org/409943003/diff/140001/mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java File mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java (left): https://codereview.chromium.org/409943003/diff/140001/mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java#oldcode682 mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java:682: handles.first.close(); On 2014/07/23 23:37:14, sky wrote: > This ...
6 years, 5 months ago (2014-07-24 03:39:47 UTC) #21
sky
Committed patchset #8 manually as r285266 (presubmit successful).
6 years, 5 months ago (2014-07-24 14:59:41 UTC) #22
sky
On 2014/07/24 03:39:47, darin wrote: > LGTM > > https://codereview.chromium.org/409943003/diff/140001/mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java > File mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java > (left): ...
6 years, 5 months ago (2014-07-24 15:01:07 UTC) #23
darin (slow to review)
6 years, 5 months ago (2014-07-24 18:26:59 UTC) #24 
On Thu, Jul 24, 2014 at 8:01 AM, <sky@chromium.org> wrote:

> On 2014/07/24 03:39:47, darin wrote:
>
>> LGTM
>>
>
>
> https://codereview.chromium.org/409943003/diff/140001/
> mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java
>
>> File
>>
> mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java
>
>> (left):
>>
>
>
> https://codereview.chromium.org/409943003/diff/140001/
> mojo/android/javatests/src/org/chromium/mojo/system/impl/
> CoreImplTest.java#oldcode682
>
> mojo/android/javatests/src/org/chromium/mojo/system/impl/
> CoreImplTest.java:682:
>
>> handles.first.close();
>> On 2014/07/23 23:37:14, sky wrote:
>> > This test doesn't make sense as we're saying the handle passed to
>> AsyncWait
>> must
>> > be valid.
>>
>
>  Perhaps the AsyncWait function should DCHECK that the given handle is
>> valid?
>>
>
> Wouldn't that require issuing a MojoWait immediately?
>

Yes, hence DCHECK. Maybe wrapped with #ifndef NDEBUG is better.

-Darin


>
> https://codereview.chromium.org/409943003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698