Implement nextHopProtocol in PerformanceResourceTiming and PerformanceNavigationTiming.

Implement nextHopProtocol in PerformanceResourceTiming and PerformanceNavigationTiming.

BUG=663175
Review-Url: https://codereview.chromium.org/2932003002
Cr-Commit-Position: refs/heads/master@{#482091}
Committed: https://chromium.googlesource.com/chromium/src/+/0d7335cd221d21f07fbc8dce72e437167e08ca7e

[shaseley, 2017-06-08 23:17:55]

shaseley@google.com changed reviewers:
+ panicker@chromium.org

[panicker, 2017-06-08 23:39:10]

Looking good overall!

https://codereview.chromium.org/2932003002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2932003002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebURLResponse.h:34: #include <memory>
add net/http to the list here:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS
?

[shaseley, 2017-06-09 00:27:51]

Resolved net/http dependency

https://codereview.chromium.org/2932003002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2932003002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebURLResponse.h:34: #include <memory>
On 2017/06/08 23:39:09, panicker wrote:
> add net/http to the list here:
> https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS
> ?
> 

Done.

[panicker, 2017-06-09 19:59:59]

panicker@chromium.org changed reviewers:
+ kinuko@chromium.org

[panicker, 2017-06-09 19:59:59]

LGTM.

Kinuko, could you please review? This is literally updating the old patch
(rebase) and addressing review comments from this change that you reviewed:
https://codereview.chromium.org/2429063002/

[Yoav Weiss, 2017-06-10 05:31:32]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2017-06-10 05:31:33]

Is this also implementing nextHop for NavTiming? Seems like some bits are
missing for that...

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_attributes_values.html
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_attributes_values.html:73:
assert_equals(entries[0].nextHopProtocol, "foo");
Why is it "foo"?

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:123:
AtomicString returnedProtocol = alpn_negotiated_protocol == "unknown"
Nit: Can you add parenthesis around the condition? IMO it'd make this more
readable.

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:128:
returnedProtocol == "unknown" ? "http/1.1" : returnedProtocol;
Nit: Can you add parenthesis around the condition? IMO it'd make this more
readable.

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
returnedProtocol = "hq";
Can you add a comment linking to the place this is defined in?

[kinuko, 2017-06-12 00:48:24]

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
returnedProtocol = "hq";
On 2017/06/10 05:31:33, Yoav Weiss wrote:
> Can you add a comment linking to the place this is defined in?

Where is it also defined this should return http/1.1 if it gets unknown?

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp:579:
std::string connectionInfoStr =
connectionInfoStr -> connection_info_string (no abbreviation preferred, and use
python_case)

[panicker, 2017-06-13 22:09:47]

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
returnedProtocol = "hq";
On 2017/06/12 00:48:23, kinuko wrote:
> On 2017/06/10 05:31:33, Yoav Weiss wrote:
> > Can you add a comment linking to the place this is defined in?
> 
> Where is it also defined this should return http/1.1 if it gets unknown?

"hq" is based on ietf working group discussion - I've asked them for any links.

The fallback was decided in this doc, not sure if it makes sense to link
https://docs.google.com/document/d/1qcpmh4K9yhGiJGduo5M71PLepKsyRHilvvZkQfz08...

[shaseley, 2017-06-13 23:14:18]

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_attributes_values.html
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_attributes_values.html:73:
assert_equals(entries[0].nextHopProtocol, "foo");
On 2017/06/10 05:31:33, Yoav Weiss wrote:
> Why is it "foo"?

Done.

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:123:
AtomicString returnedProtocol = alpn_negotiated_protocol == "unknown"
On 2017/06/10 05:31:33, Yoav Weiss wrote:
> Nit: Can you add parenthesis around the condition? IMO it'd make this more
> readable.

Done.

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:128:
returnedProtocol == "unknown" ? "http/1.1" : returnedProtocol;
On 2017/06/10 05:31:33, Yoav Weiss wrote:
> Nit: Can you add parenthesis around the condition? IMO it'd make this more
> readable.

Done.

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp:579:
std::string connectionInfoStr =
On 2017/06/12 00:48:23, kinuko wrote:
> connectionInfoStr -> connection_info_string (no abbreviation preferred, and
use
> python_case)

Done.

[shaseley, 2017-06-13 23:23:26]

On 2017/06/10 05:31:33, Yoav Weiss wrote:
> Is this also implementing nextHop for NavTiming? Seems like some bits are
> missing for that...

It is also implementing nextHop for NavTiming. Can you elaborate on what bits
seem to be missing? A lot of the NavTiming nextHop work is being done by parent
classes.

[kinuko, 2017-06-14 00:00:07]

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
returnedProtocol = "hq";
On 2017/06/13 22:09:47, panicker wrote:
> On 2017/06/12 00:48:23, kinuko wrote:
> > On 2017/06/10 05:31:33, Yoav Weiss wrote:
> > > Can you add a comment linking to the place this is defined in?
> > 
> > Where is it also defined this should return http/1.1 if it gets unknown?
> 
> "hq" is based on ietf working group discussion - I've asked them for any
links.
> 
> The fallback was decided in this doc, not sure if it makes sense to link
>
https://docs.google.com/document/d/1qcpmh4K9yhGiJGduo5M71PLepKsyRHilvvZkQfz08...

Should we file a github issue for spec discussion and add a link to it?

[Yoav Weiss, 2017-06-14 07:23:48]

On 2017/06/14 00:00:07, kinuko wrote:
>
https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
> (right):
> 
>
https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
> returnedProtocol = "hq";
> On 2017/06/13 22:09:47, panicker wrote:
> > On 2017/06/12 00:48:23, kinuko wrote:
> > > On 2017/06/10 05:31:33, Yoav Weiss wrote:
> > > > Can you add a comment linking to the place this is defined in?
> > > 
> > > Where is it also defined this should return http/1.1 if it gets unknown?
> > 
> > "hq" is based on ietf working group discussion - I've asked them for any
> links.
> > 
> > The fallback was decided in this doc, not sure if it makes sense to link
> >
>
https://docs.google.com/document/d/1qcpmh4K9yhGiJGduo5M71PLepKsyRHilvvZkQfz08...
> 
> Should we file a github issue for spec discussion and add a link to it?

yeah, that'd be great :)

[panicker, 2017-06-14 19:46:47]

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
returnedProtocol = "hq";
On 2017/06/14 00:00:06, kinuko wrote:
> On 2017/06/13 22:09:47, panicker wrote:
> > On 2017/06/12 00:48:23, kinuko wrote:
> > > On 2017/06/10 05:31:33, Yoav Weiss wrote:
> > > > Can you add a comment linking to the place this is defined in?
> > > 
> > > Where is it also defined this should return http/1.1 if it gets unknown?
> > 
> > "hq" is based on ietf working group discussion - I've asked them for any
> links.
> > 
> > The fallback was decided in this doc, not sure if it makes sense to link
> >
>
https://docs.google.com/document/d/1qcpmh4K9yhGiJGduo5M71PLepKsyRHilvvZkQfz08...
> 
> Should we file a github issue for spec discussion and add a link to it?

SGTM, let's reference this issue:
https://github.com/w3c/navigation-timing/issues/71

[shaseley, 2017-06-15 00:11:01]

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:131:
returnedProtocol = "hq";
On 2017/06/14 19:46:47, panicker wrote:
> On 2017/06/14 00:00:06, kinuko wrote:
> > On 2017/06/13 22:09:47, panicker wrote:
> > > On 2017/06/12 00:48:23, kinuko wrote:
> > > > On 2017/06/10 05:31:33, Yoav Weiss wrote:
> > > > > Can you add a comment linking to the place this is defined in?
> > > > 
> > > > Where is it also defined this should return http/1.1 if it gets unknown?
> > > 
> > > "hq" is based on ietf working group discussion - I've asked them for any
> > links.
> > > 
> > > The fallback was decided in this doc, not sure if it makes sense to link
> > >
> >
>
https://docs.google.com/document/d/1qcpmh4K9yhGiJGduo5M71PLepKsyRHilvvZkQfz08...
> > 
> > Should we file a github issue for spec discussion and add a link to it?
> 
> SGTM, let's reference this issue:
> https://github.com/w3c/navigation-timing/issues/71

Per the Github issue thread, the latest patchset falls back to the empty string
if connection info is unknown, instead of http/1.1. I've updated the comments as
well.

[Yoav Weiss, 2017-06-16 11:57:42]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 11:57:49]

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-06-16 12:00:09]

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

[commit-bot: I haz the power, 2017-06-16 12:00:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[Yoav Weiss, 2017-06-16 15:25:05]

Apologies for the delayed review... Been traveling

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:127: //
(https://github.com/w3c/navigation-timing/issues/71)
This probably should be documented elsewhere, but no point in holding off this
patch until that happens. Maybe add a TODO to update the spec pointers?

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:130: // alpn
id "hq". (https://github.com/w3c/navigation-timing/issues/71)
I don't think we should convert Google's QUIC into "hq" here. See
https://github.com/w3c/navigation-timing/issues/71#issuecomment-309054561

[shaseley, 2017-06-16 16:27:17]

The CQ bit was checked by shaseley@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 16:27:38]

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-06-16 16:31:43]

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

[commit-bot: I haz the power, 2017-06-16 16:31:45]

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

[panicker, 2017-06-16 17:37:27]

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:130: // alpn
id "hq". (https://github.com/w3c/navigation-timing/issues/71)
On 2017/06/16 15:25:04, Yoav Weiss wrote:
> I don't think we should convert Google's QUIC into "hq" here. See
> https://github.com/w3c/navigation-timing/issues/71#issuecomment-309054561

What are you proposing instead?
I'm fine with starting with blank string until we can decide; we already had
tons of back and forth on this and I'd like to unblock the rest of this CL.

[shaseley, 2017-06-16 17:59:26]

The CQ bit was checked by shaseley@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 17:59:48]

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-06-16 18:12:51]

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

[commit-bot: I haz the power, 2017-06-16 18:12:52]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[panicker, 2017-06-16 22:41:18]

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:127: //
(https://github.com/w3c/navigation-timing/issues/71)
On 2017/06/16 15:25:04, Yoav Weiss wrote:
> This probably should be documented elsewhere, but no point in holding off this
> patch until that happens. Maybe add a TODO to update the spec pointers?

What else is there to document exactly (and which spec pointer)? I opened that
github to document the fallback case, and it's been resolved AFAICT.

https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:130: // alpn
id "hq". (https://github.com/w3c/navigation-timing/issues/71)
On 2017/06/16 17:37:27, panicker wrote:
> On 2017/06/16 15:25:04, Yoav Weiss wrote:
> > I don't think we should convert Google's QUIC into "hq" here. See
> > https://github.com/w3c/navigation-timing/issues/71#issuecomment-309054561
> 
> What are you proposing instead?
> I'm fine with starting with blank string until we can decide; we already had
> tons of back and forth on this and I'd like to unblock the rest of this CL.

As Ilya suggested on bug, let's stick with "hq" and feel free to start a thread
on blink-dev with sleevi@ if you have reservations.

[shaseley, 2017-06-16 23:20:17]

The CQ bit was checked by shaseley@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 23:20:35]

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-06-16 23:47:13]

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

[commit-bot: I haz the power, 2017-06-16 23:47:14]

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

[shaseley, 2017-06-17 00:48:32]

I’m running into a problem building for Mac and Android related to adding
net as a dependency, which is needed by WebURLResponse.h . Before I took
this bug over, there was a decision to do this (relevant discussion 
here: https://crrev.com/2429063002/patch/260001/270021),
but there are some build issues.

First, //net was needed to pass some bot checks for all platforms
(patch set 6,  third_party/WebKit/public/BUILD.gn). This fixed Linux/Windows
builds.
 
But, the build logs for Mac and Android are now littered with this error:
 
./../third_party/boringssl/src/include/openssl/base.h:71:10: fatal error: 
'openssl/is_boringssl.h' file not found
#include <openssl/is_boringssl.h>

Digging into this, there’s an include that reaches into third_party/boringssl
(reached by including WebURLResponse.h), but only for Mac/Android. That library
is also making sure (by including is_boringssl.h) that it isn’t using the system
openssl. However, it isn’t able to find the right header files when compiling.
 
I duplicated this on a Macbook and solved it locally by adding //net as a
dependency in third_party/WebKit/public/BUILD.gn (patch set 7). However, it’s
still broken on the bots and I’m not sure why.
 
Does anyone know the *right* way to add this dependency?
 
And, do we still want to introduce this dependency?

Thanks!

[Yoav Weiss, 2017-06-21 00:53:48]

On 2017/06/16 15:25:05, Yoav Weiss wrote:
> 
>
https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:130: //
alpn
> id "hq". (https://github.com/w3c/navigation-timing/issues/71)
> I don't think we should convert Google's QUIC into "hq" here. See
> https://github.com/w3c/navigation-timing/issues/71#issuecomment-309054561

After talking to rch@ about this, it seems like there's no plan to ship both
Google QUIC and IETF QUIC simultaneously. From my perspective it's enough to
mean that there's no reason to block this from shipping in current form.

I still think there may be benefit from registering Google's QUIC its own ALPN
string, but if/when that happens we would be able to change the reported value
with no compatibility implications.

core/ LGTM

[kinuko, 2017-06-21 06:45:47]

Looks like it's failing to build some testing targets.  Would adding
"//third_party/boringssl" deps to the target help?

[kinuko, 2017-06-21 06:47:08]

By the way code change lgtm

[shaseley, 2017-06-22 02:57:34]

The CQ bit was checked by shaseley@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 02:57:49]

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-06-22 04:29:25]

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

[commit-bot: I haz the power, 2017-06-22 04:29:26]

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

[shaseley, 2017-06-22 21:24:52]

The CQ bit was checked by shaseley@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 21:25:12]

Dry run: CQ is trying da patch.

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

[panicker, 2017-06-22 22:23:57]

panicker@chromium.org changed reviewers:
+ haraken@chromium.org

[panicker, 2017-06-22 22:23:58]

+haraken for source/modules owners (and sanity check the BUILD updates :))

[kinuko, 2017-06-22 23:40:36]

https://codereview.chromium.org/2932003002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BUILD.gn (right):

https://codereview.chromium.org/2932003002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BUILD.gn:210:
"//third_party/WebKit/public:blink_headers",
I see, for forwarding deps...

I think adding "third_party/WebKit/Source/platform" instead would also work and
would be probably more natural (here and elsewhere)

platform has public_deps to //net and this one actually depends on it,
public:blink_headers dependency is more for external modules that just needs
public headers

[shaseley, 2017-06-23 00:08:39]

https://codereview.chromium.org/2932003002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BUILD.gn (right):

https://codereview.chromium.org/2932003002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BUILD.gn:210:
"//third_party/WebKit/public:blink_headers",
On 2017/06/22 23:40:36, kinuko wrote:
> I see, for forwarding deps...
> 
> I think adding "third_party/WebKit/Source/platform" instead would also work
and
> would be probably more natural (here and elsewhere)
> 
> platform has public_deps to //net and this one actually depends on it,
> public:blink_headers dependency is more for external modules that just needs
> public headers
> 

Yes, the build issues turned out to be any targets that have files that include
public/platform/WebURLResponse.h, but don't declare dependencies to //net (or
something that depends on //net). As boringssl is a public dep of //net, //net
suffices.

Thanks kinuko@ for the suggestion (and the original about
//third_party/boringssl). I'll switch to third_party/WebKit/Source/platform and
try the bots again.

[shaseley, 2017-06-23 00:09:01]

On 2017/06/21 00:53:48, Yoav Weiss wrote:
> On 2017/06/16 15:25:05, Yoav Weiss wrote:
> > 
> >
>
https://codereview.chromium.org/2932003002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:130: //
> alpn
> > id "hq". (https://github.com/w3c/navigation-timing/issues/71)
> > I don't think we should convert Google's QUIC into "hq" here. See
> > https://github.com/w3c/navigation-timing/issues/71#issuecomment-309054561
> 
> After talking to rch@ about this, it seems like there's no plan to ship both
> Google QUIC and IETF QUIC simultaneously. From my perspective it's enough to
> mean that there's no reason to block this from shipping in current form.
> 
> I still think there may be benefit from registering Google's QUIC its own ALPN
> string, but if/when that happens we would be able to change the reported value
> with no compatibility implications.
> 
> core/ LGTM

Thanks for the review and the discussion about this! I'll continue with using
the "hq" ALPN.

[commit-bot: I haz the power, 2017-06-23 00:20:22]

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

[commit-bot: I haz the power, 2017-06-23 00:20:23]

Dry run: This issue passed the CQ dry run.

[shaseley, 2017-06-23 01:18:05]

The CQ bit was checked by shaseley@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 01:18:28]

Dry run: CQ is trying da patch.

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

[haraken, 2017-06-23 03:58:04]

WebKit LGTM

https://codereview.chromium.org/2932003002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
(right):

https://codereview.chromium.org/2932003002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp:580: return
String::FromUTF8(connection_info_string.c_str(),

Can we use AtomicString(connection_info_string.data(),
connection_info_string.length())?

[commit-bot: I haz the power, 2017-06-23 04:06:43]

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

[commit-bot: I haz the power, 2017-06-23 04:06:45]

Dry run: This issue passed the CQ dry run.

[shaseley, 2017-06-23 16:39:22]

https://codereview.chromium.org/2932003002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
(right):

https://codereview.chromium.org/2932003002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp:580: return
String::FromUTF8(connection_info_string.c_str(),
On 2017/06/23 03:58:04, haraken wrote:
> 
> Can we use AtomicString(connection_info_string.data(),
> connection_info_string.length())?

I see, to avoid the conversion from String to AtomicString at the call site?

It appears that AtomicString doesn't have a constructor for (char*, unsigned),
but we can cast to LChar*. I've pushed a new patch set with this change.

[shaseley, 2017-06-23 20:20:09]

The CQ bit was checked by shaseley@google.com

[shaseley, 2017-06-23 20:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
panicker@chromium.org, kinuko@chromium.org, yoav@yoav.ws, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2932003002/#ps200001
(title: "Make ConnectionInfoString() return AtomicString")

[commit-bot: I haz the power, 2017-06-23 20:20:45]

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-06-23 20:31:26]

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

[commit-bot: I haz the power, 2017-06-23 20:31:27]

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

[shaseley, 2017-06-23 20:58:47]

On 2017/06/23 20:31:27, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Looks like we need a review from someone in API_OWNERS for
global-interface-listing.txt. panicker@, not sure who to add?

[Yoav Weiss, 2017-06-23 21:33:27]

Description was changed from

==========
Implement nextHopProtocol in PerformanceResourceTiming and
PerformanceNavigationTiming.

BUG=663175
==========

to

==========
Implement nextHopProtocol in PerformanceResourceTiming and
PerformanceNavigationTiming.

BUG=663175
==========

[Yoav Weiss, 2017-06-23 21:33:27]

yoav@yoav.ws changed reviewers:
+ chrishtr@chromium.org, foolip@chromium.org, tkent@chromium.org

[Yoav Weiss, 2017-06-23 21:34:29]

On 2017/06/23 20:58:47, shaseley wrote:
> On 2017/06/23 20:31:27, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Looks like we need a review from someone in API_OWNERS for
> global-interface-listing.txt. panicker@, not sure who to add?

Philip, Kent or Chris - can you take a look?

[chrishtr, 2017-06-23 21:38:02]

lgtm

LGTM for
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt.

[shaseley, 2017-06-23 21:57:52]

The CQ bit was checked by shaseley@google.com

[commit-bot: I haz the power, 2017-06-23 21:58: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, 2017-06-24 00:27:47]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498255072656490,
"parent_rev": "734b0f4b78e014a4baba05eb66128cf1bf376c37", "commit_rev":
"0d7335cd221d21f07fbc8dce72e437167e08ca7e"}

[commit-bot: I haz the power, 2017-06-24 00:28:00]

Description was changed from

==========
Implement nextHopProtocol in PerformanceResourceTiming and
PerformanceNavigationTiming.

BUG=663175
==========

to

==========
Implement nextHopProtocol in PerformanceResourceTiming and
PerformanceNavigationTiming.

BUG=663175

Review-Url: https://codereview.chromium.org/2932003002
Cr-Commit-Position: refs/heads/master@{#482091}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d7335cd221d21f07fbc8dce72e4...
==========

[commit-bot: I haz the power, 2017-06-24 00:28:01]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/0d7335cd221d21f07fbc8dce72e4...