Make NodeIterator more STL-friendly.

Make NodeIterator more STL-friendly.

This allows it to be used in various algorithms.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/49ed0e3a1623c8524d4eae827da2ae37bfba06dc
Cr-Commit-Position: refs/heads/master@{#381807}

[Alexander Semashko, 2016-02-24 15:07:11]

Description was changed from

==========
Make NodeIterator more STL-friendly.

This allows it to be used in various algorithms.

BUG=
==========

to

==========
Make NodeIterator more STL-friendly.

This allows it to be used in various algorithms.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2016-02-24 15:09:13]

ahest@yandex-team.ru changed reviewers:
+ clamy@chromium.org

[Alexander Semashko, 2016-03-01 19:38:19]

PTAL

[clamy, 2016-03-14 13:35:59]

clamy@chromium.org changed reviewers:
+ dcheng@chromium.org

[clamy, 2016-03-14 13:36:00]

@dcheng: could you PTAL since you introduced the iterator in the ifrst place?

[dcheng, 2016-03-14 20:28:18]

Is there a use case where this would come in handy?

[Alexander Semashko, 2016-03-14 21:45:46]

On 2016/03/14 20:28:18, dcheng wrote:
> Is there a use case where this would come in handy?

If applied to ConstNodes also (which is eligible for a similar change), this
would allow to change FrameTree::IsLoading() to use std::any_of. Also there is a
bunch of places where it is needed for downstream Yandex browser. Generally,
this just brings some typedefs that std algorithms need, do you see any
downsides in it?

[dcheng, 2016-03-14 23:11:18]

I don't think we'd actually want to rewrite things like FrameTree::IsLoading().
Instead of looking like this:
  for (const FrameTreeNode* node : ConstNodes()) {
    if (node->IsLoading())
      return true;
  }
  return false;

We'd have this:
  const auto& nodes = ConstNodes();
  return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* node) {
    return node->IsLoading();
  });

which is only one fewer line, and much more verbose.

In principle though, this CL lgtm.

[Alexander Semashko, 2016-03-14 23:34:05]

On 2016/03/14 23:11:18, dcheng wrote:
> I don't think we'd actually want to rewrite things like
FrameTree::IsLoading().
> Instead of looking like this:
>   for (const FrameTreeNode* node : ConstNodes()) {
>     if (node->IsLoading())
>       return true;
>   }
>   return false;
> 
> We'd have this:
>   const auto& nodes = ConstNodes();
>   return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* node)
{
>     return node->IsLoading();
>   });
> 
> which is only one fewer line, and much more verbose.
> 
> In principle though, this CL lgtm.

Yes, this case is too trivial to be a good example, but there are probably
better ones to come soon. Ok, since you lgtmed the CL, would you mind if I
update it with a change to ConstNodeIterator?

[dcheng, 2016-03-14 23:35:50]

dcheng@chromium.org changed reviewers:
+ nick@chromium.org

[dcheng, 2016-03-14 23:35:51]

On 2016/03/14 at 23:34:05, ahest wrote:
> On 2016/03/14 23:11:18, dcheng wrote:
> > I don't think we'd actually want to rewrite things like
FrameTree::IsLoading().
> > Instead of looking like this:
> >   for (const FrameTreeNode* node : ConstNodes()) {
> >     if (node->IsLoading())
> >       return true;
> >   }
> >   return false;
> > 
> > We'd have this:
> >   const auto& nodes = ConstNodes();
> >   return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode*
node) {
> >     return node->IsLoading();
> >   });
> > 
> > which is only one fewer line, and much more verbose.
> > 
> > In principle though, this CL lgtm.
> 
> Yes, this case is too trivial to be a good example, but there are probably
better ones to come soon. Ok, since you lgtmed the CL, would you mind if I
update it with a change to ConstNodeIterator?

I think we're actually planning on removing ConstNodeIterator: it's mostly
duplicated code (with the exception of a few small things), so if we don't have
to maintain two copies, that'd be better.

[Alexander Semashko, 2016-03-14 23:41:17]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2016-03-14 23:41:58]

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

[Alexander Semashko, 2016-03-14 23:45:17]

On 2016/03/14 23:35:51, dcheng wrote:
> On 2016/03/14 at 23:34:05, ahest wrote:
> > On 2016/03/14 23:11:18, dcheng wrote:
> > > I don't think we'd actually want to rewrite things like
> FrameTree::IsLoading().
> > > Instead of looking like this:
> > >   for (const FrameTreeNode* node : ConstNodes()) {
> > >     if (node->IsLoading())
> > >       return true;
> > >   }
> > >   return false;
> > > 
> > > We'd have this:
> > >   const auto& nodes = ConstNodes();
> > >   return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode*
> node) {
> > >     return node->IsLoading();
> > >   });
> > > 
> > > which is only one fewer line, and much more verbose.
> > > 
> > > In principle though, this CL lgtm.
> > 
> > Yes, this case is too trivial to be a good example, but there are probably
> better ones to come soon. Ok, since you lgtmed the CL, would you mind if I
> update it with a change to ConstNodeIterator?
> 
> I think we're actually planning on removing ConstNodeIterator: it's mostly
> duplicated code (with the exception of a few small things), so if we don't
have
> to maintain two copies, that'd be better.

Is it planned to reimplement it (e.g. by templatizing) or to just drop the
possibility to iterate a const FrameTree?

[dcheng, 2016-03-14 23:48:29]

On 2016/03/14 at 23:45:17, ahest wrote:
> On 2016/03/14 23:35:51, dcheng wrote:
> > On 2016/03/14 at 23:34:05, ahest wrote:
> > > On 2016/03/14 23:11:18, dcheng wrote:
> > > > I don't think we'd actually want to rewrite things like
> > FrameTree::IsLoading().
> > > > Instead of looking like this:
> > > >   for (const FrameTreeNode* node : ConstNodes()) {
> > > >     if (node->IsLoading())
> > > >       return true;
> > > >   }
> > > >   return false;
> > > > 
> > > > We'd have this:
> > > >   const auto& nodes = ConstNodes();
> > > >   return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode*
> > node) {
> > > >     return node->IsLoading();
> > > >   });
> > > > 
> > > > which is only one fewer line, and much more verbose.
> > > > 
> > > > In principle though, this CL lgtm.
> > > 
> > > Yes, this case is too trivial to be a good example, but there are probably
> > better ones to come soon. Ok, since you lgtmed the CL, would you mind if I
> > update it with a change to ConstNodeIterator?
> > 
> > I think we're actually planning on removing ConstNodeIterator: it's mostly
> > duplicated code (with the exception of a few small things), so if we don't
have
> > to maintain two copies, that'd be better.
> 
> Is it planned to reimplement it (e.g. by templatizing) or to just drop the
possibility to iterate a const FrameTree?

Since the use of const method qualifiers in the content public API is
discouraged, it's just going to be removed, as it's a lot of boilerplate just to
support one const method.

[Alexander Semashko, 2016-03-14 23:52:00]

On 2016/03/14 23:48:29, dcheng wrote:
> On 2016/03/14 at 23:45:17, ahest wrote:
> > On 2016/03/14 23:35:51, dcheng wrote:
> > > On 2016/03/14 at 23:34:05, ahest wrote:
> > > > On 2016/03/14 23:11:18, dcheng wrote:
> > > > > I don't think we'd actually want to rewrite things like
> > > FrameTree::IsLoading().
> > > > > Instead of looking like this:
> > > > >   for (const FrameTreeNode* node : ConstNodes()) {
> > > > >     if (node->IsLoading())
> > > > >       return true;
> > > > >   }
> > > > >   return false;
> > > > > 
> > > > > We'd have this:
> > > > >   const auto& nodes = ConstNodes();
> > > > >   return std::any_of(nodes.begin(), nodes.end(), [](const
FrameTreeNode*
> > > node) {
> > > > >     return node->IsLoading();
> > > > >   });
> > > > > 
> > > > > which is only one fewer line, and much more verbose.
> > > > > 
> > > > > In principle though, this CL lgtm.
> > > > 
> > > > Yes, this case is too trivial to be a good example, but there are
probably
> > > better ones to come soon. Ok, since you lgtmed the CL, would you mind if I
> > > update it with a change to ConstNodeIterator?
> > > 
> > > I think we're actually planning on removing ConstNodeIterator: it's mostly
> > > duplicated code (with the exception of a few small things), so if we don't
> have
> > > to maintain two copies, that'd be better.
> > 
> > Is it planned to reimplement it (e.g. by templatizing) or to just drop the
> possibility to iterate a const FrameTree?
> 
> Since the use of const method qualifiers in the content public API is
> discouraged, it's just going to be removed, as it's a lot of boilerplate just
to
> support one const method.

Ok, thanks!

[commit-bot: I haz the power, 2016-03-14 23:53:11]

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

[commit-bot: I haz the power, 2016-03-14 23:53:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ncarter (slow), 2016-03-16 21:54:09]

Link to ConstNodeRange removal CL: where
https://codereview.chromium.org/1802163002/

[Alexander Semashko, 2016-03-17 13:58:03]

Nick, WDYT about this CL? It is still missing an owner's lgtm.

[ncarter (slow), 2016-03-17 20:45:04]

lgtm

[Alexander Semashko, 2016-03-17 20:45:47]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2016-03-17 20:46:07]

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

[commit-bot: I haz the power, 2016-03-17 22:23:59]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-17 22:25:18]

Description was changed from

==========
Make NodeIterator more STL-friendly.

This allows it to be used in various algorithms.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make NodeIterator more STL-friendly.

This allows it to be used in various algorithms.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/49ed0e3a1623c8524d4eae827da2ae37bfba06dc
Cr-Commit-Position: refs/heads/master@{#381807}
==========

[commit-bot: I haz the power, 2016-03-17 22:25:19]

Patchset 1 (id:??) landed as
https://crrev.com/49ed0e3a1623c8524d4eae827da2ae37bfba06dc
Cr-Commit-Position: refs/heads/master@{#381807}