Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691323002:
Fixed memory leak in Password Manager UI. (Closed)
DescriptionFixed memory leak in Password Manager UI.
view::Combobox is not owner of a model, previously the model was not deleted in CredentialsSelectionView.
BUG=692464
Review-Url: https://codereview.chromium.org/2691323002
Cr-Commit-Position: refs/heads/master@{#450670}
Committed: https://chromium.googlesource.com/chromium/src/+/3f04263b4bf4d2d0cc447e0b162a837d60b2ba73
Patch Set 1 #Patch Set 2 : Comment updated #
Total comments: 2
Patch Set 3 : Addressing reviewer's comments #
Total comments: 2
Messages
Total messages: 19 (12 generated)
Description was changed from ========== format Fix memory leak BUG=None ========== to ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted. BUG=None ==========
Description was changed from ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted. BUG=None ========== to ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted in CredentialsSelectionView. BUG=None ==========
dvadym@chromium.org changed reviewers: + vasilii@chromium.org
Hi Vasilii, could you please review this CL? Regards, Vadym
lgtm https://codereview.chromium.org/2691323002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right): https://codereview.chromium.org/2691323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_selection_view.cc:84: views::Combobox* CredentialsSelectionView::GenerateUsernameCombobox( I'd drop the return value.
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Thanks Vasilii for review! https://codereview.chromium.org/2691323002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right): https://codereview.chromium.org/2691323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_selection_view.cc:84: views::Combobox* CredentialsSelectionView::GenerateUsernameCombobox( On 2017/02/14 16:07:18, vasilii wrote: > I'd drop the return value. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted in CredentialsSelectionView. BUG=None ========== to ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted in CredentialsSelectionView. BUG=692464 ==========
The CQ bit was unchecked by dvadym@chromium.org
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2691323002/#ps40001 (title: "Addressing reviewer's 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": 1487156413536030,
"parent_rev": "e6cfc1a979decf08475dcef3b095c8af9c8f2649", "commit_rev":
"3f04263b4bf4d2d0cc447e0b162a837d60b2ba73"}
Message was sent while issue was closed.
Description was changed from ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted in CredentialsSelectionView. BUG=692464 ========== to ========== Fixed memory leak in Password Manager UI. view::Combobox is not owner of a model, previously the model was not deleted in CredentialsSelectionView. BUG=692464 Review-Url: https://codereview.chromium.org/2691323002 Cr-Commit-Position: refs/heads/master@{#450670} Committed: https://chromium.googlesource.com/chromium/src/+/3f04263b4bf4d2d0cc447e0b162a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3f04263b4bf4d2d0cc447e0b162a...
Message was sent while issue was closed.
mathp@chromium.org changed reviewers: + mathp@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2691323002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right): https://codereview.chromium.org/2691323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_selection_view.cc:73: combobox_.reset(); you could also have combobox_model_ be above combobox_ in the member list. C++ destroys members in reverse order of their instantiation
Message was sent while issue was closed.
https://codereview.chromium.org/2691323002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right): https://codereview.chromium.org/2691323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_selection_view.cc:73: combobox_.reset(); On 2017/02/15 13:46:57, Mathieu Perreault wrote: > you could also have combobox_model_ be above combobox_ in the member list. C++ > destroys members in reverse order of their instantiation Yeah, sure. In other places it was done this way. But I like more explicit way, probably just matter of preferences. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
