View API/implementation cleanup:...

View API/implementation cleanup:

  * Don't include the container type ("vector") in the typedef for "a bunch of children".  Users generally should not know or care what the container is, so this makes reading easier as the code is not constantly pointing out to you, "hey!  I'm a vector!"  Added bonus: less verbose, allows condensing a lot of loop declarations onto one line.
  * Consistently put getters before setters.
  * Remove 4-arg form of SetBounds() and make people use Rects (we should move the codebase towards Points, Sizes, and Rects wherever possible).
  * Use "origin" instead of "position" to be consistent with Rect's terminology.
  * Minor naming changes, e.g. GetViewById() -> GetViewByID().
  * Remove const qualifier on member functions that are not logically const.  This also got rid of all the const_cast<>()s.
  * Better comments.
  * Use const ref args for Views whenever the provided View is not being modified, to make that obvious to the caller.
  * Turn some accessors into pairs of (non-const, const) accessors.  In these cases make the non-const version call the const version.  (GetWidget() does this in the header because the const version is virtual; this way people who override the const version can see why they don't need to override the non-const version).
  * Make RemoveChildView() take a bool for consistency with RemoveAllChildViews() (also eliminates the need to return a View*).
  * Add STL-style iterators and rename a few accessors to match STL terminology ("size" instead of "count").
  * Turn IsFocusable() into a cheap inline getter.
  * Greatly simplify private tree ops ("NotifyHierarchyChangedXXX()") by realizing that they were always being called with |parent| == |this|.
  * Declare iterators inside loops, not above them.
  * Standardize iterator naming to |i|.  The existing code wasn't always consistent, and while there's nothing wrong with |it|, using that would have made almost every loop declaration into two lines instead of one.
  * Simpler code, sometimes by using STL algorithms.
  * Unindent via early-returns.
  * Use CHECK_NE() and similar where possible.
  * Fix memory corruption in RemoveAllChildViews() due to using an iterator after modifying its container.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75642

[Ben Goodger (Google), 2011-02-18 19:02:28]

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

http://codereview.chromium.org/6541030/diff/1/ui/views/view.cc#newcode318
ui/views/view.cc:318: GetFocusManager());
Why do you define this here but the GetWidget one you inline in the header?

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h
File ui/views/view.h (right):

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h#newcode77
ui/views/view.h:77: // TODO(pkasting): How is this different than bounds()?
This returns the visible bounds of the view in the root view. It is only used
for clipping native views. Think of a native button scrolled out of view in a
scrollview. In that case this would return empty rect. I am still figuring out
where this should live.

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h#newcode223
ui/views/view.h:223: bool is_focusable() const { return focusable_ && enabled_
&& visible_; }
This actually needs to be more complex in reality. I am going to combine it with
IsFocusableInRootView() in the real code.

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h#newcode348
ui/views/view.h:348: // from |this|.  |has_widget| is true iff |this| is in a
widget.
if

[Peter Kasting, 2011-02-18 19:09:43]

New snap up.

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

http://codereview.chromium.org/6541030/diff/1/ui/views/view.cc#newcode318
ui/views/view.cc:318: GetFocusManager());
On 2011/02/18 19:02:28, Ben Goodger wrote:
> Why do you define this here but the GetWidget one you inline in the header?

See the comments about "GetWidget()" in my change description.

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h
File ui/views/view.h (right):

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h#newcode77
ui/views/view.h:77: // TODO(pkasting): How is this different than bounds()?
On 2011/02/18 19:02:28, Ben Goodger wrote:
> This returns the visible bounds of the view in the root view. It is only used
> for clipping native views. Think of a native button scrolled out of view in a
> scrollview. In that case this would return empty rect. I am still figuring out
> where this should live.

I updated the comment to try and capture that.

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h#newcode223
ui/views/view.h:223: bool is_focusable() const { return focusable_ && enabled_
&& visible_; }
On 2011/02/18 19:02:28, Ben Goodger wrote:
> This actually needs to be more complex in reality. I am going to combine it
with
> IsFocusableInRootView() in the real code.

Changed back to a non-inline function.

http://codereview.chromium.org/6541030/diff/1/ui/views/view.h#newcode348
ui/views/view.h:348: // from |this|.  |has_widget| is true iff |this| is in a
widget.
On 2011/02/18 19:02:28, Ben Goodger wrote:
> if

No, I really meant iff ("if and only if").

[Ben Goodger (Google), 2011-02-18 20:24:50]

LGTM... please run the views_demo exe and see if it still works with event
handling. :-)

[Peter Kasting, 2011-02-18 21:37:19]

On 2011/02/18 20:24:50, Ben Goodger wrote:
> LGTM... please run the views_demo exe and see if it still works with event
> handling. :-)

Given that it's undocumented, I'm not totally sure.  It looks to me like it
works, except that closing the window does not terminate the program.  Maybe you
should apply this patch and take a look.

I fixed views.gyp to work and all the projects in views.sln to build.  The
unittests all work except for focus not being updated properly in the root view
test, which is a bug that predates this change.