Implement nextHopProtocol in PerformanceResourceTiming and PerformanceNavigationTiming.

Implement nextHopProtocol in PerformanceResourceTiming and PerformanceNavigationTiming.


BUG=663175

[sunjian, 2016-10-18 23:38:21]

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

[panicker, 2016-10-19 00:25:11]

https://codereview.chromium.org/2429063002/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2429063002/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:1058: response->setALPNNegotiatedProtocol(
move this before setting of extra data (line 1052 above)

https://codereview.chromium.org/2429063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ResourceTimingInfo.h (right):

https://codereview.chromium.org/2429063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ResourceTimingInfo.h:70: /*void
setNextHopProtocol(const AtomicString& nextHopProtocol) {
why is this commented out?
Remove if not needed

[igrigorik, 2016-10-19 16:59:58]

Description was changed from

==========
Added feature to expose nextHopProtocol in ResourcTiming API.

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

to

==========
Added feature to expose nextHopProtocol in ResourcTiming API.

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

[sunjian, 2016-10-21 00:19:51]

https://codereview.chromium.org/2429063002/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2429063002/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:1058: response->setALPNNegotiatedProtocol(
On 2016/10/19 00:25:11, Shubhie wrote:
> move this before setting of extra data (line 1052 above)

Done.

https://codereview.chromium.org/2429063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ResourceTimingInfo.h (right):

https://codereview.chromium.org/2429063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ResourceTimingInfo.h:70: /*void
setNextHopProtocol(const AtomicString& nextHopProtocol) {
On 2016/10/19 00:25:11, Shubhie wrote:
> why is this commented out?
> Remove if not needed

Done.

[panicker, 2016-10-21 18:34:36]

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

[commit-bot: I haz the power, 2016-10-21 18:35:36]

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-10-21 20:27:28]

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

[commit-bot: I haz the power, 2016-10-21 20:27:29]

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

[panicker, 2016-10-24 18:23:31]

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

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:87:
returnedProtocol == "unknown" ? "http/1.1" : returnedProtocol;
I don't think we can assume http1 version here.

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:89:
returnedProtocol == "quic/1+spdy/3" ? "http2/+quic/35" : returnedProtocol;
cannot assume quic version

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebURLResponse.h:288: // ALPN negotiated
protocol of the socket which fetched this resource.
incorrect description, please fix

[panicker, 2016-10-24 19:44:38]

As discussed, for tests:
- fix layout tests and add expectation files to patch
- upload separate (minimal) repro CL for unittest linkage issue to investigate;
meanwhile try rejigering the test

[sunjian, 2016-10-24 23:01:39]

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

[sunjian, 2016-10-24 23:02:57]

The CQ bit was unchecked by sunjian@chromium.org

[sunjian, 2016-10-24 23:03:15]

The CQ bit was checked by sunjian@chromium.org

[sunjian, 2016-10-24 23:03:20]

The CQ bit was unchecked by sunjian@chromium.org

[sunjian, 2016-10-24 23:04:19]

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

[commit-bot: I haz the power, 2016-10-24 23:05:18]

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-10-24 23:14:19]

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

[commit-bot: I haz the power, 2016-10-24 23:14:20]

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

[sunjian, 2016-10-25 00:10:40]

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

[sunjian, 2016-10-25 00:10:55]

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

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:87:
returnedProtocol == "unknown" ? "http/1.1" : returnedProtocol;
On 2016/10/24 18:23:31, Shubhie wrote:
> I don't think we can assume http1 version here.

Talked off line. Agreed to set it to be "http/1.1" when both
alpnNegotiatedProtocol and connectionInfo happen to be unknown.

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:89:
returnedProtocol == "quic/1+spdy/3" ? "http2/+quic/35" : returnedProtocol;
On 2016/10/24 18:23:31, Shubhie wrote:
> cannot assume quic version

Done.

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebURLResponse.h:288: // ALPN negotiated
protocol of the socket which fetched this resource.
On 2016/10/24 18:23:31, Shubhie wrote:
> incorrect description, please fix

Done.

[commit-bot: I haz the power, 2016-10-25 00:11:17]

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-10-25 00:27:29]

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

[commit-bot: I haz the power, 2016-10-25 00:27:30]

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

[sunjian, 2016-10-25 18:05:40]

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

[commit-bot: I haz the power, 2016-10-25 18:06:12]

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-10-25 19:47:46]

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

[commit-bot: I haz the power, 2016-10-25 19:47:47]

Dry run: This issue passed the CQ dry run.

[panicker, 2016-10-25 23:53:26]

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:91:
returnedProtocol =
Add a comment explaining this

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:93:
std::string returnedProtocolCopy = returnedProtocol.ascii().data();
std::string and std::regex are not allowed in Source/core, use string ops in
AtomicString?

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp
(right):

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:13: bool
fallbackToConnectionInfoWhenALPNUnknown() {
move these inline into tests.

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:37:
EXPECT_TRUE(fallbackToConnectionInfoWhenALPNUnknown());
(see previous comment) use EXPECT_EQ inline

[sunjian, 2016-10-26 19:56:52]

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:91:
returnedProtocol =
On 2016/10/25 23:53:26, Shubhie wrote:
> Add a comment explaining this

Done.

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:93:
std::string returnedProtocolCopy = returnedProtocol.ascii().data();
On 2016/10/25 23:53:26, Shubhie wrote:
> std::string and std::regex are not allowed in Source/core, use string ops in
> AtomicString?

Done.

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp
(right):

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:13: bool
fallbackToConnectionInfoWhenALPNUnknown() {
On 2016/10/25 23:53:26, Shubhie wrote:
> move these inline into tests.

I tried to do that. But PerformanceResourceTiming::produceNextProtocol is not
accessible from TEST_F since it's private.

https://codereview.chromium.org/2429063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:37:
EXPECT_TRUE(fallbackToConnectionInfoWhenALPNUnknown());
On 2016/10/25 23:53:26, Shubhie wrote:
> (see previous comment) use EXPECT_EQ inline 

Same reason as above.

[sunjian, 2016-10-26 20:27:55]

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

[commit-bot: I haz the power, 2016-10-26 20:28:33]

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

[panicker, 2016-10-26 20:35:06]

https://codereview.chromium.org/2429063002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp
(right):

https://codereview.chromium.org/2429063002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:23:
AtomicString connectionInfo = "http/1.1";
Add another test for when connectionInfo is also "unknown"?

https://codereview.chromium.org/2429063002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:31:
AtomicString alpnNegotiatedProtocol = "quic/1+spdy/3";
Add test case with quic string without spdy?
Or is that not possible?

[commit-bot: I haz the power, 2016-10-26 22:06:50]

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

[commit-bot: I haz the power, 2016-10-26 22:06:51]

Dry run: This issue passed the CQ dry run.

[sunjian, 2016-10-27 20:41:19]

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

[sunjian, 2016-10-27 20:41:30]

https://codereview.chromium.org/2429063002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp
(right):

https://codereview.chromium.org/2429063002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:23:
AtomicString connectionInfo = "http/1.1";
On 2016/10/26 20:35:06, Shubhie wrote:
> Add another test for when connectionInfo is also "unknown"?

Done.

https://codereview.chromium.org/2429063002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:31:
AtomicString alpnNegotiatedProtocol = "quic/1+spdy/3";
On 2016/10/26 20:35:06, Shubhie wrote:
> Add test case with quic string without spdy?
> Or is that not possible?

Done.

[commit-bot: I haz the power, 2016-10-27 20:41:51]

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

[panicker, 2016-10-27 22:05:22]

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:91: //
Fallback to "http/1.1" when connectionInfo is also unknown
we should probably log an error here, but let's atleast mention in the comment
that this is an "error" situation

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp
(right):

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTimingTest.cpp:46:
AtomicString alpnNegotiatedProtocol = "quic/1+other/123";
what if this is just "quic/1"? Could we remove the "+something" assumption?

[commit-bot: I haz the power, 2016-10-27 22:46:01]

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

[commit-bot: I haz the power, 2016-10-27 22:46:02]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-10-28 07:00:33]

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

[Yoav Weiss, 2016-10-28 07:00:34]

AFAICT, this is not testing that the actual values filling the
alpn_negotiated_protocol and the connection_info are correct.

Are there tests for those? If so, should the be modified? If not, should we add
ones?

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:99:
returnedProtocol = "http2/+" + quicPart;
Can you add a comment indicating where the "+" syntax is defined?
I couldn't find that definition in https://tools.ietf.org/html/rfc7301 where the
ALPN ID registry is defined. (and lacking both "http2" and "quic"...)

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:99:
returnedProtocol = "http2/+" + quicPart;
Where is that behavior defined?

[panicker, 2016-10-28 15:50:22]

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

[panicker, 2016-10-28 15:50:24]

<< Re: testing that the actual values filling the alpn_negotiated_protocol and
the connection_info are correct.

I'm not sure that we could test the values under various conditions in a
unittest. OTOH it's obvious to me how to cover this in web-platform-test either.
Totally open to suggestions though.

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:99:
returnedProtocol = "http2/+" + quicPart;
On 2016/10/28 07:00:34, Yoav Weiss wrote:
> Can you add a comment indicating where the "+" syntax is defined?
> I couldn't find that definition in https://tools.ietf.org/html/rfc7301 where
the
> ALPN ID registry is defined. (and lacking both "http2" and "quic"...)

My understanding is that IETF has not decided yet whether there will be an ALPN
string for QUIC at all, 
so this is "best effort" suggestion by rch@ and igrigorik@ (added to CL).
We should update this to match IETF after that's decided.

[Yoav Weiss, 2016-11-03 08:10:23]

Regarding the tests, I was thinking browser tests would be appropriate here,
assuming we already have the infrastructure to test against QUIC and H2 servers.

+mmenke which probably knows more about the best place for such tests.

Also, as this exposes a new attribute on ResourceTiming (with proprietary syntax
in case of quic), I believe this would require an intent to implement and ship.

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:99:
returnedProtocol = "http2/+" + quicPart;
On 2016/10/28 15:50:23, Shubhie wrote:
> On 2016/10/28 07:00:34, Yoav Weiss wrote:
> > Can you add a comment indicating where the "+" syntax is defined?
> > I couldn't find that definition in https://tools.ietf.org/html/rfc7301 where
> the
> > ALPN ID registry is defined. (and lacking both "http2" and "quic"...)
> 
> My understanding is that IETF has not decided yet whether there will be an
ALPN
> string for QUIC at all, 
> so this is "best effort" suggestion by rch@ and igrigorik@ (added to CL).
> We should update this to match IETF after that's decided.
> 

It would still be good to have some definition of said syntax. Also, since we'd
be exposing that, there is a non-zero compat risk here. We should run it by the
API owners as part of the I2S.

[Ryan Hamilton, 2016-11-03 14:30:44]

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

[Ryan Hamilton, 2016-11-03 14:30:45]

Adding bnc who has been doing work in this area recently.

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:99:
returnedProtocol = "http2/+" + quicPart;
On 2016/11/03 08:10:23, Yoav Weiss wrote:
> On 2016/10/28 15:50:23, Shubhie wrote:
> > On 2016/10/28 07:00:34, Yoav Weiss wrote:
> > > Can you add a comment indicating where the "+" syntax is defined?
> > > I couldn't find that definition in https://tools.ietf.org/html/rfc7301
where
> > the
> > > ALPN ID registry is defined. (and lacking both "http2" and "quic"...)
> > 
> > My understanding is that IETF has not decided yet whether there will be an
> ALPN
> > string for QUIC at all, 
> > so this is "best effort" suggestion by rch@ and igrigorik@ (added to CL).
> > We should update this to match IETF after that's decided.
> > 
> 
> It would still be good to have some definition of said syntax. Also, since
we'd
> be exposing that, there is a non-zero compat risk here. We should run it by
the
> API owners as part of the I2S.

bnc: Can you take a look at this in light of your connection_info changes?

[mmenke, 2016-11-03 14:36:26]

For testing, there's chrome/browser/net/load_timing_browsertest.cc, which tests
that reported times are non-NULL and in the right range, though it doesn't
compare them with "real" times from the network stack.  There's fuzziness in
converting browser times to renderer times on Windows, so can't check exact
values, unfortunately.

[mmenke, 2016-11-03 14:41:53]

On 2016/11/03 14:36:26, mmenke (Busy Nov 2-3) wrote:
> For testing, there's chrome/browser/net/load_timing_browsertest.cc, which
tests
> that reported times are non-NULL and in the right range, though it doesn't
> compare them with "real" times from the network stack.  There's fuzziness in
> converting browser times to renderer times on Windows, so can't check exact
> values, unfortunately.

Sorry, thought you were asking about timing tests, not QUIC/H2 tests...We don't
have any browser tests that exercist H2 or QUIC.

I don't think we need real H2/QUIC, though, do we?  We can create mock
URLRequestJobs that pretend to be H2/QUIC responses.  Then as long as we also
have lower layer tests that check that URLRequest's fields are properly
populated for H2/QUIC, we're fine, right (Admittedly, not sure we have such
tests)

[Yoav Weiss, 2016-11-03 14:42:04]

On 2016/11/03 14:36:26, mmenke (Busy Nov 2-3) wrote:
> For testing, there's chrome/browser/net/load_timing_browsertest.cc, which
tests
> that reported times are non-NULL and in the right range, though it doesn't
> compare them with "real" times from the network stack.  There's fuzziness in
> converting browser times to renderer times on Windows, so can't check exact
> values, unfortunately.

The testing I'd like to see here is not timing values but protocol values. Can
we run browser tests against an H2/TCP server and a QUIC server just to check
that we're getting the right values back?

[Ryan Hamilton, 2017-01-30 15:03:19]

rch@chromium.org changed reviewers:
- rch@chromium.org

[Ryan Hamilton, 2017-01-30 15:03:20]

Removing myself as a reviewer. Please feel free to add me back when this is
ready.

[Bence, 2017-02-27 23:10:16]

Removing myself as a reviewer.  Please feel free to add me back when you require
action on my part.

[Bence, 2017-03-01 00:25:53]

bnc@chromium.org changed reviewers:
- bnc@chromium.org

[sunjian, 2017-03-29 18:25:50]

Patchset #10 (id:180001) has been deleted

[sunjian, 2017-03-29 18:39:24]

Patchset #10 (id:200001) has been deleted

[sunjian, 2017-04-03 19:28:21]

Description was changed from

==========
Added feature to expose nextHopProtocol in ResourcTiming API.

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

to

==========
Implement nextHopProtocol in ResourcTiming API.

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

[sunjian, 2017-04-05 18:18:25]

Can you take another look at this cl please, we have decided to move this change
forward again!

[panicker, 2017-04-05 18:52:49]

Please ensure this field is also visible in Navigation Timing 2 and add a test
to codify this.

[panicker, 2017-04-05 18:57:45]

panicker@chromium.org changed reviewers:
+ bnc@chromium.org, rch@chromium.org

[panicker, 2017-04-05 18:57:46]

Adding reviewers back, as this is ready to move forward again.

[panicker, 2017-04-05 18:58:22]

On 2017/04/05 18:52:49, panicker wrote:
> Please ensure this field is also visible in Navigation Timing 2 and add a test
> to codify this.
Also update layout test for Resource Timing.

[Ryan Hamilton, 2017-04-05 20:31:11]

https://codereview.chromium.org/2429063002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:121: }
Instead of doing the mapping from "... quic ... " to "hq" up here, would it be
better to do this down lower where either alpnNegotiatedProtocol or
connectionInfo are created (down in net/)?

[sunjian, 2017-04-05 21:46:36]

On 2017/04/05 18:52:49, panicker wrote:
> Please ensure this field is also visible in Navigation Timing 2 and add a test
> to codify this.

Done!

[sunjian, 2017-04-05 21:48:01]

On 2017/04/05 18:58:22, panicker wrote:
> On 2017/04/05 18:52:49, panicker wrote:
> > Please ensure this field is also visible in Navigation Timing 2 and add a
test
> > to codify this.
> Also update layout test for Resource Timing.

I think this needs to be done on github cause Resource Timing's wpt tests are
not sync to chromium yet?

[panicker, 2017-04-05 21:48:27]

https://codereview.chromium.org/2429063002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:121: }
On 2017/04/05 20:31:11, Ryan Hamilton wrote:
> Instead of doing the mapping from "... quic ... " to "hq" up here, would it be
> better to do this down lower where either alpnNegotiatedProtocol or
> connectionInfo are created (down in net/)?

Could we do this as a follow-up - if someone on net/ side is willing to take it
on? In which case let's add a TODO here.

[sunjian, 2017-04-05 21:48:47]

On 2017/04/05 20:31:11, Ryan Hamilton wrote:
>
https://codereview.chromium.org/2429063002/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
> (right):
> 
>
https://codereview.chromium.org/2429063002/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:121: }
> Instead of doing the mapping from "... quic ... " to "hq" up here, would it be
> better to do this down lower where either alpnNegotiatedProtocol or
> connectionInfo are created (down in net/)?

+Bence, +panicker. What do you guys think?

[sunjian, 2017-04-05 21:49:08]

Please take another look!

[sunjian, 2017-04-05 21:50:33]

Description was changed from

==========
Implement nextHopProtocol in ResourcTiming API.

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

to

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


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

[panicker, 2017-04-10 17:52:20]

LGTM
Ryan, Bence - PTAL?

[Bence, 2017-04-12 13:48:49]

I'm not an OWNER of any of these files, but this looks fine to me.  I'm sorry
for the very slow response.

[panicker, 2017-04-12 16:57:11]

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

[panicker, 2017-04-12 16:57:12]

Kinuko - could you please review for Owners?

[panicker, 2017-04-12 16:58:46]

panicker@chromium.org changed reviewers:
+ sgurun@chromium.org, torne@chromium.org

[panicker, 2017-04-12 16:58:47]

+sgurun, torne - could you review for the webview test update?

[Torne, 2017-04-12 17:37:18]

android_webview LGTM, thanks!

[panicker, 2017-04-14 19:02:28]

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

[panicker, 2017-04-14 19:02:30]

+jochen - could you review for OWNERS 

(apologies for the OWNERS fishing, but need to cast wider net)

[panicker, 2017-04-17 17:01:41]

Friendly ping.

[kinuko, 2017-04-18 05:46:25]

I think you'll need rebase (and run the renamer tool) as great reformat
happened.

https://codereview.chromium.org/2429063002/diff/260001/content/public/rendere...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/2429063002/diff/260001/content/public/rendere...
content/public/renderer/document_state.h:57: std::string connection_info() const
{ return connection_info_; }
const std::string&

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp
(right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:119: //
Fallback to connectionInfo when alpnNegotiatedProtocol is unknowm.
unknowm -> unknown

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:129: }
nit: no braces needed for one-line body

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:135:
connectionInfo());
nit: indent looks off (please just run git cl format)

[jochen (gone - plz use gerrit), 2017-04-18 11:37:59]

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:269: BLINK_PLATFORM_EXPORT
WebString alpnNegotiatedProtocol() const;
it seems that these aren't strings, but registry controlled  values. Should this
be an enum?

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:273: BLINK_PLATFORM_EXPORT
WebString connectionInfo() const;
same here, is that a free-form value, or really some enum?

[panicker, 2017-04-18 17:01:06]

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:269: BLINK_PLATFORM_EXPORT
WebString alpnNegotiatedProtocol() const;
On 2017/04/18 11:37:58, jochen wrote:
> it seems that these aren't strings, but registry controlled  values. Should
this
> be an enum?

Ditto for this (see other comment).

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:273: BLINK_PLATFORM_EXPORT
WebString connectionInfo() const;
On 2017/04/18 11:37:58, jochen wrote:
> same here, is that a free-form value, or really some enum?

There's a lot of internal and deprecated values here that the net/ team is
constantly evolving, so I'm not sure it makes sense to bake it into public
interface:
https://cs.chromium.org/chromium/src/net/http/http_response_info.h?type=cs&q=...

[jochen (gone - plz use gerrit), 2017-04-18 18:21:12]

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:273: BLINK_PLATFORM_EXPORT
WebString connectionInfo() const;
On 2017/04/18 at 17:01:06, panicker wrote:
> On 2017/04/18 11:37:58, jochen wrote:
> > same here, is that a free-form value, or really some enum?
> 
> There's a lot of internal and deprecated values here that the net/ team is
constantly evolving, so I'm not sure it makes sense to bake it into public
interface:
>
https://cs.chromium.org/chromium/src/net/http/http_response_info.h?type=cs&q=...

The interface is not really public - it used to be an interface to WebKit but
now it's just historical baggage

[panicker, 2017-04-18 21:43:50]

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:269: BLINK_PLATFORM_EXPORT
WebString alpnNegotiatedProtocol() const;
On 2017/04/18 17:01:06, panicker wrote:
> On 2017/04/18 11:37:58, jochen wrote:
> > it seems that these aren't strings, but registry controlled  values. Should
> this
> > be an enum?
> 
> Ditto for this (see other comment).

While we could use net::HttpResponseInfo::ConnectionInfo for connectionInfo
there is no enum for ALPN in net/ i.e. at the source.
Adding something here is outside the scope of this change; although I could work
with net/ folks on a follow up CL if you think it necessary.

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:273: BLINK_PLATFORM_EXPORT
WebString connectionInfo() const;
On 2017/04/18 18:21:11, jochen wrote:
> On 2017/04/18 at 17:01:06, panicker wrote:
> > On 2017/04/18 11:37:58, jochen wrote:
> > > same here, is that a free-form value, or really some enum?
> > 
> > There's a lot of internal and deprecated values here that the net/ team is
> constantly evolving, so I'm not sure it makes sense to bake it into public
> interface:
> >
>
https://cs.chromium.org/chromium/src/net/http/http_response_info.h?type=cs&q=...
> 
> The interface is not really public - it used to be an interface to WebKit but
> now it's just historical baggage

I'm open to suggestions:
while we could propagate enum around, we cannot access net/http from Blink
(Webkit/Source/core) right?
Which means maintaining another version of
net::HttpResponseInfo::ConnectionInfoToString ?
https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?q=net::Ht...

[jochen (gone - plz use gerrit), 2017-04-19 15:01:25]

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:273: BLINK_PLATFORM_EXPORT
WebString connectionInfo() const;
platform/loader already depends on net/ and public/web also has a long list of
dependencies, so I'd propose to just add a DEPS entry to net/http and use that

[kinuko, 2017-04-20 03:37:08]

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2429063002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:273: BLINK_PLATFORM_EXPORT
WebString connectionInfo() const;
On 2017/04/19 15:01:25, jochen wrote:
> platform/loader already depends on net/ and public/web also has a long list of
> dependencies, so I'd propose to just add a DEPS entry to net/http and use that

Yes, it's fine to make platform/loader code depend on net/.

[panicker, 2017-04-20 17:52:04]

Hey all,
Its been a long time since this CL was initiated and sunjian@ has meanwhile
completed residency and moved over to Android!

As the comments are beyond nits, I'll take over this CL and resend it soon. Feel
free to ignore this review until then.
Thanks :)

[panicker, 2017-06-09 19:57:29]

On 2017/04/20 17:52:04, panicker wrote:
> Hey all,
> Its been a long time since this CL was initiated and sunjian@ has meanwhile
> completed residency and moved over to Android!
> 
> As the comments are beyond nits, I'll take over this CL and resend it soon.
Feel
> free to ignore this review until then.
> Thanks :)

Update: this change is now available in this patch:
https://codereview.chromium.org/2932003002/

[Bence, 2017-06-13 11:58:28]

bnc@chromium.org changed reviewers:
- bnc@chromium.org

[Bence, 2017-06-13 11:58:53]

Removing myself as reviewer.

[Ryan Hamilton, 2017-06-28 18:53:40]

rch@chromium.org changed reviewers:
- rch@chromium.org