Add View::AddedToWidget and RemovedFromWidget.

Add View::AddedToWidget and RemovedFromWidget.

Many views implement ViewHierarchyChanged to perform some setup once a
Widget becomes available to them. This results with code duplication.
With these new methods there will be no need to implement
ViewHierarchyChanged for the common case.

BUG=5191
Review-Url: https://codereview.chromium.org/2713643002
Cr-Commit-Position: refs/heads/master@{#454542}
Committed: https://chromium.googlesource.com/chromium/src/+/88ece8a16a39921247c1a74dc1747ea00e8e9377

[msimonides, 2017-02-22 10:47:48]

msimonides@opera.com changed reviewers:
+ sky@chromium.org

[sky, 2017-02-22 17:21:33]

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

https://codereview.chromium.org/2713643002/diff/1/ui/views/view.cc#newcode2030
ui/views/view.cc:2030: ViewHierarchyChanged(details);
Please call the new functions from here. That way subclasses that are already
overriding ViewHierarchyChanged aren't impacted.

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

https://codereview.chromium.org/2713643002/diff/1/ui/views/view_unittest.cc#n...
ui/views/view_unittest.cc:3352: bool was_added_to_widget_;
Make these counts, so you know if called more often than you expect.

https://codereview.chromium.org/2713643002/diff/1/ui/views/view_unittest.cc#n...
ui/views/view_unittest.cc:3354: };
DISALLOW...

https://codereview.chromium.org/2713643002/diff/1/ui/views/view_unittest.cc#n...
ui/views/view_unittest.cc:3388: std::unique_ptr<Widget> widget(new Widget);
No need for a unique_ptr here, create on stack.

[msimonides, 2017-02-24 06:52:21]

Added fixes for review comments.

[sky, 2017-02-24 18:26:14]

https://codereview.chromium.org/2713643002/diff/20001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2713643002/diff/20001/ui/views/view.h#newcode...
ui/views/view.h:1504: Widget* attached_widget_;
We shouldn't need to cache the widget. This likely requires changing the
signature of ViewHierarchyChangedImpl() to indicate one of added to widget,
removed from widget or no change in widget status.

[msimonides, 2017-02-27 09:52:28]

https://codereview.chromium.org/2713643002/diff/20001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2713643002/diff/20001/ui/views/view.h#newcode...
ui/views/view.h:1504: Widget* attached_widget_;
On 2017/02/24 18:26:14, sky wrote:
> We shouldn't need to cache the widget. This likely requires changing the
> signature of ViewHierarchyChangedImpl() to indicate one of added to widget,
> removed from widget or no change in widget status.

Done.

[sky, 2017-02-27 16:46:54]

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

https://codereview.chromium.org/2713643002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:254: view->PropagateAddedToWidget();
I would like to avoid more tree walking. Can't you incorporate the additional
notification in ViewHierarchyChangedImpl?

[msimonides, 2017-02-28 10:07:28]

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

https://codereview.chromium.org/2713643002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:254: view->PropagateAddedToWidget();
On 2017/02/27 16:46:54, sky wrote:
> I would like to avoid more tree walking. Can't you incorporate the additional
> notification in ViewHierarchyChangedImpl?

Ok.
I will merge PropagateAddedToWidget into PropagateAddNotifications and
PropagateRemovedFromWidget into PropagateRemoveNotifications.
Calling the notifications from ViewHierarchyChangedImpl would require more
complex argument passing (widget added, widget removed, widget not changed).

Alternatively, ViewHierarchyChangedDetails could be extended with the
information about widget change. I think this would be overly complex but I can
do this if you wish.

[sky, 2017-02-28 17:35:33]

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

https://codereview.chromium.org/2713643002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:254: view->PropagateAddedToWidget();
On 2017/02/28 10:07:27, msimonides wrote:
> On 2017/02/27 16:46:54, sky wrote:
> > I would like to avoid more tree walking. Can't you incorporate the
additional
> > notification in ViewHierarchyChangedImpl?
> 
> Ok.
> I will merge PropagateAddedToWidget into PropagateAddNotifications and
> PropagateRemovedFromWidget into PropagateRemoveNotifications.
> Calling the notifications from ViewHierarchyChangedImpl would require more
> complex argument passing (widget added, widget removed, widget not changed).

That's fine. These are implementation details of Views.

> Alternatively, ViewHierarchyChangedDetails could be extended with the
> information about widget change. I think this would be overly complex but I
can
> do this if you wish.

I agree. I prefer separate functions for subclasses.

[msimonides, 2017-03-01 10:31:52]

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

https://codereview.chromium.org/2713643002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:254: view->PropagateAddedToWidget();
On 2017/02/28 17:35:33, sky wrote:
> On 2017/02/28 10:07:27, msimonides wrote:
> > On 2017/02/27 16:46:54, sky wrote:
> > > I would like to avoid more tree walking. Can't you incorporate the
> additional
> > > notification in ViewHierarchyChangedImpl?
> > 
> > Ok.
> > I will merge PropagateAddedToWidget into PropagateAddNotifications and
> > PropagateRemovedFromWidget into PropagateRemoveNotifications.
> > Calling the notifications from ViewHierarchyChangedImpl would require more
> > complex argument passing (widget added, widget removed, widget not changed).
> 
> That's fine. These are implementation details of Views.
> 
> > Alternatively, ViewHierarchyChangedDetails could be extended with the
> > information about widget change. I think this would be overly complex but I
> can
> > do this if you wish.
> 
> I agree. I prefer separate functions for subclasses.

Done.

[sky, 2017-03-01 20:26:52]

Almost there, thanks.

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

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:199: Widget* old_widget = NULL;
nullptr

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.h#newcode...
ui/views/view.h:1104: // Refer to comments in struct
|ViewHierarchyChangedDetails| for |details|.
Add a comment here to see AddedToWidget/RemovedFromWidget for detecting when
added/removed from widget.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.h#newcode...
ui/views/view.h:1124: // a widget.
Clarify this is also invoked if a view is moved to a different widget.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view_unittest....
ui/views/view_unittest.cc:3352: int added_to_widget_count_;
Either set in member initializer, or initialize to 0 here.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view_unittest....
ui/views/view_unittest.cc:3417: 
Add assertions that v1's counts haven't changed (maybe a reset on 3414 will make
it more readable).

[msimonides, 2017-03-02 07:48:56]

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

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:199: Widget* old_widget = NULL;
On 2017/03/01 20:26:51, sky wrote:
> nullptr

Done.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.h#newcode...
ui/views/view.h:1104: // Refer to comments in struct
|ViewHierarchyChangedDetails| for |details|.
On 2017/03/01 20:26:51, sky wrote:
> Add a comment here to see AddedToWidget/RemovedFromWidget for detecting when
> added/removed from widget.

Done.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view.h#newcode...
ui/views/view.h:1124: // a widget.
On 2017/03/01 20:26:51, sky wrote:
> Clarify this is also invoked if a view is moved to a different widget.

Done.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view_unittest....
ui/views/view_unittest.cc:3352: int added_to_widget_count_;
On 2017/03/01 20:26:51, sky wrote:
> Either set in member initializer, or initialize to 0 here.

Done.

https://codereview.chromium.org/2713643002/diff/60001/ui/views/view_unittest....
ui/views/view_unittest.cc:3417: 
On 2017/03/01 20:26:51, sky wrote:
> Add assertions that v1's counts haven't changed (maybe a reset on 3414 will
make
> it more readable).

Done.

[sky, 2017-03-02 18:19:31]

LGTM - thanks!

[msimonides, 2017-03-03 07:03:33]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2017-03-03 07:03:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 08:00:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488524613407860,
"parent_rev": "3fa1b666a54ad027289ec0c1752fcfb6d138b512", "commit_rev":
"88ece8a16a39921247c1a74dc1747ea00e8e9377"}

[commit-bot: I haz the power, 2017-03-03 08:01:18]

Description was changed from

==========
Add View::AddedToWidget and RemovedFromWidget.

Many views implement ViewHierarchyChanged to perform some setup once a
Widget becomes available to them. This results with code duplication.
With these new methods there will be no need to implement
ViewHierarchyChanged for the common case.

BUG=5191
==========

to

==========
Add View::AddedToWidget and RemovedFromWidget.

Many views implement ViewHierarchyChanged to perform some setup once a
Widget becomes available to them. This results with code duplication.
With these new methods there will be no need to implement
ViewHierarchyChanged for the common case.

BUG=5191

Review-Url: https://codereview.chromium.org/2713643002
Cr-Commit-Position: refs/heads/master@{#454542}
Committed:
https://chromium.googlesource.com/chromium/src/+/88ece8a16a39921247c1a74dc174...
==========

[commit-bot: I haz the power, 2017-03-03 08:01:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/88ece8a16a39921247c1a74dc174...