Rework tree APIs to reflect Google style and more const-correctness....

Rework tree APIs to reflect Google style and more const-correctness.Also, move PrintViewHierarchy/PrintFocusHierarchy out into a separate header.

BUG=72040
TEST=None


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

[Ben Goodger (Google), 2011-02-08 18:23:24]

view.h/cc first, as usual

[sky, 2011-02-08 19:11:53]

http://codereview.chromium.org/6452011/diff/1/views/accessibility/view_access...
File views/accessibility/view_accessibility.cc (right):

http://codereview.chromium.org/6452011/diff/1/views/accessibility/view_access...
views/accessibility/view_accessibility.cc:111: gfx::Rect
view_bounds(view_->bounds());
I don't think you need this line anymore.

http://codereview.chromium.org/6452011/diff/1/views/controls/menu/menu_contro...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/6452011/diff/1/views/controls/menu/menu_contro...
views/controls/menu/menu_controller.cc:107: i >= 0; --i) {
This never stops if we never find deepest.

PS I don't like size_t.

http://codereview.chromium.org/6452011/diff/1/views/controls/menu/menu_item_v...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/6452011/diff/1/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.cc:443: for (size_t i = child_count() - 1, x
= width() - item_right_margin_; i >= 0;
this never stops.

PS I don't like size_t.

http://codereview.chromium.org/6452011/diff/1/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.cc:600: for (size_t i =
submenu_->child_count() - 1; i >= 0; --i) {
this never stops.

PS I don't like size_t.

http://codereview.chromium.org/6452011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/6452011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_win.cc:440: int label_index =
parent->GetIndexOf(textfield_) - 1;
Are you sure this is right if parent doesn't contain textfield_? If the code
ends up equivalent as:

static_cast<int>(parent->GetIndexOf(textfield_) - 1), then it can never be
negative.

PS I don't like size_t

http://codereview.chromium.org/6452011/diff/1/views/layout/grid_layout_unitte...
File views/layout/grid_layout_unittest.cc (right):

http://codereview.chromium.org/6452011/diff/1/views/layout/grid_layout_unitte...
views/layout/grid_layout_unittest.cc:65: for (size_t i = host.child_count() - 1;
i >= 0; i--)
THis never stops.

PS I don't like size_t

http://codereview.chromium.org/6452011/diff/1/views/layout/grid_layout_unitte...
views/layout/grid_layout_unittest.cc:94: for (size_t i = host.child_count() - 1;
i >= 0; i--)
This never stops.

PS I don't like size_t

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

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode81
views/view.cc:81: int c = static_cast<int>(children_.size());
If we're going to use size_t every where, we should do away casts like this.

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode198
views/view.cc:198: #ifndef NDEBUG
Nuke this.

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode662
views/view.cc:662: for (int i = child_count() - 1; i >= 0; --i) {
Don't you need a static_cast? And again, we should do away with casts if we're
going to use size_t.

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1163
views/view.cc:1163: for (int i = 0, count = child_count(); i < count; ++i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1171
views/view.cc:1171: for (int i = 0, count = child_count(); i < count; ++i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1179
views/view.cc:1179: for (int i = 0, count = child_count(); i < count; ++i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1214
views/view.cc:1214: for (int i = 0, count = child_count(); i < count; ++i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1233
views/view.cc:1233: for (size_t i = 0; i < view->child_count(); ++i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1243
views/view.cc:1243: for (size_t i = 0; i < view->child_count(); ++i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1438
views/view.cc:1438: void View::InitFocusSiblings(View* v, int index) {
Can this take a size_t now? Maybe in the next pass?

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1439
views/view.cc:1439: int child_count = static_cast<int>(children_.size());
no cast.

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1484
views/view.cc:1484: for (int i = child_count() - 1; i >= 0; --i)
size_t

http://codereview.chromium.org/6452011/diff/1/views/view.cc#newcode1490
views/view.cc:1490: for (int i = child_count() - 1; i >= 0; --i)
size_t

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

http://codereview.chromium.org/6452011/diff/1/views/view.h#newcode227
views/view.h:227: size_t GetIndexOf(const View* view) const;
This can never return -1 now. I'm inclined to think this should take the size_t
and return a bool. The other option is something like std::wstring::npos, which
is all to easy to forget to compare against.

PS I don't like size_t.

http://codereview.chromium.org/6452011/diff/1/views/view.h#newcode1305
views/view.h:1305: typedef std::vector<View*> ViewVector;
all typedefs should be grouped together.

[Ben Goodger (Google), 2011-02-08 20:56:49]

Updated.

[sky, 2011-02-08 22:09:29]

LGTM

http://codereview.chromium.org/6452011/diff/13011/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6452011/diff/13011/views/view.h#newcode1305
views/view.h:1305: typedef std::vector<View*> ViewVector;
typedefs should be grouped together.

[tfarina, 2011-02-08 23:41:57]

http://codereview.chromium.org/6452011/diff/13011/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/6452011/diff/13011/views/view.cc#newcode147
views/view.cc:147: const View* View::GetChildViewAt(int index) const {
Can we do this as:

DCHECK(index >= 0 && index < child_count());
return children_[index];

? Or are there side-effects? Scott?

http://codereview.chromium.org/6452011/diff/13011/views/view.cc#newcode1125
views/view.cc:1125: const ViewVector::iterator i = find(children_.begin(),
children_.end(), view);
std::find ?

[sky, 2011-02-09 00:09:06]

http://codereview.chromium.org/6452011/diff/13011/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/6452011/diff/13011/views/view.cc#newcode147
views/view.cc:147: const View* View::GetChildViewAt(int index) const {
On 2011/02/08 23:41:57, tfarina wrote:
> Can we do this as:
> 
> DCHECK(index >= 0 && index < child_count());
> return children_[index];
> 
> ? Or are there side-effects? Scott?

Not sure, we would have to check usages.