Converting the history::StarredEntry::Type to a type defined in BookmarkNode....

Converting the history::StarredEntry::Type to a type defined in BookmarkNode.

BUG=None
TEST=None

[sky, 2009-07-07 16:02:12]

http://codereview.chromium.org/155128/diff/1/6
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/155128/diff/1/6#newcode594
Line 594: int child_count = node->GetChildCount();
move this into the loop, eg:

for (int i = 0, child_count = ...

http://codereview.chromium.org/155128/diff/1/6#newcode725
Line 725: int child_count = node->GetChildCount();
move into loop here too.

http://codereview.chromium.org/155128/diff/1/7
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/155128/diff/1/7#newcode50
Line 50: GROUP,
Rename GROUP to FOLDER.

http://codereview.chromium.org/155128/diff/1/7#newcode221
Line 221: //
nit: just nuke this line

http://codereview.chromium.org/155128/diff/1/5
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/155128/diff/1/5#newcode124
Line 124: int child_count = parent->GetChildCount();
move into loop.

http://codereview.chromium.org/155128/diff/1/5#newcode306
Line 306: int child_count = other->GetChildCount();
move into loop

http://codereview.chromium.org/155128/diff/1/3
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/155128/diff/1/3#newcode1709
Line 1709: int child_count = node->GetChildCount();
move into loop.

http://codereview.chromium.org/155128/diff/1/4
File chrome/browser/views/bookmark_editor_view.cc (left):

http://codereview.chromium.org/155128/diff/1/4#oldcode32
Line 32: using views::ColumnSet;
please leave these here and not in the method.

[tfarina (gmail-do not use), 2009-07-07 16:47:39]

On 2009/07/07 16:02:12, sky wrote:
> http://codereview.chromium.org/155128/diff/1/6
> File chrome/browser/bookmarks/bookmark_model.cc (right):
> 
> http://codereview.chromium.org/155128/diff/1/6#newcode594
> Line 594: int child_count = node->GetChildCount();
> move this into the loop, eg:
> 
> for (int i = 0, child_count = ...
> 
Scott I thought that the pasting GetChildCount outside the loop to increase
performance, because it would be called only once.
> http://codereview.chromium.org/155128/diff/1/6#newcode725
> Line 725: int child_count = node->GetChildCount();
> move into loop here too.
> 
> http://codereview.chromium.org/155128/diff/1/7
> File chrome/browser/bookmarks/bookmark_model.h (right):
> 
> http://codereview.chromium.org/155128/diff/1/7#newcode50
> Line 50: GROUP,
> Rename GROUP to FOLDER.
> 
> http://codereview.chromium.org/155128/diff/1/7#newcode221
> Line 221: //
> nit: just nuke this line
> 
> http://codereview.chromium.org/155128/diff/1/5
> File chrome/browser/importer/importer.cc (right):
> 
> http://codereview.chromium.org/155128/diff/1/5#newcode124
> Line 124: int child_count = parent->GetChildCount();
> move into loop.
> 
> http://codereview.chromium.org/155128/diff/1/5#newcode306
> Line 306: int child_count = other->GetChildCount();
> move into loop
> 
> http://codereview.chromium.org/155128/diff/1/3
> File chrome/browser/metrics/metrics_service.cc (right):
> 
> http://codereview.chromium.org/155128/diff/1/3#newcode1709
> Line 1709: int child_count = node->GetChildCount();
> move into loop.
> 
> http://codereview.chromium.org/155128/diff/1/4
> File chrome/browser/views/bookmark_editor_view.cc (left):
> 
> http://codereview.chromium.org/155128/diff/1/4#oldcode32
> Line 32: using views::ColumnSet;
> please leave these here and not in the method.

[tfarina (gmail-do not use), 2009-07-07 16:48:04]

http://codereview.chromium.org/155128/diff/1/6
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/155128/diff/1/6#newcode594
Line 594: int child_count = node->GetChildCount();
On 2009/07/07 16:02:12, sky wrote:
> move this into the loop, eg:
> 
> for (int i = 0, child_count = ...
Done

http://codereview.chromium.org/155128/diff/1/6#newcode725
Line 725: int child_count = node->GetChildCount();
On 2009/07/07 16:02:12, sky wrote:
> move into loop here too.
Done

http://codereview.chromium.org/155128/diff/1/7
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/155128/diff/1/7#newcode50
Line 50: GROUP,
On 2009/07/07 16:02:12, sky wrote:
> Rename GROUP to FOLDER.
Done

http://codereview.chromium.org/155128/diff/1/7#newcode221
Line 221: //
On 2009/07/07 16:02:12, sky wrote:
> nit: just nuke this line
Done

http://codereview.chromium.org/155128/diff/1/5
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/155128/diff/1/5#newcode124
Line 124: int child_count = parent->GetChildCount();
On 2009/07/07 16:02:12, sky wrote:
> move into loop.
Done

http://codereview.chromium.org/155128/diff/1/5#newcode306
Line 306: int child_count = other->GetChildCount();
On 2009/07/07 16:02:12, sky wrote:
> move into loop
Done

http://codereview.chromium.org/155128/diff/1/3
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/155128/diff/1/3#newcode1709
Line 1709: int child_count = node->GetChildCount();
On 2009/07/07 16:02:12, sky wrote:
> move into loop.
Done

http://codereview.chromium.org/155128/diff/1/4
File chrome/browser/views/bookmark_editor_view.cc (left):

http://codereview.chromium.org/155128/diff/1/4#oldcode32
Line 32: using views::ColumnSet;
On 2009/07/07 16:02:12, sky wrote:
> please leave these here and not in the method.
Done

[sky, 2009-07-07 20:08:00]

I was fine with you caching the GetChildCount. All I was suggesting was go from:

int child_count = xxx;
for (int i = 0; i < child_count)

to

for (int i = 0, child_count = xxx; i < child_count)

But don't feel you need to do this change.

http://codereview.chromium.org/155128/diff/1015/30
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/155128/diff/1015/30#newcode69
Line 69: break;
Add a NOTREACHED here.

http://codereview.chromium.org/155128/diff/1015/30#newcode594
Line 594: for (int i = 0; i <  node->GetChildCount(); ++i) {
nit: nuke the extra space here.

http://codereview.chromium.org/155128/diff/1015/28
File chrome/browser/views/bookmark_editor_view.cc (right):

http://codereview.chromium.org/155128/diff/1015/28#newcode31
Line 31: using views::GridLayout;
keep the ordering of these what it was.

[tfarina (gmail-do not use), 2009-07-07 20:32:09]

Scott, It's okay if I switch to:
for (int i =3D 0, child_count =3D xxx; i < child_count; ++i)
as you suggested?

On Tue, Jul 7, 2009 at 5:08 PM, <sky@chromium.org> wrote:
> I was fine with you caching the GetChildCount. All I was suggesting was
> go from:
>
> int child_count =3D xxx;
> for (int i =3D 0; i < child_count)
>
> to
>
> for (int i =3D 0, child_count =3D xxx; i < child_count)
>
> But don't feel you need to do this change.
>
>
> http://codereview.chromium.org/155128/diff/1015/30
> File chrome/browser/bookmarks/bookmark_model.cc (right):
>
> http://codereview.chromium.org/155128/diff/1015/30#newcode69
> Line 69: break;
> Add a NOTREACHED here.
>
> http://codereview.chromium.org/155128/diff/1015/30#newcode594
> Line 594: for (int i =3D 0; i < =A0node->GetChildCount(); ++i) {
> nit: nuke the extra space here.
>
> http://codereview.chromium.org/155128/diff/1015/28
> File chrome/browser/views/bookmark_editor_view.cc (right):
>
> http://codereview.chromium.org/155128/diff/1015/28#newcode31
> Line 31: using views::GridLayout;
> keep the ordering of these what it was.
>
> http://codereview.chromium.org/155128
>

[sky, 2009-07-07 20:38:15]

Certainly. Just make sure the loop doesn't change the child count.

  -Scott

On Tue, Jul 7, 2009 at 1:32 PM, Thiago Farina<thiago.farina@gmail.com> wrot=
e:
> Scott, It's okay if I switch to:
> for (int i =3D 0, child_count =3D xxx; i < child_count; ++i)
> as you suggested?
>
> On Tue, Jul 7, 2009 at 5:08 PM, <sky@chromium.org> wrote:
>> I was fine with you caching the GetChildCount. All I was suggesting was
>> go from:
>>
>> int child_count =3D xxx;
>> for (int i =3D 0; i < child_count)
>>
>> to
>>
>> for (int i =3D 0, child_count =3D xxx; i < child_count)
>>
>> But don't feel you need to do this change.
>>
>>
>> http://codereview.chromium.org/155128/diff/1015/30
>> File chrome/browser/bookmarks/bookmark_model.cc (right):
>>
>> http://codereview.chromium.org/155128/diff/1015/30#newcode69
>> Line 69: break;
>> Add a NOTREACHED here.
>>
>> http://codereview.chromium.org/155128/diff/1015/30#newcode594
>> Line 594: for (int i =3D 0; i < =A0node->GetChildCount(); ++i) {
>> nit: nuke the extra space here.
>>
>> http://codereview.chromium.org/155128/diff/1015/28
>> File chrome/browser/views/bookmark_editor_view.cc (right):
>>
>> http://codereview.chromium.org/155128/diff/1015/28#newcode31
>> Line 31: using views::GridLayout;
>> keep the ordering of these what it was.
>>
>> http://codereview.chromium.org/155128
>>
>

[tfarina (gmail-do not use), 2009-07-07 20:59:58]

http://codereview.chromium.org/155128/diff/1015/30
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/155128/diff/1015/30#newcode69
Line 69: break;
On 2009/07/07 20:08:01, sky wrote:
> Add a NOTREACHED here.

Done.

http://codereview.chromium.org/155128/diff/1015/30#newcode594
Line 594: for (int i = 0; i <  node->GetChildCount(); ++i) {
On 2009/07/07 20:08:01, sky wrote:
> nit: nuke the extra space here.

Done.

http://codereview.chromium.org/155128/diff/1015/28
File chrome/browser/views/bookmark_editor_view.cc (right):

http://codereview.chromium.org/155128/diff/1015/28#newcode31
Line 31: using views::GridLayout;
On 2009/07/07 20:08:01, sky wrote:
> keep the ordering of these what it was.

Done.

[tfarina (gmail-do not use), 2009-07-08 03:42:00]

Scott I ran the unit_tests and everything worked as expected.

[sky, 2009-07-08 21:55:11]

Landed in r20195.