Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Previously it was called whenever a view was removed, even if it
was just moved within the same widget.

Now it's only called if it's actually removed entirely, or moved
to a different widget.

BUG=581127
Committed: https://crrev.com/b5c59b11b49b910758899d8b3261a08665ca3fa0
Cr-Commit-Position: refs/heads/master@{#372448}

[dmazzoni, 2016-01-25 21:04:35]

dmazzoni@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-01-25 21:22:15]

https://codereview.chromium.org/1624343003/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1624343003/diff/1/ui/views/view.cc#newcode1827
ui/views/view.cc:1827: GetWidget()->NotifyWillRemoveView(this);
Doing this here means we have to constantly to determine the widget, which isn't
going to change. Also, I think OnWillRemoveView should only be sent if removed,
and not added to a different view in the same widget.

[dmazzoni, 2016-01-25 21:44:27]

https://codereview.chromium.org/1624343003/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1624343003/diff/1/ui/views/view.cc#newcode1827
ui/views/view.cc:1827: GetWidget()->NotifyWillRemoveView(this);
On 2016/01/25 21:22:14, sky wrote:
> Doing this here means we have to constantly to determine the widget, which
isn't
> going to change.

OK, moved to a separate helper that takes the widget as an argument.

> Also, I think OnWillRemoveView should only be sent if removed,
> and not added to a different view in the same widget.

Good point, though it seems like it should be called if it gets reparented to a
new widget, right?

I added a test for those cases.

[sky, 2016-01-25 23:35:29]

I believe the expectation is that if you cared about descendants of the view
being removed you could iterate over them. Does that not work for you?

[dmazzoni, 2016-01-26 19:19:03]

Description was changed from

==========
Call WidgetRemovalsObserver::OnWillRemoveView for child views too.

It was only being called for views explicitly removed with RemoveChildView(),
but not descendants of that view.

This was uncovered by a crash in patch set 4 of http://crrev.com/1561773002
since AXViewObjWrapper depends on knowing when the View it wraps is removed.

BUG=581127
==========

to

==========
Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Previously it was called whenever a view was removed, even if it
was just moved within the same widget.

Now it's only called if it's actually removed entirely, or moved
to a different widget.

BUG=581127
==========

[dmazzoni, 2016-01-26 19:20:01]

On 2016/01/25 23:35:29, sky wrote:
> I believe the expectation is that if you cared about descendants of the view
> being removed you could iterate over them. Does that not work for you?

OK, that's fine with me.

I updated the change description and modified the patch so that it just better
handles the case of reparenting a view.

I'll take care of descendants being deleted in a separate change.

[sky, 2016-01-26 22:32:46]

LGTM - I think you should also update the description of
WidgetRemovalsObserver::OnWillRemoveView to make it clear when OnWillRemoveView
is sent. Had the documentation been better you likely wouldn't have gone down
this road.

[dmazzoni, 2016-01-29 22:08:22]

Documentation updated. Thanks.

[dmazzoni, 2016-01-29 22:08:27]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-01-29 22:08:28]

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/1624343003/#ps60001
(title: "Update comment")

[commit-bot: I haz the power, 2016-01-29 22:09:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1624343003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1624343003/60001

[commit-bot: I haz the power, 2016-01-29 23:08:20]

Description was changed from

==========
Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Previously it was called whenever a view was removed, even if it
was just moved within the same widget.

Now it's only called if it's actually removed entirely, or moved
to a different widget.

BUG=581127
==========

to

==========
Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Previously it was called whenever a view was removed, even if it
was just moved within the same widget.

Now it's only called if it's actually removed entirely, or moved
to a different widget.

BUG=581127
==========

[commit-bot: I haz the power, 2016-01-29 23:08:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-29 23:09:34]

Description was changed from

==========
Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Previously it was called whenever a view was removed, even if it
was just moved within the same widget.

Now it's only called if it's actually removed entirely, or moved
to a different widget.

BUG=581127
==========

to

==========
Minor fixes for WidgetRemovalsObserver::OnWillRemoveView

Previously it was called whenever a view was removed, even if it
was just moved within the same widget.

Now it's only called if it's actually removed entirely, or moved
to a different widget.

BUG=581127

Committed: https://crrev.com/b5c59b11b49b910758899d8b3261a08665ca3fa0
Cr-Commit-Position: refs/heads/master@{#372448}
==========

[commit-bot: I haz the power, 2016-01-29 23:09:35]

Patchset 4 (id:??) landed as
https://crrev.com/b5c59b11b49b910758899d8b3261a08665ca3fa0
Cr-Commit-Position: refs/heads/master@{#372448}

[joedow, 2016-01-30 00:21:25]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1654433005/ by joedow@chromium.org.

The reason for reverting is: Causing a failure on the Linux Chromium OS ASan
LSan Tests bot.

Link to failure log:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

First error from log (7 in total related to widget tests):
Indirect leak of 512 byte(s) in 1 object(s) allocated from:
    #0 0x55bbab in operator new(unsigned long)
(/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__1_/build/src/out/Release/views_unittests+0x55bbab)
    #1 0xe8002c in allocate
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/ext/new_allocator.h:92:27
    #2 0xe8002c in _M_allocate_node
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:525
    #3 0xe8002c in _M_create_nodes
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:619
    #4 0xe8002c in _M_initialize_map
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:593
    #5 0xe8002c in _Deque_base
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:452
    #6 0xe8002c in deque
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:772
    #7 0xe8002c in ui::EventHandler::EventHandler()
ui/events/event_handler.cc:12
    #8 0x103aa66 in views::View::View() ui/views/view.cc:100:7
    #9 0xb1cb16 in
views::test::WidgetTest_WidgetRemovalsObserverCalled_Test::TestBody()
ui/views/widget/widget_unittest.cc:3479:21
    #10 0xd196ab in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12
    #11 0xd196ab in testing::Test::Run() testing/gtest/src/gtest.cc:2474
    #12 0xd1ba77 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5
    #13 0xd1c86a in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5
    #14 0xd31660 in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4647:11
    #15 0xd30a8b in
HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2458:12
    #16 0xd30a8b in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255
    #17 0xcdd3a6 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10
    #18 0xcdd3a6 in base::TestSuite::Run() base/test/test_suite.cc:231
    #19 0xcd0642 in Run base/callback.h:394:12
    #20 0xcd0642 in base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, int,
bool, base::Callback<void ()> const&)
base/test/launcher/unit_test_launcher.cc:206
    #21 0xcd02dc in base::LaunchUnitTests(int, char**, base::Callback<int ()>
const&) base/test/launcher/unit_test_launcher.cc:445:10
    #22 0x981c94 in main ui/views/run_all_unittests.cc:53:10
    #23 0x7ff93a18876c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226.