Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2461723002:
[Sync] DeviceInfoService static method and error cleanup. (Closed)
Description[Sync] DeviceInfoService static method and error cleanup.
* Exampling upon ModelTypeService's comments to explain when OnMetadataLoaded should be called.
* Removed old TODOs from DeviceInfoService
* Updated error handling to be slightly more consistent.
* Moving static methods to be anonymous namespaced.
* Updated unittests to not need static DeviceInfoService methods by modifying the way test data is created.
BUG=659263
Committed: https://crrev.com/fc8171bfb29731cb8f3c302594eb2b9ba9b9239d
Cr-Commit-Position: refs/heads/master@{#428527}
Patch Set 1 #Patch Set 2 : Reworking unittests to not need any static methods, updated static method names. #Patch Set 3 : Moved local suffix into a constant. #
Total comments: 12
Patch Set 4 : This is why we can't have nice things. #Patch Set 5 : Updates for Max. #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : removing stdint.h from device_info_service.h #
Dependent Patchsets: Messages
Total messages: 30 (20 generated)
Description was changed from ========== [Sync] DeviceInfoService static method and error cleanup. BUG=659263 ========== to ========== [Sync] DeviceInfoService static method and error cleanup. * Exampling upon ModelTypeService's comments to explain when OnMetadataLoaded should be called. * Removed old TODOs from DeviceInfoService * Updated error handling to be slightly more consistent. * Moving static methods to be anonymous namespaced. * Updated unittests to not need static DeviceInfoService methods by modifying the way test data is created. BUG=659263 ==========
The CQ bit was checked by skym@chromium.org 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...
skym@chromium.org changed reviewers: + maxbogue@chromium.org
PTAL. Bunch of simplistic cleanup so that the next CL to fix dealing with the provider being cleared can be all by itself.
The CQ bit was unchecked by commit-bot@chromium.org
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_arm6...)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Removing unused method.
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Note that this will conflict with http://crrev.com/2458013002. https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... File components/sync/device_info/device_info_service.cc (right): https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service.cc:42: Time GetLastUpdateTime(const DeviceInfoSpecifics& specifics) { I genuinely find it more confusing to see just Time and not base::Time >_> https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service.cc:51: std::unique_ptr<DeviceInfo> SpecificsToModel( Here as well I find Model to be vague. I think SpecificsToDeviceInfo and DeviceInfoToSpecifics would be fine, descriptive names for these things. https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... File components/sync/device_info/device_info_service_unittest.cc (right): https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service_unittest.cc:71: std::unique_ptr<DeviceInfo> CreateModel(int suffix) { I find CreateModel to be a confusingly vague term. I dunno what is model. Maybe CreateDeviceInfo? https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service_unittest.cc:656: std::unique_ptr<DeviceInfoSpecifics> specifics = auto https://codereview.chromium.org/2461723002/diff/40001/components/sync/model/m... File components/sync/model/model_type_service.h (right): https://codereview.chromium.org/2461723002/diff/40001/components/sync/model/m... components/sync/model/model_type_service.h:29: // metadata for entities, as well as the model type state. Service s/Service/Sync bridge/ https://codereview.chromium.org/2461723002/diff/40001/components/sync/model/m... components/sync/model/model_type_service.h:33: // start calling into the service. bridge
The CQ bit was checked by skym@chromium.org 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...
Updates for Max. https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... File components/sync/device_info/device_info_service.cc (right): https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service.cc:42: Time GetLastUpdateTime(const DeviceInfoSpecifics& specifics) { On 2016/10/28 20:20:01, maxbogue wrote: > I genuinely find it more confusing to see just Time and not base::Time >_> Why? Is there some other 'Time' object you think I might be using? https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service.cc:51: std::unique_ptr<DeviceInfo> SpecificsToModel( On 2016/10/28 20:20:01, maxbogue wrote: > Here as well I find Model to be vague. I think SpecificsToDeviceInfo and > DeviceInfoToSpecifics would be fine, descriptive names for these things. DeviceInfo describes everything going on here. DeviceInfo and DeviceInfoSpecifics are both DeviceInfo. When every object is a DeviceInfo, no object is a Device Info. Model, contextually, has a very specific meaning. The old function was called 'CopyToModel' if that makes you feel any better, I'm not even introducing this! https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... File components/sync/device_info/device_info_service_unittest.cc (right): https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service_unittest.cc:71: std::unique_ptr<DeviceInfo> CreateModel(int suffix) { On 2016/10/28 20:20:02, maxbogue wrote: > I find CreateModel to be a confusingly vague term. I dunno what is model. Maybe > CreateDeviceInfo? Why do you find create model to be confusing? There are two sides. The sync side, and the model side. The bridge connects those two. Model represents one of those sides, and they have objects, model objects in fact! https://codereview.chromium.org/2461723002/diff/40001/components/sync/device_... components/sync/device_info/device_info_service_unittest.cc:656: std::unique_ptr<DeviceInfoSpecifics> specifics = On 2016/10/28 20:20:02, maxbogue wrote: > auto Done. https://codereview.chromium.org/2461723002/diff/40001/components/sync/model/m... File components/sync/model/model_type_service.h (right): https://codereview.chromium.org/2461723002/diff/40001/components/sync/model/m... components/sync/model/model_type_service.h:29: // metadata for entities, as well as the model type state. Service On 2016/10/28 20:20:02, maxbogue wrote: > s/Service/Sync bridge/ Done. https://codereview.chromium.org/2461723002/diff/40001/components/sync/model/m... components/sync/model/model_type_service.h:33: // start calling into the service. On 2016/10/28 20:20:02, maxbogue wrote: > bridge Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by skym@chromium.org 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...
Rebase
lgtm; just ignore the Model and Time stuff, I don't actually care about it. https://codereview.chromium.org/2461723002/diff/100001/components/sync/device... File components/sync/device_info/device_info_service.h (right): https://codereview.chromium.org/2461723002/diff/100001/components/sync/device... components/sync/device_info/device_info_service.h:8: #include <stdint.h> Is this still needed? I'm not actually sure what's in stdint.
Update for Max. https://codereview.chromium.org/2461723002/diff/100001/components/sync/device... File components/sync/device_info/device_info_service.h (right): https://codereview.chromium.org/2461723002/diff/100001/components/sync/device... components/sync/device_info/device_info_service.h:8: #include <stdint.h> On 2016/10/28 21:33:20, maxbogue wrote: > Is this still needed? I'm not actually sure what's in stdint. Good catch! Done. I actually just added it to the .cc too for int64_t!
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2461723002/#ps120001 (title: "removing stdint.h from device_info_service.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Sync] DeviceInfoService static method and error cleanup. * Exampling upon ModelTypeService's comments to explain when OnMetadataLoaded should be called. * Removed old TODOs from DeviceInfoService * Updated error handling to be slightly more consistent. * Moving static methods to be anonymous namespaced. * Updated unittests to not need static DeviceInfoService methods by modifying the way test data is created. BUG=659263 ========== to ========== [Sync] DeviceInfoService static method and error cleanup. * Exampling upon ModelTypeService's comments to explain when OnMetadataLoaded should be called. * Removed old TODOs from DeviceInfoService * Updated error handling to be slightly more consistent. * Moving static methods to be anonymous namespaced. * Updated unittests to not need static DeviceInfoService methods by modifying the way test data is created. BUG=659263 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] DeviceInfoService static method and error cleanup. * Exampling upon ModelTypeService's comments to explain when OnMetadataLoaded should be called. * Removed old TODOs from DeviceInfoService * Updated error handling to be slightly more consistent. * Moving static methods to be anonymous namespaced. * Updated unittests to not need static DeviceInfoService methods by modifying the way test data is created. BUG=659263 ========== to ========== [Sync] DeviceInfoService static method and error cleanup. * Exampling upon ModelTypeService's comments to explain when OnMetadataLoaded should be called. * Removed old TODOs from DeviceInfoService * Updated error handling to be slightly more consistent. * Moving static methods to be anonymous namespaced. * Updated unittests to not need static DeviceInfoService methods by modifying the way test data is created. BUG=659263 Committed: https://crrev.com/fc8171bfb29731cb8f3c302594eb2b9ba9b9239d Cr-Commit-Position: refs/heads/master@{#428527} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fc8171bfb29731cb8f3c302594eb2b9ba9b9239d Cr-Commit-Position: refs/heads/master@{#428527} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
