[views] Changes iteration over |children_| to use range-based for loops (reland)

[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=Added DCHECKs that |children_| is not mutated while inside
     the iterations.

Committed: https://crrev.com/bdd2b6f4ac1942a60b5d4323929de9ee666e5276
Cr-Commit-Position: refs/heads/master@{#440549}

[varkha, 2016-12-22 19:02:57]

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

[varkha, 2016-12-22 19:02:57]

sadrul@, this should be ready for re-land with
https://codereview.chromium.org/2597733002/ out of the way. I will run the
sync_integration_tests again just in case before landing this.

[varkha, 2016-12-22 19:07:03]

On 2016/12/22 19:02:57, varkha wrote:
> sadrul@, this should be ready for re-land with
> https://codereview.chromium.org/2597733002/ out of the way. I will run the
> sync_integration_tests again just in case before landing this.

... and add the DCHECKs.

[sadrul, 2016-12-22 19:25:34]

It may make sense to add the DCHECK()s in a separate CL. lgtm

[varkha, 2016-12-22 19:53:32]

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

[commit-bot: I haz the power, 2016-12-22 19:53:56]

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

[varkha, 2016-12-22 19:54:44]

I thought it would be safer to do it in the same CL, especially since all the
touch points are essentially same. PTAL.

[varkha, 2016-12-22 20:19:31]

The CQ bit was unchecked by varkha@chromium.org

[commit-bot: I haz the power, 2016-12-22 20:19:38]

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

[commit-bot: I haz the power, 2016-12-22 20:19:38]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[varkha, 2016-12-22 20:30:15]

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

[varkha, 2016-12-22 20:30:23]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-22 20:30:29]

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

[varkha, 2016-12-22 20:34:34]

Description was changed from

==========
[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=TBD (will add DCHECKs that |children_| is not mutated while inside
     the iterations).
==========

to

==========
[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=Added DCHECKs that |children_| is not mutated while inside
     the iterations.
==========

[commit-bot: I haz the power, 2016-12-22 21:13:54]

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

[commit-bot: I haz the power, 2016-12-22 21:13:55]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-22 21:37:52]

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

https://codereview.chromium.org/2591323002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:192: DCHECK(!iterating_);
Since this is in a DCHECK, do you need the DCHECK_IS_ON check for it?

https://codereview.chromium.org/2591323002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:270: children_.insert(children_.begin() + index, view);
Don't need both this and in 252:254?

https://codereview.chromium.org/2591323002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:2505: }
An alternative to do this is something like this:

  ScopedChildLock lock(this);
  for (auto* child : base::Reversed(children_))
    child->....
  lock.reset();

Where ScopedChildLock looks like:

#if DCHECK_IS_ON()
  class ScopedChildLock {
   public:
    explicit ScopedChildLock(View* view) : view_(view) {
      DCHECK(!view->iterating_);
      view->iterating_ = true;
    }

    ~ScopedChildLock() { reset(); }

    void reset() {
      if (!view_)
        return;
      view_->iterating_ = false;
      view_ = nullptr;
    }

    private:
     View* view_;
     DISALLOW_COPY_AND_ASSIGN(ScopedChildLock);
  };
#else
  class ScopedChildLock {
    public:
      explicit ScopedChildLock(View* view) {}
      ~ScopedChildLock() {}
      void reset() {}
  };
#endif

This way, you don't need the rest of the DCHECK_IS_ON(). We use this pattern
elsewhere (e.g. base::ThreadChecker), so that we don't need to use
DCHECK_IS_ON() in so many places inside the code.

[varkha, 2016-12-22 22:35:05]

Patchset #3 (id:60001) has been deleted

[varkha, 2016-12-22 22:36:02]

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

https://codereview.chromium.org/2591323002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:192: DCHECK(!iterating_);
On 2016/12/22 21:37:52, sadrul wrote:
> Since this is in a DCHECK, do you need the DCHECK_IS_ON check for it?

Yes, I get compiler "error: use of undeclared identifier 'iterating_'" in
Release build if I omit #ifdef.

https://codereview.chromium.org/2591323002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:270: children_.insert(children_.begin() + index, view);
On 2016/12/22 21:37:52, sadrul wrote:
> Don't need both this and in 252:254?

Done.

https://codereview.chromium.org/2591323002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:2505: }
On 2016/12/22 21:37:52, sadrul wrote:
> An alternative to do this is something like this:
> 
>   ScopedChildLock lock(this);
>   for (auto* child : base::Reversed(children_))
>     child->....
>   lock.reset();
> 
> Where ScopedChildLock looks like:
> 
> #if DCHECK_IS_ON()
>   class ScopedChildLock {
>    public:
>     explicit ScopedChildLock(View* view) : view_(view) {
>       DCHECK(!view->iterating_);
>       view->iterating_ = true;
>     }
> 
>     ~ScopedChildLock() { reset(); }
> 
>     void reset() {
>       if (!view_)
>         return;
>       view_->iterating_ = false;
>       view_ = nullptr;
>     }
> 
>     private:
>      View* view_;
>      DISALLOW_COPY_AND_ASSIGN(ScopedChildLock);
>   };
> #else
>   class ScopedChildLock {
>     public:
>       explicit ScopedChildLock(View* view) {}
>       ~ScopedChildLock() {}
>       void reset() {}
>   };
> #endif
> 
> This way, you don't need the rest of the DCHECK_IS_ON(). We use this pattern
> elsewhere (e.g. base::ThreadChecker), so that we don't need to use
> DCHECK_IS_ON() in so many places inside the code.

Done.

[varkha, 2016-12-22 22:36:12]

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

[commit-bot: I haz the power, 2016-12-22 22:36:39]

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-12-22 23:55:04]

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

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

Dry run: This issue passed the CQ dry run.

[varkha, 2016-12-23 00:02:55]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-12-23 00:02:55]

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/2591323002/#ps80001
(title: "[views] Changes iteration over |children_| to use range-based for loops
(comments)")

[commit-bot: I haz the power, 2016-12-23 00:03:29]

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-12-23 00:07:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482451375263670,
"parent_rev": "6ac36db6e8bb6efe174796e2b2bcdc030bc777cf", "commit_rev":
"a1a8370e8e8a82b4d8566f20339c1282017ce1fb"}

[commit-bot: I haz the power, 2016-12-23 00:08:21]

Description was changed from

==========
[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=Added DCHECKs that |children_| is not mutated while inside
     the iterations.
==========

to

==========
[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=Added DCHECKs that |children_| is not mutated while inside
     the iterations.

Review-Url: https://codereview.chromium.org/2591323002
==========

[commit-bot: I haz the power, 2016-12-23 00:08:22]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-12-23 00:10:47]

Description was changed from

==========
[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=Added DCHECKs that |children_| is not mutated while inside
     the iterations.

Review-Url: https://codereview.chromium.org/2591323002
==========

to

==========
[views] Changes iteration over |children_| to use range-based for loops

This is a reland of https://codereview.chromium.org/2583343003/
It depends on https://codereview.chromium.org/2597733002/ to eliminate
the scenario where |View::children_| was mutated while inside iteration.

BUG=NONE
TEST=Added DCHECKs that |children_| is not mutated while inside
     the iterations.

Committed: https://crrev.com/bdd2b6f4ac1942a60b5d4323929de9ee666e5276
Cr-Commit-Position: refs/heads/master@{#440549}
==========

[commit-bot: I haz the power, 2016-12-23 00:10:48]

Patchset 3 (id:??) landed as
https://crrev.com/bdd2b6f4ac1942a60b5d4323929de9ee666e5276
Cr-Commit-Position: refs/heads/master@{#440549}

[Daniele Castagna, 2016-12-23 23:45:13]

dcastagna@chromium.org changed reviewers:
+ dcastagna@chromium.org

[Daniele Castagna, 2016-12-23 23:45:14]

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

https://codereview.chromium.org/2591323002/diff/80001/ui/views/view.cc#newcode98
ui/views/view.cc:98: view->iterating_ = true;
|iterating_| is private and this code doesn't compile with DCHECKS on.
crrev.com/2606513002 if you are fine making |iterating_| public.

[sadrul, 2016-12-24 00:13:11]

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

https://codereview.chromium.org/2591323002/diff/80001/ui/views/view.cc#newcode98
ui/views/view.cc:98: view->iterating_ = true;
On 2016/12/23 23:45:14, Daniele Castagna wrote:
> |iterating_| is private and this code doesn't compile with DCHECKS on.
> crrev.com/2606513002 if you are fine making |iterating_| public.

|iterating_| should remain private. But I think ScopedChildrenLock needs to be
in non-anon namespace for the friend declared in .h to work. But I believe our
trybot/buildbots use dcheck_always_on flag, which means they do exercise the
DCHECKs. It's curious that those bots are not failing to build. Do you have a
link to a failing bot, or if you are getting the failures locally, are you using
some non-standard configs?

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

https://codereview.chromium.org/2591323002/diff/80001/ui/views/view.h#newcode89
ui/views/view.h:89: }
Oh, missed this, sorry. This should not be in anonymous namespace. This can move
into the internal namespace.

[Daniele Castagna, 2016-12-24 00:48:01]

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

https://codereview.chromium.org/2591323002/diff/80001/ui/views/view.cc#newcode98
ui/views/view.cc:98: view->iterating_ = true;
On 2016/12/24 at 00:13:11, sadrul wrote:
> On 2016/12/23 23:45:14, Daniele Castagna wrote:
> > |iterating_| is private and this code doesn't compile with DCHECKS on.
> > crrev.com/2606513002 if you are fine making |iterating_| public.
> 
> |iterating_| should remain private. But I think ScopedChildrenLock needs to be
in non-anon namespace for the friend declared in .h to work. But I believe our
trybot/buildbots use dcheck_always_on flag, which means they do exercise the
DCHECKs. It's curious that those bots are not failing to build. Do you have a
link to a failing bot, or if you are getting the failures locally, are you using
some non-standard configs?

I'm not sure why we're not catching that on trybots.

I had trouble building locally for cyan with $GN_ARGS plus
dcheck_always_on=true, so, nothing non-standard.

I noticed ScopedChildrenLock was already friend (or at least, that seemed the
intention), crrev.com/2606513002 has been updated moving that class to internal
namesapce.