Add iterator support to blink::EventPath.

Add iterator support to blink::EventPath.

This CL has no behavior changes.

BUG=668300
Committed: https://crrev.com/d33aa11bc5d41c5c8f85d6d7d1a0af3ec746f82d
Cr-Commit-Position: refs/heads/master@{#437814}

[tkent, 2016-12-08 01:36:09]

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

[commit-bot: I haz the power, 2016-12-08 01:36:49]

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

[tkent, 2016-12-08 03:07:32]

tkent@chromium.org changed reviewers:
+ hayato@chromium.org

[tkent, 2016-12-08 03:07:33]

hayato@, would you review this please?

[dtapuska, 2016-12-08 03:14:23]

On 2016/12/08 03:07:33, tkent wrote:
> hayato@, would you review this please?

what about here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Ev...

And what about adding a reverse iterator as well?

[tkent, 2016-12-08 03:23:47]

On 2016/12/08 at 03:14:23, dtapuska wrote:
> what about here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Ev...

Will do.

> And what about adding a reverse iterator as well?

Hmm, I feel adding four more functions to EventPath is too much.  How about
adding only |const HeapVector<NodeEventContext>& nodeEventContexts()| to
EventPath, and adding no begin()/end()/rbegin()/rend().

[dtapuska, 2016-12-08 03:30:04]

On 2016/12/08 03:23:47, tkent wrote:
> On 2016/12/08 at 03:14:23, dtapuska wrote:
> > what about here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Ev...
> 
> Will do.
> 
> > And what about adding a reverse iterator as well?
> 
> Hmm, I feel adding four more functions to EventPath is too much.  How about
> adding only |const HeapVector<NodeEventContext>& nodeEventContexts()| to
> EventPath, and adding no begin()/end()/rbegin()/rend().

The ones in the EventDispatcher are special because the skip index 0 (which is
dispatched as at-target; so perhaps it might be to confusing to change those.
Perhaps what you have is fine.

[tkent, 2016-12-08 03:37:05]

On 2016/12/08 at 03:30:04, dtapuska wrote:
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Ev...
> > 
> > Will do.

> The ones in the EventDispatcher are special because the skip index 0 (which is
dispatched as at-target; so perhaps it might be to confusing to change those.
Perhaps what you have is fine.

Converting 1-to-end loop is not hard though it doesn't look simpler than
operator[] loop.

auto it = m_event->eventPath().begin();
auto end = m_event->eventPath().end();
while (++it < end) {
  const NodeEventContext& eventContext = *it;
  ...
}

[dtapuska, 2016-12-08 03:42:13]

On 2016/12/08 03:37:05, tkent wrote:
> On 2016/12/08 at 03:30:04, dtapuska wrote:
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Ev...
> > > 
> > > Will do.
> 
> > The ones in the EventDispatcher are special because the skip index 0 (which
is
> dispatched as at-target; so perhaps it might be to confusing to change those.
> Perhaps what you have is fine.
> 
> Converting 1-to-end loop is not hard though it doesn't look simpler than
> operator[] loop.
> 
> auto it = m_event->eventPath().begin();
> auto end = m_event->eventPath().end();
> while (++it < end) {
>   const NodeEventContext& eventContext = *it;
>   ...
> }

I find it a little harder to read that it is intentional.

[commit-bot: I haz the power, 2016-12-08 03:43:45]

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

[commit-bot: I haz the power, 2016-12-08 03:43:45]

Dry run: This issue passed the CQ dry run.

[hayato, 2016-12-08 06:58:04]

lgtm

[tkent, 2016-12-08 07:04:24]

> > And what about adding a reverse iterator as well?
> 
> Hmm, I feel adding four more functions to EventPath is too much.  How about
adding only |const HeapVector<NodeEventContext>& nodeEventContexts()| to
EventPath, and adding no begin()/end()/rbegin()/rend().

Hayato-san, do you have any opinion about this?

[hayato, 2016-12-08 08:03:48]

On 2016/12/08 at 07:04:24, tkent wrote:
> > > And what about adding a reverse iterator as well?
> > 
> > Hmm, I feel adding four more functions to EventPath is too much.  How about
adding only |const HeapVector<NodeEventContext>& nodeEventContexts()| to
EventPath, and adding no begin()/end()/rbegin()/rend().
> 
> Hayato-san, do you have any opinion about this?

Yeah, that makes sense.

The CL would become something like this:

> for (const auto& xx: eventPath.nodeEventContext())

I like that.

[tkent, 2016-12-09 01:12:41]

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

[commit-bot: I haz the power, 2016-12-09 01:13:08]

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

[tkent, 2016-12-09 01:47:17]

I updated the patch.

- Don't convert 1-to-end loops due to code readability
- As for reverse-order loops, I found they were end-to-1 loops.  So the patch
don't convert them due to the same reason.

[commit-bot: I haz the power, 2016-12-09 03:38:40]

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

[commit-bot: I haz the power, 2016-12-09 03:38:41]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-12-12 01:49:36]

The CQ bit was checked by tkent@chromium.org

[tkent, 2016-12-12 01:49:36]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/2555743005/#ps20001
(title: "Exposed the vector")

[commit-bot: I haz the power, 2016-12-12 01:49:44]

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-12 03:35:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481507376324430,
"parent_rev": "15fe027e6efd9f133b3caff34df248031dbedd5f", "commit_rev":
"d97183291c623d6232b240869bba8d6eaac9d94a"}

[commit-bot: I haz the power, 2016-12-12 03:36:25]

Description was changed from

==========
Add iterator support to blink::EventPath.

This CL has no behavior changes.

BUG=668300
==========

to

==========
Add iterator support to blink::EventPath.

This CL has no behavior changes.

BUG=668300

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

[commit-bot: I haz the power, 2016-12-12 03:36:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-12 15:10:22]

Description was changed from

==========
Add iterator support to blink::EventPath.

This CL has no behavior changes.

BUG=668300

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

to

==========
Add iterator support to blink::EventPath.

This CL has no behavior changes.

BUG=668300

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

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

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