Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917793002:
[GRC] Coordination Unit Key-Value Storage (Closed)
|
Created:
3 years, 6 months ago by matthalp Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, chrome-grc-reviews_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[GRC] Coordination Unit Key-Value Storage
GRC's coordination unit abstraction has been enhanced to provide an
internal key-value "property store" that can be used to store *interesting*
runtime information relevant to what the coordination unit is tracking.
In its current form, the key-value store consists of an enum-based key
and a base::Value value. Using an enum for the key helps to enforce
isolation between the property store users, while the base::Value allows
the property store to work for a variety of different types.
*** The final key-value pair type is still being iterated upon ***
While the key-value store can be accessed through a member accessor,
it is intended to be used through:
- CoordinationUnitImpl::SetProperty to set properties
- CoordinationUnitImpl::GetProperty to access properties
- CoordinationUnitImpl::ClearProperty to clear properties
Additionally, we extend the CoordinationUnitImpl mojo interface to
allow code outside of GRC to set properties. The interface may be
extended in the future to support the other operations, but that is
not an immediate concern here.
This CL depends on: https://codereview.chromium.org/2914003002
R=oysteine@chromium.org
BUG=724306
Review-Url: https://codereview.chromium.org/2917793002
Cr-Commit-Position: refs/heads/master@{#477317}
Committed: https://chromium.googlesource.com/chromium/src/+/ea83d48cabb2932915f6d812f0ca83c89c0c5add
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address review feedback #
Total comments: 6
Patch Set 3 : Rebase #Patch Set 4 : Address reviewer feedback #
Total comments: 4
Patch Set 5 : Fix comments #
Depends on Patchset: Messages
Total messages: 21 (10 generated)
https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:40: void SetStorageProperty(mojom::StoragePairPtr pair) override; General feedback: "Storage" usually in Chrome implies something more permanent, I would just rename the APIs to "SetProperty", "GetProperty", etc, throughout. https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:187: // empty value should be returned if property is not found nit: s/empty/An empty/ https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:191: // a empty value should be able to set a property nit: s/a/An/? https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/public/interfaces/coordination_unit.mojom:56: SetStorageProperty(StoragePair pair); For the interface as well, I'll bikeshed the naming and suggest simply "SetProperty", and "Property" for the struct.
https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:40: void SetStorageProperty(mojom::StoragePairPtr pair) override; On 2017/06/01 at 18:24:27, oystein wrote: > General feedback: "Storage" usually in Chrome implies something more permanent, I would just rename the APIs to "SetProperty", "GetProperty", etc, throughout. Totally makes sense. Removed all mentions of [S|s]torage throughout the code. Changed the key-value store to be property_store. https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:187: // empty value should be returned if property is not found On 2017/06/01 at 18:24:27, oystein wrote: > nit: s/empty/An empty/ Done https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:187: // empty value should be returned if property is not found On 2017/06/01 at 18:24:27, oystein wrote: > nit: s/empty/An empty/ Done. https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:191: // a empty value should be able to set a property On 2017/06/01 at 18:24:27, oystein wrote: > nit: s/a/An/? Done. https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... services/resource_coordinator/public/interfaces/coordination_unit.mojom:56: SetStorageProperty(StoragePair pair); On 2017/06/01 at 18:24:27, oystein wrote: > For the interface as well, I'll bikeshed the naming and suggest simply "SetProperty", and "Property" for the struct. Done.
matthalp@google.com changed reviewers: + nasko@chromium.org
On 2017/06/01 at 21:27:38, matthalp wrote: > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > services/resource_coordinator/coordination_unit/coordination_unit_impl.h:40: void SetStorageProperty(mojom::StoragePairPtr pair) override; > On 2017/06/01 at 18:24:27, oystein wrote: > > General feedback: "Storage" usually in Chrome implies something more permanent, I would just rename the APIs to "SetProperty", "GetProperty", etc, throughout. > > Totally makes sense. Removed all mentions of [S|s]torage throughout the code. Changed the key-value store to be property_store. > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:187: // empty value should be returned if property is not found > On 2017/06/01 at 18:24:27, oystein wrote: > > nit: s/empty/An empty/ > > Done > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:187: // empty value should be returned if property is not found > On 2017/06/01 at 18:24:27, oystein wrote: > > nit: s/empty/An empty/ > > Done. > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:191: // a empty value should be able to set a property > On 2017/06/01 at 18:24:27, oystein wrote: > > nit: s/a/An/? > > Done. > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): > > https://codereview.chromium.org/2917793002/diff/1/services/resource_coordinat... > services/resource_coordinator/public/interfaces/coordination_unit.mojom:56: SetStorageProperty(StoragePair pair); > On 2017/06/01 at 18:24:27, oystein wrote: > > For the interface as well, I'll bikeshed the naming and suggest simply "SetProperty", and "Property" for the struct. > > Done. +nasko for security approval regarding Mojo file modifications (coordination_unit.mojom)
lgtm with comments https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:229: property_store_.erase(value_it); std::unordered_map::erase has a key_type overload, so you should be able to call property_store_.erase(property) directly I think? https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:45: std::unordered_map<mojom::PropertyType, base::Value>& property_store() { looks like this is only for testing? make it const and call it property_store_for_testing() in that case.
Description was changed from ========== [GRC] Coordination Unit Key-Value Storage GRC's coordination unit abstraction has been enhanced to provide an internal key-value store that can be used to store *interesting* runtime information relevant to what the coordination unit is tracking. In its current form, the key-value store consists of an enum-based key and a base::Value value. Using an enum for the key helps to enforce isolation between the storage users, while the base::Value allows the storage to work for a variety of different types. *** The final key-value pair type is still being iterated upon *** While the key-value store can be accessed through a member accessor, it is indended to be used through: - CoordinationUnitImpl::SetStorageProperty to set properties - CoordinationUnitImpl::GetStorageProperty to access properties - CoordinationUnitImpl::ClearStorageProperty to clear properties Additionally, we extend the CoordinationUnitImpl mojo interface to allow code outside of GRC to set properties. The interface may be extended in the future to support the other operations, but that is not an immediate concern here. This CL depends on: https://codereview.chromium.org/2914003002 R=oysteine@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Key-Value Storage GRC's coordination unit abstraction has been enhanced to provide an internal key-value "property store" that can be used to store *interesting* runtime information relevant to what the coordination unit is tracking. In its current form, the key-value store consists of an enum-based key and a base::Value value. Using an enum for the key helps to enforce isolation between the property store users, while the base::Value allows the property store to work for a variety of different types. *** The final key-value pair type is still being iterated upon *** While the key-value store can be accessed through a member accessor, it is intended to be used through: - CoordinationUnitImpl::SetProperty to set properties - CoordinationUnitImpl::GetProperty to access properties - CoordinationUnitImpl::ClearProperty to clear properties Additionally, we extend the CoordinationUnitImpl mojo interface to allow code outside of GRC to set properties. The interface may be extended in the future to support the other operations, but that is not an immediate concern here. This CL depends on: https://codereview.chromium.org/2914003002 R=oysteine@chromium.org BUG=724306 ==========
LGTM provided some comments are added. https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... services/resource_coordinator/public/interfaces/coordination_unit.mojom:56: SetProperty(Property property); Can we add some comments on the method and associated structs describing what they are used for? For people not intimately familiar with GRC, it is not clear what those are.
https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:229: property_store_.erase(value_it); On 2017/06/01 at 21:52:02, oystein wrote: > std::unordered_map::erase has a key_type overload, so you should be able to call property_store_.erase(property) directly I think? That overload exists -- Done. https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:45: std::unordered_map<mojom::PropertyType, base::Value>& property_store() { On 2017/06/01 at 21:52:02, oystein wrote: > looks like this is only for testing? make it const and call it property_store_for_testing() in that case. Done. https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): https://codereview.chromium.org/2917793002/diff/20001/services/resource_coord... services/resource_coordinator/public/interfaces/coordination_unit.mojom:56: SetProperty(Property property); On 2017/06/03 at 00:21:14, nasko (slow) wrote: > Can we add some comments on the method and associated structs describing what they are used for? For people not intimately familiar with GRC, it is not clear what those are. Absolutely! Done.
https://codereview.chromium.org/2917793002/diff/60001/services/resource_coord... File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): https://codereview.chromium.org/2917793002/diff/60001/services/resource_coord... services/resource_coordinator/public/interfaces/coordination_unit.mojom:33: // Storage property keys for CoordinationUnitImpl internal key-value store nit: End sentences with ".". https://codereview.chromium.org/2917793002/diff/60001/services/resource_coord... services/resource_coordinator/public/interfaces/coordination_unit.mojom:59: // Set a property on the CoordinationUnitImpl's internal key-value store nit: s/Set/Sets/
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by matthalp@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2917793002/#ps80001 (title: "Fix comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496768198948970,
"parent_rev": "745f3dff09793d3a5afe793a2599eadb341d2ee0", "commit_rev":
"ea83d48cabb2932915f6d812f0ca83c89c0c5add"}
Message was sent while issue was closed.
Description was changed from ========== [GRC] Coordination Unit Key-Value Storage GRC's coordination unit abstraction has been enhanced to provide an internal key-value "property store" that can be used to store *interesting* runtime information relevant to what the coordination unit is tracking. In its current form, the key-value store consists of an enum-based key and a base::Value value. Using an enum for the key helps to enforce isolation between the property store users, while the base::Value allows the property store to work for a variety of different types. *** The final key-value pair type is still being iterated upon *** While the key-value store can be accessed through a member accessor, it is intended to be used through: - CoordinationUnitImpl::SetProperty to set properties - CoordinationUnitImpl::GetProperty to access properties - CoordinationUnitImpl::ClearProperty to clear properties Additionally, we extend the CoordinationUnitImpl mojo interface to allow code outside of GRC to set properties. The interface may be extended in the future to support the other operations, but that is not an immediate concern here. This CL depends on: https://codereview.chromium.org/2914003002 R=oysteine@chromium.org BUG=724306 ========== to ========== [GRC] Coordination Unit Key-Value Storage GRC's coordination unit abstraction has been enhanced to provide an internal key-value "property store" that can be used to store *interesting* runtime information relevant to what the coordination unit is tracking. In its current form, the key-value store consists of an enum-based key and a base::Value value. Using an enum for the key helps to enforce isolation between the property store users, while the base::Value allows the property store to work for a variety of different types. *** The final key-value pair type is still being iterated upon *** While the key-value store can be accessed through a member accessor, it is intended to be used through: - CoordinationUnitImpl::SetProperty to set properties - CoordinationUnitImpl::GetProperty to access properties - CoordinationUnitImpl::ClearProperty to clear properties Additionally, we extend the CoordinationUnitImpl mojo interface to allow code outside of GRC to set properties. The interface may be extended in the future to support the other operations, but that is not an immediate concern here. This CL depends on: https://codereview.chromium.org/2914003002 R=oysteine@chromium.org BUG=724306 Review-Url: https://codereview.chromium.org/2917793002 Cr-Commit-Position: refs/heads/master@{#477317} Committed: https://chromium.googlesource.com/chromium/src/+/ea83d48cabb2932915f6d812f0ca... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ea83d48cabb2932915f6d812f0ca...
Message was sent while issue was closed.
https://codereview.chromium.org/2917793002/diff/60001/services/resource_coord... File services/resource_coordinator/public/interfaces/coordination_unit.mojom (right): https://codereview.chromium.org/2917793002/diff/60001/services/resource_coord... services/resource_coordinator/public/interfaces/coordination_unit.mojom:33: // Storage property keys for CoordinationUnitImpl internal key-value store On 2017/06/06 at 01:06:17, nasko (slow) wrote: > nit: End sentences with ".". Done. https://codereview.chromium.org/2917793002/diff/60001/services/resource_coord... services/resource_coordinator/public/interfaces/coordination_unit.mojom:59: // Set a property on the CoordinationUnitImpl's internal key-value store On 2017/06/06 at 01:06:17, nasko (slow) wrote: > nit: s/Set/Sets/ Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
