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

Issue 1429553002: API definition for WaitSet. (Closed)

Created:
5 years, 1 month ago by Anand Mistry (off Chromium)
Modified:
5 years ago
CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

API definition for WaitSet. BUG=556865 Committed: https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de Cr-Commit-Position: refs/heads/master@{#363914}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Update API. #

Total comments: 2

Patch Set 3 : Rebase. #

Patch Set 4 : Fix comment. #

Total comments: 7

Patch Set 5 : Review comments. #

Total comments: 10

Patch Set 6 : Change GetHandles to return multiple handles, and address other comments. #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+232 lines, -0 lines) Patch
M mojo/public/c/system/BUILD.gn View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M mojo/public/c/system/core.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A mojo/public/c/system/wait_set.h View 1 2 3 4 5 1 chunk +130 lines, -0 lines 0 comments Download
M third_party/mojo/mojo_public.gyp View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/mojo/src/mojo/edk/embedder/entrypoints.cc View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
M third_party/mojo/src/mojo/edk/system/core.h View 1 2 3 4 5 1 chunk +15 lines, -0 lines 0 comments Download
M third_party/mojo/src/mojo/edk/system/core.cc View 1 2 3 4 5 1 chunk +50 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 48 (9 generated)
Anand Mistry (off Chromium)
Hi all, This change is what I would consider and API for wait sets in ...
5 years, 1 month ago (2015-10-26 23:50:25 UTC) #2
Anand Mistry (off Chromium)
Oh yeah, and problem #6, naming.
5 years, 1 month ago (2015-10-26 23:55:25 UTC) #3
darin (slow to review)
https://codereview.chromium.org/1429553002/diff/1/third_party/mojo/src/mojo/public/c/system/wait_set.h File third_party/mojo/src/mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/1/third_party/mojo/src/mojo/public/c/system/wait_set.h#newcode44 third_party/mojo/src/mojo/public/c/system/wait_set.h:44: MojoWaitSetHandle* wait_set_handle); // Out. Even though it is technically ...
5 years, 1 month ago (2015-10-26 23:57:30 UTC) #5
darin (slow to review)
Actually, if you do reuse MojoHandle, then you can reuse MojoClose and ScopedHandle. Hmmm...
5 years, 1 month ago (2015-10-26 23:58:40 UTC) #6
Ken Rockot(use gerrit already)
On 2015/10/26 23:50:25, Anand Mistry wrote: > Hi all, > > This change is what ...
5 years, 1 month ago (2015-10-28 21:18:03 UTC) #7
Anand Mistry (off Chromium)
> On 2015/10/26 23:50:25, Anand Mistry wrote: > > Hi all, > > > > ...
5 years, 1 month ago (2015-10-29 06:36:25 UTC) #8
darin (slow to review)
Can MojoWait be used to wait on a WaitSet? Could the signalled state be queried ...
5 years, 1 month ago (2015-10-30 01:50:09 UTC) #9
Anand Mistry (off Chromium)
Yes and yes, if the set is represented by a MojoHandle. Then the questions become: ...
5 years, 1 month ago (2015-10-30 03:38:07 UTC) #10
Ken Rockot(use gerrit already)
On 2015/10/30 at 03:38:07, amistry wrote: > Yes and yes, if the set is represented ...
5 years, 1 month ago (2015-11-09 20:48:41 UTC) #11
darin (slow to review)
On 2015/11/09 20:48:41, Ken Rockot wrote: > On 2015/10/30 at 03:38:07, amistry wrote: > > ...
5 years, 1 month ago (2015-11-09 21:04:43 UTC) #12
Ken Rockot(use gerrit already)
On 2015/11/09 at 21:04:43, darin wrote: > On 2015/11/09 20:48:41, Ken Rockot wrote: > > ...
5 years, 1 month ago (2015-11-09 21:16:45 UTC) #13
darin (slow to review)
On Mon, Nov 9, 2015 at 1:16 PM, <rockot@chromium.org> wrote: > On 2015/11/09 at 21:04:43, ...
5 years, 1 month ago (2015-11-09 21:47:00 UTC) #14
Anand Mistry (off Chromium)
Hi all, I've updated this change based on the discussion, as well as my own ...
5 years, 1 month ago (2015-11-13 00:21:30 UTC) #15
darin (slow to review)
Makes sense to me. I like your bikeshed color. (I didn't do a detailed review.) ...
5 years, 1 month ago (2015-11-13 05:12:51 UTC) #16
Anand Mistry (off Chromium)
reviewers: Ping! Despite the lack of implementation here, I still want to submit this.
5 years, 1 month ago (2015-11-17 01:42:15 UTC) #17
Ken Rockot(use gerrit already)
Sorry for the delayed response. lgtm but you'll need to rebase and adjust all the ...
5 years, 1 month ago (2015-11-17 01:50:24 UTC) #18
Anand Mistry (off Chromium)
https://codereview.chromium.org/1429553002/diff/20001/third_party/mojo/src/mojo/public/c/system/BUILD.gn File third_party/mojo/src/mojo/public/c/system/BUILD.gn (right): https://codereview.chromium.org/1429553002/diff/20001/third_party/mojo/src/mojo/public/c/system/BUILD.gn#newcode21 third_party/mojo/src/mojo/public/c/system/BUILD.gn:21: "wait_set.h", On 2015/11/17 01:50:23, Ken Rockot wrote: > Might ...
5 years, 1 month ago (2015-11-17 03:11:32 UTC) #20
Anand Mistry (off Chromium)
jam, yzshen, sammc: Ping! Please review for submit.
5 years, 1 month ago (2015-11-18 22:25:55 UTC) #21
yzshen1
https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h#newcode35 mojo/public/c/system/wait_set.h:35: // Add a wait on |handle| to |wait_set_handle|. Nit: ...
5 years, 1 month ago (2015-11-18 23:50:58 UTC) #22
Anand Mistry (off Chromium)
https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h#newcode35 mojo/public/c/system/wait_set.h:35: // Add a wait on |handle| to |wait_set_handle|. On ...
5 years, 1 month ago (2015-11-19 04:13:45 UTC) #23
yzshen1
https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h#newcode101 mojo/public/c/system/wait_set.h:101: // subsequent calls to |MojoGetReadyHandle()| will return the same ...
5 years, 1 month ago (2015-11-19 06:52:25 UTC) #24
Anand Mistry (off Chromium)
On 2015/11/19 06:52:25, yzshen1 wrote: > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h > File mojo/public/c/system/wait_set.h (right): > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wait_set.h#newcode101 > ...
5 years, 1 month ago (2015-11-19 07:51:26 UTC) #25
yzshen1
On 2015/11/19 07:51:26, Anand Mistry wrote: > On 2015/11/19 06:52:25, yzshen1 wrote: > > > ...
5 years, 1 month ago (2015-11-19 17:05:42 UTC) #26
Anand Mistry (off Chromium)
On 2015/11/19 17:05:42, yzshen1 wrote: > On 2015/11/19 07:51:26, Anand Mistry wrote: > > On ...
5 years, 1 month ago (2015-11-20 05:16:57 UTC) #27
yzshen1
On Thu, Nov 19, 2015 at 9:16 PM, <amistry@chromium.org> wrote: > On 2015/11/19 17:05:42, yzshen1 ...
5 years, 1 month ago (2015-11-20 07:16:05 UTC) #28
Anand Mistry (off Chromium)
On 20 November 2015 at 18:15, Yuzhu Shen <yzshen@chromium.org> wrote: > > > On Thu, ...
5 years, 1 month ago (2015-11-23 05:42:03 UTC) #29
Sam McNally
https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wait_set.h File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wait_set.h#newcode48 mojo/public/c/system/wait_set.h:48: // It is safe to add a handle to ...
5 years, 1 month ago (2015-11-23 06:51:57 UTC) #30
darin (slow to review)
https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wait_set.h File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wait_set.h#newcode50 mojo/public/c/system/wait_set.h:50: MOJO_SYSTEM_EXPORT MojoResult MojoAddWaiter( naming nit (bikeshedding): I'm not sure ...
5 years, 1 month ago (2015-11-23 22:19:01 UTC) #31
Anand Mistry (off Chromium)
I'm still addressing the comments, but I've implemented this in the new EDK and run ...
5 years ago (2015-11-25 03:32:30 UTC) #32
Anand Mistry (off Chromium)
> > I imagine something like this: (The names, etc. haven't been carefully > considered. ...
5 years ago (2015-12-02 03:47:40 UTC) #33
yzshen1
Thanks for getting the numbers and giving detailed explanation of your idea! As you said, ...
5 years ago (2015-12-02 05:25:52 UTC) #34
yzshen1
On 2015/12/02 05:25:52, yzshen1 wrote: > Thanks for getting the numbers and giving detailed explanation ...
5 years ago (2015-12-02 05:33:35 UTC) #35
Anand Mistry (off Chromium)
On 2015/12/02 05:25:52, yzshen1 wrote: > Thanks for getting the numbers and giving detailed explanation ...
5 years ago (2015-12-02 06:38:02 UTC) #36
Anand Mistry (off Chromium)
Ok, I think there's been enough bikesheading, so I'm calling it. If there are no ...
5 years ago (2015-12-03 00:47:13 UTC) #37
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429553002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429553002/160001
5 years ago (2015-12-09 00:14:48 UTC) #39
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-12-09 01:42:46 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429553002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429553002/160001
5 years ago (2015-12-09 02:05:57 UTC) #44
commit-bot: I haz the power
Committed patchset #9 (id:160001)
5 years ago (2015-12-09 02:16:55 UTC) #46
commit-bot: I haz the power
5 years ago (2015-12-09 02:18:09 UTC) #48
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de
Cr-Commit-Position: refs/heads/master@{#363914}

Powered by Google App Engine
This is Rietveld 408576698