Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605953002:
Remove base::ScopedPtrHashMap from extensions/. (Closed)
|
Created:
3 years, 11 months ago by Avi (use Gerrit) Modified:
3 years, 11 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove base::ScopedPtrHashMap from extensions/.
BUG=579229
Committed: https://crrev.com/7368a3ecad7c18c90855a7c449d4b36c2ba10fef
Cr-Commit-Position: refs/heads/master@{#440921}
Patch Set 1 #
Total comments: 6
Patch Set 2 : tweaks #
Messages
Total messages: 20 (13 generated)
The CQ bit was checked by avi@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.
avi@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, you're in the office! For you, a gift.
https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... File extensions/renderer/wake_event_page.cc (left): https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... extensions/renderer/wake_event_page.cc:186: request_data = requests_.take(request_id); FYI, this seems to have been a bug. It removes the scoped pointer, but leaves the entry in the map, unused forever. There's no reason to abandon rows, so I changed the code to remove the entry once it retrieves the request.
On 2016/12/28 22:50:08, Avi wrote: > Devlin, you're in the office! For you, a gift. I was hoping for one of these! :) lgtm https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... File extensions/renderer/wake_event_page.cc (left): https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... extensions/renderer/wake_event_page.cc:186: request_data = requests_.take(request_id); On 2016/12/28 22:51:19, Avi wrote: > FYI, this seems to have been a bug. It removes the scoped pointer, but leaves > the entry in the map, unused forever. There's no reason to abandon rows, so I > changed the code to remove the entry once it retrieves the request. Ah, that is bad. It almost certainly should have been take_and_erase(). It's reasons like these I'm so thrilled we can just use std:: containers now! :) https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... File extensions/renderer/wake_event_page.cc (right): https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... extensions/renderer/wake_event_page.cc:167: requests_[request_id] = std::move(request_data); optional nit: maybe just construct here? https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... extensions/renderer/wake_event_page.cc:191: CHECK(request_data) << "No request with ID " << request_id; Maybe move this to be: CHECK(it != requests_.end()) <<... below line 187?
The CQ bit was checked by avi@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 avi@chromium.org
The CQ bit was checked by avi@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/2605953002/#ps20001 (title: "tweaks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... File extensions/renderer/wake_event_page.cc (right): https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... extensions/renderer/wake_event_page.cc:167: requests_[request_id] = std::move(request_data); On 2016/12/28 23:34:32, Devlin wrote: > optional nit: maybe just construct here? Done. https://codereview.chromium.org/2605953002/diff/1/extensions/renderer/wake_ev... extensions/renderer/wake_event_page.cc:191: CHECK(request_data) << "No request with ID " << request_id; On 2016/12/28 23:34:32, Devlin wrote: > Maybe move this to be: > CHECK(it != requests_.end()) <<... > below line 187? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482971781193230,
"parent_rev": "b03c3e09241bb21b550521e563c74d5b20e042ad", "commit_rev":
"0d998a2dfc184cf81cf4a23ce4870d2e889e0947"}
Message was sent while issue was closed.
Description was changed from ========== Remove base::ScopedPtrHashMap from extensions/. BUG=579229 ========== to ========== Remove base::ScopedPtrHashMap from extensions/. BUG=579229 Review-Url: https://codereview.chromium.org/2605953002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove base::ScopedPtrHashMap from extensions/. BUG=579229 Review-Url: https://codereview.chromium.org/2605953002 ========== to ========== Remove base::ScopedPtrHashMap from extensions/. BUG=579229 Committed: https://crrev.com/7368a3ecad7c18c90855a7c449d4b36c2ba10fef Cr-Commit-Position: refs/heads/master@{#440921} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7368a3ecad7c18c90855a7c449d4b36c2ba10fef Cr-Commit-Position: refs/heads/master@{#440921} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
