Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669593004:
[Sync] Add the Model API document. (Closed)
Description[Sync] Add the Model API document.
This document contains organic, artisanal links of the highest quality,
hand-crafted for maximum robustness in the face of code change.
Note that you can patch this CL and use
./tools/md_browser/md_browser.py to see a rendered version, though the
styling isn't quite the same as Gitiles.
BUG=658298
Review-Url: https://codereview.chromium.org/2669593004
Cr-Commit-Position: refs/heads/master@{#447613}
Committed: https://chromium.googlesource.com/chromium/src/+/119f6c9a974e33480723f429d42e3b787c59c0b5
Patch Set 1 #
Total comments: 2
Patch Set 2 : Link Put properly. #
Total comments: 8
Patch Set 3 : Address comments #Messages
Total messages: 25 (17 generated)
The CQ bit was checked by maxbogue@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Sync] Add the Model API document. BUG=658298 ========== to ========== [Sync] Add the Model API document. This document contains organic, artisanal links of the highest quality, hand-crafted for maximum robustness in the face of code change. Note that you can patch this CL and use ./tools/md_browser/md_browser.py to see a rendered version, though the styling isn't quite the same as Gitiles. BUG=658298 ==========
maxbogue@chromium.org changed reviewers: + pavely@chromium.org, skym@chromium.org, zea@chromium.org
Pavel and/or Sky and/or Nicolas, PTAL! This document will need to be updated and improved as USS continues to develop.
lgtm https://codereview.chromium.org/2669593004/diff/1/docs/sync/model_api.md File docs/sync/model_api.md (right): https://codereview.chromium.org/2669593004/diff/1/docs/sync/model_api.md#newc... docs/sync/model_api.md:152: [`ModelTypeChangeProcessor::Put`][MTCP]. This doesn't link to ::Put though, right?
The CQ bit was checked by maxbogue@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...
https://codereview.chromium.org/2669593004/diff/1/docs/sync/model_api.md File docs/sync/model_api.md (right): https://codereview.chromium.org/2669593004/diff/1/docs/sync/model_api.md#newc... docs/sync/model_api.md:152: [`ModelTypeChangeProcessor::Put`][MTCP]. On 2017/02/01 01:10:37, skym wrote: > This doesn't link to ::Put though, right? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry it took me so long to get around to reviewing this part of the doc. https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md File docs/sync/model_api.md (right): https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:253: ## Sync Integration Checklist Always: https://cs.chromium.org/chromium/src/components/sync/base/data_type_histogram.h https://cs.chromium.org/chromium/src/components/sync/driver/model_association... https://cs.chromium.org/search/?q="39+%3D%3D+MODEL_TYPE_COUNT"&type=cs https://cs.chromium.org/chromium/src/components/sync/syncable/nigori_util.cc?... https://cs.chromium.org/chromium/src/components/sync/protocol/nigori_specific... https://cs.chromium.org/chromium/src/components/sync/base/pref_names.cc https://cs.chromium.org/chromium/src/components/sync/base/pref_names.h https://cs.chromium.org/chromium/src/components/sync/base/sync_prefs.cc?rcl=5... https://cs.chromium.org/chromium/src/components/sync/base/sync_prefs.cc?rcl=5... Sometimes: https://cs.chromium.org/chromium/src/components/sync/driver/user_selectable_s... https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:257: * Add it to the [`ModelType`][ModelType] enum. Should we link to .h and .cc files? I'm not sure it's obvious, especially when they're in different folders. I realize it says to update the .cc in the .h's class comments, but reading is hard. https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:258: * Add it to the [proto value conversions][conversions] files. I think they should update proto enum conversions as well. Though this one seems less consistent. https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:259: * Add to the SyncModelTypes histogram enum in [`histograms.xml`][histograms]. And "SyncModelType" suffix.
lgtm
The CQ bit was checked by maxbogue@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...
https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md File docs/sync/model_api.md (right): https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:253: ## Sync Integration Checklist On 2017/02/01 02:42:41, skym wrote: > Always: > https://cs.chromium.org/chromium/src/components/sync/base/data_type_histogram.h > https://cs.chromium.org/chromium/src/components/sync/driver/model_association... > https://cs.chromium.org/search/?q="39+%3D%3D+MODEL_TYPE_COUNT"&type=cs > https://cs.chromium.org/chromium/src/components/sync/syncable/nigori_util.cc?... > https://cs.chromium.org/chromium/src/components/sync/protocol/nigori_specific... > https://cs.chromium.org/chromium/src/components/sync/base/pref_names.cc > https://cs.chromium.org/chromium/src/components/sync/base/pref_names.h > https://cs.chromium.org/chromium/src/components/sync/base/sync_prefs.cc?rcl=5... > https://cs.chromium.org/chromium/src/components/sync/base/sync_prefs.cc?rcl=5... > > Sometimes: > https://cs.chromium.org/chromium/src/components/sync/driver/user_selectable_s... Done. https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:257: * Add it to the [`ModelType`][ModelType] enum. On 2017/02/01 02:42:41, skym wrote: > Should we link to .h and .cc files? I'm not sure it's obvious, especially when > they're in different folders. I realize it says to update the .cc in the .h's > class comments, but reading is hard. Done. https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:258: * Add it to the [proto value conversions][conversions] files. On 2017/02/01 02:42:41, skym wrote: > I think they should update proto enum conversions as well. Though this one seems > less consistent. Yeah, doesn't seem needed necessarily so I'm omitting. https://codereview.chromium.org/2669593004/diff/20001/docs/sync/model_api.md#... docs/sync/model_api.md:259: * Add to the SyncModelTypes histogram enum in [`histograms.xml`][histograms]. On 2017/02/01 02:42:41, skym wrote: > And "SyncModelType" suffix. Done.
The CQ bit was unchecked by maxbogue@chromium.org
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2669593004/#ps40001 (title: "Address 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": 40001, "attempt_start_ts": 1485982994022180,
"parent_rev": "c84d488aa22e7c9a660b2c8f84a9a7edcc9a7e3f", "commit_rev":
"119f6c9a974e33480723f429d42e3b787c59c0b5"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Add the Model API document. This document contains organic, artisanal links of the highest quality, hand-crafted for maximum robustness in the face of code change. Note that you can patch this CL and use ./tools/md_browser/md_browser.py to see a rendered version, though the styling isn't quite the same as Gitiles. BUG=658298 ========== to ========== [Sync] Add the Model API document. This document contains organic, artisanal links of the highest quality, hand-crafted for maximum robustness in the face of code change. Note that you can patch this CL and use ./tools/md_browser/md_browser.py to see a rendered version, though the styling isn't quite the same as Gitiles. BUG=658298 Review-Url: https://codereview.chromium.org/2669593004 Cr-Commit-Position: refs/heads/master@{#447613} Committed: https://chromium.googlesource.com/chromium/src/+/119f6c9a974e33480723f429d42e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/119f6c9a974e33480723f429d42e... |
