Add logging for ResourceScheduler events.

Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
  no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/37a0ddea749eae80bad9b2e10fd94bc3af8000a2
Cr-Commit-Position: refs/heads/master@{#441259}

[Randy Smith (Not in Mondays), 2016-12-15 23:31:55]

Description was changed from

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
==========

to

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Randy Smith (Not in Mondays), 2016-12-15 23:34:43]

Description was changed from

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
   no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Randy Smith (Not in Mondays), 2016-12-15 23:35:15]

Description was changed from

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
   no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
  no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Randy Smith (Not in Mondays), 2016-12-15 23:35:29]

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

[commit-bot: I haz the power, 2016-12-15 23:36:05]

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

[Randy Smith (Not in Mondays), 2016-12-15 23:38:08]

Charles, Matt: PTAL?  

Charles: content/browser/loader/resource_scheduler.cc,
net/log/net_log_event_type_list.h (and more generally, the value and depth of
the debugging).

Matt: net/url_request/*, chrome/browser/resources (and, more generally, the
changes I've made to URLRequest logging and the netlog system).

I wasn't sure what level of testing is required for a logging change, or where
that testing should go.  Though if the try jobs (currently active :-}) barf on
my url_request changes I'll have at least a partial answer :-}.

[Randy Smith (Not in Mondays), 2016-12-16 00:14:31]

On 2016/12/15 23:38:08, Randy Smith - Not in Mondays wrote:
> Charles, Matt: PTAL?  
> 
> Charles: content/browser/loader/resource_scheduler.cc,
> net/log/net_log_event_type_list.h (and more generally, the value and depth of
> the debugging).
> 
> Matt: net/url_request/*, chrome/browser/resources (and, more generally, the
> changes I've made to URLRequest logging and the netlog system).
> 
> I wasn't sure what level of testing is required for a logging change, or where
> that testing should go.  Though if the try jobs (currently active :-}) barf on
> my url_request changes I'll have at least a partial answer :-}.

I should have included an output example:

t=6995 [st=  0] +REQUEST_ALIVE  [dt=325]
                 --> priority = "LOWEST"
                 --> url =
"http://www.harvard.edu/sites/all/themes/hedu2015/assets/img/search-icon@2x.png"
t=6995 [st=  0]    RESOURCE_SCHEDULER_REQUEST_BLOCKED
t=6995 [st=  0]   +DELEGATE_INFO  [dt=308]
                   --> delegate_info = "ResourceScheduler"
t=7302 [st=307]      RESOURCE_SCHEDULER_REQUEST_STARTED
                     --> trigger = "Request completed (post-body tag)"
t=7303 [st=308]   -DELEGATE_INFO
t=7303 [st=308]    URL_REQUEST_DELEGATE  [dt=0]
t=7303 [st=308]   +URL_REQUEST_START_JOB  [dt=13]
                   --> load_flags = 33024 (MAYBE_USER_GESTURE | VERIFY_EV_CERT)
                   --> method = "GET"
                   --> url =
"http://www.harvard.edu/sites/all/themes/hedu2015/assets/img/search-icon@2x.png"
....

[mmenke, 2016-12-16 03:54:33]

On 2016/12/16 00:14:31, Randy Smith - Not in Mondays wrote:
> On 2016/12/15 23:38:08, Randy Smith - Not in Mondays wrote:
> > Charles, Matt: PTAL?  
> > 
> > Charles: content/browser/loader/resource_scheduler.cc,
> > net/log/net_log_event_type_list.h (and more generally, the value and depth
of
> > the debugging).
> > 
> > Matt: net/url_request/*, chrome/browser/resources (and, more generally, the
> > changes I've made to URLRequest logging and the netlog system).
> > 
> > I wasn't sure what level of testing is required for a logging change, or
where
> > that testing should go.  Though if the try jobs (currently active :-}) barf
on
> > my url_request changes I'll have at least a partial answer :-}.
> 
> I should have included an output example:
> 
> t=6995 [st=  0] +REQUEST_ALIVE  [dt=325]
>                  --> priority = "LOWEST"
>                  --> url =
>
"http://www.harvard.edu/sites/all/themes/hedu2015/assets/img/search-icon@2x.png"
> t=6995 [st=  0]    RESOURCE_SCHEDULER_REQUEST_BLOCKED
> t=6995 [st=  0]   +DELEGATE_INFO  [dt=308]
>                    --> delegate_info = "ResourceScheduler"
> t=7302 [st=307]      RESOURCE_SCHEDULER_REQUEST_STARTED
>                      --> trigger = "Request completed (post-body tag)"
> t=7303 [st=308]   -DELEGATE_INFO
> t=7303 [st=308]    URL_REQUEST_DELEGATE  [dt=0]
> t=7303 [st=308]   +URL_REQUEST_START_JOB  [dt=13]
>                    --> load_flags = 33024 (MAYBE_USER_GESTURE |
VERIFY_EV_CERT)
>                    --> method = "GET"
>                    --> url =
>
"http://www.harvard.edu/sites/all/themes/hedu2015/assets/img/search-icon@2x.png"
> ....

Haven't looked at the CL yet, but do we get anything from the
"RESOURCE_SCHEDULER_REQUEST_BLOCKED" events?  We're already implicitly loggic it
through the (perhaps not-so-wonderfully named) DELEGATE_INFO event.

[mmenke, 2016-12-16 03:54:55]

On 2016/12/16 03:54:33, mmenke (Out Dec 17 to Jan 2) wrote:
> On 2016/12/16 00:14:31, Randy Smith - Not in Mondays wrote:
> > On 2016/12/15 23:38:08, Randy Smith - Not in Mondays wrote:
> > > Charles, Matt: PTAL?  
> > > 
> > > Charles: content/browser/loader/resource_scheduler.cc,
> > > net/log/net_log_event_type_list.h (and more generally, the value and depth
> of
> > > the debugging).
> > > 
> > > Matt: net/url_request/*, chrome/browser/resources (and, more generally,
the
> > > changes I've made to URLRequest logging and the netlog system).
> > > 
> > > I wasn't sure what level of testing is required for a logging change, or
> where
> > > that testing should go.  Though if the try jobs (currently active :-})
barf
> on
> > > my url_request changes I'll have at least a partial answer :-}.
> > 
> > I should have included an output example:
> > 
> > t=6995 [st=  0] +REQUEST_ALIVE  [dt=325]
> >                  --> priority = "LOWEST"
> >                  --> url =
> >
>
"http://www.harvard.edu/sites/all/themes/hedu2015/assets/img/search-icon@2x.png"
> > t=6995 [st=  0]    RESOURCE_SCHEDULER_REQUEST_BLOCKED
> > t=6995 [st=  0]   +DELEGATE_INFO  [dt=308]
> >                    --> delegate_info = "ResourceScheduler"
> > t=7302 [st=307]      RESOURCE_SCHEDULER_REQUEST_STARTED
> >                      --> trigger = "Request completed (post-body tag)"
> > t=7303 [st=308]   -DELEGATE_INFO
> > t=7303 [st=308]    URL_REQUEST_DELEGATE  [dt=0]
> > t=7303 [st=308]   +URL_REQUEST_START_JOB  [dt=13]
> >                    --> load_flags = 33024 (MAYBE_USER_GESTURE |
> VERIFY_EV_CERT)
> >                    --> method = "GET"
> >                    --> url =
> >
>
"http://www.harvard.edu/sites/all/themes/hedu2015/assets/img/search-icon@2x.png"
> > ....
> 
> Haven't looked at the CL yet, but do we get anything from the
> "RESOURCE_SCHEDULER_REQUEST_BLOCKED" events?  We're already implicitly loggic
it
> through the (perhaps not-so-wonderfully named) DELEGATE_INFO event.

loggic=logging it, rather

[commit-bot: I haz the power, 2016-12-16 04:00:40]

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

[commit-bot: I haz the power, 2016-12-16 04:00:41]

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

[mmenke, 2016-12-16 16:46:46]

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:78: return "HTTPS proxy detected";
Note that we support non-SPDY HTTPS proxies, too.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:80: return "Request reprioritized";
Optional nit:  I suggest just matching the enum strings here.  net-internals is
meant to be used for debugging Chrome, and strings you can search the code for
are more useful than strings you search the code for to find the real string you
want to search for.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:134: using ScopedTrigger =
ScopedSetter<RequestStartTrigger>;
This seems like over-engineering to me, and I'm not a fan of this general
pattern (Despite adding something somewhat similar in two of my ResourceLoader
CLs), but I'll defer to Charles on it.

[Randy Smith (Not in Mondays), 2016-12-19 21:44:02]

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

[commit-bot: I haz the power, 2016-12-19 21:44:47]

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

[Randy Smith (Not in Mondays), 2016-12-19 22:37:09]

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

[commit-bot: I haz the power, 2016-12-19 22:37: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, 2016-12-19 23:19:16]

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

[commit-bot: I haz the power, 2016-12-19 23:19:16]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Randy Smith (Not in Mondays), 2016-12-20 15:11:12]

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

[commit-bot: I haz the power, 2016-12-20 15:11:26]

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-12-20 16:51:22]

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

[commit-bot: I haz the power, 2016-12-20 16:51:23]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-12-20 16:54:17]

Comments incorporated; PTAL?

I didn't like the fact that in the net-internals output the "delegate_info"
event didn't suggest that what the delegate was doing was blocking the request,
so I changed that.  Happy to change it back if that's not wanted.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:78: return "HTTPS proxy detected";
On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> Note that we support non-SPDY HTTPS proxies, too.

I think this means you think I should change the string "HTTPS proxy detected"
to "SPDY proxy detected"?  (I.e. I'm not sure under what circumstances we get
the OnReceivedSpdyProxiedHttpResponse() notification--the name suggests it's
only for the SPDY case, but I don't understand why we wouldn't shift over to not
throttling mode for any HTTPS proxy.)

Moot since I'm going to go with your suggestion below, but I'm still curious.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:80: return "Request reprioritized";
On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> Optional nit:  I suggest just matching the enum strings here.  net-internals
is
> meant to be used for debugging Chrome, and strings you can search the code for
> are more useful than strings you search the code for to find the real string
you
> want to search for.

Done.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:134: using ScopedTrigger =
ScopedSetter<RequestStartTrigger>;
On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> This seems like over-engineering to me, and I'm not a fan of this general
> pattern (Despite adding something somewhat similar in two of my ResourceLoader
> CLs), but I'll defer to Charles on it.

I dunno if this helps, but I finally tracked down my memory of the equivalent
functionality from base:: and replaced my gasket with that.   Willing to say
something about why you don't like the pattern?  I hate having to remember
(either in coding or maintenance) to reset variables back to earlier values on
exit from functions.

[mmenke, 2017-01-03 20:32:29]

Going to defer to Charlie for the rest of the review because I'm
laz...erm...because he's more knowledgeable about/more interested in the request
ordering/perf stuff than I am.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:78: return "HTTPS proxy detected";
On 2016/12/20 16:54:17, Randy Smith - Not in Mondays wrote:
> On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> > Note that we support non-SPDY HTTPS proxies, too.
> 
> I think this means you think I should change the string "HTTPS proxy detected"
> to "SPDY proxy detected"?  (I.e. I'm not sure under what circumstances we get
> the OnReceivedSpdyProxiedHttpResponse() notification--the name suggests it's
> only for the SPDY case, but I don't understand why we wouldn't shift over to
not
> throttling mode for any HTTPS proxy.)
> 
> Moot since I'm going to go with your suggestion below, but I'm still curious.

Yea, I was thinking SPDY proxy detected (Or H2).

Why would we shift over to not throttling for an HTTP/1.x over HTTPS proxy?  The
reason we don't throttle for H2 proxies is that they have prioritization
themselves, and we defer to that instead of relying on holding back requests to
properly order responses.  HTTPS/1.x proxies don't have that capability, they're
exactly like HTTP/1.x proxies, except they have extra round trips to negotiate a
connection (And traffic to them is encrypted, of course).

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:134: using ScopedTrigger =
ScopedSetter<RequestStartTrigger>;
On 2016/12/20 16:54:17, Randy Smith - Not in Mondays wrote:
> On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> > This seems like over-engineering to me, and I'm not a fan of this general
> > pattern (Despite adding something somewhat similar in two of my
ResourceLoader
> > CLs), but I'll defer to Charles on it.
> 
> I dunno if this helps, but I finally tracked down my memory of the equivalent
> functionality from base:: and replaced my gasket with that.   Willing to say
> something about why you don't like the pattern?  I hate having to remember
> (either in coding or maintenance) to reset variables back to earlier values on
> exit from functions.

I don't think functions should know or care exactly what's happening above them
on the stack.  I think functions having that sort of information breaks the
contract "This function does X".

[Charlie Harrison, 2017-01-03 21:22:49]

LGTM, though I'll have another look if you decide to remove the AutoReset.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:134: using ScopedTrigger =
ScopedSetter<RequestStartTrigger>;
On 2017/01/03 20:32:29, mmenke wrote:
> On 2016/12/20 16:54:17, Randy Smith - Not in Mondays wrote:
> > On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> > > This seems like over-engineering to me, and I'm not a fan of this general
> > > pattern (Despite adding something somewhat similar in two of my
> ResourceLoader
> > > CLs), but I'll defer to Charles on it.
> > 
> > I dunno if this helps, but I finally tracked down my memory of the
equivalent
> > functionality from base:: and replaced my gasket with that.   Willing to say
> > something about why you don't like the pattern?  I hate having to remember
> > (either in coding or maintenance) to reset variables back to earlier values
on
> > exit from functions.
> 
> I don't think functions should know or care exactly what's happening above
them
> on the stack.  I think functions having that sort of information breaks the
> contract "This function does X".

mmenke, I'm guessing you would prefer threading the value down through to each
call to StartRequest? That would be okay with me but I don't have strong
feelings here.

[mmenke, 2017-01-03 21:24:44]

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:134: using ScopedTrigger =
ScopedSetter<RequestStartTrigger>;
On 2017/01/03 21:22:49, Charlie Harrison wrote:
> On 2017/01/03 20:32:29, mmenke wrote:
> > On 2016/12/20 16:54:17, Randy Smith - Not in Mondays wrote:
> > > On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> > > > This seems like over-engineering to me, and I'm not a fan of this
general
> > > > pattern (Despite adding something somewhat similar in two of my
> > ResourceLoader
> > > > CLs), but I'll defer to Charles on it.
> > > 
> > > I dunno if this helps, but I finally tracked down my memory of the
> equivalent
> > > functionality from base:: and replaced my gasket with that.   Willing to
say
> > > something about why you don't like the pattern?  I hate having to remember
> > > (either in coding or maintenance) to reset variables back to earlier
values
> on
> > > exit from functions.
> > 
> > I don't think functions should know or care exactly what's happening above
> them
> > on the stack.  I think functions having that sort of information breaks the
> > contract "This function does X".
> 
> mmenke, I'm guessing you would prefer threading the value down through to each
> call to StartRequest? That would be okay with me but I don't have strong
> feelings here.

Yes, that's what I'd do (Though, like I said, certainly not going to insist on
it, or even passive-aggressively whine about it whenever someone touches this
code).

[Randy Smith (Not in Mondays), 2017-01-03 22:19:29]

Object in future was smaller than it appeared :-}.  Charles, PTAL?

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:78: return "HTTPS proxy detected";
On 2017/01/03 20:32:29, mmenke wrote:
> On 2016/12/20 16:54:17, Randy Smith - Not in Mondays wrote:
> > On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> > > Note that we support non-SPDY HTTPS proxies, too.
> > 
> > I think this means you think I should change the string "HTTPS proxy
detected"
> > to "SPDY proxy detected"?  (I.e. I'm not sure under what circumstances we
get
> > the OnReceivedSpdyProxiedHttpResponse() notification--the name suggests it's
> > only for the SPDY case, but I don't understand why we wouldn't shift over to
> not
> > throttling mode for any HTTPS proxy.)
> > 
> > Moot since I'm going to go with your suggestion below, but I'm still
curious.
> 
> Yea, I was thinking SPDY proxy detected (Or H2).
> 
> Why would we shift over to not throttling for an HTTP/1.x over HTTPS proxy? 
The
> reason we don't throttle for H2 proxies is that they have prioritization
> themselves, and we defer to that instead of relying on holding back requests
to
> properly order responses.  HTTPS/1.x proxies don't have that capability,
they're
> exactly like HTTP/1.x proxies, except they have extra round trips to negotiate
a
> connection (And traffic to them is encrypted, of course).

Arggh, I was typing "HTTPS" and meaning "H2".  Gotcha; thanks for the
explanation.

https://codereview.chromium.org/2579933002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_scheduler.cc:134: using ScopedTrigger =
ScopedSetter<RequestStartTrigger>;
On 2017/01/03 21:24:44, mmenke wrote:
> On 2017/01/03 21:22:49, Charlie Harrison wrote:
> > On 2017/01/03 20:32:29, mmenke wrote:
> > > On 2016/12/20 16:54:17, Randy Smith - Not in Mondays wrote:
> > > > On 2016/12/16 16:46:46, mmenke (Out Dec 17 to Jan 2) wrote:
> > > > > This seems like over-engineering to me, and I'm not a fan of this
> general
> > > > > pattern (Despite adding something somewhat similar in two of my
> > > ResourceLoader
> > > > > CLs), but I'll defer to Charles on it.
> > > > 
> > > > I dunno if this helps, but I finally tracked down my memory of the
> > equivalent
> > > > functionality from base:: and replaced my gasket with that.   Willing to
> say
> > > > something about why you don't like the pattern?  I hate having to
remember
> > > > (either in coding or maintenance) to reset variables back to earlier
> values
> > on
> > > > exit from functions.
> > > 
> > > I don't think functions should know or care exactly what's happening above
> > them
> > > on the stack.  I think functions having that sort of information breaks
the
> > > contract "This function does X".
> > 
> > mmenke, I'm guessing you would prefer threading the value down through to
each
> > call to StartRequest? That would be okay with me but I don't have strong
> > feelings here.
> 
> Yes, that's what I'd do (Though, like I said, certainly not going to insist on
> it, or even passive-aggressively whine about it whenever someone touches this
> code).

I hear and sympathize with you both, but I think threading the values down would
be a lot of plumbing.  I'll take a swing at it before attempting to whine the
problem away, though.

ETA: As usual, if you actually look closely at a problem, it's not as big as you
think.  Done.

[Randy Smith (Not in Mondays), 2017-01-03 22:20:17]

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

[commit-bot: I haz the power, 2017-01-03 22:20:55]

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

[mmenke, 2017-01-03 22:24:18]

It's...it's the most beauteous CL I've ever seen.  *sniff*.  :)

LGTM

https://codereview.chromium.org/2579933002/diff/100001/content/browser/loader...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/100001/content/browser/loader...
content/browser/loader/resource_scheduler.cc:601: if (start_mode == START_ASYNC)
{
optional:  Would it be better to check if trigger is RequestStartTrigger::NONE?

[Charlie Harrison, 2017-01-03 22:26:47]

still LGTM yeah that wasn't so bad

https://codereview.chromium.org/2579933002/diff/100001/content/browser/loader...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/100001/content/browser/loader...
content/browser/loader/resource_scheduler.cc:601: if (start_mode == START_ASYNC)
{
On 2017/01/03 22:24:18, mmenke wrote:
> optional:  Would it be better to check if trigger is
RequestStartTrigger::NONE?

Whichever, DCHECKing that start_mode == START_ASYNC implies RequestStartTrigger
!= NONE would be nice.

[Randy Smith (Not in Mondays), 2017-01-03 22:36:38]

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

[commit-bot: I haz the power, 2017-01-03 22:37:06]

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

[Randy Smith (Not in Mondays), 2017-01-03 22:37:09]

Thanks for the reviews!

https://codereview.chromium.org/2579933002/diff/100001/content/browser/loader...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/2579933002/diff/100001/content/browser/loader...
content/browser/loader/resource_scheduler.cc:601: if (start_mode == START_ASYNC)
{
On 2017/01/03 22:26:47, Charlie Harrison wrote:
> On 2017/01/03 22:24:18, mmenke wrote:
> > optional:  Would it be better to check if trigger is
> RequestStartTrigger::NONE?
> 
> Whichever, DCHECKing that start_mode == START_ASYNC implies
RequestStartTrigger
> != NONE would be nice.

I'd prefer this check, since just conditionalizing on !NONE makes it easier to
dodge doing logging in hypothetical future triggers for starting requests.  We
want to make sure we log a trigger for all async started requests.  Done.

[commit-bot: I haz the power, 2017-01-03 23:44:03]

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

[commit-bot: I haz the power, 2017-01-03 23:44:04]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-01-04 00:06:58]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2017-01-04 00:06:59]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2579933002/#ps120001
(title: "DCHECK to make sure that all async starts have defined triggers.")

[commit-bot: I haz the power, 2017-01-04 00:07:21]

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-01-04 00:12:11]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483488418723450,
"parent_rev": "9220fd29b3ef1c80eaf6a99a7b69457208413821", "commit_rev":
"244609779da83d3ff6952bee04a2bb04baef8640"}

[commit-bot: I haz the power, 2017-01-04 00:13:08]

Description was changed from

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
  no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
  no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2579933002
==========

[commit-bot: I haz the power, 2017-01-04 00:13:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2017-01-04 00:16:49]

Description was changed from

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
  no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2579933002
==========

to

==========
Add logging for ResourceScheduler events.

Specifically, log the ResourceScheduler throttling a request, and starting
a throttled requests.

This CL  also includes modifying some aspects of URLRequest logging:
* Log priority and url at URLRequest creation time (so if the ResourceScheduler
  blocks it, there will be some identifier for the request).
* Remove priority from URLRequest::StartJob logging (redundant with logging
  in the constructor and SetPriority method).
* Always log SetPriority (it's relevant to the ResourceScheduler even if there's
  no job attached to the URLRequest).

BUG=None
R=mmenke@chromium.org
R=csharrison@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/37a0ddea749eae80bad9b2e10fd94bc3af8000a2
Cr-Commit-Position: refs/heads/master@{#441259}
==========

[commit-bot: I haz the power, 2017-01-04 00:16:50]

Patchset 7 (id:??) landed as
https://crrev.com/37a0ddea749eae80bad9b2e10fd94bc3af8000a2
Cr-Commit-Position: refs/heads/master@{#441259}