Remove BrowserIterator

Remove BrowserIterator

Use BrowserList directly instead.

R=sky@chromium.org
BUG=558054
Committed: https://crrev.com/8abbff836d5218218f4c18f9a51c4167f62a9f04
Cr-Commit-Position: refs/heads/master@{#372197}

[scottmg, 2016-01-28 01:52:50]

Patchset #3 (id:40001) has been deleted

[scottmg, 2016-01-28 02:19:54]

https://codereview.chromium.org/1644773003/diff/20001/chrome/browser/lifetime...
File chrome/browser/lifetime/browser_close_manager.cc (right):

https://codereview.chromium.org/1644773003/diff/20001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager.cc:154: for
(BrowserList::const_iterator browser =
This is the only one that's non-trivial because of the re-initialization of the
iterator in the loop body.

[sky, 2016-01-28 03:04:05]

I started looking at this, but either I'm totally missing something or you
haven't tried compiling this... If I'm wrong here, let me know and I'll take
another look.

https://codereview.chromium.org/1644773003/diff/80001/chrome/browser/after_st...
File chrome/browser/after_startup_task_utils.cc (right):

https://codereview.chromium.org/1644773003/diff/80001/chrome/browser/after_st...
chrome/browser/after_startup_task_utils.cc:165: for (auto& browser :
*BrowserList::GetInstance()) {
Does this work? BrowserList::GerInstance() returns a Browser* right? This code
wants to iterate over the browsers, not a single browser.

https://codereview.chromium.org/1644773003/diff/80001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1644773003/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:638: for (auto& browser :
*BrowserList::GetInstance()) {
Similar comment as to this working.

[scottmg, 2016-01-28 03:09:23]

(It looks like there are a couple compile problems on Mac though, so if you want
I can fix those before you take a look.)

https://codereview.chromium.org/1644773003/diff/80001/chrome/browser/after_st...
File chrome/browser/after_startup_task_utils.cc (right):

https://codereview.chromium.org/1644773003/diff/80001/chrome/browser/after_st...
chrome/browser/after_startup_task_utils.cc:165: for (auto& browser :
*BrowserList::GetInstance()) {
On 2016/01/28 03:04:05, sky wrote:
> Does this work? BrowserList::GerInstance() returns a Browser* right? This code
> wants to iterate over the browsers, not a single browser.

It's a BrowserList& here. The * is needed in a range-based for loop, as the
contract is that the object has to have a begin() and an end() method, which
when it's a BrowserList* it doesn't have.

[scottmg, 2016-01-28 06:38:20]

Switched auto& to auto*.

[sky, 2016-01-28 16:58:13]

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/app_con...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/app_con...
chrome/browser/app_controller_mac.mm:638: for (auto* browser :
*BrowserList::GetInstance()) {
nit: no {}

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_window.cc:78: for (auto* browser_it :
*BrowserList::GetInstance()) {
With the auto browser_it is rather confusing. _it implies the type is an
iterator, but that isn't right. I believe you went with browser_it here to avoid
conflicts with the browser in param. Maybe rename that to found_browser or
something? Or rename to b like you did elsewhere.
Alternatively if you use Browser* rather than auto* it's clearer.

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager.cc (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager.cc:154: for
(BrowserList::const_iterator browser =
browser -> browser_it, or maybe iter.

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager.cc:154: for
(BrowserList::const_iterator browser =
I think this code would be less tricky if it copied the BrowserList and iterated
over the copy.

[scottmg, 2016-01-28 17:36:28]

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/app_con...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/app_con...
chrome/browser/app_controller_mac.mm:638: for (auto* browser :
*BrowserList::GetInstance()) {
On 2016/01/28 16:58:12, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_window.cc:78: for (auto* browser_it :
*BrowserList::GetInstance()) {
On 2016/01/28 16:58:12, sky wrote:
> With the auto browser_it is rather confusing. _it implies the type is an
> iterator, but that isn't right. I believe you went with browser_it here to
avoid
> conflicts with the browser in param. Maybe rename that to found_browser or
> something? Or rename to b like you did elsewhere.
> Alternatively if you use Browser* rather than auto* it's clearer. 

Done.

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager.cc (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager.cc:154: for
(BrowserList::const_iterator browser =
On 2016/01/28 16:58:12, sky wrote:
> I think this code would be less tricky if it copied the BrowserList and
iterated
> over the copy.

Done.

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager.cc:154: for
(BrowserList::const_iterator browser =
On 2016/01/28 16:58:12, sky wrote:
> browser -> browser_it, or maybe iter.

(It's really a browser now in the improved version.)

[sky, 2016-01-28 19:12:05]

LGTM

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager.cc (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager.cc:173: if (browser !=
BrowserList::GetInstance()->end() &&
I'm assuming you intentionally didn't want to carry over the NOTREACHED.

[scottmg, 2016-01-28 19:14:55]

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
File chrome/browser/lifetime/browser_close_manager.cc (right):

https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
chrome/browser/lifetime/browser_close_manager.cc:173: if (browser !=
BrowserList::GetInstance()->end() &&
On 2016/01/28 19:12:05, sky wrote:
> I'm assuming you intentionally didn't want to carry over the NOTREACHED.

It was intentional. It didn't seem that useful in the new version. I guess I
could search the real (non-copy) list to confirm that it really was removed. Do
you think that's worthwhile?

[sky, 2016-01-28 21:19:18]

I think a DCHECK would be nice, but the early return shouldn't be necessary.

On Thu, Jan 28, 2016 at 11:14 AM,  <scottmg@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
> File chrome/browser/lifetime/browser_close_manager.cc (right):
>
>
https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
> chrome/browser/lifetime/browser_close_manager.cc:173: if (browser !=
> BrowserList::GetInstance()->end() &&
> On 2016/01/28 19:12:05, sky wrote:
>> I'm assuming you intentionally didn't want to carry over the
> NOTREACHED.
>
> It was intentional. It didn't seem that useful in the new version. I
> guess I could search the real (non-copy) list to confirm that it really
> was removed. Do you think that's worthwhile?
>
> https://codereview.chromium.org/1644773003/

-- 
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.

[scottmg, 2016-01-28 21:42:33]

On 2016/01/28 21:19:18, sky wrote:
> I think a DCHECK would be nice, but the early return shouldn't be necessary.

Done.

> 
> On Thu, Jan 28, 2016 at 11:14 AM,  <mailto:scottmg@chromium.org> wrote:
> >
> >
> >
>
https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
> > File chrome/browser/lifetime/browser_close_manager.cc (right):
> >
> >
>
https://codereview.chromium.org/1644773003/diff/100001/chrome/browser/lifetim...
> > chrome/browser/lifetime/browser_close_manager.cc:173: if (browser !=
> > BrowserList::GetInstance()->end() &&
> > On 2016/01/28 19:12:05, sky wrote:
> >> I'm assuming you intentionally didn't want to carry over the
> > NOTREACHED.
> >
> > It was intentional. It didn't seem that useful in the new version. I
> > guess I could search the real (non-copy) list to confirm that it really
> > was removed. Do you think that's worthwhile?
> >
> > https://codereview.chromium.org/1644773003/
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-01-28 22:19:33]

LGTM

[scottmg, 2016-01-28 22:36:28]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2016-01-28 22:37:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644773003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644773003/140001

[commit-bot: I haz the power, 2016-01-28 22:57:46]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-28 22:58:38]

Description was changed from

==========
Remove BrowserIterator

Use BrowserList directly instead.

R=sky@chromium.org
BUG=558054
==========

to

==========
Remove BrowserIterator

Use BrowserList directly instead.

R=sky@chromium.org
BUG=558054

Committed: https://crrev.com/8abbff836d5218218f4c18f9a51c4167f62a9f04
Cr-Commit-Position: refs/heads/master@{#372197}
==========

[commit-bot: I haz the power, 2016-01-28 22:58:40]

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