Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1486603002:
ExtensionServiceSyncTest cleanup (Closed)
|
Created:
5 years ago by Marc Treib Modified:
5 years ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ext_dont_selfnotify Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensionServiceSyncTest cleanup
Committed: https://crrev.com/65f10304086940c5c7f2e6d5e0de1b57d21f4ca6
Cr-Commit-Position: refs/heads/master@{#362942}
Patch Set 1 #
Total comments: 9
Patch Set 2 : review, rebase #Messages
Total messages: 11 (4 generated)
treib@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
lgtm https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:69: SyncChangeList MakeSyncChangeList(const std::string& id, Doesn't bother me much, but isn't the id in the specifics already? https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:75: return SyncChangeList(1, sync_change); nit: could even inline sync_change here. https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:180: make_scoped_ptr(new syncer::FakeSyncChangeProcessor), nitty nit: even though it doesn't matter, prefer explicit construction with (). https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:1816: // This means that it should have all-hosts withheld on this machine, ?
https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:69: SyncChangeList MakeSyncChangeList(const std::string& id, On 2015/12/02 17:24:50, Devlin wrote: > Doesn't bother me much, but isn't the id in the specifics already? Hm. Yes, but it's in the ExtensionSpecifics, so we'd have to get it either via .extension() or via .app().extension() depending on whether it's an App or an Extension. That seems more trouble than it's worth. https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:75: return SyncChangeList(1, sync_change); On 2015/12/02 17:24:50, Devlin wrote: > nit: could even inline sync_change here. Done. https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:180: make_scoped_ptr(new syncer::FakeSyncChangeProcessor), On 2015/12/02 17:24:50, Devlin wrote: > nitty nit: even though it doesn't matter, prefer explicit construction with (). Done. (Is there anything in the style guide about this? I see both styles with roughly the same frequency.) https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:1816: // This means that it should have all-hosts withheld on this machine, On 2015/12/02 17:24:50, Devlin wrote: > ? Err... no idea how that happened?! Reverted, thanks!
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1486603002/#ps20001 (title: "review, rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1486603002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1486603002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== ExtensionServiceSyncTest cleanup ========== to ========== ExtensionServiceSyncTest cleanup Committed: https://crrev.com/65f10304086940c5c7f2e6d5e0de1b57d21f4ca6 Cr-Commit-Position: refs/heads/master@{#362942} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/65f10304086940c5c7f2e6d5e0de1b57d21f4ca6 Cr-Commit-Position: refs/heads/master@{#362942}
Message was sent while issue was closed.
https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/1486603002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_sync_unittest.cc:180: make_scoped_ptr(new syncer::FakeSyncChangeProcessor), On 2015/12/03 09:29:41, Marc Treib wrote: > On 2015/12/02 17:24:50, Devlin wrote: > > nitty nit: even though it doesn't matter, prefer explicit construction with > (). > > Done. (Is there anything in the style guide about this? I see both styles with > roughly the same frequency.) Probably not, except as it pertains to POD structs, where it actually matters. But not 100% sure. So I'm just using my awesome OWNERS powers of preferences :) |
