Convert ignore_limits from a SocketParam to a socket request argument.

Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571
Committed: https://crrev.com/d3641e1cd52ef1c5fb3abdfd2d066d1789720c68
Cr-Commit-Position: refs/heads/master@{#372095}

[mmenke, 2016-01-14 03:28:50]

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

[mmenke, 2016-01-14 03:29:03]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-01-14 03:34:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580903002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580903002/80001

[commit-bot: I haz the power, 2016-01-14 05:01:50]

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

[commit-bot: I haz the power, 2016-01-14 05:01:56]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-01-14 18:18:50]

mmenke@chromium.org changed reviewers:
+ davidben@chromium.org

[mmenke, 2016-01-14 18:18:51]

David:  PTAL when you get a chance, though there's certainly no rush on this
one.  Moving towards saner socket pools at a snail's pace.

[mmenke, 2016-01-20 23:45:08]

David:  Ping!  Just want to make sure this is in your queue.  No rush on it, and
merge conflicts aren't too likely, since people rarely muck with the socket pool
code, and I don't plan to follow up on it any time soon.

[mmenke, 2016-01-20 23:45:32]

On 2016/01/20 23:45:08, mmenke wrote:
> David:  Ping!  Just want to make sure this is in your queue.  No rush on it,
and
> merge conflicts aren't too likely, since people rarely muck with the socket
pool
> code, and I don't plan to follow up on it any time soon.

Or I could send this to eroman...Not sure who else knows the socket pools.

[davidben, 2016-01-21 02:42:06]

On 2016/01/20 23:45:32, mmenke wrote:
> On 2016/01/20 23:45:08, mmenke wrote:
> > David:  Ping!  Just want to make sure this is in your queue.  No rush on it,
> and
> > merge conflicts aren't too likely, since people rarely muck with the socket
> pool
> > code, and I don't plan to follow up on it any time soon.
> 
> Or I could send this to eroman...Not sure who else knows the socket pools.

Yeah, I'm drowning right now and not likely to get to it anytime soon. :-(

[mmenke, 2016-01-21 04:15:26]

On 2016/01/21 02:42:06, davidben (slow) wrote:
> On 2016/01/20 23:45:32, mmenke wrote:
> > On 2016/01/20 23:45:08, mmenke wrote:
> > > David:  Ping!  Just want to make sure this is in your queue.  No rush on
it,
> > and
> > > merge conflicts aren't too likely, since people rarely muck with the
socket
> > pool
> > > code, and I don't plan to follow up on it any time soon.
> > 
> > Or I could send this to eroman...Not sure who else knows the socket pools.
> 
> Yeah, I'm drowning right now and not likely to get to it anytime soon. :-(

I've had a fairly light load these past few weeks.  I'll go ahead and take over
your reviews from csharrison.  Not sure if any of your other reviews fit in or
near my areas of knowledge, but feel free to toss those that do at me.

[mmenke, 2016-01-21 04:19:15]

Description was changed from

==========
Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571
==========

to

==========
Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571
==========

[mmenke, 2016-01-21 04:19:15]

mmenke@chromium.org changed reviewers:
- davidben@chromium.org

[mmenke, 2016-01-21 16:39:16]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2016-01-21 16:39:17]

[+eroman]:  Mind reviewing this?  Background is just that SocketParams really
should unique determine a group in the socket pool, and this is a small step
towards that eventual goal.  Don't plan to follow up on this anytime soon, so no
rush on the review.

[eroman, 2016-01-21 21:38:57]

Sure, I can review

[eroman, 2016-01-26 23:04:57]

Design comments:

  * The boolean (basically false everwhere) is hard to read in the various
callsites. I recommend an enum instead.

  * Have you explored how hard it would be to make this a special priority
instead? like HIGHEST_IGNORE_LIMITS? I guess problematic with the layering of
RequestPriority being in net/base. Hmm.

https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool.h (right):

https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.h:73: bool ignore_limits() const { return
ignore_limits_; }
Delete this

https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.h:91: bool ignore_limits_;
Delete this

[mmenke, 2016-01-26 23:48:26]

On 2016/01/26 23:04:57, eroman wrote:
> Design comments:
> 
>   * The boolean (basically false everwhere) is hard to read in the various
> callsites. I recommend an enum instead.

Good idea, I'll do that.

>   * Have you explored how hard it would be to make this a special priority
> instead? like HIGHEST_IGNORE_LIMITS? I guess problematic with the layering of
> RequestPriority being in net/base. Hmm.

I'm kinda concerned with the implications of that code.  It would require a lot
of changes... SPDY, QUIC, the resource scheduler, RDH...  375 files include
net::RequestPriority.  I agree it should have been done that way in the first
place, it just seems like too big a change for the gain, and I say that as
someone who's spent the last week or so doing purely refactor changes to the
CookieStore, with no goal other than cleaning up the code, and providing
lifetime guarantees.

>
https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
> File net/http/http_proxy_client_socket_pool.h (right):
> 
>
https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
> net/http/http_proxy_client_socket_pool.h:73: bool ignore_limits() const {
return
> ignore_limits_; }
> Delete this
> 
>
https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
> net/http/http_proxy_client_socket_pool.h:91: bool ignore_limits_;
> Delete this

[eroman, 2016-01-26 23:58:23]

On Tue, Jan 26, 2016 at 3:48 PM, <mmenke@chromium.org> wrote:

> On 2016/01/26 23:04:57, eroman wrote:
>
>> Design comments:
>>
>
>    * The boolean (basically false everwhere) is hard to read in the various
>> callsites. I recommend an enum instead.
>>
>
> Good idea, I'll do that.
>
>    * Have you explored how hard it would be to make this a special priority
>> instead? like HIGHEST_IGNORE_LIMITS? I guess problematic with the
>> layering of
>> RequestPriority being in net/base. Hmm.
>>
>
> I'm kinda concerned with the implications of that code.  It would require
> a lot
> of changes... SPDY, QUIC, the resource scheduler, RDH...  375 files include
> net::RequestPriority.  I agree it should have been done that way in the
> first
> place, it just seems like too big a change for the gain, and I say that as
> someone who's spent the last week or so doing purely refactor changes to
> the
> CookieStore, with no goal other than cleaning up the code, and providing
> lifetime guarantees.


Sure that is fine.

This change as-is is an improvement and will sign-off after reviewing next
patchset.
Just feels like there is a strange coupling between the priority and ignore
limits.


>
>
>
>
https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
>
>> File net/http/http_proxy_client_socket_pool.h (right):
>>
>
>
>
>
https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
>
>> net/http/http_proxy_client_socket_pool.h:73: bool ignore_limits() const {
>>
> return
>
>> ignore_limits_; }
>> Delete this
>>
>
>
>
>
https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
>
>> net/http/http_proxy_client_socket_pool.h:91: bool ignore_limits_;
>> Delete this
>>
>
>
>
> https://codereview.chromium.org/1580903002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2016-01-27 00:28:06]

On 2016/01/26 23:58:23, eroman wrote:
> On Tue, Jan 26, 2016 at 3:48 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > On 2016/01/26 23:04:57, eroman wrote:
> >
> >> Design comments:
> >>
> >
> >    * The boolean (basically false everwhere) is hard to read in the various
> >> callsites. I recommend an enum instead.
> >>
> >
> > Good idea, I'll do that.
> >
> >    * Have you explored how hard it would be to make this a special priority
> >> instead? like HIGHEST_IGNORE_LIMITS? I guess problematic with the
> >> layering of
> >> RequestPriority being in net/base. Hmm.
> >>
> >
> > I'm kinda concerned with the implications of that code.  It would require
> > a lot
> > of changes... SPDY, QUIC, the resource scheduler, RDH...  375 files include
> > net::RequestPriority.  I agree it should have been done that way in the
> > first
> > place, it just seems like too big a change for the gain, and I say that as
> > someone who's spent the last week or so doing purely refactor changes to
> > the
> > CookieStore, with no goal other than cleaning up the code, and providing
> > lifetime guarantees.
> 
> 
> Sure that is fine.
> 
> This change as-is is an improvement and will sign-off after reviewing next
> patchset.
> Just feels like there is a strange coupling between the priority and ignore
> limits.

I agree strongly with that, just think it's sufficiently late in the game that
it's become very hard to change, and there are much more important refactors to
do.  If we ever find something willing to do it, I'm happy to help review.  :)

[mmenke, 2016-01-27 18:16:25]

https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool.h (right):

https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.h:73: bool ignore_limits() const { return
ignore_limits_; }
On 2016/01/26 23:04:57, eroman wrote:
> Delete this

Done.

https://codereview.chromium.org/1580903002/diff/140001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool.h:91: bool ignore_limits_;
On 2016/01/26 23:04:57, eroman wrote:
> Delete this

Done.  And caught a couple more I missed - the enum made them easier to find. 
:)

[eroman, 2016-01-27 20:15:13]

lgtm

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_handle.h:53: // |ignore_limits| will bypass the wait
list, if true, but |priority| must
Update comment (ignore_limits is no longer a boolean)

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool.h (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool.h:71: enum class IgnoreLimits { DISABLED, ENABLED
};
Not sure what to suggest in its place, but "ENABLED" to my ear sounds like the
normal flow, and DISABLED as the exceptional case.

Ideally I would like for the name to sound scarier for the enabled case, since
this is really a very special case that callers shouldn't be using.

Maybe something that implies "default" or "normal" for the DISABLED case, or
even flipping the enum's meaning to "respect limits" or "enforce limits" so
disabled becomes the unusual case.

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:394: request.ignore_limits() ==
ClientSocketPool::IgnoreLimits::DISABLED) {
Perhaps there aren't enough callers, but could create a shorthand function that
returns a boolean.

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_base_unittest.cc:1338:
ClientSocketPool::IgnoreLimits::DISABLED,
Might consider an overload for Init() that doesn't take IgnoreLimits. Then again
if it is only helpful for unittests, meh.

[mmenke, 2016-01-27 20:37:27]

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool.h (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool.h:71: enum class IgnoreLimits { DISABLED, ENABLED
};
On 2016/01/27 20:15:12, eroman wrote:
> Not sure what to suggest in its place, but "ENABLED" to my ear sounds like the
> normal flow, and DISABLED as the exceptional case.
> 
> Ideally I would like for the name to sound scarier for the enabled case, since
> this is really a very special case that callers shouldn't be using.
> 
> Maybe something that implies "default" or "normal" for the DISABLED case, or
> even flipping the enum's meaning to "respect limits" or "enforce limits" so
> disabled becomes the unusual case.

Hrm...  IgnoreLimits::DEFAULT / NORMAL seems less clear to me.  Could go with
ClientSocketPool::PoolLimits::IGNORE (BYPASS?) / DEFAULT (I think we're missing
a RETRY and ABORT in there).  That does sound a bit like it affects some
permanent property of the socket pool, though, rather than a per-request value.

Think I'll just go with RespectLimits::ENABLED / DISABLED, unless you chime in. 
Now that I've stuck IgnoreLimits everywhere, mass renaming is pretty easy, just
have to be careful with comments.

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:394: request.ignore_limits() ==
ClientSocketPool::IgnoreLimits::DISABLED) {
On 2016/01/27 20:15:13, eroman wrote:
> Perhaps there aren't enough callers, but could create a shorthand function
that
> returns a boolean.

Too bad enum classes can't have methods.  Think it's more effort than it's
worth, just because the method doesn't really fit anywhere, though there are a
lot of checks for this.

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_base_unittest.cc:1338:
ClientSocketPool::IgnoreLimits::DISABLED,
On 2016/01/27 20:15:13, eroman wrote:
> Might consider an overload for Init() that doesn't take IgnoreLimits. Then
again
> if it is only helpful for unittests, meh.

I prefer to avoid overloads, just because it's easy to get confused about which
is being called, particularly when there are 47-gazillion arguments.

[eroman, 2016-01-27 20:40:22]

lgtm

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool.h (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool.h:71: enum class IgnoreLimits { DISABLED, ENABLED
};
On 2016/01/27 20:37:27, mmenke wrote:
> On 2016/01/27 20:15:12, eroman wrote:
> > Not sure what to suggest in its place, but "ENABLED" to my ear sounds like
the
> > normal flow, and DISABLED as the exceptional case.
> > 
> > Ideally I would like for the name to sound scarier for the enabled case,
since
> > this is really a very special case that callers shouldn't be using.
> > 
> > Maybe something that implies "default" or "normal" for the DISABLED case, or
> > even flipping the enum's meaning to "respect limits" or "enforce limits" so
> > disabled becomes the unusual case.
> 
> Hrm...  IgnoreLimits::DEFAULT / NORMAL seems less clear to me.  Could go with
> ClientSocketPool::PoolLimits::IGNORE (BYPASS?) / DEFAULT (I think we're
missing
> a RETRY and ABORT in there).  That does sound a bit like it affects some
> permanent property of the socket pool, though, rather than a per-request
value.
> 
> Think I'll just go with RespectLimits::ENABLED / DISABLED, unless you chime
in. 
> Now that I've stuck IgnoreLimits everywhere, mass renaming is pretty easy,
just
> have to be careful with comments.

ok

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:394: request.ignore_limits() ==
ClientSocketPool::IgnoreLimits::DISABLED) {
On 2016/01/27 20:37:27, mmenke wrote:
> On 2016/01/27 20:15:13, eroman wrote:
> > Perhaps there aren't enough callers, but could create a shorthand function
> that
> > returns a boolean.
> 
> Too bad enum classes can't have methods.  Think it's more effort than it's
> worth, just because the method doesn't really fit anywhere, though there are a
> lot of checks for this.

ok

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_base_unittest.cc:1338:
ClientSocketPool::IgnoreLimits::DISABLED,
On 2016/01/27 20:37:27, mmenke wrote:
> On 2016/01/27 20:15:13, eroman wrote:
> > Might consider an overload for Init() that doesn't take IgnoreLimits. Then
> again
> > if it is only helpful for unittests, meh.
> 
> I prefer to avoid overloads, just because it's easy to get confused about
which
> is being called, particularly when there are 47-gazillion arguments.

ok, i figured as much

[mmenke, 2016-01-28 15:55:12]

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1580903002/diff/180001/net/socket/client_sock...
net/socket/client_socket_handle.h:53: // |ignore_limits| will bypass the wait
list, if true, but |priority| must
On 2016/01/27 20:15:12, eroman wrote:
> Update comment (ignore_limits is no longer a boolean)

And just for the record, I've fixed this (And updated all reference to
ignore_limits in other methods as well).

[mmenke, 2016-01-28 15:55:17]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-01-28 15:55:18]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1580903002/#ps200001
(title: "RespectLimits")

[commit-bot: I haz the power, 2016-01-28 15:55:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580903002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580903002/200001

[commit-bot: I haz the power, 2016-01-28 16:06:21]

Description was changed from

==========
Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571
==========

to

==========
Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571
==========

[commit-bot: I haz the power, 2016-01-28 16:06:22]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-01-28 16:07:07]

Description was changed from

==========
Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571
==========

to

==========
Convert ignore_limits from a SocketParam to a socket request argument.

The SocketParams for all requests in a group should be identical.

BUG=533571

Committed: https://crrev.com/d3641e1cd52ef1c5fb3abdfd2d066d1789720c68
Cr-Commit-Position: refs/heads/master@{#372095}
==========

[commit-bot: I haz the power, 2016-01-28 16:07:09]

Patchset 10 (id:??) landed as
https://crrev.com/d3641e1cd52ef1c5fb3abdfd2d066d1789720c68
Cr-Commit-Position: refs/heads/master@{#372095}