Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565213002:
Add destructor to ReadingListStoreDelegate (Closed)
DescriptionAdd destructor to ReadingListStoreDelegate
ReadingListStoreDelegate has virtual methods, so it should have a virtual
destructor.
BUG=673169
Committed: https://crrev.com/4cc7bad91ea05773979249d6b92abe6eb2cebc60
Cr-Commit-Position: refs/heads/master@{#437801}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #Messages
Total messages: 14 (8 generated)
olivierrobin@chromium.org changed reviewers: + sdefresne@chromium.org
I checked all RL classes. Sorry for the mess.
The CQ bit was checked by olivierrobin@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...
lgtm https://codereview.chromium.org/2565213002/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_store_delegate.h (right): https://codereview.chromium.org/2565213002/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_store_delegate.h:21: // These three mathods handle callbacks from a ReadingListStore. nit: can you fix typo here "mathods" should be spelled "methods" https://codereview.chromium.org/2565213002/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_store_delegate.h:22: virtual void StoreLoaded(std::unique_ptr<ReadingListEntries> entries) = 0; nit: this method has no comment (the comment above is common for the three methods, and the next two methods have comment, just not this one). Can you add one as a followup?
Done, thanks https://codereview.chromium.org/2565213002/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_store_delegate.h (right): https://codereview.chromium.org/2565213002/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_store_delegate.h:21: // These three mathods handle callbacks from a ReadingListStore. On 2016/12/11 19:34:51, sdefresne wrote: > nit: can you fix typo here "mathods" should be spelled "methods" Done. https://codereview.chromium.org/2565213002/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_store_delegate.h:22: virtual void StoreLoaded(std::unique_ptr<ReadingListEntries> entries) = 0; On 2016/12/11 19:34:51, sdefresne wrote: > nit: this method has no comment (the comment above is common for the three > methods, and the next two methods have comment, just not this one). Can you add > one as a followup? Done.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2565213002/#ps20001 (title: "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": 20001, "attempt_start_ts": 1481485174554630,
"parent_rev": "af194f64e904f07a766dae36e3e5510fa75dc84f", "commit_rev":
"424a77fa0681b6c0fb9ada15272709535a2c6d56"}
Message was sent while issue was closed.
Description was changed from ========== Add destructor to ReadingListStoreDelegate ReadingListStoreDelegate has virtual methods, so it should have a virtual destructor. BUG=673169 ========== to ========== Add destructor to ReadingListStoreDelegate ReadingListStoreDelegate has virtual methods, so it should have a virtual destructor. BUG=673169 Review-Url: https://codereview.chromium.org/2565213002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add destructor to ReadingListStoreDelegate ReadingListStoreDelegate has virtual methods, so it should have a virtual destructor. BUG=673169 Review-Url: https://codereview.chromium.org/2565213002 ========== to ========== Add destructor to ReadingListStoreDelegate ReadingListStoreDelegate has virtual methods, so it should have a virtual destructor. BUG=673169 Committed: https://crrev.com/4cc7bad91ea05773979249d6b92abe6eb2cebc60 Cr-Commit-Position: refs/heads/master@{#437801} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4cc7bad91ea05773979249d6b92abe6eb2cebc60 Cr-Commit-Position: refs/heads/master@{#437801} |
