Add the UMA PLT.BeginToFinish{,Doc}_AfterPreconnectRequest

Add the UMA PLT.BeginToFinish{,Doc}_AfterPreconnectRequest to measure PLT just after mouseevent preconnect triggers.

[Note to sheriff] This patch requires blink patch http://crrev.com/18598003 to be landed first.

This UMA aims to measure PLT on link navigations with {mouse,gesture}-event preconnects. The PLT is to be compared between experiment/control groups using Finch.

BUG=235361
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214313

[kouhei (in TOK), 2013-07-03 03:23:55]

First draft, but working.
This requires blink side patch to be landed first.

[kouhei (in TOK), 2013-07-11 09:16:23]

LGTM on Blink side. The patch should be ready for review now.

jam: Would you take a look?

[jam, 2013-07-11 16:23:52]

https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
File webkit/glue/weburlrequest_extradata_impl.h (right):

https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
webkit/glue/weburlrequest_extradata_impl.h:37: int type_id() const { return
type_id_; }
hmm, seems best to avoid this hand rolled RTTI

[kouhei (in TOK), 2013-07-12 01:06:11]

On 2013/07/11 16:23:52, jam wrote:
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> File webkit/glue/weburlrequest_extradata_impl.h (right):
> 
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> webkit/glue/weburlrequest_extradata_impl.h:37: int type_id() const { return
> type_id_; }
> hmm, seems best to avoid this hand rolled RTTI

Yes. I would like to avoid this, but didn't have a good idea.

I would like to use the flag was_after_preconnect_request() in willSendRequest,
but the problem is that currently there is no way to tell if ExtraData* is
actually content::RequestExtraData with the flag, or
webkit_glue::WebURLRequestExtradataImpl without the flag. An option would be to
move the flag to WebURLRequestExtraDataImpl, but the flag is never used outside
content/.

[kouhei (in TOK), 2013-07-16 06:10:07]

On 2013/07/12 01:06:11, kouhei wrote:
> On 2013/07/11 16:23:52, jam wrote:
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > File webkit/glue/weburlrequest_extradata_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > webkit/glue/weburlrequest_extradata_impl.h:37: int type_id() const { return
> > type_id_; }
> > hmm, seems best to avoid this hand rolled RTTI
> 
> Yes. I would like to avoid this, but didn't have a good idea.
> 
> I would like to use the flag was_after_preconnect_request() in
willSendRequest,
> but the problem is that currently there is no way to tell if ExtraData* is
> actually content::RequestExtraData with the flag, or
> webkit_glue::WebURLRequestExtradataImpl without the flag. An option would be
to
> move the flag to WebURLRequestExtraDataImpl, but the flag is never used
outside
> content/.

jam: Hi. Do you have any good idea for avoiding this RTTI? Or, can I just add
the flag in WebURLRequestExtraDataImpl?

[jam, 2013-07-17 00:37:57]

On 2013/07/16 06:10:07, kouhei wrote:
> On 2013/07/12 01:06:11, kouhei wrote:
> > On 2013/07/11 16:23:52, jam wrote:
> > >
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > > File webkit/glue/weburlrequest_extradata_impl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > > webkit/glue/weburlrequest_extradata_impl.h:37: int type_id() const {
return
> > > type_id_; }
> > > hmm, seems best to avoid this hand rolled RTTI
> > 
> > Yes. I would like to avoid this, but didn't have a good idea.
> > 
> > I would like to use the flag was_after_preconnect_request() in
> willSendRequest,
> > but the problem is that currently there is no way to tell if ExtraData* is
> > actually content::RequestExtraData with the flag, or
> > webkit_glue::WebURLRequestExtradataImpl without the flag. An option would be
> to
> > move the flag to WebURLRequestExtraDataImpl, but the flag is never used
> outside
> > content/.
> 
> jam: Hi. Do you have any good idea for avoiding this RTTI? Or, can I just add
> the flag in WebURLRequestExtraDataImpl?

Under what conditions is there a WebURLRequestExtraDataImpl which doesn't derive
from content::RequestExtraData and which gets passed to RenderViewImpl?

[kouhei (in TOK), 2013-07-17 01:11:27]

On 2013/07/17 00:37:57, jam wrote:
> On 2013/07/16 06:10:07, kouhei wrote:
> > On 2013/07/12 01:06:11, kouhei wrote:
> > > On 2013/07/11 16:23:52, jam wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > > > File webkit/glue/weburlrequest_extradata_impl.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > > > webkit/glue/weburlrequest_extradata_impl.h:37: int type_id() const {
> return
> > > > type_id_; }
> > > > hmm, seems best to avoid this hand rolled RTTI
> > > 
> > > Yes. I would like to avoid this, but didn't have a good idea.
> > > 
> > > I would like to use the flag was_after_preconnect_request() in
> > willSendRequest,
> > > but the problem is that currently there is no way to tell if ExtraData* is
> > > actually content::RequestExtraData with the flag, or
> > > webkit_glue::WebURLRequestExtradataImpl without the flag. An option would
be
> > to
> > > move the flag to WebURLRequestExtraDataImpl, but the flag is never used
> > outside
> > > content/.
> > 
> > jam: Hi. Do you have any good idea for avoiding this RTTI? Or, can I just
add
> > the flag in WebURLRequestExtraDataImpl?
> 
> Under what conditions is there a WebURLRequestExtraDataImpl which doesn't
derive
> from content::RequestExtraData and which gets passed to RenderViewImpl?

The class content::RequestExtraData is derived from
webkit_glue::WebURLRequestExtraDataImpl. The base class
webkit_glue::WebURLRequestExtraDataImpl may be used via ppapi before the
WebURLRequest reaches content::RenderViewImpl. In content/, we attach more
content/ specific data to WebURLRequest, so we switch to class RequestExtraData.
Currently, the switch is only done in willSendRequest, so RTTI is not used.

However, I would like to switch to content::RequestExtraData in callback
willRequestAfterPreconnect to attach new flag "m_afterPreconnectRequest". Then,
willSendRequest can no longer assume the extradata has only
webkit_glue::WebURLRequestExtraDataImpl data. I introduced typeid in the CL and
I agree this is not clean.

Another way I was thinking of is adding the flag m_afterPreconnectRequest in
webkit_glue::WebURLRequestExtraDataImpl and do not switch to
content::RequestExtraData in callback willRequestAfterPreconnect. This way, we
don't need RTTI, but it may be layer violation to add a flag to webkit_glue
which is only used in content/.

[jam, 2013-07-22 23:53:28]

On 2013/07/17 01:11:27, kouhei wrote:
> On 2013/07/17 00:37:57, jam wrote:
> > On 2013/07/16 06:10:07, kouhei wrote:
> > > On 2013/07/12 01:06:11, kouhei wrote:
> > > > On 2013/07/11 16:23:52, jam wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > > > > File webkit/glue/weburlrequest_extradata_impl.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18601004/diff/14001/webkit/glue/weburlrequest...
> > > > > webkit/glue/weburlrequest_extradata_impl.h:37: int type_id() const {
> > return
> > > > > type_id_; }
> > > > > hmm, seems best to avoid this hand rolled RTTI
> > > > 
> > > > Yes. I would like to avoid this, but didn't have a good idea.
> > > > 
> > > > I would like to use the flag was_after_preconnect_request() in
> > > willSendRequest,
> > > > but the problem is that currently there is no way to tell if ExtraData*
is
> > > > actually content::RequestExtraData with the flag, or
> > > > webkit_glue::WebURLRequestExtradataImpl without the flag. An option
would
> be
> > > to
> > > > move the flag to WebURLRequestExtraDataImpl, but the flag is never used
> > > outside
> > > > content/.
> > > 
> > > jam: Hi. Do you have any good idea for avoiding this RTTI? Or, can I just
> add
> > > the flag in WebURLRequestExtraDataImpl?
> > 
> > Under what conditions is there a WebURLRequestExtraDataImpl which doesn't
> derive
> > from content::RequestExtraData and which gets passed to RenderViewImpl?
> 
> The class content::RequestExtraData is derived from
> webkit_glue::WebURLRequestExtraDataImpl. The base class
> webkit_glue::WebURLRequestExtraDataImpl may be used via ppapi before the
> WebURLRequest reaches content::RenderViewImpl. In content/, we attach more
> content/ specific data to WebURLRequest, so we switch to class
RequestExtraData.
> Currently, the switch is only done in willSendRequest, so RTTI is not used.
> 
> However, I would like to switch to content::RequestExtraData in callback
> willRequestAfterPreconnect to attach new flag "m_afterPreconnectRequest".
Then,
> willSendRequest can no longer assume the extradata has only
> webkit_glue::WebURLRequestExtraDataImpl data. I introduced typeid in the CL
and
> I agree this is not clean.
> 
> Another way I was thinking of is adding the flag m_afterPreconnectRequest in
> webkit_glue::WebURLRequestExtraDataImpl and do not switch to
> content::RequestExtraData in callback willRequestAfterPreconnect. This way, we
> don't need RTTI, but it may be layer violation to add a flag to webkit_glue
> which is only used in content/.

all the files in src/webkit are going to be moving to src/content, now that
layout tests run in content_shell. so feel free to add content-specific flags to
that struct.

[kouhei (in TOK), 2013-07-23 02:23:09]

Updated patch.

jam: Would you take a look?

> all the files in src/webkit are going to be moving to src/content, now that
> layout tests run in content_shell. so feel free to add content-specific flags
to
> that struct.

Thanks for the explanation. I added the flag to
webkit_glue::WebURLRequestExtraDataImpl and removed the RTTI.

[jam, 2013-07-26 04:59:39]

lgtm, sorry for the delay I missed your message. In the future feel free to
email or IM me if I take more than half a day.

https://codereview.chromium.org/18601004/diff/35001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18601004/diff/35001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3995: if(request.extraData()) {
nit: "if ("

https://codereview.chromium.org/18601004/diff/35001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4098: if(request.targetType() ==
WebURLRequest::TargetIsPrefetch)
ditto and also below

[kouhei (in TOK), 2013-07-26 10:50:02]

Thanks for review!

https://codereview.chromium.org/18601004/diff/35001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18601004/diff/35001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3995: if(request.extraData()) {
On 2013/07/26 04:59:39, jam wrote:
> nit: "if ("

Done.

https://codereview.chromium.org/18601004/diff/35001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4098: if(request.targetType() ==
WebURLRequest::TargetIsPrefetch)
On 2013/07/26 04:59:39, jam wrote:
> ditto and also below

Done.

[commit-bot: I haz the power, 2013-07-26 10:56:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/18601004/40001

[commit-bot: I haz the power, 2013-07-26 10:56:43]

Failed to apply patch for content/renderer/render_view_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/render_view_impl.cc
  Hunk #1 succeeded at 3870 with fuzz 1 (offset -117 lines).
  Hunk #2 FAILED at 4060.
  Hunk #3 FAILED at 4077.
  Hunk #4 FAILED at 4088.
  3 out of 4 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.cc.rej

Patch:       content/renderer/render_view_impl.cc
Index: content/renderer/render_view_impl.cc
diff --git a/content/renderer/render_view_impl.cc
b/content/renderer/render_view_impl.cc
index
9cf33c0fae0ef84d7c4d81dea6cba0e09f9b2e1c..00c4341b55ed18b79b6ea08d145c8bf8f0102602
100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -3987,6 +3987,29 @@ void RenderViewImpl::assignIdentifierToRequest(
   // Ignore
 }
 
+void RenderViewImpl::willRequestAfterPreconnect(WebFrame* frame,
+                                                WebURLRequest& request) {
+  WebKit::WebReferrerPolicy referrer_policy = WebKit::WebReferrerPolicyDefault;
+  WebString custom_user_agent;
+
+  if (request.extraData()) {
+    // This will only be called before willSendRequest, so only ExtraData
+    // members we have to copy here is on WebURLRequestExtraDataImpl.
+    webkit_glue::WebURLRequestExtraDataImpl* old_extra_data =
+        static_cast<webkit_glue::WebURLRequestExtraDataImpl*>(
+            request.extraData());
+
+    referrer_policy = old_extra_data->referrer_policy();
+    custom_user_agent = old_extra_data->custom_user_agent();
+  }
+
+  bool was_after_preconnect_request = true;
+  // The args after |was_after_preconnect_request| are not used, and set to
+  // correct values at |willSendRequest|.
+  request.setExtraData(new webkit_glue::WebURLRequestExtraDataImpl(
+      referrer_policy, custom_user_agent, was_after_preconnect_request));
+}
+
 void RenderViewImpl::willSendRequest(WebFrame* frame,
                                      unsigned identifier,
                                      WebURLRequest& request,
@@ -4037,11 +4060,14 @@ void RenderViewImpl::willSendRequest(WebFrame* frame,
   // agent. This needs to be done here, after WebKit is through with setting
the
   // user agent on its own.
   WebString custom_user_agent;
+  bool was_after_preconnect_request = false;
   if (request.extraData()) {
     webkit_glue::WebURLRequestExtraDataImpl* old_extra_data =
         static_cast<webkit_glue::WebURLRequestExtraDataImpl*>(
             request.extraData());
     custom_user_agent = old_extra_data->custom_user_agent();
+    was_after_preconnect_request =
+        old_extra_data->was_after_preconnect_request();
 
     if (!custom_user_agent.isNull()) {
       if (custom_user_agent.isEmpty())
@@ -4054,6 +4080,7 @@ void RenderViewImpl::willSendRequest(WebFrame* frame,
   request.setExtraData(
       new RequestExtraData(referrer_policy,
                            custom_user_agent,
+                           was_after_preconnect_request,
                            (frame == top_frame),
                            frame->identifier(),
                            frame->parent() == top_frame,
@@ -4065,11 +4092,15 @@ void RenderViewImpl::willSendRequest(WebFrame* frame,
 
   DocumentState* top_document_state =
       DocumentState::FromDataSource(top_data_source);
-  // TODO(gavinp): separate out prefetching and prerender field trials
-  // if the rel=prerender rel type is sticking around.
-  if (top_document_state &&
-      request.targetType() == WebURLRequest::TargetIsPrefetch)
-    top_document_state->set_was_prefetcher(true);
+  if (top_document_state) {
+    // TODO(gavinp): separate out prefetching and prerender field trials
+    // if the rel=prerender rel type is sticking around.
+    if (request.targetType() == WebURLRequest::TargetIsPrefetch)
+      top_document_state->set_was_prefetcher(true);
+
+    if (was_after_preconnect_request)
+      top_document_state->set_was_after_preconnect_request(true);
+  }
 
   request.setRequestorID(routing_id_);
  
request.setHasUserGesture(WebUserGestureIndicator::isProcessingUserGesture());

[kouhei (in TOK), 2013-07-29 00:38:36]

kinuko: Would you take a look at webkit/child/** and
content/renderer/pepper/url_request_info_util.cc?

[kinuko, 2013-07-29 02:24:01]

On 2013/07/29 00:38:36, kouhei wrote:
> kinuko: Would you take a look at webkit/child/** and
> content/renderer/pepper/url_request_info_util.cc?

webkit/child changes lgtm

[kouhei (in TOK), 2013-07-29 02:26:30]

On 2013/07/29 02:24:01, kinuko wrote:
> On 2013/07/29 00:38:36, kouhei wrote:
> > kinuko: Would you take a look at webkit/child/** and
> > content/renderer/pepper/url_request_info_util.cc?
> 
> webkit/child changes lgtm

Thanks for review!

[commit-bot: I haz the power, 2013-07-29 02:26:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/18601004/50001

[commit-bot: I haz the power, 2013-07-29 04:06:54]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-29 04:19:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/18601004/50001

[commit-bot: I haz the power, 2013-07-29 05:53:59]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-29 07:45:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/18601004/50001

[commit-bot: I haz the power, 2013-07-29 09:31:56]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-29 14:11:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/18601004/50001

[commit-bot: I haz the power, 2013-07-29 17:15:28]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-30 00:16:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/18601004/50001

[commit-bot: I haz the power, 2013-07-30 07:16:57]

Change committed as 214313

[jochen (gone - plz use gerrit), 2013-12-20 09:02:24]

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:496: WebKit::WebReferrerPolicy
referrer_policy = WebKit::WebReferrerPolicyDefault;
this should be frame->document().referrerPolicy();

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:497: WebString custom_user_agent;
this will never be a non-empty string, so we always end up clearing the user
agent in willSendRequest()

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:500: // This will only be called before
willSendRequest, so only ExtraData
only willSendRequest() ever sets extra data, so this branch is dead code.

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:514: referrer_policy, custom_user_agent,
was_after_preconnect_request));
the request is inspected in RenderFrameImpl::decidePolicyForNavigation which is
invoked before willSendRequest().

In RenderViewImpl::GetReferrerPolicyFromRequest() we now end up incorrectly
using the default policy.

[kouhei (in TOK), 2013-12-20 09:33:34]

Thanks for pointing this out.

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:500: // This will only be called before
willSendRequest, so only ExtraData
On 2013/12/20 09:02:25, jochen wrote:
> only willSendRequest() ever sets extra data, so this branch is dead code.
 
Pepper also sets this? custom_user_agent only seems to be set via extraData().
My intention here was to not erase that info here.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[kouhei (in TOK), 2013-12-20 09:50:07]

On 2013/12/20 09:33:34, kouhei wrote:
> Thanks for pointing this out.
> 
>
https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/18601004/diff/50001/content/renderer/render_f...
> content/renderer/render_frame_impl.cc:500: // This will only be called before
> willSendRequest, so only ExtraData
> On 2013/12/20 09:02:25, jochen wrote:
> > only willSendRequest() ever sets extra data, so this branch is dead code.
>  
> Pepper also sets this? custom_user_agent only seems to be set via extraData().
> My intention here was to not erase that info here.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

Discussed. Pepper would never willRequestAfterPreconnect, so this would be fine.