Issue 2798713002: Global Resource Coordinator: Basic service internals

oystein (OOO til 10th of July)

First pass; ptal! Missing some docs and probably some tests still, but should otherwise be ...

3 years, 8 months ago (2017-04-04 21:28:18 UTC) #1

oystein (OOO til 10th of July)

oysteine@chromium.org changed reviewers: + chrisha@chromium.org

3 years, 8 months ago (2017-04-05 17:36:38 UTC) #2

Primiano Tucci (use gerrit)

Had a first pass, glad to see this coming along! As a general comment, I ...

3 years, 8 months ago (2017-04-06 18:09:47 UTC) #4

oystein (OOO til 10th of July)

Thanks On 2017/04/06 at 18:09:47, primiano wrote: > Had a first pass, glad to see ...

3 years, 8 months ago (2017-04-10 20:02:59 UTC) #5

Thanks

On 2017/04/06 at 18:09:47, primiano wrote:
> Had a first pass, glad to see this coming along!
> 
> As a general comment, I think would be nice if the CoordinationUnit could have
some more specific name / subclass to reflect what it reallyy represents (a tab?
frame?).
> Right now it sounds a bit too generic.

I added a comment about that in
CoordinationUnitProviderImpl::CreateCoordinationUnit, the idea is that we can
use that as a factory function to instantiate different subclasses of
CoordinationUnit based on the type of the CoordinationUnitID which gets passed
in (there just hasn't been any need for it yet).

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:19:
base::LazyInstance<std::unordered_map<int64_t, CoordinationUnitImpl*>>::Leaky
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> Since we have thread-safe statics in C++11, I think you can just rewrite this
as:
> 
> using CUIDMap = std::unordered_map<int64_t, CoordinationUnitImpl*>;
> CUIDMap* g_cuid_map() {
>   static CUIDMap* instance = new CUIDMap();
>   return instance;
> }
> 
> that would avoid the extra Get() below
> 
> Precedent for this:
>
https://cs.chromium.org/search/?q=%5C(%5C)%5C+%7B%5Cn%5C+%5C+static.*%5Cn%5C+...

Oh glad we can do this now; done!

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:23:
return (((uint64_t)id.type) << 32) ^ id.id;
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> (uint64_t) -> static_cast<uint64_t>.
> I hate it and find it redundant, but our style guide explicitly says "Do not
use C-style casts." :-/
> Also, purely documentational, but since id is a int32_t, this could just be a
s/^/|/ right?
> 
> In any case, if you just make this a proper hash operator of
CoordinationUnitID, then you can just make a 
> unordered_map<CUID, CoordinationUnitImpl*>, which simplifies a bit the code
below, avoiding to hash explicitly

Done and done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:28:
CoordinationUnitImpl::CoordinationUnitImpl(
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> on which thread are these CUnits built and destroyed?
> By the lack of locking, it seems that the assumptions is that everything
happens on the same thread.
> Maybe you could have a global threadchecker (or sequencechecker) next to the
map to make sure we don't accidentally bereak this in future (threading did bite
me lot of times in the past in memory infra, especially in the cases where it
was out of my control and up to the clients)

Mojo guarantees it'll all be run on the same taskrunner, but I added a
threadchecker to the ResourceCoordinatorInterface since that's client code and
could potentially be used from multiple threads.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:35:
DCHECK(cu_map.end() == cu_map.find(id_hash_));
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> really a minor perf thingy, as this is debug only, but I think that
map.count(id_hash) == 0 is slightly faster (or generally not slower than .find)
as doesn't have to build up an iterator.ù
> 
> Or eventually you could just
> auto it = cu_map.insert(std::make_pair(id_hash_, this));
> DCHECK(it.second /* did insert*/);

Done; didn't realize this but makes sense in hindsight given that unordered_map
won't have duplicate elements and could in theory just stop at the first
encountered element and return 1. Some surface Googling suggests count() may
often be implemented using find() internally, but I went with the second
suggestion regardless since it looks a little cleaner.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:118:
callback.Run(id_);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> uh? This might deserve some comment :)
> why the caller doesn't just do it?

Added a comment in the .mojom.

It's mainly if you do things like:
* Create CU a
* Create CU b
* a.AddChild(b)

Since each CU has a separate messagepipe and these are async calls, there's no
guarantee that B will exist when then AddChild gets executed on the service
side. GetID() is basically just there to force a roundtrip over b's messagepipe
so we don't have to deal with a map of pending parent-child relationships or
something.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:135:
DCHECK(child->id_ == child_id);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> Both DCHECK(a == b) or DCHECK_EQ(a, b) are fine, I guess just use them
consistently (you use the _EQ down below on line 155).

That was my first attempt at this check, but I get:
../../base/logging.h:659:3: error: no matching function for call to
'MakeCheckOpValueString'

(...)

../../base/logging.h:649:18: note: candidate function not viable: no known
conversion from 'const resource_coordinator::CoordinationUnitID' to
'std::nullptr_t' (aka 'nullptr_t') for 2nd argument

I've no idea why this needs to be convertable to nullptr_ and it felt like it'd
be a rathole trying to figure that out :).

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:145:
if (children_.find(child) != children_.end())
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> just
> return children_.count(child) ? false : childer_.insert(child).second ?
> would save 5 lines :)

Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:155:
CHECK_EQ(1u, children_removed);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> just checking: is this really intended to be a check, or typoed a D? All the
others are DCHECK?

Typo; fixed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:193:
&CoordinationUnitImpl::UnregisterPolicyCallback, base::Unretained(this)));
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> what happens if the CUI is destroyed in the meanwhile? the dtor suggests this
is not undefinitely lived.
> Should this have a weakptr instead?

Mojo handles the lifespan and will only call this on alive bindings, seems to be
the pattern used elsewhere too.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:58:
int64_t id_hash_;
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> one thing that is not immediatley clear from this cl is what this hash is.
maybe add acomment?

Removed this, after adding the std::hash(CoordinationUnitID) operator. It'll
mean an additional hashing done in the destructor, but seems worth it for the
reduced code verbosity (and hey, memory).

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h:11:
#include "services/service_manager/public/cpp/connection.h"
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> is this include used?

Leftover; removed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h:12:
#include "services/service_manager/public/cpp/service_context_ref.h"
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> ServiceContextRef could be ust forward declared, right?

Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h:24:
resource_coordinator::mojom::CoordinationUnitProviderRequest request) {
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> why this is inlined? inlining should be only for trivial accessors IIRC

Copy pasted from elsewhere; no good reason that I can tell. Fixed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/coordination_unit_events.h
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_events.h:4: #ifndef
SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EVENTS_H_
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> nit: macro doesn't match the file name

Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_events.h:9: enum
EventType : uint32_t {
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> shouldn't this be an enum class to reduce namespace pollution?
> These enum names will end up in the general resource_coordinator namespace,
which means that you won't be able to use "NUM_EVENTS" or "TEST_EVENT" anywhere
else (in any other enum) if this is just an enum.

Oops, meant to do this. Fixed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:11: namespace
{}  // namespace
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> I guess this is a leftover?

Yep; done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:13:
CoordinationUnitID::CoordinationUnitID() = default;
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> my ignorance: will the =Default ctor zero initialize the pods here?

Nope; holdover from when id was an std::string.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:18: id =
base::Hash(new_id);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> I got very bitten by base::Hash in the past with memory-infra. It doesn't play
very well with strings that are identical % suffix.
> 
> See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-rXNiOoIU4A/LI4tn...
for my horror story.
> If this case is similar nad performance isn't critical here, I suggest using
base::Sha1

Ah good to know. I switched to Murmur2. skyostil@ might have more
performance-critical use-cases for CoordinationUnitID in the future so Murmur2
might be faster? We can reevaluate this though.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21: int32_t id;
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> should these be immutable (+const both)?

That doesn't seem to play well with the Mojo serialization.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:28:
static int next_id = 0;
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> you might be bitten by this in component builds similar to Ehsan.
> 
> resource_coordinator_interface.cc is part of the cpp source_set. If that
source_set gets included by different target, each target will have a local
next_id instance, so you will get duplicated ids.
> 
> On top of this, don't you ned this ID to be generated in a thread safe way?
> In other words, can a ResourceCoordinatorInterface be created by multiple
threads at the same time? If so  there is StaticAtomicSequenceNumber  in base
for this.

Yikes. Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:53:
DCHECK(service_);
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> I think the patttern in chrome is to not DCHECK for nullpointers if you
dereference the nullptr in the same scope, because that will fail anyways on
line 57.

Done.

oystein (OOO til 10th of July)

Thanks On 2017/04/06 at 18:09:47, primiano wrote: > Had a first pass, glad to see ...

3 years, 8 months ago (2017-04-10 20:03:00 UTC) #6

Thanks

On 2017/04/06 at 18:09:47, primiano wrote:
> Had a first pass, glad to see this coming along!
> 
> As a general comment, I think would be nice if the CoordinationUnit could have
some more specific name / subclass to reflect what it reallyy represents (a tab?
frame?).
> Right now it sounds a bit too generic.

I added a comment about that in
CoordinationUnitProviderImpl::CreateCoordinationUnit, the idea is that we can
use that as a factory function to instantiate different subclasses of
CoordinationUnit based on the type of the CoordinationUnitID which gets passed
in (there just hasn't been any need for it yet).

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:19:
base::LazyInstance<std::unordered_map<int64_t, CoordinationUnitImpl*>>::Leaky
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> Since we have thread-safe statics in C++11, I think you can just rewrite this
as:
> 
> using CUIDMap = std::unordered_map<int64_t, CoordinationUnitImpl*>;
> CUIDMap* g_cuid_map() {
>   static CUIDMap* instance = new CUIDMap();
>   return instance;
> }
> 
> that would avoid the extra Get() below
> 
> Precedent for this:
>
https://cs.chromium.org/search/?q=%5C(%5C)%5C+%7B%5Cn%5C+%5C+static.*%5Cn%5C+...

Oh glad we can do this now; done!

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:23:
return (((uint64_t)id.type) << 32) ^ id.id;
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> (uint64_t) -> static_cast<uint64_t>.
> I hate it and find it redundant, but our style guide explicitly says "Do not
use C-style casts." :-/
> Also, purely documentational, but since id is a int32_t, this could just be a
s/^/|/ right?
> 
> In any case, if you just make this a proper hash operator of
CoordinationUnitID, then you can just make a 
> unordered_map<CUID, CoordinationUnitImpl*>, which simplifies a bit the code
below, avoiding to hash explicitly

Done and done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:28:
CoordinationUnitImpl::CoordinationUnitImpl(
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> on which thread are these CUnits built and destroyed?
> By the lack of locking, it seems that the assumptions is that everything
happens on the same thread.
> Maybe you could have a global threadchecker (or sequencechecker) next to the
map to make sure we don't accidentally bereak this in future (threading did bite
me lot of times in the past in memory infra, especially in the cases where it
was out of my control and up to the clients)

Mojo guarantees it'll all be run on the same taskrunner, but I added a
threadchecker to the ResourceCoordinatorInterface since that's client code and
could potentially be used from multiple threads.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:118:
callback.Run(id_);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> uh? This might deserve some comment :)
> why the caller doesn't just do it?

Added a comment in the .mojom.

It's mainly if you do things like:
* Create CU a
* Create CU b
* a.AddChild(b)

Since each CU has a separate messagepipe and these are async calls, there's no
guarantee that B will exist when then AddChild gets executed on the service
side. GetID() is basically just there to force a roundtrip over b's messagepipe
so we don't have to deal with a map of pending parent-child relationships or
something.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:145:
if (children_.find(child) != children_.end())
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> just
> return children_.count(child) ? false : childer_.insert(child).second ?
> would save 5 lines :)

Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:155:
CHECK_EQ(1u, children_removed);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> just checking: is this really intended to be a check, or typoed a D? All the
others are DCHECK?

Typo; fixed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:193:
&CoordinationUnitImpl::UnregisterPolicyCallback, base::Unretained(this)));
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> what happens if the CUI is destroyed in the meanwhile? the dtor suggests this
is not undefinitely lived.
> Should this have a weakptr instead?

Mojo handles the lifespan and will only call this on alive bindings, seems to be
the pattern used elsewhere too.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.h
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.h:58:
int64_t id_hash_;
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> one thing that is not immediatley clear from this cl is what this hash is.
maybe add acomment?

Removed this, after adding the std::hash(CoordinationUnitID) operator. It'll
mean an additional hashing done in the destructor, but seems worth it for the
reduced code verbosity (and hey, memory).

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h:11:
#include "services/service_manager/public/cpp/connection.h"
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> is this include used?

Leftover; removed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h:12:
#include "services/service_manager/public/cpp/service_context_ref.h"
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> ServiceContextRef could be ust forward declared, right?

Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h:24:
resource_coordinator::mojom::CoordinationUnitProviderRequest request) {
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> why this is inlined? inlining should be only for trivial accessors IIRC

Copy pasted from elsewhere; no good reason that I can tell. Fixed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/coordination_unit_events.h
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_events.h:4: #ifndef
SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EVENTS_H_
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> nit: macro doesn't match the file name

Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_events.h:9: enum
EventType : uint32_t {
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> shouldn't this be an enum class to reduce namespace pollution?
> These enum names will end up in the general resource_coordinator namespace,
which means that you won't be able to use "NUM_EVENTS" or "TEST_EVENT" anywhere
else (in any other enum) if this is just an enum.

Oops, meant to do this. Fixed.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:11: namespace
{}  // namespace
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> I guess this is a leftover?

Yep; done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:13:
CoordinationUnitID::CoordinationUnitID() = default;
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> my ignorance: will the =Default ctor zero initialize the pods here?

Nope; holdover from when id was an std::string.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:18: id =
base::Hash(new_id);
On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> I got very bitten by base::Hash in the past with memory-infra. It doesn't play
very well with strings that are identical % suffix.
> 
> See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-rXNiOoIU4A/LI4tn...
for my horror story.
> If this case is similar nad performance isn't critical here, I suggest using
base::Sha1

Ah good to know. I switched to Murmur2. skyostil@ might have more
performance-critical use-cases for CoordinationUnitID in the future so Murmur2
might be faster? We can reevaluate this though.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21: int32_t id;
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> should these be immutable (+const both)?

That doesn't seem to play well with the Mojo serialization.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:28:
static int next_id = 0;
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> you might be bitten by this in component builds similar to Ehsan.
> 
> resource_coordinator_interface.cc is part of the cpp source_set. If that
source_set gets included by different target, each target will have a local
next_id instance, so you will get duplicated ids.
> 
> On top of this, don't you ned this ID to be generated in a thread safe way?
> In other words, can a ResourceCoordinatorInterface be created by multiple
threads at the same time? If so  there is StaticAtomicSequenceNumber  in base
for this.

Yikes. Done.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:53:
DCHECK(service_);
On 2017/04/06 at 18:09:47, Primiano Tucci wrote:
> I think the patttern in chrome is to not DCHECK for nullpointers if you
dereference the nullptr in the same scope, because that will fail anyways on
line 57.

Done.

Ken Rockot(use gerrit already)

Looks thoroughly awesome in general. Didn't get a chance to review the tests yet, but ...

3 years, 8 months ago (2017-04-10 23:58:00 UTC) #7

oystein (OOO til 10th of July)

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h File services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h (right): https://codereview.chromium.org/2798713002/diff/20001/services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h#newcode13 services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h:13: IPC_ENUM_TRAITS_MIN_MAX_VALUE( On 2017/04/10 at 23:58:00, Ken Rockot wrote: > ...

3 years, 8 months ago (2017-04-11 00:11:58 UTC) #8

Ken Rockot(use gerrit already)

On 2017/04/11 at 00:11:58, oysteine wrote: > https://codereview.chromium.org/2798713002/diff/20001/services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h > File services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h (right): > > https://codereview.chromium.org/2798713002/diff/20001/services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h#newcode13 ...

3 years, 8 months ago (2017-04-11 17:46:09 UTC) #9

On 2017/04/11 at 00:11:58, oysteine wrote:
>
https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
> File
services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h
(right):
> 
>
https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
> services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h:13:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(
> On 2017/04/10 at 23:58:00, Ken Rockot wrote:
> > I am thoroughly confused. Why are you introducing a typemap and traits for
these enums? Why not just define the enum in mojom and have consumers use the
generated enum type directly?
> 
> Separate reasons for each enum basically:
> * For the events, I'm expecting the number of different ones to grow to a
sizable amount over time. As far as I can tell every modification to a .mojom
file requires a security lgtm, which seems unnecessary for new enum entries and
I'm worried about a chilling effect on utilizing this. Is there an accepted way
to avoid needing security LGTM's on new enum entries (as in not do the per-file
*mojom=file:/securitypeople thing for mojom with just the enum), or is there a
good reason for having them still?

That would be annoying, and I would encourage security TBR for such changes, of
course that requires good discretion. This is however quite a heavy-handed way
to circumvent the issue. For now I'd do TBR and we should figure out a better
solution, e.g. a way to add per-file exemptions to the SECURITY_OWNERS rules.

> 
> * For the type, skyostil@ and lucky-luke folks wants to use CoordinationUnits
for cost attribution at a fairly basic scheduler level; essentially
CoordinationUnits which are too granular to have their own messagepipes but will
have a parent channel to push events through (this is fairly preliminary plans
btw, still under discussion). I made the type and the CoordinationUnitID struct
a native type as it's then likely to be needed under //base, but maybe they can
comment on whether that'll likely be needed or not [hereby poking skyostil@ +
chrisha@]. That said, we're not likely to add new types as frequently as we do
new events, so I don't mind having both a native struct and a mojom struct that
we typemap between if that's the preferred way.

I think it would be preferable to have a real mojom definition of the type enum.
If it ends up mirroring some native enum defined in //base, we can address that
later. Note that [Native] or anything like it strictly constrains the type to
being used only from C++ code. Won't we also eventually care about using
coordination units from Java and maybe even JS code? Having a mojom definition
means we get equivalent definitions generated for those languages.

The same logic applies to the struct, and in general anyway we do not want
anything in //services depending on //ipc. We should probably add an explicit
"-ipc" to services/DEPS to that effect.

Ken Rockot(use gerrit already)

On 2017/04/11 at 17:46:09, Ken Rockot wrote: > On 2017/04/11 at 00:11:58, oysteine wrote: > ...

3 years, 8 months ago (2017-04-11 17:46:56 UTC) #10

On 2017/04/11 at 17:46:09, Ken Rockot wrote:
> On 2017/04/11 at 00:11:58, oysteine wrote:
> >
https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
> > File
services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h
(right):
> > 
> >
https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
> >
services/resource_coordinator/public/cpp/coordination_unit_struct_traits.h:13:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(
> > On 2017/04/10 at 23:58:00, Ken Rockot wrote:
> > > I am thoroughly confused. Why are you introducing a typemap and traits for
these enums? Why not just define the enum in mojom and have consumers use the
generated enum type directly?
> > 
> > Separate reasons for each enum basically:
> > * For the events, I'm expecting the number of different ones to grow to a
sizable amount over time. As far as I can tell every modification to a .mojom
file requires a security lgtm, which seems unnecessary for new enum entries and
I'm worried about a chilling effect on utilizing this. Is there an accepted way
to avoid needing security LGTM's on new enum entries (as in not do the per-file
*mojom=file:/securitypeople thing for mojom with just the enum), or is there a
good reason for having them still?
> 
> That would be annoying, and I would encourage security TBR for such changes,
of course that requires good discretion. This is however quite a heavy-handed
way to circumvent the issue. For now I'd do TBR and we should figure out a
better solution, e.g. a way to add per-file exemptions to the SECURITY_OWNERS
rules.
> 
> > 
> > * For the type, skyostil@ and lucky-luke folks wants to use
CoordinationUnits for cost attribution at a fairly basic scheduler level;
essentially CoordinationUnits which are too granular to have their own
messagepipes but will have a parent channel to push events through (this is
fairly preliminary plans btw, still under discussion). I made the type and the
CoordinationUnitID struct a native type as it's then likely to be needed under
//base, but maybe they can comment on whether that'll likely be needed or not
[hereby poking skyostil@ + chrisha@]. That said, we're not likely to add new
types as frequently as we do new events, so I don't mind having both a native
struct and a mojom struct that we typemap between if that's the preferred way.
> 
> I think it would be preferable to have a real mojom definition of the type
enum. If it ends up mirroring some native enum defined in //base, we can address
that later. Note that [Native] or anything like it strictly constrains the type
to being used only from C++ code. Won't we also eventually care about using
coordination units from Java and maybe even JS code? Having a mojom definition
means we get equivalent definitions generated for those languages.
> 
> The same logic applies to the struct, and in general anyway we do not want
anything in //services depending on //ipc. We should probably add an explicit
"-ipc" to services/DEPS to that effect.

(UI service gets a pass because it unfortunately starting doing this before we
established such guidelines)

Primiano Tucci (use gerrit)

Took another pass. Mostly minor comments below (% GetProcessId which I think returns always 1 ...

3 years, 8 months ago (2017-04-11 18:04:52 UTC) #11

Took another pass. Mostly minor comments below (% GetProcessId which I think
returns always 1 under Linux because of the uid namespace isolation when using
the sandbox).

Oystein and I had some initial chat offline. I think the only thing it's
worrying me right now is that, IIUC everything correctly, we'll end up with
various CU instances that have parent<>child relationships and are exposed to
various clients, but live on different sequences on the service sides (Because
each mojo pipe is a different sequence IIRC). I am a bit scared on how all that
will be kept in sync. I really don't know enough of mojo to make any more
thoughtful comment.

If this was pre-mojo, he old-school part of me would have expected:
- all the CUs to live on a thread (or just a SequencedTaskRunner) on the service
process
- all clients to do IPCs to the service process, based on a handle that
represents the CU (very likely your CUID)
- all those IPCs to be received and processed on the service process on the same
STT

now, not sure if this model makes sense to mojo, and if it does, how that should
translate.
Probably this is just because you are pretending (to your API clients) that the
CreateCU method is synchronous, while it's not, which requires that GetId dance
to re-linearize?
I guess my question is: say that you have a method in CU that needs to return
something to the client (say GetState()). How are you going to deal with that?
That will have to expose a base::callback in its interface right?

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:28:
CoordinationUnitImpl::CoordinationUnitImpl(
On 2017/04/10 20:02:59, oystein wrote:
> On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> > on which thread are these CUnits built and destroyed?
> > By the lack of locking, it seems that the assumptions is that everything
> happens on the same thread.
> > Maybe you could have a global threadchecker (or sequencechecker) next to the
> map to make sure we don't accidentally bereak this in future (threading did
bite
> me lot of times in the past in memory infra, especially in the cases where it
> was out of my control and up to the clients)
> 
> Mojo guarantees it'll all be run on the same taskrunner, but I added a
> threadchecker to the ResourceCoordinatorInterface since that's client code and
> could potentially be used from multiple threads.

Acknowledged.

https://codereview.chromium.org/2798713002/diff/1/services/resource_coordinat...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:35:
DCHECK(cu_map.end() == cu_map.find(id_hash_));
On 2017/04/10 20:02:57, oystein wrote:
> On 2017/04/06 at 18:09:46, Primiano Tucci wrote:
> > really a minor perf thingy, as this is debug only, but I think that
> map.count(id_hash) == 0 is slightly faster (or generally not slower than
.find)
> as doesn't have to build up an iterator.ù
> > 
> > Or eventually you could just
> > auto it = cu_map.insert(std::make_pair(id_hash_, this));
> > DCHECK(it.second /* did insert*/);
> 
> Done; didn't realize this but makes sense in hindsight given that
unordered_map
> won't have duplicate elements and could in theory just stop at the first
> encountered element and return 1. Some surface Googling suggests count() may
> often be implemented using find() internally, but I went with the second
> suggestion regardless since it looks a little cleaner.

Acknowledged.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:8:
#include "base/lazy_instance.h"
I think you don't need this anymore

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:34:
DCHECK(it.second); /* did insert */
micro nit: /* -> // , I usually see this for single line comments

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:42:
for (auto* child : children_) {
not very strong on this, but usually I find the full type (i.e.
s/auto*/CoordinationUnitImpl*/) easier to read, as long as it says in one line
(read: I use auto only if the full type wouldn't fit in 80 cols). But we don't
have a rule for this, so this is fine as well

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:152:
DCHECK(parents_.find(parent) == parents_.end());
maybe just use .count() for uniformity with ::AddChild?

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:20: id =
MurmurHash2(&new_id.front(), new_id.size(), kMurmur2HashSeed);
if you initialize this in the initializer list ou can make both id and type
const, which would make this class immutable, which might help reason about it
and avoid distraction bugs. (or maybe I'm missing something and this class needs
to be mutable, in which case ignore my comment)

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21: uint64_t
operator()(const CoordinationUnitID& id) const;
this one doesn't seem to be defined in the .cc file. I guess is a leftover from
the previous way of doing the hash

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:31:
std::string id = base::IntToString(base::GetCurrentProcId()) + "." +
isn't GetCurrentProcId going to be always 1 in the renderers because of sandbox?
Thinking more I would have expected the id to be generated by the service after
registration, not by the client.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.h
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/resource_coordinator_interface.h:42: //
The WeakPtrFactory should come last so the weak ptrs are invalidated
isn't this the other way round (fiedls in the bottom are constructed last and
destructed first)?
Anyhow feels a bit fragile to depend on the order of fields, why do you need
this at all?
As long as everything happens in the same thread, there should be never a moment
when you see the pointer but not the rest of the struct.
What is the case that makes this guarantee necessary?

oystein (OOO til 10th of July)

On 2017/04/11 at 18:04:52, primiano wrote: > Took another pass. Mostly minor comments below (% ...

3 years, 8 months ago (2017-04-12 17:18:02 UTC) #12

On 2017/04/11 at 18:04:52, primiano wrote:
> Took another pass. Mostly minor comments below (% GetProcessId which I think
returns always 1 under Linux because of the uid namespace isolation when using
the sandbox).
> 
> Oystein and I had some initial chat offline. I think the only thing it's
worrying me right now is that, IIUC everything correctly, we'll end up with
various CU instances that have parent<>child relationships and are exposed to
various clients, but live on different sequences on the service sides (Because
each mojo pipe is a different sequence IIRC). I am a bit scared on how all that
will be kept in sync. I really don't know enough of mojo to make any more
thoughtful comment.
> 
> If this was pre-mojo, he old-school part of me would have expected:
> - all the CUs to live on a thread (or just a SequencedTaskRunner) on the
service process
> - all clients to do IPCs to the service process, based on a handle that
represents the CU (very likely your CUID)
> - all those IPCs to be received and processed on the service process on the
same STT
> 
> now, not sure if this model makes sense to mojo, and if it does, how that
should translate.
> Probably this is just because you are pretending (to your API clients) that
the CreateCU method is synchronous, while it's not, which requires that GetId
dance to re-linearize?
> I guess my question is: say that you have a method in CU that needs to return
something to the client (say GetState()). How are you going to deal with that?
That will have to expose a base::callback in its interface right?
> 

Quick reply to this while I'm working on the rest of the fixes:

I think fundamentally the idea of async and out-of-order-ness between different
CUs is something that has to be fundamentally a part of the backend service, as
different CUs with parent/child relationships will at times live in different
processes anyway. Relying on ordering of messages in specific groups of CUs just
because their clients happen to live in the same process sounds fairly broken to
me, so I actually like the idea that this forces us to treat everything
identically.

As to specifically the issue of CreateCU being sync from the point of view of
consumers of the ResourceCoordinatorInterface and the AddChild round-trip: The
only alternatives I see to this is something like:

class ResourceCoordinatorInterface {
   ResourceCoordinatorInterface(Callback OnCUIDReady);

   void SendEvent(Event foo);

   // Only call the below functions once OnCUIDReady has been called by the
child)
   void AddChild(child);
};

... which seems iffy to me. Or, on the service-side, we could have a map of
pending relationships: if AddChild() is called with a child which hasn't (yet?)
been created, we add the relationship to a map and do a lookup there every time
we create a CU. This would work but adds a fair bit of complexity as you'd have
to deal with edgecases like CUs which aren't found not because they haven't been
created yet, but because they've been already destroyed, which means having to
deal with cleaning up this map etc (probably adding some sequence numbering into
the mix or something like that) and gets smelly.

Having GetState(Callback meh) for getters in the interface seems fairly
straightforward as well? That's still a pretty clear and understandable API for
clients to deal with.

oystein (OOO til 10th of July)

On 2017/04/12 at 17:18:02, oystein wrote: > On 2017/04/11 at 18:04:52, primiano wrote: > > ...

3 years, 8 months ago (2017-04-12 17:25:23 UTC) #13

On 2017/04/12 at 17:18:02, oystein wrote:
> On 2017/04/11 at 18:04:52, primiano wrote:
> > Took another pass. Mostly minor comments below (% GetProcessId which I think
returns always 1 under Linux because of the uid namespace isolation when using
the sandbox).
> > 
> > Oystein and I had some initial chat offline. I think the only thing it's
worrying me right now is that, IIUC everything correctly, we'll end up with
various CU instances that have parent<>child relationships and are exposed to
various clients, but live on different sequences on the service sides (Because
each mojo pipe is a different sequence IIRC). I am a bit scared on how all that
will be kept in sync. I really don't know enough of mojo to make any more
thoughtful comment.
> > 
> > If this was pre-mojo, he old-school part of me would have expected:
> > - all the CUs to live on a thread (or just a SequencedTaskRunner) on the
service process
> > - all clients to do IPCs to the service process, based on a handle that
represents the CU (very likely your CUID)
> > - all those IPCs to be received and processed on the service process on the
same STT
> > 
> > now, not sure if this model makes sense to mojo, and if it does, how that
should translate.
> > Probably this is just because you are pretending (to your API clients) that
the CreateCU method is synchronous, while it's not, which requires that GetId
dance to re-linearize?
> > I guess my question is: say that you have a method in CU that needs to
return something to the client (say GetState()). How are you going to deal with
that? That will have to expose a base::callback in its interface right?
> > 
> 
> Quick reply to this while I'm working on the rest of the fixes:
> 
> I think fundamentally the idea of async and out-of-order-ness between
different CUs is something that has to be fundamentally a part of the backend
service, as different CUs with parent/child relationships will at times live in
different processes anyway. Relying on ordering of messages in specific groups
of CUs just because their clients happen to live in the same process sounds
fairly broken to me, so I actually like the idea that this forces us to treat
everything identically.
> 
> As to specifically the issue of CreateCU being sync from the point of view of
consumers of the ResourceCoordinatorInterface and the AddChild round-trip: The
only alternatives I see to this is something like:
> 
> class ResourceCoordinatorInterface {
>    ResourceCoordinatorInterface(Callback OnCUIDReady);
> 
>    void SendEvent(Event foo);
> 
>    // Only call the below functions once OnCUIDReady has been called by the
child)
>    void AddChild(child);
> };
> 
> ... which seems iffy to me. Or, on the service-side, we could have a map of
pending relationships: if AddChild() is called with a child which hasn't (yet?)
been created, we add the relationship to a map and do a lookup there every time
we create a CU. This would work but adds a fair bit of complexity as you'd have
to deal with edgecases like CUs which aren't found not because they haven't been
created yet, but because they've been already destroyed, which means having to
deal with cleaning up this map etc (probably adding some sequence numbering into
the mix or something like that) and gets smelly.
> 
> Having GetState(Callback meh) for getters in the interface seems fairly
straightforward as well? That's still a pretty clear and understandable API for
clients to deal with.

Third alternative:

 class ResourceCoordinatorInterface {
    ResourceCoordinatorInterface();
 
    void SendEvent(Event foo);
 
    void GetCUID(Callback onCUIDGotten);
    // Only call the below functions once OnCUIDReady has been called by the
child)
    void AddChild(CUID child);
 };

...which means that the clients of this have to deal with the async nature of
this much more directly. I think it's a cleaner API if we have as a constraint
that any function in the ResourceCoordinatorInterface (RCI) API which deals with
other RCIs, require an actual RCI argument rather than a CUID argument, so that
we can always deal with the async nature of this within the RCI itself rather
than forcing the client code to deal with it.

oystein (OOO til 10th of July)

On 2017/04/12 at 17:25:23, oystein wrote: > On 2017/04/12 at 17:18:02, oystein wrote: > > ...

3 years, 8 months ago (2017-04-12 17:25:55 UTC) #14

On 2017/04/12 at 17:25:23, oystein wrote:
> On 2017/04/12 at 17:18:02, oystein wrote:
> > On 2017/04/11 at 18:04:52, primiano wrote:
> > > Took another pass. Mostly minor comments below (% GetProcessId which I
think returns always 1 under Linux because of the uid namespace isolation when
using the sandbox).
> > > 
> > > Oystein and I had some initial chat offline. I think the only thing it's
worrying me right now is that, IIUC everything correctly, we'll end up with
various CU instances that have parent<>child relationships and are exposed to
various clients, but live on different sequences on the service sides (Because
each mojo pipe is a different sequence IIRC). I am a bit scared on how all that
will be kept in sync. I really don't know enough of mojo to make any more
thoughtful comment.
> > > 
> > > If this was pre-mojo, he old-school part of me would have expected:
> > > - all the CUs to live on a thread (or just a SequencedTaskRunner) on the
service process
> > > - all clients to do IPCs to the service process, based on a handle that
represents the CU (very likely your CUID)
> > > - all those IPCs to be received and processed on the service process on
the same STT
> > > 
> > > now, not sure if this model makes sense to mojo, and if it does, how that
should translate.
> > > Probably this is just because you are pretending (to your API clients)
that the CreateCU method is synchronous, while it's not, which requires that
GetId dance to re-linearize?
> > > I guess my question is: say that you have a method in CU that needs to
return something to the client (say GetState()). How are you going to deal with
that? That will have to expose a base::callback in its interface right?
> > > 
> > 
> > Quick reply to this while I'm working on the rest of the fixes:
> > 
> > I think fundamentally the idea of async and out-of-order-ness between
different CUs is something that has to be fundamentally a part of the backend
service, as different CUs with parent/child relationships will at times live in
different processes anyway. Relying on ordering of messages in specific groups
of CUs just because their clients happen to live in the same process sounds
fairly broken to me, so I actually like the idea that this forces us to treat
everything identically.
> > 
> > As to specifically the issue of CreateCU being sync from the point of view
of consumers of the ResourceCoordinatorInterface and the AddChild round-trip:
The only alternatives I see to this is something like:
> > 
> > class ResourceCoordinatorInterface {
> >    ResourceCoordinatorInterface(Callback OnCUIDReady);
> > 
> >    void SendEvent(Event foo);
> > 
> >    // Only call the below functions once OnCUIDReady has been called by the
child)
> >    void AddChild(child);
> > };
> > 
> > ... which seems iffy to me. Or, on the service-side, we could have a map of
pending relationships: if AddChild() is called with a child which hasn't (yet?)
been created, we add the relationship to a map and do a lookup there every time
we create a CU. This would work but adds a fair bit of complexity as you'd have
to deal with edgecases like CUs which aren't found not because they haven't been
created yet, but because they've been already destroyed, which means having to
deal with cleaning up this map etc (probably adding some sequence numbering into
the mix or something like that) and gets smelly.
> > 
> > Having GetState(Callback meh) for getters in the interface seems fairly
straightforward as well? That's still a pretty clear and understandable API for
clients to deal with.
> 
> Third alternative:
> 
>  class ResourceCoordinatorInterface {
>     ResourceCoordinatorInterface();
>  
>     void SendEvent(Event foo);
>  
>     void GetCUID(Callback onCUIDGotten);
>     // Only call the below functions once OnCUIDReady has been called by the
child)
>     void AddChild(CUID child);
>  };
> 
> ...which means that the clients of this have to deal with the async nature of
this much more directly. I think it's a cleaner API if we have as a constraint
that any function in the ResourceCoordinatorInterface (RCI) API which deals with
other RCIs, require an actual RCI argument rather than a CUID argument, so that
we can always deal with the async nature of this within the RCI itself rather
than forcing the client code to deal with it.

oystein (OOO til 10th of July)

On 2017/04/12 at 17:25:55, oystein wrote: > On 2017/04/12 at 17:25:23, oystein wrote: > > ...

3 years, 8 months ago (2017-04-12 17:27:16 UTC) #15

On 2017/04/12 at 17:25:55, oystein wrote:
> On 2017/04/12 at 17:25:23, oystein wrote:
> > On 2017/04/12 at 17:18:02, oystein wrote:
> > > On 2017/04/11 at 18:04:52, primiano wrote:
> > > > Took another pass. Mostly minor comments below (% GetProcessId which I
think returns always 1 under Linux because of the uid namespace isolation when
using the sandbox).
> > > > 
> > > > Oystein and I had some initial chat offline. I think the only thing it's
worrying me right now is that, IIUC everything correctly, we'll end up with
various CU instances that have parent<>child relationships and are exposed to
various clients, but live on different sequences on the service sides (Because
each mojo pipe is a different sequence IIRC). I am a bit scared on how all that
will be kept in sync. I really don't know enough of mojo to make any more
thoughtful comment.
> > > > 
> > > > If this was pre-mojo, he old-school part of me would have expected:
> > > > - all the CUs to live on a thread (or just a SequencedTaskRunner) on the
service process
> > > > - all clients to do IPCs to the service process, based on a handle that
represents the CU (very likely your CUID)
> > > > - all those IPCs to be received and processed on the service process on
the same STT
> > > > 
> > > > now, not sure if this model makes sense to mojo, and if it does, how
that should translate.
> > > > Probably this is just because you are pretending (to your API clients)
that the CreateCU method is synchronous, while it's not, which requires that
GetId dance to re-linearize?
> > > > I guess my question is: say that you have a method in CU that needs to
return something to the client (say GetState()). How are you going to deal with
that? That will have to expose a base::callback in its interface right?
> > > > 
> > > 
> > > Quick reply to this while I'm working on the rest of the fixes:
> > > 
> > > I think fundamentally the idea of async and out-of-order-ness between
different CUs is something that has to be fundamentally a part of the backend
service, as different CUs with parent/child relationships will at times live in
different processes anyway. Relying on ordering of messages in specific groups
of CUs just because their clients happen to live in the same process sounds
fairly broken to me, so I actually like the idea that this forces us to treat
everything identically.
> > > 
> > > As to specifically the issue of CreateCU being sync from the point of view
of consumers of the ResourceCoordinatorInterface and the AddChild round-trip:
The only alternatives I see to this is something like:
> > > 
> > > class ResourceCoordinatorInterface {
> > >    ResourceCoordinatorInterface(Callback OnCUIDReady);
> > > 
> > >    void SendEvent(Event foo);
> > > 
> > >    // Only call the below functions once OnCUIDReady has been called by
the child)
> > >    void AddChild(child);
> > > };
> > > 
> > > ... which seems iffy to me. Or, on the service-side, we could have a map
of pending relationships: if AddChild() is called with a child which hasn't
(yet?) been created, we add the relationship to a map and do a lookup there
every time we create a CU. This would work but adds a fair bit of complexity as
you'd have to deal with edgecases like CUs which aren't found not because they
haven't been created yet, but because they've been already destroyed, which
means having to deal with cleaning up this map etc (probably adding some
sequence numbering into the mix or something like that) and gets smelly.
> > > 
> > > Having GetState(Callback meh) for getters in the interface seems fairly
straightforward as well? That's still a pretty clear and understandable API for
clients to deal with.
> > 
> > Third alternative:
> > 
> >  class ResourceCoordinatorInterface {
> >     ResourceCoordinatorInterface();
> >  
> >     void SendEvent(Event foo);
> >  
> >     void GetCUID(Callback onCUIDGotten);
> >     // Only call the below functions once OnCUIDReady has been called by the
child)
> >     void AddChild(CUID child);
> >  };
> > 
> > ...which means that the clients of this have to deal with the async nature
of this much more directly. I think it's a cleaner API if we have as a
constraint that any function in the ResourceCoordinatorInterface (RCI) API which
deals with other RCIs, require an actual RCI argument rather than a CUID
argument, so that we can always deal with the async nature of this within the
RCI itself rather than forcing the client code to deal with it.

(The last alternative there also gets confusing in the cases where the CUID is
user-constructed rather than generated, in which case you'd have to make it a
constraint of the API that the clients can't use the CUID they themselves
generated for AddChild, they have to use the one they get from GetCUID. Which
gets confusing. Or you'd have to deal with separate CUID types or something.

oystein (OOO til 10th of July)

Thanks guys! New version up; ptal! https://codereview.chromium.org/2798713002/diff/20001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2798713002/diff/20001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc#newcode8 services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:8: #include "base/lazy_instance.h" On ...

3 years, 8 months ago (2017-04-12 19:15:31 UTC) #16

Thanks guys!

New version up; ptal!

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:8:
#include "base/lazy_instance.h"
On 2017/04/11 at 18:04:51, Primiano Tucci wrote:
> I think you don't need this anymore

Done.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:34:
DCHECK(it.second); /* did insert */
On 2017/04/11 at 18:04:51, Primiano Tucci wrote:
> micro nit: /* -> // , I usually see this for single line comments

Done.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:42:
for (auto* child : children_) {
On 2017/04/11 at 18:04:51, Primiano Tucci wrote:
> not very strong on this, but usually I find the full type (i.e.
s/auto*/CoordinationUnitImpl*/) easier to read, as long as it says in one line
(read: I use auto only if the full type wouldn't fit in 80 cols). But we don't
have a rule for this, so this is fine as well

Done.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:152:
DCHECK(parents_.find(parent) == parents_.end());
On 2017/04/11 at 18:04:51, Primiano Tucci wrote:
> maybe just use .count() for uniformity with ::AddChild?

Done.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:20: id =
MurmurHash2(&new_id.front(), new_id.size(), kMurmur2HashSeed);
On 2017/04/11 at 18:04:51, Primiano Tucci wrote:
> if you initialize this in the initializer list ou can make both id and type
const, which would make this class immutable, which might help reason about it
and avoid distraction bugs. (or maybe I'm missing something and this class needs
to be mutable, in which case ignore my comment)

Doesn't work with the Mojo serialization/deserialization stuff.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/coordination_unit_id.h (right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/coordination_unit_id.h:21: uint64_t
operator()(const CoordinationUnitID& id) const;
On 2017/04/11 at 18:04:51, Primiano Tucci wrote:
> this one doesn't seem to be defined in the .cc file. I guess is a leftover
from the previous way of doing the hash

Yep, done.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:31:
std::string id = base::IntToString(base::GetCurrentProcId()) + "." +
On 2017/04/11 at 18:04:52, Primiano Tucci wrote:
> isn't GetCurrentProcId going to be always 1 in the renderers because of
sandbox?
> Thinking more I would have expected the id to be generated by the service
after registration, not by the client.

Yep that makes perfect sense; moved to the service, if the provided id is 0.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.h
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/cpp/resource_coordinator_interface.h:42: //
The WeakPtrFactory should come last so the weak ptrs are invalidated
On 2017/04/11 at 18:04:52, Primiano Tucci wrote:
> isn't this the other way round (fiedls in the bottom are constructed last and
destructed first)?
> Anyhow feels a bit fragile to depend on the order of fields, why do you need
this at all?
> As long as everything happens in the same thread, there should be never a
moment when you see the pointer but not the rest of the struct.
> What is the case that makes this guarantee necessary?

I mixed up the wording; the WeakPtrs should be destructed *before* the rest of
the members:
https://cs.chromium.org/webrtc/src/base/memory/weak_ptr.h?q=WeakPtrFactory+sh...

It shouldn't be an issue in this specific case, more of a general
safeguard/habit in case the destruction of the other members end up referring
back to a WeakPtr, I assume (on the same thread).

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom
(right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom:10:
CreateCoordinationUnit(CoordinationUnit& request, CoordinationUnitID options);
On 2017/04/10 at 23:58:00, Ken Rockot wrote:
> nit: s/options/id/ ?

Leftover; done.

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
File services/resource_coordinator/resource_coordinator_service.cc (right):

https://codereview.chromium.org/2798713002/diff/20001/services/resource_coord...
services/resource_coordinator/resource_coordinator_service.cc:38: bool
ResourceCoordinatorService::OnConnect(
On 2017/04/10 at 23:58:00, Ken Rockot wrote:
> Please just implement OnBindInterface instead of OnConnect, and have client
code use BindInterface. You can own your own single InterfaceRegistry which you
initialize in the ctor instead (and forward incoming OnBindInterface args to
InterfaceRegistry::BindInterface). Then there's no need to do this stupid junk
with the OnConnectionLost to keep alive the service. We plan to delete the
concept of a service "connection" and thus delete Service::OnConnect.

Done.

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 19:17:06 UTC) #17

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 19:18:33 UTC) #18

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 19:20:27 UTC) #19

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 19:21:05 UTC) #20

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 19:48:39 UTC) #21

commit-bot: I haz the power

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_swarming_rel/builds/156067) linux_android_rel_ng on ...

3 years, 8 months ago (2017-04-12 19:48:40 UTC) #22

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 21:06:48 UTC) #23

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 21:07:15 UTC) #24

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 21:15:41 UTC) #25

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 21:16:21 UTC) #26

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 21:34:19 UTC) #27

commit-bot: I haz the power

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_linux/builds/346175)

3 years, 8 months ago (2017-04-12 21:34:20 UTC) #28

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 21:57:25 UTC) #29

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 21:58:15 UTC) #30

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 22:38:36 UTC) #31

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 22:40:11 UTC) #32

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 22:53:56 UTC) #33

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/246780) mac_chromium_rel_ng on ...

3 years, 8 months ago (2017-04-12 22:53:57 UTC) #34

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 22:56:42 UTC) #35

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 22:57:46 UTC) #36

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 23:10:21 UTC) #37

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/246796) cast_shell_linux on ...

3 years, 8 months ago (2017-04-12 23:10:22 UTC) #38

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 23:26:23 UTC) #39

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 23:27:01 UTC) #40

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-12 23:45:32 UTC) #41

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-12 23:46:22 UTC) #42

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 00:13:37 UTC) #43

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 00:13:38 UTC) #44

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-13 01:26:44 UTC) #45

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 01:27:45 UTC) #46

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 01:58:52 UTC) #47

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 01:58:53 UTC) #48

Primiano Tucci (use gerrit)

oysteine@ and I had a chat offline about the Async GetID stuff. Essentially any alternative ...

3 years, 8 months ago (2017-04-13 17:22:19 UTC) #49

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-13 17:38:56 UTC) #50

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 17:39:59 UTC) #51

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 18:46:40 UTC) #52

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 18:46:41 UTC) #53

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-13 21:20:53 UTC) #54

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 21:21:26 UTC) #55

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 22:27:31 UTC) #56

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-13 22:27:32 UTC) #57

oystein (OOO til 10th of July)

On 2017/04/13 at 17:22:19, primiano wrote: > oysteine@ and I had a chat offline about ...

3 years, 8 months ago (2017-04-14 22:07:02 UTC) #58

On 2017/04/13 at 17:22:19, primiano wrote:
> oysteine@ and I had a chat offline about the Async GetID stuff. Essentially
any alternative we thought about would have implied extra complications. since
we don't know yet how all the future use cases will look like in details does't
make sense to over design something too complex and that roundtrip seems a good
tradeoff for the moment.
> 
> LGTM for the all the GRC parts. Know very little about mojo, but you seem to
have the right people to cover that.

Thanks!

Back to rockot, friendly ping. Also if anyone has *any* idea what could cause
this .apk build error on Android dbg for resource_coordinator_unittests I'd be
eternally grateful... (on the trybots, error pasted below).

FAILED:
resource_coordinator_unittests_apk/resource_coordinator_unittests-debug.apk 
python ../../build/android/gyp/finalize_apk.py --depfile
gen/services/resource_coordinator/resource_coordinator_unittests_apk__create__finalize.d
--zipalign-path ../../third_party/android_tools/sdk/build-tools/25.0.2/zipalign
--unsigned-apk-path
gen/services/resource_coordinator/resource_coordinator_unittests_apk/resource_coordinator_unittests_apk.apk_intermediates.unfinished.apk
--final-apk-path
resource_coordinator_unittests_apk/resource_coordinator_unittests-debug.apk
--key-path ../../build/android/ant/chromium-debug.keystore --key-name
chromiumdebugkey --key-passwd chromium
Traceback (most recent call last):
  File "../../build/android/gyp/finalize_apk.py", line 123, in <module>
    sys.exit(main(sys.argv[1:]))
  File "../../build/android/gyp/finalize_apk.py", line 89, in main
    output_paths=[options.final_apk_path])
  File
"/b/c/b/android_clang_dbg_recipe/src/build/android/gyp/util/build_utils.py",
line 583, in CallAndWriteDepfileIfStale
    pass_changes=True)
  File
"/b/c/b/android_clang_dbg_recipe/src/build/android/gyp/util/md5_check.py", line
87, in CallAndRecordIfStale
    function(*args)
  File
"/b/c/b/android_clang_dbg_recipe/src/build/android/gyp/util/build_utils.py",
line 566, in on_stale_md5
    function(*args)
  File "../../build/android/gyp/finalize_apk.py", line 84, in <lambda>
    lambda: FinalizeApk(options),
  File "../../build/android/gyp/finalize_apk.py", line 115, in FinalizeApk
    options.unsigned_apk_path, signed_apk_path)
  File "../../build/android/gyp/finalize_apk.py", line 36, in JarSigner
    build_utils.CheckOutput(sign_cmd)
  File
"/b/c/b/android_clang_dbg_recipe/src/build/android/gyp/util/build_utils.py",
line 170, in CheckOutput
    raise CalledProcessError(cwd, args, stdout + stderr)
util.build_utils.CalledProcessError: Command failed: ( cd
/b/c/b/android_clang_dbg_recipe/src/out/Debug; jarsigner -sigalg MD5withRSA
-digestalg SHA1 -keystore ../../build/android/ant/chromium-debug.keystore
-storepass chromium /tmp/tmphH5YJG chromiumdebugkey )
jarsigner: unable to sign jar: java.util.zip.ZipException: duplicate entry:
lib/armeabi-v7a/libbase.cr.so

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-17 22:00:46 UTC) #59

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-17 22:01:18 UTC) #60

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-17 22:48:23 UTC) #61

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-17 22:48:43 UTC) #62

oystein (OOO til 10th of July)

oysteine@chromium.org changed reviewers: + dcheng@chromium.org

3 years, 8 months ago (2017-04-17 22:49:22 UTC) #63

oystein (OOO til 10th of July)

Yay, .apk issue fixed, looks like the trybots are all green now. rockot: Ready for ...

3 years, 8 months ago (2017-04-17 22:50:09 UTC) #64

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-18 00:29:48 UTC) #65

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-18 00:29:49 UTC) #66

dcheng

If I understand correctly, child processes are responsible for generating the IDs. Is this a ...

3 years, 8 months ago (2017-04-18 05:40:47 UTC) #67

oystein (OOO til 10th of July)

oysteine@chromium.org changed reviewers: + nasko@chromium.org

3 years, 8 months ago (2017-04-18 20:47:37 UTC) #68

oystein (OOO til 10th of July)

Thanks dcheng! +nasko as dcheng@ is going OOO. On 2017/04/18 at 05:40:47, dcheng wrote: > ...

3 years, 8 months ago (2017-04-18 20:55:34 UTC) #69

Thanks dcheng!

+nasko as dcheng@ is going OOO.

On 2017/04/18 at 05:40:47, dcheng wrote:
> If I understand correctly, child processes are responsible for generating the
IDs. Is this a requirement, especially if we're round-tripping to the CUProvider
anyway?

The idea was that different pieces of client code could generate the same IDs
that would then correspond to the same CoordinationUnits within the service
(validated by the browser process); the use-case which has been brought up is
CUs that are per-origin and not per-tab/frame in case we want to do site-wide
attribution of scheduler cost, for example. I suspect this is something that'll
evolve over time.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:21:
return instance;
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> Consider returning a ref, since this is never null.

Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:32:
base::IntToString(base::GetCurrentProcId()) + "." +
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> I don't entirely understand the purpose of building this string and then
hashing it.
> 
> Why not just generate completely random IDs in this case? We have
mojo.common.mojom.UnguessableToken for this.
> 
> (I'm assuming this is to support the 'create initial token' vs 'get existing
token' use case, but perhaps that would also be useful to explicitly call out in
the comments)

UnguessableToken makes more sense; this was more an artifact from when I was
generating this client-side and wanted to ensure we didn't have collisions.
Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:41:
service_ref_ = std::move(service_ref);
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc:17:
if (service_ref_factory)
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> I'm assuming this is one of those null for testing things... is there any way
to just fake this out in the unit test to make the non-test code easier to
understand?

Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc:33:
std::unique_ptr<service_manager::ServiceContextRef> service_ref;
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> #include <memory> (and <utility>) here and elsewhere.

Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:24:
static_cast<std::string::size_type>(std::numeric_limits<int>::max()),
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> Maybe:
> 
> DCHECK(base::IsValueInRangeForNumericType<int>(new_id.size());

Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
File services/resource_coordinator/public/cpp/coordination_unit_struct_traits.cc
(right):

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/cpp/coordination_unit_struct_traits.cc:26:
CHECK(false) << "Invalid type: " << static_cast<uint8_t>(type);
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> NOTREACHED() is more appropriate here.

Done.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
File services/resource_coordinator/public/interfaces/coordination_unit.mojom
(right):

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:9: //
Any new type here needs to be mirrored between coordination_unit_types.h and
On 2017/04/18 at 05:40:46, dcheng (OOO through May 2) wrote:
> Out of curiosity, why not use the mojo enum directly?

The idea is that the CUIDs will become more integrated with scheduler cost
attribution and will to be used by base/ and other places that can't use Mojo,
down the road.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:13:
INVALID_TYPE,
On 2017/04/18 at 05:40:47, dcheng (OOO through May 2) wrote:
> This doesn't seem to be used. Is it needed?

Nope really; removed.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
File services/resource_coordinator/public/interfaces/events.mojom (right):

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/interfaces/events.mojom:8: INVALID_EVENT,
On 2017/04/18 at 05:40:47, dcheng (OOO through May 2) wrote:
> This doesn't seem to be used.

Removed.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/interfaces/events.mojom:9: TEST_EVENT,
On 2017/04/18 at 05:40:47, dcheng (OOO through May 2) wrote:
> Add some comments to the enumerator values here. It's not necessarily clear
what "ON_COMMIT" is.

Expanded the name a bit; I'm expecting the actual enums to go through some
iterations when the service starts being used for more real usecases.

https://codereview.chromium.org/2798713002/diff/320030/services/resource_coor...
services/resource_coordinator/public/interfaces/events.mojom:19: EventType type;
On 2017/04/18 at 05:40:47, dcheng (OOO through May 2) wrote:
> Indent is funny

Ah, tab rather than spaces somehow. Fixed.

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-18 20:55:39 UTC) #70

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-18 20:56:14 UTC) #71

oystein (OOO til 10th of July)

On 2017/04/18 at 20:55:34, oystein wrote: > Thanks dcheng! > > +nasko as dcheng@ is ...

3 years, 8 months ago (2017-04-18 21:00:38 UTC) #72

Ken Rockot(use gerrit already)

Mojo stuff LGTM - sorry about the long delay https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/public/cpp/resource_coordinator_interface.cc File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc (right): https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/public/cpp/resource_coordinator_interface.cc#newcode15 services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:15: ...

3 years, 8 months ago (2017-04-18 21:56:54 UTC) #73

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-18 22:08:07 UTC) #74

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-18 22:08:09 UTC) #75

nasko

There are a few more files I need to go through, but sending some preliminary ...

3 years, 8 months ago (2017-04-21 00:17:33 UTC) #76

dcheng

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc#newcode23 services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:23: CUIDMap& g_cu_map() { On 2017/04/21 00:17:32, nasko wrote: > ...

3 years, 8 months ago (2017-04-21 00:23:17 UTC) #77

oystein (OOO til 10th of July)

Thanks nasko@! On 2017/04/21 at 00:17:33, nasko wrote: > There are a few more files ...

3 years, 8 months ago (2017-04-21 17:57:55 UTC) #78

Thanks nasko@!

On 2017/04/21 at 00:17:33, nasko wrote:
> There are a few more files I need to go through, but sending some preliminary
comments.
> 
> Let's also add an OWNERS file in the services/resource_coordinator/ directory
and ensure that *.mojom files require IPC owners review.

Don't the OWNERS files have to be in the same directory as the .mojom files? I
thought I tried having a single entry in the parent folder this and presubmit
yelled at me, that's why I added the IPC_OWNERS rules to the various
subdirectories.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:23:
CUIDMap& g_cu_map() {
On 2017/04/21 at 00:23:16, dcheng (OOO through May 2) wrote:
> On 2017/04/21 00:17:32, nasko wrote:
> > Can't we just use base::LazyInstance<CUIDMap>?
> 
> I'm technically not here, but the jury is out on this.
> 
> Advantages of LazyInstance<T> / disadvantages of function-local static:
> - Better locality (the allocated instance is in bss, so it doesn't require
following a pointer indirection through bss into the heap)
> - Saves a word (see previous)
> 
> Disadvantages of LazyInstance<T> / advantages of function-local static:
> - Not really standard C++
> - Doesn't do default initialization

Yeah I was originally using LazyInstance, but there seems to be a shift against
that (and primiano suggested the change in #2):
https://groups.google.com/a/chromium.org/forum/#!msg/cxx/j5rFewBzSBQ/JFvyU959...

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:9: #include
"third_party/smhasher/src/MurmurHash2.h"
On 2017/04/21 at 00:17:32, nasko wrote:
> Doesn't std::hash suffice here?

I had that originally as well, primiano in #2 suggested the change as std::hash
has a high chance of collisions when string prefixes of the hashed strings
remain unchanged (which could often be the case here):
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-rXNiOoIU4A...

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:20: const
std::string& new_id)
On 2017/04/21 at 00:17:32, nasko wrote:
> Let's keep the parameter names in sync between header and implementation. It
is listed as "id" in the header.

Done.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File services/resource_coordinator/public/cpp/resource_coordinator_interface.cc
(right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/cpp/resource_coordinator_interface.cc:15:
CHECK(false);
On 2017/04/18 at 21:56:54, Ken Rockot wrote:
> nit: DCHECK?

Done.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File services/resource_coordinator/public/interfaces/coordination_unit.mojom
(right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:11: //
coordination_unit_struct_traits.h/.cc
On 2017/04/21 at 00:17:32, nasko wrote:
> Add a short note as to why this type is duplicated in
coordination_unit_types.h as well.

Done.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:13:
WEBCONTENTS,
On 2017/04/21 at 00:17:32, nasko wrote:
> nit: Maybe start moving to k-prefixed enum values as per
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5AqwVQt-Etg/pBDaP...
Otherwise, WEB_CONTENTS is a bit more readable.

Ah, great. Done. I much prefer the k-prefixed version anyway :).

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit.mojom:21:
int32 id;
On 2017/04/21 at 00:17:32, nasko wrote:
> Would 32bits be good enough for very long lived browser session? A web page
can very easily spam with a pushState() in a tight loop, which is technically
considered a navigation. If we attach an ID to each one of those, attackers can
potentially cause an overflow.

Probably better to err on the side of safety here, so I'm fine with 64bits. We
can re-evaluate if we start using CUIDs at a low enough level of granularity
that it becomes an issue. Done.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom
(right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom:10:
CreateCoordinationUnit(CoordinationUnit& request, CoordinationUnitID id);
On 2017/04/21 at 00:17:32, nasko wrote:
> I know dcheng@ also commented on this, but what if we start out with an ID
generated in the browser process? If it needs to be calculated (based on origin
and such), then we should evaluate whether we have all the information in the
browser process. If we don't, then we are missing information and we likely
should have it, but if at that point we need the renderer process generating it,
it can be done at the time needed with the exact requirements and limitations
clearly articulated.

I'm slightly confused by this; what's the suggestion exactly? In the current
prototype CL I've got the manifests set up in such a way that only the browser
process can call CreateCoordinationUnit; if a renderer process wants a
messagepipe to a CU, it has to bounce through the browser process. So the in the
cases where we don't want auto-generated IDs, the browser process can either
construct it itself, or validate it at that point.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File services/resource_coordinator/public/interfaces/events.mojom (right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/events.mojom:8: TEST_EVENT,
On 2017/04/21 at 00:17:33, nasko wrote:
> nit: Another plug for k-prefixed enum values.

Done.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/events.mojom:10:
ON_WEBCONTENTS_SHOWN,
On 2017/04/21 at 00:17:33, nasko wrote:
> nit: I personally find WEBCONTENTS hard to read and WEB_CONTENTS a bit easier.

Done.

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-21 18:29:53 UTC) #79

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-21 18:30:33 UTC) #80

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-21 19:10:34 UTC) #81

commit-bot: I haz the power

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_swarming_rel/builds/162800)

3 years, 8 months ago (2017-04-21 19:10:35 UTC) #82

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-24 15:50:56 UTC) #83

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-24 15:51:21 UTC) #84

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-24 16:33:09 UTC) #85

commit-bot: I haz the power

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_swarming_rel/builds/163539)

3 years, 8 months ago (2017-04-24 16:33:11 UTC) #86

nasko

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/public/cpp/coordination_unit_id.cc File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right): https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/public/cpp/coordination_unit_id.cc#newcode9 services/resource_coordinator/public/cpp/coordination_unit_id.cc:9: #include "third_party/smhasher/src/MurmurHash2.h" On 2017/04/21 17:57:55, oystein wrote: > On ...

3 years, 8 months ago (2017-04-24 22:10:51 UTC) #87

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File services/resource_coordinator/public/cpp/coordination_unit_id.cc (right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/cpp/coordination_unit_id.cc:9: #include
"third_party/smhasher/src/MurmurHash2.h"
On 2017/04/21 17:57:55, oystein wrote:
> On 2017/04/21 at 00:17:32, nasko wrote:
> > Doesn't std::hash suffice here?
> 
> I had that originally as well, primiano in #2 suggested the change as
std::hash
> has a high chance of collisions when string prefixes of the hashed strings
> remain unchanged (which could often be the case here):
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-rXNiOoIU4A...

Acknowledged.

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom
(right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom:10:
CreateCoordinationUnit(CoordinationUnit& request, CoordinationUnitID id);
On 2017/04/21 17:57:55, oystein wrote:
> On 2017/04/21 at 00:17:32, nasko wrote:
> > I know dcheng@ also commented on this, but what if we start out with an ID
> generated in the browser process? If it needs to be calculated (based on
origin
> and such), then we should evaluate whether we have all the information in the
> browser process. If we don't, then we are missing information and we likely
> should have it, but if at that point we need the renderer process generating
it,
> it can be done at the time needed with the exact requirements and limitations
> clearly articulated.
> 
> I'm slightly confused by this; what's the suggestion exactly? In the current
> prototype CL I've got the manifests set up in such a way that only the browser
> process can call CreateCoordinationUnit; 

This is where my lack of full understanding of manifests is showing. How is this
actually enforced? Looking at the manifest file, it says coordination_unit is
provided by resource_coordinator::mojom::CoordinationUnitProvider. However, it
doesn't specify any restrictions on which process can call this.

> if a renderer process wants a
> messagepipe to a CU, it has to bounce through the browser process. So the in
the
> cases where we don't want auto-generated IDs, the browser process can either
> construct it itself, or validate it at that point.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:84: if
(!current_policy_)
Be consistent about omitting {} for one line if statements. The policy_callback_
check above has them, which will be more readable without them and they are
missing here, where there is an else if clause. I think here they are needed,
regardless of the decision on the check above.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:85:
current_policy_ = mojom::Policy::New();
Maybe add a bit more scope to the name of the Policy object, as this now reads
as a very generic Mojo policy object, which it isn't.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:95:
policy_callback_->SetPolicy(std::move(policy));
Is policy here supposed to be a copy of the current_policy_ that we just supply
to the callback? Might be worthwhile a comment to clarify.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:130:
void CoordinationUnitImpl::AddChild(const CoordinationUnitID& child_id) {
Some comment on what the expected behavior of this method is will be useful. It
looks like if the child_id supplied has an existing CoordinationUnit for it, but
it isn't in the immediate tree reachable from this unit, then it is added.
However, it could still be part of another tree, is this intended?

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:155:
DCHECK_EQ(1u, children_removed);
Shouldn't the policy be recalculated on child removal?

Ken Rockot(use gerrit already)

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom File services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom (right): https://codereview.chromium.org/2798713002/diff/340001/services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom#newcode10 services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom:10: CreateCoordinationUnit(CoordinationUnit& request, CoordinationUnitID id); On 2017/04/24 at 22:10:50, nasko ...

3 years, 8 months ago (2017-04-24 22:17:56 UTC) #88

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
File
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom
(right):

https://codereview.chromium.org/2798713002/diff/340001/services/resource_coor...
services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom:10:
CreateCoordinationUnit(CoordinationUnit& request, CoordinationUnitID id);
On 2017/04/24 at 22:10:50, nasko wrote:
> On 2017/04/21 17:57:55, oystein wrote:
> > On 2017/04/21 at 00:17:32, nasko wrote:
> > > I know dcheng@ also commented on this, but what if we start out with an ID
> > generated in the browser process? If it needs to be calculated (based on
origin
> > and such), then we should evaluate whether we have all the information in
the
> > browser process. If we don't, then we are missing information and we likely
> > should have it, but if at that point we need the renderer process generating
it,
> > it can be done at the time needed with the exact requirements and
limitations
> > clearly articulated.
> > 
> > I'm slightly confused by this; what's the suggestion exactly? In the current
> > prototype CL I've got the manifests set up in such a way that only the
browser
> > process can call CreateCoordinationUnit; 
> 
> This is where my lack of full understanding of manifests is showing. How is
this actually enforced? Looking at the manifest file, it says coordination_unit
is provided by resource_coordinator::mojom::CoordinationUnitProvider. However,
it doesn't specify any restrictions on which process can call this.

Services do not specify who is allowed to request their capabilities. That would
necessitate layering violations. Instead they simply name their capabilities so
that any other dependent service in the system may "require" them by name.

The Service Manager enforces the static intersection of provided and required
capabilities. It's up to security review to decide whether one service requiring
any given capability from another should be allowed.

> 
> > if a renderer process wants a
> > messagepipe to a CU, it has to bounce through the browser process. So the in
the
> > cases where we don't want auto-generated IDs, the browser process can either
> > construct it itself, or validate it at that point.

oystein (OOO til 10th of July)

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2798713002/diff/400001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc#newcode84 services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:84: if (!current_policy_) On 2017/04/24 at 22:10:50, nasko wrote: > ...

3 years, 8 months ago (2017-04-25 17:19:17 UTC) #89

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:84: if
(!current_policy_)
On 2017/04/24 at 22:10:50, nasko wrote:
> Be consistent about omitting {} for one line if statements. The
policy_callback_ check above has them, which will be more readable without them
and they are missing here, where there is an else if clause. I think here they
are needed, regardless of the decision on the check above.

Done.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:85:
current_policy_ = mojom::Policy::New();
On 2017/04/24 at 22:10:51, nasko wrote:
> Maybe add a bit more scope to the name of the Policy object, as this now reads
as a very generic Mojo policy object, which it isn't.

Done; renamed to CoordinationPolicy.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:95:
policy_callback_->SetPolicy(std::move(policy));
On 2017/04/24 at 22:10:51, nasko wrote:
> Is policy here supposed to be a copy of the current_policy_ that we just
supply to the callback? Might be worthwhile a comment to clarify.

Done.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:130:
void CoordinationUnitImpl::AddChild(const CoordinationUnitID& child_id) {
On 2017/04/24 at 22:10:51, nasko wrote:
> Some comment on what the expected behavior of this method is will be useful.
It looks like if the child_id supplied has an existing CoordinationUnit for it,
but it isn't in the immediate tree reachable from this unit, then it is added.
However, it could still be part of another tree, is this intended?

Added a comment to the .mojom here; the child_id can't be an ascendant or
descendant to avoid infinite recursion when recalculating policies etc. In
theory it could be also inserted in another part of the DAG; I'm honestly not
sure if this is something we want/need to prevent, or if there's use-cases where
that's useful. Happy to make any changes here that makes us more future-safe.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:155:
DCHECK_EQ(1u, children_removed);
On 2017/04/24 at 22:10:50, nasko wrote:
> Shouldn't the policy be recalculated on child removal?

The current design is that policies only bubble down, to be able to have clear
rules around priorities (like, visibility=true on a higher level could be
overridden by a visibility=false flag on a lower level; like a tab might be
visible but a frame is occluded, etc). If we start looking downwards in the tree
this gets more confusing (may mean that specific events will have to bubble up).
Added a comment about this.

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-25 18:57:45 UTC) #90

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-25 18:58:18 UTC) #91

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-25 19:33:28 UTC) #92

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/369895) mac_chromium_rel_ng on ...

3 years, 8 months ago (2017-04-25 19:33:30 UTC) #93

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-25 20:44:43 UTC) #94

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-25 20:45:32 UTC) #95

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-25 21:24:13 UTC) #96

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-25 21:24:14 UTC) #97

nasko

There are still some compile failures. Overall LGTM. https://codereview.chromium.org/2798713002/diff/400001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2798713002/diff/400001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc#newcode155 services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:155: DCHECK_EQ(1u, ...

3 years, 8 months ago (2017-04-25 21:40:56 UTC) #98

There are still some compile failures. Overall LGTM.

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/400001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:155:
DCHECK_EQ(1u, children_removed);
On 2017/04/25 17:19:16, oystein wrote:
> On 2017/04/24 at 22:10:50, nasko wrote:
> > Shouldn't the policy be recalculated on child removal?
> 
> The current design is that policies only bubble down, to be able to have clear
> rules around priorities (like, visibility=true on a higher level could be
> overridden by a visibility=false flag on a lower level; like a tab might be
> visible but a frame is occluded, etc). If we start looking downwards in the
tree
> this gets more confusing (may mean that specific events will have to bubble
up).
> Added a comment about this.

Awesome, thanks!

https://codereview.chromium.org/2798713002/diff/440001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
(right):

https://codereview.chromium.org/2798713002/diff/440001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:96:
mojom::CoordinationPolicyPtr policy = mojom::CoordinationPolicy::New();
nit: Since this is supposed to be a copy, it might be less error prone longer
term to have a Duplicate method on the policy, so a copy creation is handled by
the object itself. Right now it is trivial enough that leaving it as is and
adding that in future CLs.

https://codereview.chromium.org/2798713002/diff/440001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:154:
LOG(ERROR) << "CoordinationUnitImpl::AddChild failed due to child_id "
Is this intended to stay and be committed?

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-26 18:14:36 UTC) #99

oystein (OOO til 10th of July)

Thanks nasko! https://codereview.chromium.org/2798713002/diff/440001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2798713002/diff/440001/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc#newcode96 services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:96: mojom::CoordinationPolicyPtr policy = mojom::CoordinationPolicy::New(); On 2017/04/25 at ...

3 years, 8 months ago (2017-04-26 18:15:00 UTC) #100

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-26 18:15:14 UTC) #101

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-26 18:15:22 UTC) #102

oystein (OOO til 10th of July)

+jam for //services/resource_coordinator/DEPS on //third_party/smhasher

3 years, 8 months ago (2017-04-26 18:15:51 UTC) #103

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-26 18:20:10 UTC) #104

commit-bot: I haz the power

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_compile_dbg/builds/257079) linux_android_rel_ng on ...

3 years, 8 months ago (2017-04-26 18:20:12 UTC) #105

oystein (OOO til 10th of July)

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

3 years, 8 months ago (2017-04-26 18:31:19 UTC) #106

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-26 18:32:28 UTC) #107

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-26 18:37:39 UTC) #108

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-26 18:37:41 UTC) #109

jam

On 2017/04/26 18:15:51, oystein wrote: > +jam for //services/resource_coordinator/DEPS on //third_party/smhasher curious why you use ...

3 years, 7 months ago (2017-04-27 15:39:36 UTC) #110

Primiano Tucci (use gerrit)

On 2017/04/27 15:39:36, jam wrote: > On 2017/04/26 18:15:51, oystein wrote: > > +jam for ...

3 years, 7 months ago (2017-04-27 15:56:26 UTC) #111

oystein (OOO til 10th of July)

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

3 years, 7 months ago (2017-04-27 17:48:50 UTC) #112

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 17:49:36 UTC) #113

oystein (OOO til 10th of July)

primiano/rockot: I had to update this a bit due to https://codereview.chromium.org/2819413002, I chose to generalize ...

3 years, 7 months ago (2017-04-27 17:50:40 UTC) #114

oystein (OOO til 10th of July)

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

3 years, 7 months ago (2017-04-27 17:54:52 UTC) #115

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 17:55:31 UTC) #116

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 18:00:32 UTC) #117

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 18:00:34 UTC) #118

oystein (OOO til 10th of July)

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

3 years, 7 months ago (2017-04-27 18:33:19 UTC) #119

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 18:34:05 UTC) #120

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 19:04:57 UTC) #121

commit-bot: I haz the power

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

3 years, 7 months ago (2017-04-27 19:04:58 UTC) #122

jam

On 2017/04/27 15:56:26, Primiano Tucci wrote: > On 2017/04/27 15:39:36, jam wrote: > > On ...

3 years, 7 months ago (2017-04-28 01:01:11 UTC) #123

Primiano Tucci (use gerrit)

On 2017/04/28 01:01:11, jam wrote: > On 2017/04/27 15:56:26, Primiano Tucci wrote: > > On ...

3 years, 7 months ago (2017-04-28 09:12:04 UTC) #124

On 2017/04/28 01:01:11, jam wrote:
> On 2017/04/27 15:56:26, Primiano Tucci wrote:
> > On 2017/04/27 15:39:36, jam wrote:
> > > On 2017/04/26 18:15:51, oystein wrote:
> > > > +jam for //services/resource_coordinator/DEPS on //third_party/smhasher
> > > 
> > > curious why you use that instead of the existing hash methods in base/ ?
> > 
> > I adviced against that because I tried using them in the past in
memory-infra
> > and got seriously bitten by collision.
> > See full discussion in
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-rXNiOoIU4A/LI4tn...
> > TL;DR: SuperFastHash really sucks if you have two strings that are similar
but
> > differ only for the last characters.
> 
> hmm, ok well it's up to you to pick which hashing method you use. lgtm so to
not
> block this.
> 
> my followup questions are:
> -do we know that the same cases that you hit before apply here? is this by
> definition, since memory infra code will move here?

Yes, memory-infra code is slowly moving here (we're slowly moving pieces, see
services/resource_coordinator/public/interfaces/memory/) % the few APIs used by
all the codebase for probing that we have to keep in //base/. 
In all honesty is not just memory-infra that worries me. My understanding from
base::hash is that SuperFastHash is just too risky to be used with string and
should be used only with blobs or anything which has enough entropy.
If you look at the thread linked above, that base::Hash has known flaws on
strings that differ only on the last characters.
It is extremely realistic that resource coordinator IDs are going to fall into
this bucket, regardless of memory-infra, if you think to IDs like
"//process0/tab1/frame1" "//process0/tab1/frame2".

> -it seems unfortunate that if we have this problem with the base hashing
> function, parts of the code will use a different one. shouldn't we switch the
> one in base? (even if a few callsites have to use the old one per the thread)

I personally agree with you, and this is precisely the reason why I started that
thread back then in
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-rXNiOoIU4A...
where I wrote "I'm expecting too much from base::Hash, or should it be
strengthened".
But a mixture of "I wasn't persuasive enough", "nobody generally cared too much"
and "some people even felt that something might depends on the specific hashing"
killed the thread.
Feel free to reopen that discussion, I'll be extremely supportive.

oystein (OOO til 10th of July)

The CQ bit was checked by oysteine@chromium.org

3 years, 7 months ago (2017-04-28 13:53:04 UTC) #125

oystein (OOO til 10th of July)

The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, rockot@chromium.org, nasko@chromium.org Link ...

3 years, 7 months ago (2017-04-28 13:53:05 UTC) #126

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2798713002/560001

3 years, 7 months ago (2017-04-28 13:53:37 UTC) #127

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 560001, "attempt_start_ts": 1493387584742920, "parent_rev": "611639da9364de0f00eb0136523482a9b7b394b7", "commit_rev": "de0f39d2a625433e60b5e22e1d2d3803f3d219e5"}

3 years, 7 months ago (2017-04-28 15:21:21 UTC) #128

commit-bot: I haz the power

Description was changed from ========== Global Resource Coordinator: Basic service internals This adds the basic ...

3 years, 7 months ago (2017-04-28 15:21:34 UTC) #129

commit-bot: I haz the power

Committed patchset #30 (id:560001) as https://chromium.googlesource.com/chromium/src/+/de0f39d2a625433e60b5e22e1d2d3803f3d219e5

3 years, 7 months ago (2017-04-28 15:21:36 UTC) #130

sky

A revert of this CL (patchset #30 id:560001) has been created in https://codereview.chromium.org/2851813002/ by sky@chromium.org. ...

3 years, 7 months ago (2017-04-28 18:13:25 UTC) #131

Khushal

A revert of this CL (patchset #30 id:560001) has been created in https://codereview.chromium.org/2848053002/ by khushalsagar@chromium.org. ...

3 years, 7 months ago (2017-04-28 19:50:38 UTC) #132

oystein (OOO til 10th of July)

Description was changed from ========== Global Resource Coordinator: Basic service internals This adds the basic ...

3 years, 7 months ago (2017-05-02 22:01:03 UTC) #133

oystein (OOO til 10th of July)

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

3 years, 7 months ago (2017-05-03 18:34:23 UTC) #134

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-03 18:35:17 UTC) #135

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-03 19:45:48 UTC) #136

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-03 19:45:49 UTC) #137

oystein (OOO til 10th of July)

The CQ bit was checked by oysteine@chromium.org

3 years, 7 months ago (2017-05-03 20:24:18 UTC) #138

oystein (OOO til 10th of July)

The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, rockot@chromium.org, jam@chromium.org, nasko@chromium.org ...

3 years, 7 months ago (2017-05-03 20:24:19 UTC) #139

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2798713002/620001

3 years, 7 months ago (2017-05-03 20:24:43 UTC) #140

oystein (OOO til 10th of July)

On 2017/04/28 at 19:50:38, khushalsagar wrote: > A revert of this CL (patchset #30 id:560001) ...

3 years, 7 months ago (2017-05-03 20:25:05 UTC) #141

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 620001, "attempt_start_ts": 1493843058897180, "parent_rev": "3a882470d81dff3f3aa4e6c64838f6dbe26f9509", "commit_rev": "8b47351be59f0d590d8d5fcffe1c07350ba110f2"}

3 years, 7 months ago (2017-05-03 20:31:08 UTC) #142

commit-bot: I haz the power

Description was changed from ========== Reland: Global Resource Coordinator: Basic service internals This adds the ...

3 years, 7 months ago (2017-05-03 20:31:22 UTC) #143

commit-bot: I haz the power

Committed patchset #33 (id:620001) as https://chromium.googlesource.com/chromium/src/+/8b47351be59f0d590d8d5fcffe1c07350ba110f2

3 years, 7 months ago (2017-05-03 20:31:24 UTC) #144

oystein (OOO til 10th of July)

A revert of this CL (patchset #33 id:620001) has been created in https://codereview.chromium.org/2856173003/ by oysteine@chromium.org. ...

3 years, 7 months ago (2017-05-03 22:31:45 UTC) #145

oystein (OOO til 10th of July)

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

3 years, 7 months ago (2017-05-04 23:11:59 UTC) #146

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-04 23:12:51 UTC) #147

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-04 23:27:05 UTC) #148

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-04 23:27:06 UTC) #149

oystein (OOO til 10th of July)

The CQ bit was checked by oysteine@chromium.org

3 years, 7 months ago (2017-05-05 16:24:53 UTC) #150

oystein (OOO til 10th of July)

The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, rockot@chromium.org, jam@chromium.org, nasko@chromium.org ...

3 years, 7 months ago (2017-05-05 16:24:54 UTC) #151

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2798713002/660001

3 years, 7 months ago (2017-05-05 16:25:23 UTC) #152

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 660001, "attempt_start_ts": 1494001493582940, "parent_rev": "1290a798ea209beefb9c00e8836aa91e0cf8b87f", "commit_rev": "ea57f11806604d841b31336cb4da7dda36c8b1b7"}

3 years, 7 months ago (2017-05-05 17:52:50 UTC) #153

commit-bot: I haz the power

Description was changed from ========== Reland: Global Resource Coordinator: Basic service internals This adds the ...

3 years, 7 months ago (2017-05-05 17:53:06 UTC) #154

commit-bot: I haz the power

3 years, 7 months ago (2017-05-05 17:53:09 UTC) #155

Message was sent while issue was closed.

Committed patchset #35 (id:660001) as
https://chromium.googlesource.com/chromium/src/+/ea57f11806604d841b31336cb4da...

Issue 2798713002: Global Resource Coordinator: Basic service internals (Closed)

Description

Patch Set 1 #

Patch Set 2 : Review fixes #

Patch Set 3 : WIP review fixes #

Patch Set 4 : WIP reviews #

Patch Set 5 : Review fixes #

Patch Set 6 : Removed leftover stuff #

Patch Set 7 : libc++ buildfix #

Patch Set 8 : MSVC buildfix #

Patch Set 9 : Buildfix #

Patch Set 10 : MSVC buildfix? #

Patch Set 11 : Buildfixes #

Patch Set 12 : Buildfix #

Patch Set 13 : Buildfix #

Patch Set 14 : argh #

Patch Set 15 : resource_coordinator_unittests target buildfix #

Patch Set 16 : Potential .apk buildfix #

Patch Set 17 : Another potential .apk fix #

Patch Set 18 : Wrap EventType directly in resource_coordinator namespace when using helper #

Patch Set 19 : Review fixes #

Patch Set 20 : Docs #

Patch Set 21 : Review fixes #

Patch Set 22 : Use MurmurHash64A instead of MurmurHash2 now that id is 64bit #

Patch Set 23 : Review fixes #

Patch Set 24 : Fixed test failure #

Patch Set 25 : Review fixes and resource_coordination_unittests compile fix #

Patch Set 26 : Rebase #

Patch Set 27 : Rebase #

Patch Set 28 : Generalized to work with component export macros from https://codereview.chromium.org/2819413002 #

Patch Set 29 : Generalized to work with component macros from https://codereview.chromium.org/2819413002 #

Patch Set 30 : Buildfix #

Patch Set 31 : Windows(dbg) crash fix #

Patch Set 32 : Rebase + buildfix #

Patch Set 33 : Fixed BUILD.gn data_deps #

Patch Set 34 : Fixed manifest issue on Android #

Patch Set 35 : Buildfix after rebase #

Messages