[mojo] Ports EDK

[mojo] Ports EDK

One Big CL to land the new EDK implementation based on Ports.

BUG=577688
Committed: https://crrev.com/ce69a049f1b705b0bb3d7e61d31908dfc8d968b2
Cr-Commit-Position: refs/heads/master@{#371556}

[Ken Rockot(use gerrit already), 2016-01-24 06:32:07]

Description was changed from

==========
[mojo] Ports EDK

BUG=
==========

to

==========
[mojo] Ports EDK

One Big CL to land the new EDK implementation based on Ports.

BUG=577688
==========

[Ken Rockot(use gerrit already), 2016-01-25 23:57:32]

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

[Ken Rockot(use gerrit already), 2016-01-25 23:57:35]

Anand, did you want to do a review pass?

[jam, 2016-01-26 18:00:08]

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

[jam, 2016-01-26 18:00:10]

lgtm (per offline discussion, Anand can review in detail after landing since
this code was already reviewed by Darin/Ken)

[Ken Rockot(use gerrit already), 2016-01-26 18:04:01]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-01-26 18:04:27]

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

[commit-bot: I haz the power, 2016-01-26 19:23:32]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-01-26 19:24:20]

Description was changed from

==========
[mojo] Ports EDK

One Big CL to land the new EDK implementation based on Ports.

BUG=577688
==========

to

==========
[mojo] Ports EDK

One Big CL to land the new EDK implementation based on Ports.

BUG=577688

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

[commit-bot: I haz the power, 2016-01-26 19:24:22]

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

[Nico, 2016-01-26 23:04:42]

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

[Nico, 2016-01-26 23:04:44]

..\..\mojo\edk\system\channel_win.cc(27,14) :  error: unused variable
'kMaxBatchReadCapacity' [-Werror,-Wunused-const-variable]
const size_t kMaxBatchReadCapacity = 256 * 1024;
             ^

(and possibly more). Can you take a quick look?

[Ken Rockot(use gerrit already), 2016-01-26 23:05:46]

Yes, taking a look.

On Tue, Jan 26, 2016 at 3:04 PM, <thakis@chromium.org> wrote:

> ..\..\mojo\edk\system\channel_win.cc(27,14) :  error: unused variable
> 'kMaxBatchReadCapacity' [-Werror,-Wunused-const-variable]
> const size_t kMaxBatchReadCapacity = 256 * 1024;
>              ^
>
> (and possibly more). Can you take a quick look?
>
> https://codereview.chromium.org/1585493002/
>

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

[Ken Rockot(use gerrit already), 2016-01-26 23:06:52]

Also, that should be the only one. It's the only Windows-only code in the
CL. Unused vars seem to be caught more easily for me with clang. Fix
incoming.

On Tue, Jan 26, 2016 at 3:05 PM, Ken Rockot <rockot@chromium.org> wrote:

> Yes, taking a look.
>
> On Tue, Jan 26, 2016 at 3:04 PM, <thakis@chromium.org> wrote:
>
>> ..\..\mojo\edk\system\channel_win.cc(27,14) :  error: unused variable
>> 'kMaxBatchReadCapacity' [-Werror,-Wunused-const-variable]
>> const size_t kMaxBatchReadCapacity = 256 * 1024;
>>              ^
>>
>> (and possibly more). Can you take a quick look?
>>
>> https://codereview.chromium.org/1585493002/
>>
>
>

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

[Anand Mistry (off Chromium), 2016-01-28 02:26:25]

I started reviewing this, but the change is just too big (~16,000 delta lines)
to review effectively. So, I'm just going to dig in to the implementation and
start making changes where I think there are issues.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/embedder/embe...
File mojo/edk/embedder/embedder.h (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/embedder/embe...
mojo/edk/embedder/embedder.h:13: #include "base/command_line.h"
You don't use base::CommandLine anywhere in this file, so it shouldn't be here.
IWYU!

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_posix.cc (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:19: #include "base/rand_util.h"
You can remove since there are no other uses of base::Rand* in here.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/channel.h
File mojo/edk/system/channel.h (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/channe...
mojo/edk/system/channel.h:117: void ShutDown();
It's not clear which threads these can be called on. If it's the IO thread,
please document and add a thread checker. If it's any thread, also document, but
I'm concerned there are data races here.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/ports/...
File mojo/edk/system/ports/hash_functions.h (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/ports/...
mojo/edk/system/ports/hash_functions.h:17: size_t h1 =
hash<uint64_t>()(name.v1);
Since avoid //base doesn't appear to be a goal for ports, please use
base::HashInts.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/shared...
File mojo/edk/system/shared_buffer_dispatcher.cc (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/shared...
mojo/edk/system/shared_buffer_dispatcher.cc:27: size_t num_bytes;
Please don't use size_t in serialised messages.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/shared...
File mojo/edk/system/shared_buffer_dispatcher.h (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/shared...
mojo/edk/system/shared_buffer_dispatcher.h:108: ScopedPlatformHandle
handle_for_transit_;
Instead of keeping track, why not just pass ownership of the duplicated handle
and let the caller discard it if something goes wrong?

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/wait_s...
File mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/wait_s...
mojo/edk/system/wait_set_dispatcher.cc:48: base::AutoLock lock(lock_);
Why not just hold lock_ for the entire function. It's difficult to reason, but
this code was written under that assumption, so there might be races now.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/wait_s...
mojo/edk/system/wait_set_dispatcher.cc:115: base::AutoLock lock(lock_);
Need to handle the closed state. This can return "not found" when the wait set
is being closed, which isn't right.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/wait_s...
mojo/edk/system/wait_set_dispatcher.cc:225: HandleSignalsState
WaitSetDispatcher::GetHandleSignalsState() const {
Need to handle the closed state. Without it, it's possible to add an awakable to
a closed wait set.

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/wait_s...
File mojo/edk/system/wait_set_dispatcher.h (right):

https://codereview.chromium.org/1585493002/diff/440001/mojo/edk/system/wait_s...
mojo/edk/system/wait_set_dispatcher.h:68: // Guards |is_closed_|,
|waiting_dispatchers_|, and |waiter_|.
nit: not |waiter_|, which doesn't need locking.