Add load flag indicating a request is probably the result of a user gesture.

Add load flag indicating a request is probably the result of a user gesture.

This gets set by ResourceDispatcherHost on URLRequest objects created
soon after a user gesture is detected.

Bypass anti-DDoS throttling on this load flag, with option to bypass
on other load flags in the future (e.g. possibly LOAD_BYPASS_CACHE).

BUG=83775
TEST=net_unittests. Also confirmed manually that the LOAD_MAYBE_USER_GESTURE load flag gets applied appropriately.

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

[Jói, 2011-08-05 13:07:22]

yzshen: Main reviewer.
jam: content/OWNERS
eroman: net/OWNERS

Cheers,
Jói

[jam, 2011-08-05 17:03:38]

content lgtm

[yzshen1, 2011-08-05 21:50:41]

LGTM

Thanks!

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

http://codereview.chromium.org/7582004/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:120: //
NET_MAYBE_USER_GESTURE load flag.
1) NET_MAYBE_USER_GESTURE -> LOAD_MAYBE_USER_GESTURE, please.
2) [just out of curiosity] how is the number 3500 decided?

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
net/url_request/url_request_throttler_entry.cc:451: return (load_flags &
(LOAD_MAYBE_USER_GESTURE)) != 0;
nit: the inner parentheses seem unnecessary.

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
net/url_request/url_request_throttler_entry_interface.h:28: //
net/base/load_flags.h) that mean it is definitely user-initiated.
nit: now that the flag is 'MAYBE', 'definitely' may not be accurate here.

[Jói, 2011-08-08 17:09:45]

Thanks John and Yuzhu.

Yuzhu, I've addressed your comments and uploaded a new version.

Cheers,
Jói

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

http://codereview.chromium.org/7582004/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:120: //
NET_MAYBE_USER_GESTURE load flag.
On 2011/08/05 21:50:41, yzshen1 wrote:
> 1) NET_MAYBE_USER_GESTURE -> LOAD_MAYBE_USER_GESTURE, please.
> 2) [just out of curiosity] how is the number 3500 decided?

1) Done.

2) See updated comment for this constant:

  118 // The number of milliseconds after noting a user gesture that we will
  119 // tag newly-created URLRequest objects with the
  120 // net::LOAD_MAYBE_USER_GESTURE load flag. This is a fairly arbitrary
  121 // guess at how long to expect direct impact from a user gesture, but
  122 // this should be OK as the load flag is a best-effort thing only,
  123 // rather than being intended as fully accurate.

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
net/url_request/url_request_throttler_entry.cc:451: return (load_flags &
(LOAD_MAYBE_USER_GESTURE)) != 0;
On 2011/08/05 21:50:41, yzshen1 wrote:
> nit: the inner parentheses seem unnecessary.

Done.

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
net/url_request/url_request_throttler_entry_interface.h:28: //
net/base/load_flags.h) that mean it is definitely user-initiated.
On 2011/08/05 21:50:41, yzshen1 wrote:
> nit: now that the flag is 'MAYBE', 'definitely' may not be accurate here.

Thanks, that part of the comment is now like this:

  // Returns true when we have encountered server errors and are doing
  // exponential back-off, unless the request has |load_flags| (from
  // net/base/load_flags.h) that mean it is likely to be
  // user-initiated.

[Jói, 2011-08-08 17:10:39]

eroman: ping for net/OWNERS approval

[yzshen, 2011-08-08 17:12:20]

Thanks, Joi.

On Mon, Aug 8, 2011 at 10:09 AM, <joi@chromium.org> wrote:

> Thanks John and Yuzhu.
>
> Yuzhu, I've addressed your comments and uploaded a new version.
>
> Cheers,
> Jói
>
>
>
>
> http://codereview.chromium.**org/7582004/diff/1/content/**
>
browser/renderer_host/**resource_dispatcher_host.cc<http://codereview.chromium.org/7582004/diff/1/content/browser/renderer_host/resource_dispatcher_host.cc>
> File content/browser/renderer_host/**resource_dispatcher_host.cc (right):
>
> http://codereview.chromium.**org/7582004/diff/1/content/**
>
browser/renderer_host/**resource_dispatcher_host.cc#**newcode120<http://codereview.chromium.org/7582004/diff/1/content/browser/renderer_host/resource_dispatcher_host.cc#newcode120>
> content/browser/renderer_host/**resource_dispatcher_host.cc:**120: //
> NET_MAYBE_USER_GESTURE load flag.
> On 2011/08/05 21:50:41, yzshen1 wrote:
>
>> 1) NET_MAYBE_USER_GESTURE -> LOAD_MAYBE_USER_GESTURE, please.
>> 2) [just out of curiosity] how is the number 3500 decided?
>>
>
> 1) Done.
>
> 2) See updated comment for this constant:
>
>  118 // The number of milliseconds after noting a user gesture that we
> will
>  119 // tag newly-created URLRequest objects with the
>  120 // net::LOAD_MAYBE_USER_GESTURE load flag. This is a fairly
> arbitrary
>  121 // guess at how long to expect direct impact from a user gesture,
> but
>  122 // this should be OK as the load flag is a best-effort thing only,
>  123 // rather than being intended as fully accurate.
>
>
> http://codereview.chromium.**org/7582004/diff/1/net/url_**
>
request/url_request_throttler_**entry.cc<http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_throttler_entry.cc>
> File net/url_request/url_request_**throttler_entry.cc (right):
>
> http://codereview.chromium.**org/7582004/diff/1/net/url_**
>
request/url_request_throttler_**entry.cc#newcode451<http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_throttler_entry.cc#newcode451>
> net/url_request/url_request_**throttler_entry.cc:451: return (load_flags &
> (LOAD_MAYBE_USER_GESTURE)) != 0;
> On 2011/08/05 21:50:41, yzshen1 wrote:
>
>> nit: the inner parentheses seem unnecessary.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7582004/diff/1/net/url_**
>
request/url_request_throttler_**entry_interface.h<http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_throttler_entry_interface.h>
> File net/url_request/url_request_**throttler_entry_interface.h (right):
>
> http://codereview.chromium.**org/7582004/diff/1/net/url_**
>
request/url_request_throttler_**entry_interface.h#newcode28<http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_throttler_entry_interface.h#newcode28>
> net/url_request/url_request_**throttler_entry_interface.h:**28: //
> net/base/load_flags.h) that mean it is definitely user-initiated.
> On 2011/08/05 21:50:41, yzshen1 wrote:
>
>> nit: now that the flag is 'MAYBE', 'definitely' may not be accurate
>>
> here.
>
> Thanks, that part of the comment is now like this:
>
>
>  // Returns true when we have encountered server errors and are doing
>  // exponential back-off, unless the request has |load_flags| (from
>  // net/base/load_flags.h) that mean it is likely to be
>  // user-initiated.
>
>
>
http://codereview.chromium.**org/7582004/<http://codereview.chromium.org/7582...
>



-- 
Best regards,
Yuzhu Shen.

[Jói, 2011-08-09 09:19:36]

eroman: ping for net/OWNERS approval

On Mon, Aug 8, 2011 at 5:12 PM, Yuzhu Shen <yzshen@google.com> wrote:
> Thanks, Joi.
>
> On Mon, Aug 8, 2011 at 10:09 AM, <joi@chromium.org> wrote:
>>
>> Thanks John and Yuzhu.
>>
>> Yuzhu, I've addressed your comments and uploaded a new version.
>>
>> Cheers,
>> Jói
>>
>>
>>
>>
>>
http://codereview.chromium.org/7582004/diff/1/content/browser/renderer_host/r...
>> File content/browser/renderer_host/resource_dispatcher_host.cc (right):
>>
>>
>>
http://codereview.chromium.org/7582004/diff/1/content/browser/renderer_host/r...
>> content/browser/renderer_host/resource_dispatcher_host.cc:120: //
>> NET_MAYBE_USER_GESTURE load flag.
>> On 2011/08/05 21:50:41, yzshen1 wrote:
>>>
>>> 1) NET_MAYBE_USER_GESTURE -> LOAD_MAYBE_USER_GESTURE, please.
>>> 2) [just out of curiosity] how is the number 3500 decided?
>>
>> 1) Done.
>>
>> 2) See updated comment for this constant:
>>
>>  118 // The number of milliseconds after noting a user gesture that we
>> will
>>  119 // tag newly-created URLRequest objects with the
>>  120 // net::LOAD_MAYBE_USER_GESTURE load flag. This is a fairly
>> arbitrary
>>  121 // guess at how long to expect direct impact from a user gesture,
>> but
>>  122 // this should be OK as the load flag is a best-effort thing only,
>>  123 // rather than being intended as fully accurate.
>>
>>
>>
http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
>> File net/url_request/url_request_throttler_entry.cc (right):
>>
>>
>>
http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
>> net/url_request/url_request_throttler_entry.cc:451: return (load_flags &
>> (LOAD_MAYBE_USER_GESTURE)) != 0;
>> On 2011/08/05 21:50:41, yzshen1 wrote:
>>>
>>> nit: the inner parentheses seem unnecessary.
>>
>> Done.
>>
>>
>>
http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
>> File net/url_request/url_request_throttler_entry_interface.h (right):
>>
>>
>>
http://codereview.chromium.org/7582004/diff/1/net/url_request/url_request_thr...
>> net/url_request/url_request_throttler_entry_interface.h:28: //
>> net/base/load_flags.h) that mean it is definitely user-initiated.
>> On 2011/08/05 21:50:41, yzshen1 wrote:
>>>
>>> nit: now that the flag is 'MAYBE', 'definitely' may not be accurate
>>
>> here.
>>
>> Thanks, that part of the comment is now like this:
>>
>>  // Returns true when we have encountered server errors and are doing
>>  // exponential back-off, unless the request has |load_flags| (from
>>  // net/base/load_flags.h) that mean it is likely to be
>>  // user-initiated.
>>
>> http://codereview.chromium.org/7582004/
>
>
>
> --
> Best regards,
> Yuzhu Shen.
>
>

[eroman, 2011-08-09 18:52:52]

A couple concerns:

(a) I am not very familiar with the "user gesture" code, but as best I can tell
it only applies to when you mouse click, type a space, or type return inside a
renderview. As such, I don't imagine it will apply to "main frame" loads (when
you directly type a URL into omnibox, or refresh it). These however are the
single most important "user gesture". I guess you could test the existing load
flag MAIN_FRAME.

(b) I don't like pushing the "user gesture" concept into the load flags. This is
a layering violation, since a user gesture really has nothing to do with the
network stack. (I am already displeased that load flags like MAIN_FRAME and
PRERENDER have found there way down here).

I guess I'll LGTM, but I am not particularly happy with this trend. Perhaps the
throttling module (implementation) could be moved into chrome's code, so it can
play with this sort of state more easily. (extract custom URLRequest data added
by browser).

[Jói, 2011-08-09 20:30:17]

Hi Eric,

Thanks for the feedback.

Based on my testing, clicking a NTP link or typing directly into the
omnibox does trigger the user gesture.  I don't remember exactly why
that works but I recall seeing the code for that somewhere during my
explorations, I think.

Understood about the layering violation concern, although I'm not
quite sure whether I share it for this flag in particular (I think I
do for the MAIN_FRAME and PRERENDER flags though, as they are quite
browser-specific).  I think there might be other valid cases in the
future where knowing that a request is being made as a direct result
of a user action might mean net/ treats the request differently, and
the fact that the request may be based on a user gesture is not a
browser-specific thing but probably fairly general for all clients of
net/ (although some might never set the flag).

Independent of that, I think the HTTP throttling belongs in net/ as
all clients of net/ should probably get it for free, so I don't think
it would be right to move the URLRequestThrottler* stuff to chrome/ or
content/.  On the other hand, it is on my list of things to do to make
the URLRequestThrottlingManager a member of URLRequestContext instead
of a singleton, so that clients can configure whether they use one,
and probably use different implementations of one if desired.

Cheers,
Jói


On Tue, Aug 9, 2011 at 6:52 PM,  <eroman@chromium.org> wrote:
> A couple concerns:
>
> (a) I am not very familiar with the "user gesture" code, but as best I can
> tell
> it only applies to when you mouse click, type a space, or type return inside
> a
> renderview. As such, I don't imagine it will apply to "main frame" loads
> (when
> you directly type a URL into omnibox, or refresh it). These however are the
> single most important "user gesture". I guess you could test the existing
> load
> flag MAIN_FRAME.
>
> (b) I don't like pushing the "user gesture" concept into the load flags.
> This is
> a layering violation, since a user gesture really has nothing to do with the
> network stack. (I am already displeased that load flags like MAIN_FRAME and
> PRERENDER have found there way down here).
>
> I guess I'll LGTM, but I am not particularly happy with this trend. Perhaps
> the
> throttling module (implementation) could be moved into chrome's code, so it
> can
> play with this sort of state more easily. (extract custom URLRequest data
> added
> by browser).
>
> http://codereview.chromium.org/7582004/
>

[cbentzel, 2011-08-09 20:41:37]

Entered http://code.google.com/p/chromium/issues/detail?id=92214 for
the PRERENDER case - I agree that it should be removed, and I think it
can be removed fairly easily.

2011/8/9 Jói Sigurðsson <joi@chromium.org>:
> Hi Eric,
>
> Thanks for the feedback.
>
> Based on my testing, clicking a NTP link or typing directly into the
> omnibox does trigger the user gesture.  I don't remember exactly why
> that works but I recall seeing the code for that somewhere during my
> explorations, I think.
>
> Understood about the layering violation concern, although I'm not
> quite sure whether I share it for this flag in particular (I think I
> do for the MAIN_FRAME and PRERENDER flags though, as they are quite
> browser-specific).  I think there might be other valid cases in the
> future where knowing that a request is being made as a direct result
> of a user action might mean net/ treats the request differently, and
> the fact that the request may be based on a user gesture is not a
> browser-specific thing but probably fairly general for all clients of
> net/ (although some might never set the flag).
>
> Independent of that, I think the HTTP throttling belongs in net/ as
> all clients of net/ should probably get it for free, so I don't think
> it would be right to move the URLRequestThrottler* stuff to chrome/ or
> content/.  On the other hand, it is on my list of things to do to make
> the URLRequestThrottlingManager a member of URLRequestContext instead
> of a singleton, so that clients can configure whether they use one,
> and probably use different implementations of one if desired.
>
> Cheers,
> Jói
>
>
> On Tue, Aug 9, 2011 at 6:52 PM,  <eroman@chromium.org> wrote:
>> A couple concerns:
>>
>> (a) I am not very familiar with the "user gesture" code, but as best I can
>> tell
>> it only applies to when you mouse click, type a space, or type return inside
>> a
>> renderview. As such, I don't imagine it will apply to "main frame" loads
>> (when
>> you directly type a URL into omnibox, or refresh it). These however are the
>> single most important "user gesture". I guess you could test the existing
>> load
>> flag MAIN_FRAME.
>>
>> (b) I don't like pushing the "user gesture" concept into the load flags.
>> This is
>> a layering violation, since a user gesture really has nothing to do with the
>> network stack. (I am already displeased that load flags like MAIN_FRAME and
>> PRERENDER have found there way down here).
>>
>> I guess I'll LGTM, but I am not particularly happy with this trend. Perhaps
>> the
>> throttling module (implementation) could be moved into chrome's code, so it
>> can
>> play with this sort of state more easily. (extract custom URLRequest data
>> added
>> by browser).
>>
>> http://codereview.chromium.org/7582004/
>>
>

[eroman, 2011-08-09 20:53:23]

lgtm then it works with mainframe loads.