Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482193003:
views_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion. (Closed)
Descriptionviews_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion.
FocusTraversalTest's BorderView uses a views::Widget as a convenient way to
create a NativeWindow, but it sets another Widget's NativeWindow as the parent
while holding a weak pointer BorderView::widget_. To safely clean up, make
sure |widget_|'s NativeWidget is only owned by |widget_| and make it a
unique_ptr instead. This avoids problems when the parent NativeWindow cleans up
its native children.
BUG=663418
Committed: https://crrev.com/b22818b5f930aa590f9c5021194904e49b742bda
Cr-Commit-Position: refs/heads/master@{#431189}
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 2
Patch Set 3 : Use base::MakeUnique, add to imports. #Messages
Total messages: 23 (17 generated)
Description was changed from ========== views_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion. BUG=663418 ========== to ========== views_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion. FocusTraversalTest's BorderView uses a views::Widget as a convenient way to create a NativeWindow, but it sets another Widget's NativeWindow as the parent while holding a weak pointer BorderView::widget_. To safely clean up, make sure |widget_|'s NativeWidget is only owned by |widget_| and make it a unique_ptr instead. This avoids problems when the parent NativeWindow cleans up its native children. BUG=663418 ==========
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@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.
patricialor@chromium.org changed reviewers: + sky@chromium.org, tapted@chromium.org
Hi Trent, this is the fix for FocusTraversalTest BorderView weirdness split out from https://codereview.chromium.org/2345183002/. I haven't changed anything since you've seen it. sky@ - This was the alternative fix tapted@ mentioned in https://bugs.chromium.org/p/chromium/issues/detail?id=663418#c2. PTAL, thank you!
I'm fine with this. LGTM https://codereview.chromium.org/2482193003/diff/20001/ui/views/focus/focus_tr... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2482193003/diff/20001/ui/views/focus/focus_tr... ui/views/focus/focus_traversal_unittest.cc:154: widget_.reset(new Widget); Use MakeUnique (see threads on chromium-dev for details).
The CQ bit was checked by patricialor@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...
Thanks for the review! https://codereview.chromium.org/2482193003/diff/20001/ui/views/focus/focus_tr... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2482193003/diff/20001/ui/views/focus/focus_tr... ui/views/focus/focus_traversal_unittest.cc:154: widget_.reset(new Widget); On 2016/11/10 04:16:10, sky wrote: > Use MakeUnique (see threads on chromium-dev for details). Done, thanks for the FYI!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2482193003/#ps40001 (title: "Use base::MakeUnique, add to imports.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== views_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion. FocusTraversalTest's BorderView uses a views::Widget as a convenient way to create a NativeWindow, but it sets another Widget's NativeWindow as the parent while holding a weak pointer BorderView::widget_. To safely clean up, make sure |widget_|'s NativeWidget is only owned by |widget_| and make it a unique_ptr instead. This avoids problems when the parent NativeWindow cleans up its native children. BUG=663418 ========== to ========== views_unittests: Fix FocusTraversalTest's BorderView::widget_ deletion. FocusTraversalTest's BorderView uses a views::Widget as a convenient way to create a NativeWindow, but it sets another Widget's NativeWindow as the parent while holding a weak pointer BorderView::widget_. To safely clean up, make sure |widget_|'s NativeWidget is only owned by |widget_| and make it a unique_ptr instead. This avoids problems when the parent NativeWindow cleans up its native children. BUG=663418 Committed: https://crrev.com/b22818b5f930aa590f9c5021194904e49b742bda Cr-Commit-Position: refs/heads/master@{#431189} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b22818b5f930aa590f9c5021194904e49b742bda Cr-Commit-Position: refs/heads/master@{#431189} |
