Create frame iterator for frame types except HEADER, PUSH_PROMISE and DATA. This helps use unified …

Create frame iterator for frame types except HEADER, PUSH_PROMISE and DATA. This helps use unified interface at call site.

This CL lands server change 151846071 by yasong.

BUG=488484
Review-Url: https://codereview.chromium.org/2844083003
Cr-Commit-Position: refs/heads/master@{#468033}
Committed: https://chromium.googlesource.com/chromium/src/+/4c4914e6e841e18cfbc6d13ebb2ba38276aaff5f

[yasong, 2017-04-27 19:49:04]

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

[commit-bot: I haz the power, 2017-04-27 19:49:27]

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, 2017-04-27 19:54:06]

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

[commit-bot: I haz the power, 2017-04-27 19:54:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yasong, 2017-04-27 21:49:23]

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

[commit-bot: I haz the power, 2017-04-27 21:50:02]

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, 2017-04-27 22:35:12]

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

[commit-bot: I haz the power, 2017-04-27 22:35:12]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yasong, 2017-04-27 22:53:42]

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

[commit-bot: I haz the power, 2017-04-27 22:54:57]

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, 2017-04-28 01:37:03]

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

[commit-bot: I haz the power, 2017-04-28 01:37:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[yasong, 2017-04-28 12:43:52]

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

[commit-bot: I haz the power, 2017-04-28 12:44:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2017-04-28 12:52:24]

Please note that new presubmit rules have just landed at
https://crrev.com/2847613002 that will prevent you from checking in a change
that has NET_EXPORT_PRIVATE in it within the net/spdy/core directory: you'll
need to use SPDY_EXPORT_PRIVATE instead.

[chromium-reviews, 2017-04-28 13:12:31]

Thanks for the reminder. Will change it to the new macro.

On Fri, Apr 28, 2017 at 8:52 AM, <bnc@chromium.org> wrote:

> Please note that new presubmit rules have just landed at
> https://crrev.com/2847613002 that will prevent you from checking in a
> change
> that has NET_EXPORT_PRIVATE in it within the net/spdy/core directory:
> you'll
> need to use SPDY_EXPORT_PRIVATE instead.
>
> https://codereview.chromium.org/2844083003/
>

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

[commit-bot: I haz the power, 2017-04-28 13:55:06]

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

[commit-bot: I haz the power, 2017-04-28 13:55:07]

Dry run: This issue passed the CQ dry run.

[yasong, 2017-04-28 14:26:20]

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

[commit-bot: I haz the power, 2017-04-28 14:26:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yasong, 2017-04-28 14:27:22]

Description was changed from

==========
Create frame iterator for frame types except HEADER, PUSH_PROMISE and DATA. This
helps use unified interface at call site.

This CL lands server change 151846071 by yasong.

BUG=488484
==========

to

==========
Create frame iterator for frame types except HEADER, PUSH_PROMISE and DATA. This
helps use unified interface at call site.

This CL lands server change 151846071 by yasong.

BUG=488484
==========

[yasong, 2017-04-28 14:27:23]

yasong@chromium.org changed reviewers:
+ bnc@chromium.org

[yasong, 2017-04-28 14:28:54]

On 2017/04/28 13:12:31, chromium-reviews wrote:
> Thanks for the reminder. Will change it to the new macro.
> 
> On Fri, Apr 28, 2017 at 8:52 AM, <mailto:bnc@chromium.org> wrote:
> 
> > Please note that new presubmit rules have just landed at
> > https://crrev.com/2847613002 that will prevent you from checking in a
> > change
> > that has NET_EXPORT_PRIVATE in it within the net/spdy/core directory:
> > you'll
> > need to use SPDY_EXPORT_PRIVATE instead.
> >
> > https://codereview.chromium.org/2844083003/
> >
> 
> -- 
> 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.

Ready for review! Sorry about the delay!

[Bence, 2017-04-28 14:50:20]

LGTM with nit.  Thank you!

https://codereview.chromium.org/2844083003/diff/80001/net/spdy/core/spdy_fram...
File net/spdy/core/spdy_framer_test.cc (right):

https://codereview.chromium.org/2844083003/diff/80001/net/spdy/core/spdy_fram...
net/spdy/core/spdy_framer_test.cc:295:
EXPECT_GT(it.NextFrame(&frame_list_buffer), (size_t)0);
Could you use 0u instead of (size_t)0, here and below throughout this file? 
(Function call style cast is discouraged.)

[yasong, 2017-04-28 14:56:15]

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

[commit-bot: I haz the power, 2017-04-28 14:56:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yasong, 2017-04-28 14:57:57]

On 2017/04/28 14:50:20, Bence wrote:
> LGTM with nit.  Thank you!
> 
>
https://codereview.chromium.org/2844083003/diff/80001/net/spdy/core/spdy_fram...
> File net/spdy/core/spdy_framer_test.cc (right):
> 
>
https://codereview.chromium.org/2844083003/diff/80001/net/spdy/core/spdy_fram...
> net/spdy/core/spdy_framer_test.cc:295:
> EXPECT_GT(it.NextFrame(&frame_list_buffer), (size_t)0);
> Could you use 0u instead of (size_t)0, here and below throughout this file? 
> (Function call style cast is discouraged.)

Done.

Thanks for the review!

[commit-bot: I haz the power, 2017-04-28 16:08:04]

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

[commit-bot: I haz the power, 2017-04-28 16:08:05]

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

[yasong, 2017-04-28 16:16:31]

The CQ bit was checked by yasong@chromium.org

[yasong, 2017-04-28 16:16:31]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/2844083003/#ps100001
(title: "remove cast to 0.")

[commit-bot: I haz the power, 2017-04-28 16:17:04]

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, 2017-04-28 17:05:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493396191309350,
"parent_rev": "14d9d00fcdd545dc47387443e17c3d9fbb41e5fb", "commit_rev":
"4c4914e6e841e18cfbc6d13ebb2ba38276aaff5f"}

[commit-bot: I haz the power, 2017-04-28 17:05:51]

Description was changed from

==========
Create frame iterator for frame types except HEADER, PUSH_PROMISE and DATA. This
helps use unified interface at call site.

This CL lands server change 151846071 by yasong.

BUG=488484
==========

to

==========
Create frame iterator for frame types except HEADER, PUSH_PROMISE and DATA. This
helps use unified interface at call site.

This CL lands server change 151846071 by yasong.

BUG=488484

Review-Url: https://codereview.chromium.org/2844083003
Cr-Commit-Position: refs/heads/master@{#468033}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c4914e6e841e18cfbc6d13ebb2b...
==========

[commit-bot: I haz the power, 2017-04-28 17:05:53]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4c4914e6e841e18cfbc6d13ebb2b...

[Lei Zhang, 2017-05-04 21:22:45]

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

[Lei Zhang, 2017-05-04 21:22:47]

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
File net/spdy/core/spdy_framer.cc (right):

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1797: // TODO(yasong): remove all the down_casts.
I don't see any down_casts?

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1801: std::unique_ptr<SpdyFrameSequence> result =
nullptr;
You don't have to explicitly initialize std::unique_ptrs, but if that's the norm
in SPDY code, then ok.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1805: result = nullptr;
Or further set them to nullptr when they already are.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1811:
base::WrapUnique(static_cast<SpdyHeadersIR*>(frame_ir.get())));
Wait, how does this work? You can creating a new unique_ptr out of the raw
pointer in an existing unique_ptr? Doesn't this create double ownership?

[Bence, 2017-05-09 18:27:52]

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
File net/spdy/core/spdy_framer.cc (right):

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1801: std::unique_ptr<SpdyFrameSequence> result =
nullptr;
On 2017/05/04 21:22:46, Lei Zhang wrote:
> You don't have to explicitly initialize std::unique_ptrs, but if that's the
norm
> in SPDY code, then ok.

Acknowledged.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1805: result = nullptr;
On 2017/05/04 21:22:46, Lei Zhang wrote:
> Or further set them to nullptr when they already are.

Acknowledged.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1811:
base::WrapUnique(static_cast<SpdyHeadersIR*>(frame_ir.get())));
On 2017/05/04 21:22:46, Lei Zhang wrote:
> Wait, how does this work? You can creating a new unique_ptr out of the raw
> pointer in an existing unique_ptr? Doesn't this create double ownership?

Oops, good catch!  Thank you.

[yasong, 2017-05-09 20:42:07]

Thanks for the comments!

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
File net/spdy/core/spdy_framer.cc (right):

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1797: // TODO(yasong): remove all the down_casts.
On 2017/05/04 21:22:46, Lei Zhang wrote:
> I don't see any down_casts?

Good catch! This code is merged from google3, and down_cast is changed to
static_cast, but this comment is missed.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1801: std::unique_ptr<SpdyFrameSequence> result =
nullptr;
On 2017/05/09 18:27:51, Bence wrote:
> On 2017/05/04 21:22:46, Lei Zhang wrote:
> > You don't have to explicitly initialize std::unique_ptrs, but if that's the
> norm
> > in SPDY code, then ok.
> 
> Acknowledged.

Will be Fixed by cl/155545539.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1805: result = nullptr;
On 2017/05/04 21:22:46, Lei Zhang wrote:
> Or further set them to nullptr when they already are.

Fixed by 155536594.

https://codereview.chromium.org/2844083003/diff/100001/net/spdy/core/spdy_fra...
net/spdy/core/spdy_framer.cc:1811:
base::WrapUnique(static_cast<SpdyHeadersIR*>(frame_ir.get())));
On 2017/05/09 18:27:51, Bence wrote:
> On 2017/05/04 21:22:46, Lei Zhang wrote:
> > Wait, how does this work? You can creating a new unique_ptr out of the raw
> > pointer in an existing unique_ptr? Doesn't this create double ownership?
> 
> Oops, good catch!  Thank you.

Fixed by 155536594.