Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2606553002:
Remove base::ScopedPtrHashMap from chrome/browser/extensions. (Closed)
|
Created:
3 years, 12 months ago by Avi (use Gerrit) Modified:
3 years, 11 months ago 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 chrome/browser/extensions.
BUG=579229
Committed: https://crrev.com/3ec9c0d0b1c26c51d9c2f0478de243298151e4e1
Cr-Commit-Position: refs/heads/master@{#440800}
Patch Set 1 #
Total comments: 11
Patch Set 2 : fixes #
Messages
Total messages: 19 (10 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: + asargent@chromium.org, benwells@chromium.org, rdevlin.cronin@chromium.org, reillyg@chromium.org
To anyone who's around this time of year.
https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:116: for (auto it = settings_by_id_.begin(); it != settings_by_id_.end(); ++it) { for (const auto& entry : settings_by_id_) { https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:129: for (auto it = settings_by_id_.begin(); it != settings_by_id_.end(); ++it) { for (const auto& entry : settings_by_id_) { https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:449: it = settings_by_id_.find(id); it = settings_by_id_.insert(std::make_pair(id, std::move(settings))).first; https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:462: it = settings_by_update_url_.find(update_url); it = settings_by_update_url_.insert(std::make_pair(update_url, std::move(settings))).first;
lgtm once Reilly's nits are addressed. (FYI, I'll be here every day except US holidays and possibly next Tuesday.) https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:449: it = settings_by_id_.find(id); On 2016/12/27 16:30:59, Reilly Grant wrote: > it = settings_by_id_.insert(std::make_pair(id, std::move(settings))).first; Or even, on line 444: std::unique_ptr<internal::IndividualSettings>& settings = settings_by_id_[id]; if (!settings) settings = base::MakeUnique<internal::IndividualSettings>(default_settings_.get()); return settings.get(); https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:462: it = settings_by_update_url_.find(update_url); On 2016/12/27 16:31:00, Reilly Grant wrote: > it = settings_by_update_url_.insert(std::make_pair(update_url, > std::move(settings))).first; ditto
https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:449: it = settings_by_id_.find(id); On 2016/12/27 16:41:58, Devlin wrote: > On 2016/12/27 16:30:59, Reilly Grant wrote: > > it = settings_by_id_.insert(std::make_pair(id, std::move(settings))).first; > > Or even, on line 444: > std::unique_ptr<internal::IndividualSettings>& settings = settings_by_id_[id]; > if (!settings) > settings = > base::MakeUnique<internal::IndividualSettings>(default_settings_.get()); > return settings.get(); Oh, I like that better.
https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:116: for (auto it = settings_by_id_.begin(); it != settings_by_id_.end(); ++it) { On 2016/12/27 16:30:59, Reilly Grant wrote: > for (const auto& entry : settings_by_id_) { Done. https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:129: for (auto it = settings_by_id_.begin(); it != settings_by_id_.end(); ++it) { On 2016/12/27 16:31:00, Reilly Grant wrote: > for (const auto& entry : settings_by_id_) { Done. https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:449: it = settings_by_id_.find(id); On 2016/12/27 16:57:48, Reilly Grant wrote: > On 2016/12/27 16:41:58, Devlin wrote: > > On 2016/12/27 16:30:59, Reilly Grant wrote: > > > it = settings_by_id_.insert(std::make_pair(id, std::move(settings))).first; > > > > Or even, on line 444: > > std::unique_ptr<internal::IndividualSettings>& settings = settings_by_id_[id]; > > if (!settings) > > settings = > > base::MakeUnique<internal::IndividualSettings>(default_settings_.get()); > > return settings.get(); > > Oh, I like that better. Done. I use that pattern elsewhere; not sure why I didn't see that here. https://codereview.chromium.org/2606553002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_management.cc:462: it = settings_by_update_url_.find(update_url); On 2016/12/27 16:41:58, Devlin wrote: > On 2016/12/27 16:31:00, Reilly Grant wrote: > > it = settings_by_update_url_.insert(std::make_pair(update_url, > > std::move(settings))).first; > > ditto Done.
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/2606553002/#ps20001 (title: "fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482875204632500,
"parent_rev": "dcec1fee61286fb37d7de4362133744db42cc2a4", "commit_rev":
"393f797de25abeef2946a6f6de8b6bc55f91d333"}
Message was sent while issue was closed.
Description was changed from ========== Remove base::ScopedPtrHashMap from chrome/browser/extensions. BUG=579229 ========== to ========== Remove base::ScopedPtrHashMap from chrome/browser/extensions. BUG=579229 Review-Url: https://codereview.chromium.org/2606553002 ==========
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 chrome/browser/extensions. BUG=579229 Review-Url: https://codereview.chromium.org/2606553002 ========== to ========== Remove base::ScopedPtrHashMap from chrome/browser/extensions. BUG=579229 Committed: https://crrev.com/3ec9c0d0b1c26c51d9c2f0478de243298151e4e1 Cr-Commit-Position: refs/heads/master@{#440800} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3ec9c0d0b1c26c51d9c2f0478de243298151e4e1 Cr-Commit-Position: refs/heads/master@{#440800} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
