Add ViewObserver to View for view updates

Add ViewObserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
Committed: https://crrev.com/7d1faa6c9ea5f4d5a2c10414d8a33b75a9f190e7
Cr-Commit-Position: refs/heads/master@{#434026}

[Sarmad Hashmi, 2016-11-11 21:47:14]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 21:48:00]

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

[Sarmad Hashmi, 2016-11-11 22:52:51]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 22:53:01]

Dry run: 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, 2016-11-11 23:47:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 23:47:31]

Dry run: This issue passed the CQ dry run.

[Sarmad Hashmi, 2016-11-12 09:03:40]

mhashmi@chromium.org changed reviewers:
+ sadrul@chromium.org

[Sarmad Hashmi, 2016-11-12 09:03:41]

PTAL sadrul@

[sadrul, 2016-11-15 17:21:49]

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

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view.cc#newcod...
ui/views/view.cc:123: devtools_observer_(NULL) {
nullptr

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view.cc#newcod...
ui/views/view.cc:1898: devtools_observer_->OnViewHierarchyChanged(this,
details);
Is this the right place for this? Should this be in one of  the
Propagate*Notifications functions instead?

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

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view.h#newcode...
ui/views/view.h:1008: void SetDevToolsObserver(DevToolsObserver*
devtools_observer);
set_devtools_observer

and inline the impl in here.

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

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view_unittest....
ui/views/view_unittest.cc:4599: view_hierarchy_changed_details_ = details;
I think we want to check that this isn't called more than it needs to be. Can
you add checks for the number of times this is called for each step in the tests
too?

[Sarmad Hashmi, 2016-11-15 17:54:29]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 17:54:55]

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

[Sarmad Hashmi, 2016-11-15 17:54:56]

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

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view.cc#newcod...
ui/views/view.cc:123: devtools_observer_(NULL) {
On 2016/11/15 17:21:49, sadrul wrote:
> nullptr

Done.

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view.cc#newcod...
ui/views/view.cc:1898: devtools_observer_->OnViewHierarchyChanged(this,
details);
On 2016/11/15 17:21:49, sadrul wrote:
> Is this the right place for this? Should this be in one of  the
> Propagate*Notifications functions instead?

ViewHierarchyChangedImpl is called from the Propogate*Notifications functions,
so not sure if it would make a difference?

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

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view.h#newcode...
ui/views/view.h:1008: void SetDevToolsObserver(DevToolsObserver*
devtools_observer);
On 2016/11/15 17:21:49, sadrul wrote:
> set_devtools_observer
> 
> and inline the impl in here.

Done.

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

https://codereview.chromium.org/2500623002/diff/20001/ui/views/view_unittest....
ui/views/view_unittest.cc:4599: view_hierarchy_changed_details_ = details;
On 2016/11/15 17:21:49, sadrul wrote:
> I think we want to check that this isn't called more than it needs to be. Can
> you add checks for the number of times this is called for each step in the
tests
> too?

Done.

[Sarmad Hashmi, 2016-11-15 21:19:36]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 21:19:48]

Dry run: 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, 2016-11-15 21:33:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 21:33:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sadrul, 2016-11-16 18:35:23]

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

https://codereview.chromium.org/2500623002/diff/60001/ui/views/view_unittest....
ui/views/view_unittest.cc:4678: EXPECT_EQ(2, views_hierarchy_changed_times());
Does this need to be called twice? Shouldn't once be sufficient for devtools
frontend to update correctly?

[Sarmad Hashmi, 2016-11-16 20:51:40]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[Sarmad Hashmi, 2016-11-16 20:52:27]

As discussed, added OnViewHierarchyChanged call under AddChildViewAt and
DoRemoveChildView.

[commit-bot: I haz the power, 2016-11-16 20:52:43]

Dry run: 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, 2016-11-16 22:14:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 22:14:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sadrul, 2016-11-18 19:07:50]

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

[sadrul, 2016-11-18 19:07:51]

+sky@

lgtm, but please wait for approval from sky@ too.

https://codereview.chromium.org/2500623002/diff/80001/ui/views/devtools_obser...
File ui/views/devtools_observer.h (right):

https://codereview.chromium.org/2500623002/diff/80001/ui/views/devtools_obser...
ui/views/devtools_observer.h:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
No (c)

[Sarmad Hashmi, 2016-11-19 00:23:18]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 00:24:05]

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

[Sarmad Hashmi, 2016-11-19 00:29:11]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[Sarmad Hashmi, 2016-11-19 00:29:51]

https://codereview.chromium.org/2500623002/diff/80001/ui/views/devtools_obser...
File ui/views/devtools_observer.h (right):

https://codereview.chromium.org/2500623002/diff/80001/ui/views/devtools_obser...
ui/views/devtools_observer.h:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
On 2016/11/18 19:07:51, sadrul wrote:
> No (c)

Done.

[commit-bot: I haz the power, 2016-11-19 00:29:51]

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

[sky, 2016-11-19 00:46:31]

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
File ui/views/devtools_observer.h (right):

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:18: class VIEWS_EXPORT DevToolsObserver {
I don't think there is a good reason to limit this observer to just devtools.
Please rename to ViewsObserver with the typical add/remove pattern else where.

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:23: virtual void OnViewHierarchyChanged(View*
target,
This name has too much baggage with the existing View implementation. How about
OnViewParentChanged?

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:27: virtual void OnViewVisibilityChanged(View*
view, bool visible) {}
Clearly indicate when this is sent. I assume it's just when the internal
visibility of the view changes, but the view may still be hidden if an ancestor
is hidden.

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:29: virtual void OnViewEnabledChanged(View* view,
int index) {}
What is index here?

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:31: virtual void OnViewBoundsChanged(View* view,
const gfx::Rect& new_bounds) {}
Why do you need the new bounds? Can't the caller do view->bounds() if it wants
the new bounds? I could see passing the old.

[commit-bot: I haz the power, 2016-11-19 01:24:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-19 01:24:57]

Dry run: This issue passed the CQ dry run.

[Sarmad Hashmi, 2016-11-20 04:55:01]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-20 04:55:12]

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

[Sarmad Hashmi, 2016-11-20 06:46:21]

Description was changed from

==========
Add DevToolsobserver to View for view updates

- OnViewHierarchyChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

to

==========
Add DevToolsobserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

[Sarmad Hashmi, 2016-11-20 06:46:54]

Description was changed from

==========
Add DevToolsobserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

to

==========
Add ViewObserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

[Sarmad Hashmi, 2016-11-20 06:47:08]

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
File ui/views/devtools_observer.h (right):

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:18: class VIEWS_EXPORT DevToolsObserver {
On 2016/11/19 00:46:31, sky wrote:
> I don't think there is a good reason to limit this observer to just devtools.
> Please rename to ViewsObserver with the typical add/remove pattern else where.

Done.

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:23: virtual void OnViewHierarchyChanged(View*
target,
On 2016/11/19 00:46:31, sky wrote:
> This name has too much baggage with the existing View implementation. How
about
> OnViewParentChanged?

Done.

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:27: virtual void OnViewVisibilityChanged(View*
view, bool visible) {}
On 2016/11/19 00:46:31, sky wrote:
> Clearly indicate when this is sent. I assume it's just when the internal
> visibility of the view changes, but the view may still be hidden if an
ancestor
> is hidden.

Done.

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:29: virtual void OnViewEnabledChanged(View* view,
int index) {}
On 2016/11/19 00:46:31, sky wrote:
> What is index here?

Sorry, this was supposed to bool (enabled state). Removed it since
view->enabled() returns the same thing.

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:31: virtual void OnViewBoundsChanged(View* view,
const gfx::Rect& new_bounds) {}
On 2016/11/19 00:46:31, sky wrote:
> Why do you need the new bounds? Can't the caller do view->bounds() if it wants
> the new bounds? I could see passing the old.

Fixed.

[commit-bot: I haz the power, 2016-11-20 07:28:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-20 07:28:25]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-21 16:33:38]

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:254: observer.OnChildViewReordered(view, index);
Is index really useful for this notification?

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:453: observer.OnViewEnabledChanged(this);
I mildly favor calling this after OnEnabledChanged. That better matches other
places you're notifying observers.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:1383: // Observers
-------------------------------------------------------------------
No need for this comment, please remove.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:1880: NotifyViewParentChanged(view, this, nullptr);
DoRemoveChildView is called from AddChildView. So, notifying here is wrong in
some cases. Please add test coverage of this case, e.g. adding when a view
already has a parent.

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.h#newcod...
ui/views/view.h:1004: // Observers
-----------------------------------------------------------------
No need for this comment, please remove.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.h#newcod...
ui/views/view.h:1009: void NotifyViewParentChanged(View* view, View* old_parent,
View* new_parent);
Why is this function public? Also, why does it take a view? Can't the view be
|this|?

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_observer.h
File ui/views/view_observer.h (right):

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_observer...
ui/views/view_observer.h:19: // Called whenever target |view| is being moved. If
|old_parent| is NULL,
NULL -> null for all cases.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_observer...
ui/views/view_observer.h:26: // Called when the visibile property is changed
(the |view| may still be
How about: Called when View::SetEnabled() is called with a new value.

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_unittest...
ui/views/view_unittest.cc:4595: void OnViewParentChanged(View* target,
Generally we prefix overrides with the class they are overriding, e.g.:
// ViewObserver:

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_unittest...
ui/views/view_unittest.cc:4663: };
DISALLOW...

[Sarmad Hashmi, 2016-11-21 17:09:41]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[Sarmad Hashmi, 2016-11-21 17:09:54]

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:254: observer.OnChildViewReordered(view, index);
On 2016/11/21 16:33:37, sky wrote:
> Is index really useful for this notification?

Avoids looking up where the view was placed after being reordered. Removed for
now.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:453: observer.OnViewEnabledChanged(this);
On 2016/11/21 16:33:37, sky wrote:
> I mildly favor calling this after OnEnabledChanged. That better matches other
> places you're notifying observers.

Done.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:1383: // Observers
-------------------------------------------------------------------
On 2016/11/21 16:33:37, sky wrote:
> No need for this comment, please remove.

Done.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:1880: NotifyViewParentChanged(view, this, nullptr);
On 2016/11/21 16:33:37, sky wrote:
> DoRemoveChildView is called from AddChildView. So, notifying here is wrong in
> some cases. Please add test coverage of this case, e.g. adding when a view
> already has a parent.

Already have this under the ViewParentChanged test.

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.h#newcod...
ui/views/view.h:1004: // Observers
-----------------------------------------------------------------
On 2016/11/21 16:33:37, sky wrote:
> No need for this comment, please remove.

Done.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.h#newcod...
ui/views/view.h:1009: void NotifyViewParentChanged(View* view, View* old_parent,
View* new_parent);
On 2016/11/21 16:33:37, sky wrote:
> Why is this function public? Also, why does it take a view? Can't the view be
> |this|?

Made it private. Updated NotifyViewParentChanged to only be called on the |view|
itself (was called on |parent| before). NotifyViewParentChanged and
OnViewParentChanged then only need |old_parent| because |new_parent| ==
View::parent().

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_observer.h
File ui/views/view_observer.h (right):

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_observer...
ui/views/view_observer.h:19: // Called whenever target |view| is being moved. If
|old_parent| is NULL,
On 2016/11/21 16:33:37, sky wrote:
> NULL -> null for all cases.

Done.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_observer...
ui/views/view_observer.h:26: // Called when the visibile property is changed
(the |view| may still be
On 2016/11/21 16:33:37, sky wrote:
> How about: Called when View::SetEnabled() is called with a new value. 

Done.

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_unittest...
ui/views/view_unittest.cc:4595: void OnViewParentChanged(View* target,
On 2016/11/21 16:33:37, sky wrote:
> Generally we prefix overrides with the class they are overriding, e.g.:
> // ViewObserver:

Done.

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view_unittest...
ui/views/view_unittest.cc:4663: };
On 2016/11/21 16:33:37, sky wrote:
> DISALLOW...

Done.

[commit-bot: I haz the power, 2016-11-21 17:10:22]

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

[sadrul, 2016-11-21 18:07:34]

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
File ui/views/devtools_observer.h (right):

https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
ui/views/devtools_observer.h:18: class VIEWS_EXPORT DevToolsObserver {
On 2016/11/19 00:46:31, sky wrote:
> I don't think there is a good reason to limit this observer to just devtools.
> Please rename to ViewsObserver with the typical add/remove pattern else where.

This was my suggestion to have a devtools-focused observer. My worry was that
there may be unwarranted use of this (especially since we have lived for so long
without a generic observer for View). But maybe that's not really true. I am OK
with a generic observer.

[sky, 2016-11-21 18:20:40]

On Mon, Nov 21, 2016 at 10:07 AM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
> File ui/views/devtools_observer.h (right):
>
>
https://codereview.chromium.org/2500623002/diff/120001/ui/views/devtools_obse...
> ui/views/devtools_observer.h:18: class VIEWS_EXPORT DevToolsObserver {
> On 2016/11/19 00:46:31, sky wrote:
>> I don't think there is a good reason to limit this observer to just
> devtools.
>> Please rename to ViewsObserver with the typical add/remove pattern
> else where.
>
> This was my suggestion to have a devtools-focused observer. My worry was
> that there may be unwarranted use of this (especially since we have
> lived for so long without a generic observer for View). But maybe that's
> not really true. I am OK with a generic observer.
>
> https://codereview.chromium.org/2500623002/

Understood. To do anything interesting with View pretty much requires
subclassing. I think there is going to be a push to change that.
Starting with adding an observer seems reasonable to me.

  -Scott

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-11-21 18:27:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 18:27:05]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-21 18:32:12]

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

https://codereview.chromium.org/2500623002/diff/160001/ui/views/view.cc#newco...
ui/views/view.cc:1880: NotifyViewParentChanged(view, this, nullptr);
On 2016/11/21 17:09:53, Sarmad Hashmi wrote:
> On 2016/11/21 16:33:37, sky wrote:
> > DoRemoveChildView is called from AddChildView. So, notifying here is wrong
in
> > some cases. Please add test coverage of this case, e.g. adding when a view
> > already has a parent.
> 
> Already have this under the ViewParentChanged test.

Sure, but your test is verifying the observer is called with a null parent, then
a new parent. It should ensure only one change is observed with the new parent.

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

https://codereview.chromium.org/2500623002/diff/200001/ui/views/view.cc#newco...
ui/views/view.cc:225: view->NotifyViewParentChanged(nullptr);
Don't you want to pass parent here? Please add test coverage of this case.

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

https://codereview.chromium.org/2500623002/diff/200001/ui/views/view.h#newcod...
ui/views/view.h:1443: void NotifyViewParentChanged(View* old_parent);
Add description.

[Sarmad Hashmi, 2016-11-21 19:14:13]

Sorry, misunderstood what you said first. Passed |parent| to OnViewParentChanged
now but since we don't want it to get called twice when reparenting we need to
check if |new_parent| exists before notifying in DoRemoveChildView. If
|new_parent| does exist, the observers are only notified once with the
|old_parent| in AddChildViewAt. If not, |view| is being removed, so |new_parent|
doesn't exist and we would notify in DoRemoveChildView.

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

https://codereview.chromium.org/2500623002/diff/200001/ui/views/view.cc#newco...
ui/views/view.cc:225: view->NotifyViewParentChanged(nullptr);
On 2016/11/21 18:32:12, sky wrote:
> Don't you want to pass parent here? Please add test coverage of this case.

Done.

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

https://codereview.chromium.org/2500623002/diff/200001/ui/views/view.h#newcod...
ui/views/view.h:1443: void NotifyViewParentChanged(View* old_parent);
On 2016/11/21 18:32:12, sky wrote:
> Add description.

Done.

[Sarmad Hashmi, 2016-11-21 19:15:12]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 19:16:00]

Dry run: 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, 2016-11-21 20:27:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 20:27:33]

Dry run: This issue passed the CQ dry run.

[Sarmad Hashmi, 2016-11-22 01:31:28]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[Sarmad Hashmi, 2016-11-22 01:32:07]

PTAL sky@. Added OnChildViewRemoved/Added as discussed.

[commit-bot: I haz the power, 2016-11-22 01:32:30]

Dry run: 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, 2016-11-22 04:05:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 04:05:30]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-22 04:40:48]

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

https://codereview.chromium.org/2500623002/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:1445: void NotifyChildViewAdded(View* child);
You only call these functions from one place, inline the implementation there.

[Sarmad Hashmi, 2016-11-22 04:46:56]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[Sarmad Hashmi, 2016-11-22 04:47:00]

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

https://codereview.chromium.org/2500623002/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:1445: void NotifyChildViewAdded(View* child);
On 2016/11/22 04:40:48, sky wrote:
> You only call these functions from one place, inline the implementation there.

Moved the implementations to where they were called from instead.

[commit-bot: I haz the power, 2016-11-22 04:47:14]

Dry run: 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, 2016-11-22 06:30:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 06:30:27]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-22 20:06:19]

LGTM

[Sarmad Hashmi, 2016-11-22 20:06:41]

The CQ bit was checked by mhashmi@chromium.org

[Sarmad Hashmi, 2016-11-22 20:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2500623002/#ps260001
(title: "Moved NotifyChildView* impl to call location")

[commit-bot: I haz the power, 2016-11-22 20:07:06]

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

[Sarmad Hashmi, 2016-11-22 20:16:37]

The CQ bit was unchecked by mhashmi@chromium.org

[Sarmad Hashmi, 2016-11-22 20:24:01]

The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 20:24:38]

Dry run: 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, 2016-11-22 22:04:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 22:04:29]

Dry run: This issue passed the CQ dry run.

[Sarmad Hashmi, 2016-11-22 23:34:43]

The CQ bit was checked by mhashmi@chromium.org

[Sarmad Hashmi, 2016-11-22 23:34:43]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2500623002/#ps280001
(title: "Remove Rect forward declaration and view.h include")

[commit-bot: I haz the power, 2016-11-22 23:35:41]

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, 2016-11-22 23:53:24]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1479857683048870,
"parent_rev": "276d2b6941aa56dcaa92ad6b0fb65257d1149abd", "commit_rev":
"1506fdd936d1ec2addb9991e90df0b2260682a15"}

[commit-bot: I haz the power, 2016-11-22 23:53:47]

Description was changed from

==========
Add ViewObserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

to

==========
Add ViewObserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

[commit-bot: I haz the power, 2016-11-22 23:53:49]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-11-22 23:55:20]

Description was changed from

==========
Add ViewObserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701
==========

to

==========
Add ViewObserver to View for view updates

- OnViewParentChanged for whenever a view is added/removed
- OnViewVisibilityChanged, OnViewEnabledChanged, OnViewBoundsChanged
are all for attribute changes
- OnViewChildReordered for whenever a view is reordered within its parent

Future CLs will allow devtools for listen to view changes and push them
to the inspector.

BUG=648701

Committed: https://crrev.com/7d1faa6c9ea5f4d5a2c10414d8a33b75a9f190e7
Cr-Commit-Position: refs/heads/master@{#434026}
==========

[commit-bot: I haz the power, 2016-11-22 23:55:22]

Patchset 15 (id:??) landed as
https://crrev.com/7d1faa6c9ea5f4d5a2c10414d8a33b75a9f190e7
Cr-Commit-Position: refs/heads/master@{#434026}