SPDY - chrome:loadtimes() should display negotiated NPN protocol.

SPDY - chrome:loadtimes() should display negotiated NPN protocol.

BUG=108472
R=willchan
TEST=in js console chrome:loadtimes() should print SPDY 
protocol that was negotiated.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123857

[Ryan Sleevi, 2012-02-17 05:37:08]

http://codereview.chromium.org/9413038/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9413038/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:51: #include
"content/common/response_extra_data.h"
Do you still need this header for this file?

http://codereview.chromium.org/9413038/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9413038/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:3010: response.extraData());
Would this make more sense as a helper function on
content/common/ResponseExtraData - similar to DocumentState::FromDataSource()

http://codereview.chromium.org/9413038/diff/1/webkit/glue/resource_loader_bri...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/9413038/diff/1/webkit/glue/resource_loader_bri...
webkit/glue/resource_loader_bridge.h:36: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLResponse.h"
Is this still necessary?

http://codereview.chromium.org/9413038/diff/1/webkit/tools/test_shell/test_we...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/9413038/diff/1/webkit/tools/test_shell/test_we...
webkit/tools/test_shell/test_webview_delegate.cc:64: #include
"webkit/glue/weburlresponse_extradata_impl.h"
You don't seem to be using this class in this file

[ramant (doing other things), 2012-02-17 21:47:35]

Hi Ryan,
  Made all the changes you had suggested.

thanks,
raman

http://codereview.chromium.org/9413038/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9413038/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:51: #include
"content/common/response_extra_data.h"
On 2012/02/17 05:37:08, Ryan Sleevi wrote:
> Do you still need this header for this file?

Done.

http://codereview.chromium.org/9413038/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9413038/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:3010: response.extraData());
On 2012/02/17 05:37:08, Ryan Sleevi wrote:
> Would this make more sense as a helper function on
> content/common/ResponseExtraData - similar to DocumentState::FromDataSource()

Done.

http://codereview.chromium.org/9413038/diff/1/webkit/glue/resource_loader_bri...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/9413038/diff/1/webkit/glue/resource_loader_bri...
webkit/glue/resource_loader_bridge.h:36: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLResponse.h"
On 2012/02/17 05:37:08, Ryan Sleevi wrote:
> Is this still necessary?

Done.

http://codereview.chromium.org/9413038/diff/1/webkit/tools/test_shell/test_we...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/9413038/diff/1/webkit/tools/test_shell/test_we...
webkit/tools/test_shell/test_webview_delegate.cc:64: #include
"webkit/glue/weburlresponse_extradata_impl.h"
On 2012/02/17 05:37:08, Ryan Sleevi wrote:
> You don't seem to be using this class in this file

Done.

[Ryan Sleevi, 2012-02-17 22:10:24]

You'll need a content/ owners to stamp this (I'm guessing Darin), but this looks
right to me :)

http://codereview.chromium.org/9413038/diff/11003/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9413038/diff/11003/content/renderer/render_vie...
content/renderer/render_view_impl.cc:330: static WebURLResponseExtraDataImpl*
getExtraDataFromResponse(
While I understand this matches the style of getReferrerPolicyFromRequest, it
seems like both of these should match Chromium style, not WebKit style, as they
live in content/renderer

[ramant (doing other things), 2012-02-18 01:09:56]

+ darin

This patch relies on commit of webkit patch in issue
https://bugs.webkit.org/show_bug.cgi?id=77349. This patch uses ExtraData in
WebURLResponse.

The following is the original patch (which relied on WebKit API call changes).
https://chromiumcodereview.appspot.com/9315021/

Would appreciate a quick high level review of this patch.

thanks much,
raman

[ramant (doing other things), 2012-02-25 04:31:31]

Hi willchan and darin,
  Changes to send spdy_version via chrome::loadtimes(). Sorry for the delay.
Would appreciate your comments.

thanks,
raman

[darin (slow to review), 2012-02-27 16:50:30]

LGTM

How are we using chrome.loadTimes?  I'm a little sad to see it being used since
it is non-standard.

http://codereview.chromium.org/9413038/diff/26001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9413038/diff/26001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:324: static WebReferrerPolicy
GetReferrerPolicyFromRequest(
Thanks for fixing this.

http://codereview.chromium.org/9413038/diff/26001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3033:
document_state->set_npn_negotiated_protocol("");
nit: use std::string() instead.  std::string("") likely does a small heap
allocation.

[ramant (doing other things), 2012-02-27 18:11:36]

> How are we using chrome.loadTimes?
Satish (CSI team) is going to use to decide how many users are using SPDY/2,
SPDY/2.1 (and in future SPDY/3).

Many many thanks for reviewing my changes so quickly,
raman

http://codereview.chromium.org/9413038/diff/26001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9413038/diff/26001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3033:
document_state->set_npn_negotiated_protocol("");
On 2012/02/27 16:50:30, darin wrote:
> nit: use std::string() instead.  std::string("") likely does a small heap
> allocation.

Thanks very much for catching this (I am guilty).
Done.

[commit-bot: I haz the power, 2012-02-27 18:11:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/9413038/35001

[commit-bot: I haz the power, 2012-02-27 18:11:49]

Can't process patch for file webkit/glue/weburlresponse_extradata_impl.h.
Unsupported svn property format.

[commit-bot: I haz the power, 2012-02-27 18:38:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/9413038/41001

[commit-bot: I haz the power, 2012-02-27 18:38:51]

Presubmit check for 9413038-41001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/public/renderer/document_state.h

Presubmit checks took 1.8s to calculate.

[ramant (doing other things), 2012-02-27 18:44:30]

Hi jam,
  Would appreciate if you could look at changes to document_state.h
(content/public).

thanks much,
raman

[jam, 2012-02-27 21:48:26]

lgtm

[commit-bot: I haz the power, 2012-02-27 23:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/9413038/35038

[commit-bot: I haz the power, 2012-02-28 01:50:42]

Change committed as 123857