[GRC] Add Additional CoordinationUnitProperty Types

[GRC] Add Additional CoordinationUnitProperty Types

This CL adds additional enum entries to resource_coordinator::mojom::CoordinationUnitProperty for new use cases for the key-value store.

As more entries are added, it is worth discussing the best way to go about getting these changes approved with the security team (for example, using TBR instead of R for CLs that add a new enum entry).

Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org
BUG=724306

[matthalp, 2017-06-14 23:48:25]

Description was changed from

==========
[GRC] Create struct traits for CoordinationUnitProperty Type

This CL creates a native resource_coordinator::CoordinationUnitProperty
enum to replace the use of
resource_coordinator::mojom::ProperyType which is, as of this CL,
resource_coordinator::mojom::CoordinationUnitProperyType. Previously,
the CoordinationUnitImpl key-value store key type was
resource_coordinator::mojom::ProperyType, but given that this store
can be used outside of mojo-related functionalities it makes more
sense for a non-mojo resource_coordinator::CoordinationUnitProperty
type to be the key.

This CL also adds additional enum entries as there are emerging
use cases for the key-value store. As more entries are added, it is
worth discussing the best way to go about getting these changes
approved with the security team (for example, using TBR instead
of R for CLs that add a new enum entry).

There are also a few code health improvements sprinkled into the
CL as raised by cpplint.


Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org,zhenw@chromium.org
BUG=724306
==========

to

==========
[GRC] Create struct traits for CoordinationUnitProperty Type

This CL creates a native resource_coordinator::CoordinationUnitProperty
enum to replace the use of
resource_coordinator::mojom::ProperyType which is, as of this CL,
resource_coordinator::mojom::CoordinationUnitProperyType. Previously,
the CoordinationUnitImpl key-value store key type was
resource_coordinator::mojom::ProperyType, but given that this store
can be used outside of mojo-related functionalities it makes more
sense for a non-mojo resource_coordinator::CoordinationUnitProperty
type to be the key.

This CL also adds additional enum entries as there are emerging
use cases for the key-value store. As more entries are added, it is
worth discussing the best way to go about getting these changes
approved with the security team (for example, using TBR instead
of R for CLs that add a new enum entry).

There are also a few code health improvements sprinkled into the
CL as raised by cpplint.

Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org,zhenw@chromium.org
BUG=724306
==========

[matthalp, 2017-06-14 23:48:32]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 23:48:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[matthalp, 2017-06-15 00:14:16]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-15 00:22:25]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 00:22:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 02:21:05]

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

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

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-20 23:41:58]

Description was changed from

==========
[GRC] Create struct traits for CoordinationUnitProperty Type

This CL creates a native resource_coordinator::CoordinationUnitProperty
enum to replace the use of
resource_coordinator::mojom::ProperyType which is, as of this CL,
resource_coordinator::mojom::CoordinationUnitProperyType. Previously,
the CoordinationUnitImpl key-value store key type was
resource_coordinator::mojom::ProperyType, but given that this store
can be used outside of mojo-related functionalities it makes more
sense for a non-mojo resource_coordinator::CoordinationUnitProperty
type to be the key.

This CL also adds additional enum entries as there are emerging
use cases for the key-value store. As more entries are added, it is
worth discussing the best way to go about getting these changes
approved with the security team (for example, using TBR instead
of R for CLs that add a new enum entry).

There are also a few code health improvements sprinkled into the
CL as raised by cpplint.

Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org,zhenw@chromium.org
BUG=724306
==========

to

==========
[GRC] Add Additional CoordinationUnitProperty Types

This CL adds additional enum entries to
resource_coordinator::mojom::CoordinationUnitProperty for new use cases for the
key-value store.

As more entries are added, it is worth discussing the best way to go about
getting these changes approved with the security team (for example, using TBR
instead of R for CLs that add a new enum entry).

There are also a few code health improvements sprinkled into the
CL as raised by cpplint.

Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org,zhenw@chromium.org
BUG=724306
==========

[oystein (OOO til 10th of July), 2017-06-20 23:50:27]

lgtm

rockot: We discussed in the past the possibility of TBRing these kinds of .mojom
changes to security review; who'd be the people to ask about that?

[Ken Rockot(use gerrit already), 2017-06-20 23:52:13]

On 2017/06/20 at 23:50:27, oysteine wrote:
> lgtm
> 
> rockot: We discussed in the past the possibility of TBRing these kinds of
.mojom changes to security review; who'd be the people to ask about that?

[matthalp, 2017-06-20 23:55:34]

matthalp@google.com changed reviewers:
- zhenw@chromium.org

[Ken Rockot(use gerrit already), 2017-06-21 00:04:20]

On 2017/06/20 at 23:52:13, Ken Rockot(use gerrit already) wrote:
> On 2017/06/20 at 23:50:27, oysteine wrote:
> > lgtm
> > 
> > rockot: We discussed in the past the possibility of TBRing these kinds of
.mojom changes to security review; who'd be the people to ask about that?

Ahh hmm, not sure specifically what you're referring to. Especially in
//services I think we might be inclined to discourage any kind of TBR on
interface changes, security or otherwise. In fact there's been discussion[1]
about having general API review, not just security review for any public
interface changes in //services.

Obviously we don't want prohibitive review process overhead, but we also want to
encourage a reasonably high degree of caution and forethought in designing
interfaces exposed by foundational services.

I am surprised to see the concept of "tab URL" appearing in a foundational
service, though now that I look I also see enums referring to things like
navigation and even WebContents. Is this actually a foundational service?

[1]
https://groups.google.com/a/chromium.org/forum/#!topic/services-dev/Z3GELKCbULM

[oystein (OOO til 10th of July), 2017-06-21 00:30:32]

On 2017/06/21 at 00:04:20, rockot wrote:
> On 2017/06/20 at 23:52:13, Ken Rockot(use gerrit already) wrote:
> > On 2017/06/20 at 23:50:27, oysteine wrote:
> > > lgtm
> > > 
> > > rockot: We discussed in the past the possibility of TBRing these kinds of
.mojom changes to security review; who'd be the people to ask about that?
> 
> Ahh hmm, not sure specifically what you're referring to. Especially in
//services I think we might be inclined to discourage any kind of TBR on
interface changes, security or otherwise. In fact there's been discussion[1]
about having general API review, not just security review for any public
interface changes in //services.
> 
> Obviously we don't want prohibitive review process overhead, but we also want
to encourage a reasonably high degree of caution and forethought in designing
interfaces exposed by foundational services.

The context was a discussion about native enums vs. Mojo enums, and the
additional review friction for the latter when people start adding more and more
events here. Extending an enum is an interface change, so a general API review
would apply, but I'm not sure if it really increases any kind of attack surface?

> I am surprised to see the concept of "tab URL" appearing in a foundational
service, though now that I look I also see enums referring to things like
navigation and even WebContents. Is this actually a foundational service?
> 
> [1]
https://groups.google.com/a/chromium.org/forum/#!topic/services-dev/Z3GELKCbULM

The URL is needed for forwarding metrics to UKM; the properties (and the
CoordinationUnit types, like "WebContents") could be keyed by string rather than
by enum, but I'm not sure if that gains us anything other than overhead (similar
to the discussion in
https://groups.google.com/a/chromium.org/forum/#!msg/services-dev/v85DHRCtrl4...).
Actual Chrome-logic will live in //chrome/browser/resource_coordinator.

[Ken Rockot(use gerrit already), 2017-06-21 13:51:11]

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

[Ken Rockot(use gerrit already), 2017-06-21 13:51:12]

K, thanks for the explanation. I am still dubious of these things (and even the
process type awareness from earlier) but I think I'm alone there and there's
definitely no short-term practical value in doing anything about it.

Also forgot to +dcheng for security opinion. Derp!

[dcheng, 2017-06-21 21:56:02]

The CL description seems to be out-of-date (I see no cpplint related changes).

Generally speaking, I would also not be comfortable adding new values and TBRing
the security review. Adding a new value is often fine, but to some degree, that
depends on how it's used. Is there a followup CL that actually uses the new
enumerator values?

[dcheng, 2017-06-21 22:05:47]

On 2017/06/21 21:56:02, dcheng wrote:
> The CL description seems to be out-of-date (I see no cpplint related changes).
> 
> Generally speaking, I would also not be comfortable adding new values and
TBRing
> the security review. Adding a new value is often fine, but to some degree,
that
> depends on how it's used. Is there a followup CL that actually uses the new
> enumerator values?

(As for review latency, I have some thoughts for improving this--in particular,
it would be nice if there was a queue that people could grab from, rather than
trying to pick a reviewer that you hope is around and available right that
instant. But overall, if the usage is trivially safe and easy to evaluate,  I
would not expect the review itself to be very long)

[matthalp, 2017-06-21 23:06:18]

Description was changed from

==========
[GRC] Add Additional CoordinationUnitProperty Types

This CL adds additional enum entries to
resource_coordinator::mojom::CoordinationUnitProperty for new use cases for the
key-value store.

As more entries are added, it is worth discussing the best way to go about
getting these changes approved with the security team (for example, using TBR
instead of R for CLs that add a new enum entry).

There are also a few code health improvements sprinkled into the
CL as raised by cpplint.

Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org,zhenw@chromium.org
BUG=724306
==========

to

==========
[GRC] Add Additional CoordinationUnitProperty Types

This CL adds additional enum entries to
resource_coordinator::mojom::CoordinationUnitProperty for new use cases for the
key-value store.

As more entries are added, it is worth discussing the best way to go about
getting these changes approved with the security team (for example, using TBR
instead of R for CLs that add a new enum entry).

Add property types enum
R=*rockot@chromium.org,*oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-21 23:08:30]

On 2017/06/21 at 22:05:47, dcheng wrote:
> On 2017/06/21 21:56:02, dcheng wrote:
> > The CL description seems to be out-of-date (I see no cpplint related
changes).
> > 
> > Generally speaking, I would also not be comfortable adding new values and
TBRing
> > the security review. Adding a new value is often fine, but to some degree,
that
> > depends on how it's used. Is there a followup CL that actually uses the new
> > enumerator values?
> 
> (As for review latency, I have some thoughts for improving this--in
particular, it would be nice if there was a queue that people could grab from,
rather than trying to pick a reviewer that you hope is around and available
right that instant. But overall, if the usage is trivially safe and easy to
evaluate,  I would not expect the review itself to be very long)

There is a follow up CL with the usage. I'm going to go ahead and close this,
then add the change there (+ a suitable reviewer) as the discussion this meant
to initiate has subsided.