CrNet: add pauseable NSURLProtocol and switch to using it

CrNet: add pauseable NSURLProtocol and switch to using it

On iOS 8 with NSURLProtocol, CFNetwork appears to treat |-startLoading| as
meaning "start or resume" and |-stopLoading| as "pause", and may deliver many
such notifications over the lifetime of a request. The old
CRNHTTPProtocolHandler is not designed for this behavior, so this change adds
a separate NSURLProtocol implementation that can defer sending callbacks until
the request is "resumed".

This change should not affect the behavior of Chromium on iOS in any way, since
Chromium will continue using the old NSURLProtocol implementation with the old
behavior. The new NSURLProtocol ought to be safe to use in the same situations,
but demonstrating this conclusively is difficult.

Also, replace crnet_consumer with a non-graphical app with its own NSURLSession
delegate to make testing behavior easier.

BUG=

Committed: https://crrev.com/7d0f7a2bc58307ec844f5d82c03a333117d74f2b
Cr-Commit-Position: refs/heads/master@{#334168}

[Elly Fong-Jones, 2015-05-22 16:20:20]

ellyjones@chromium.org changed reviewers:
+ marq@chromium.org, stuartmorgan@chromium.org

[Elly Fong-Jones, 2015-05-22 16:20:50]

marq, stuart: PTAL? This CL is a draft, currently:

TODO: unit tests
TODO: documentation
TODO: flesh out crnet_consumer a bit
TODO: clean up dead GUI crnet_consumer assets

[marq (ping after 24h), 2015-05-22 16:56:53]

I think this is OK as a design; as discussed it seems 100% safe for Bling since
it doesn't change anything about Bling's network stack behavior.

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler.mm:966: // TODO(ellyjones): restructure so
that is not true.
Yes -- can this class just be a specialization of CRNHTTPProtocolHandler?

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
File ios/net/crn_http_protocol_handler_pauseable_proxy.mm (right):

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler_pauseable_proxy.mm:13: // The |QueuedSelector|
class represents a single queued callback, with the
Just use NSInvocation to instead of a custom class here.

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler_pauseable_proxy.mm:78: withObject:arg];
(This is a memory leak, BTW).

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler_pauseable_proxy.mm:79: [_queuedSelectors
addObject:qs];
Try this:

scoped_nsobject<NSInvocation> qs([[NSInvocation alloc] init]);
[qs setSelector:aSelector];
[qs setTarget:arg];
[_queuedSelectors addObject:qs];

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler_pauseable_proxy.mm:89: for (QueuedSelector* qs
: _queuedSelectors) {
for (NSInvocation* qs : _queuedSelectors) {
  [self performSelectorOnClientThread:[qs selector] withObject:[qs target]];
}

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler_pauseable_proxy.mm:90: [self
performSelectorOnClientThread:[qs selector] withObject:[qs object]];
What if [self pause] is a queued selector, or executing the selector causes a
pause? 

I think it's safer to iteratively drain the queued selectors while unpaused,
instead of assuming that each 'resume' can execute all of the queued messages.

[Elly Fong-Jones, 2015-05-26 21:16:09]

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler.mm:966: // TODO(ellyjones): restructure so
that is not true.
On 2015/05/22 16:56:53, marq wrote:
> Yes -- can this class just be a specialization of CRNHTTPProtocolHandler?

Done.

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
File ios/net/crn_http_protocol_handler_pauseable_proxy.mm (right):

https://codereview.chromium.org/1142383006/diff/1/ios/net/crn_http_protocol_h...
ios/net/crn_http_protocol_handler_pauseable_proxy.mm:13: // The |QueuedSelector|
class represents a single queued callback, with the
On 2015/05/22 16:56:53, marq wrote:
> Just use NSInvocation to instead of a custom class here.

Done.

[Elly Fong-Jones, 2015-05-27 16:29:05]

ellyjones@chromium.org changed reviewers:
+ droger@chromium.org

[droger, 2015-05-27 17:09:01]

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:1002: - (void)dealloc {
I am concerned that dealloc might be called from any thread, but cancelRequest
expects to be called from the "client" thread.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm (right):

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:76: _paused = NO;
Nit: in objective-C, instance variables are auto-zeroed.
The usual style is to not re-initialize them to zero.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:91: //
DCHECK(_queuedInvocations.get().count == 0);
This might be caused by the problem with |_paused| below.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:120: if (!_paused)
{
I don't think it is safe to access _paused here.

I don't know what is the best way to do it, but I think we should be able to do
it without locks, by catching the tasks on the other thread, instead of catching
them before posting them.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:272: _paused =
YES;
We probably need thread checks here
DCHECK([NSThread currentThread] == _clientThread);

|_paused| should only be accessed from one thread.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:276: _paused = NO;
Thread checks.

[stuartmorgan, 2015-05-28 13:49:40]

I'll defer to marq and droger on this unless there's something you specifically
want my input on; they know this layer better than I do.

[Elly Fong-Jones, 2015-06-10 17:06:36]

droger: ptal?

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:1002: - (void)dealloc {
On 2015/05/27 17:09:00, droger wrote:
> I am concerned that dealloc might be called from any thread, but cancelRequest
> expects to be called from the "client" thread.

Okay, I think I've fixed this. It's somewhat of a bad hack; I added a method to
the parent class to schedule a cancel to be performed on the client thread,
since we are guaranteed by the iOS docs that the client thread will run its run
loop even after dealloc is run, which is very convenient for us.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm (right):

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:76: _paused = NO;
On 2015/05/27 17:09:00, droger wrote:
> Nit: in objective-C, instance variables are auto-zeroed.
> The usual style is to not re-initialize them to zero.

Done.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:91: //
DCHECK(_queuedInvocations.get().count == 0);
On 2015/05/27 17:09:01, droger wrote:
> This might be caused by the problem with |_paused| below.

Done.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:120: if (!_paused)
{
On 2015/05/27 17:09:00, droger wrote:
> I don't think it is safe to access _paused here.
> 
> I don't know what is the best way to do it, but I think we should be able to
do
> it without locks, by catching the tasks on the other thread, instead of
catching
> them before posting them.

Done.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:272: _paused =
YES;
On 2015/05/27 17:09:01, droger wrote:
> We probably need thread checks here
> DCHECK([NSThread currentThread] == _clientThread);
> 
> |_paused| should only be accessed from one thread.

Done.

https://codereview.chromium.org/1142383006/diff/20001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:276: _paused = NO;
On 2015/05/27 17:09:01, droger wrote:
> Thread checks.

Done.

[Elly Fong-Jones, 2015-06-10 17:06:37]

droger: ptal?

[droger, 2015-06-11 08:56:20]

Looks fine from a threading perspective, I don't see any major issue.

Minor issues:

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:884: _core.swap(core);
Nit: We're not swapping anything here,
_core = core;
seems clearer.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:980: -
(id<CRNHTTPProtocolHandlerProxy>)getProtocolHandlerProxy {
DCHECK that the current thread is _clientThread here or inside the if.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:984: clientThread:[NSThread currentThread]
Why not using _clientThread here?

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:997: - (void)scheduleCancelRequest {
Optional: 
Why is this method on CRNHTTPProtocolHandler and not on
PauseableHttpProtocolHandler?
I see that moving it would require exposing _clientThread and _core, but maybe
that's better?

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:1057: -
(instancetype)initWithRequest:(NSURLRequest*)request
Is this constructor useful? Is it only here for the DCHECK?

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy.h (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy.h:18: // Called from the client thread.
Maybe remove this "Called from the client thread." and add a comment for the
whole class saying that:
- methods inherited from CRNNetworkClientProtocol can be called from any thread
(but all on the same)
- the other methods must be called on the client thread.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:92:
DCHECK(_queuedInvocations.get().count == 0);
I think it's expected that the queue may not be empty. For example in the case
where iOS cancels or pauses the request, but the network stack keeps loading
data which enqueues invocations. And then the system destroys the request.

Or did I misunderstood?


I would just do:
_queuedInvocations.reset();
to drop all the pending invocations.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:98: while
(!_paused && _queuedInvocations.get().count > 0) {
I don't understand this. Do you expect _paused or the queue to be mutated during
the iteration?

I would have expected a code like:

DCHECK(!_paused);
for (NSInvocation* invocation in _queuedInvocations) {
  [inv invoke];
}
_queuedInvocations.reset();


What did I miss?

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:132: [inv retain];
Can we remove this retain and the corresponding releases in
runInvocationQueueOnClientThread and invokeOnClientThread?
I would expect performSelector to do it automatically.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:156: if
(!_protocol)
Optional:

I realize that this check is not trivial.
Not related to your CL but maybe add a comment like:
Test |_protocol| from the wrong thread to possibly avoid posting the task. It
will be checked from the right thread later.

[droger, 2015-06-11 08:58:36]

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy.h (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy.h:18: // Called from the client thread.
On 2015/06/11 08:56:19, droger wrote:
> Maybe remove this "Called from the client thread." and add a comment for the
> whole class saying that:
> - methods inherited from CRNNetworkClientProtocol can be called from any
thread
> (but all on the same)
> - the other methods must be called on the client thread.

Actually I'm not sure if all the methods of CRNNetworkClientProtocol have to be
called from the same thread or not. For sure there aren't any checks that they
indeed are.

[droger, 2015-06-11 09:01:21]

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy.h (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy.h:18: // Called from the client thread.
On 2015/06/11 08:58:36, droger wrote:
> On 2015/06/11 08:56:19, droger wrote:
> > Maybe remove this "Called from the client thread." and add a comment for the
> > whole class saying that:
> > - methods inherited from CRNNetworkClientProtocol can be called from any
> thread
> > (but all on the same)
> > - the other methods must be called on the client thread.
> 
> Actually I'm not sure if all the methods of CRNNetworkClientProtocol have to
be
> called from the same thread or not. For sure there aren't any checks that they
> indeed are.

Looking at the code I would think that in practice they are all called from the
same thread, and other CRNNetworkClientProtocol have DCHECKs for that (see for
example CRNForwardingNetworkClient).
Digging into this may be out of scope of your work though.

[Elly Fong-Jones, 2015-06-11 13:49:34]

ptal?

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:884: _core.swap(core);
On 2015/06/11 08:56:19, droger wrote:
> Nit: We're not swapping anything here,
> _core = core;
> seems clearer.

Done.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:980: -
(id<CRNHTTPProtocolHandlerProxy>)getProtocolHandlerProxy {
On 2015/06/11 08:56:19, droger wrote:
> DCHECK that the current thread is _clientThread here or inside the if.

Done.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:984: clientThread:[NSThread currentThread]
On 2015/06/11 08:56:19, droger wrote:
> Why not using _clientThread here?

Done.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:997: - (void)scheduleCancelRequest {
On 2015/06/11 08:56:19, droger wrote:
> Optional: 
> Why is this method on CRNHTTPProtocolHandler and not on
> PauseableHttpProtocolHandler?
> I see that moving it would require exposing _clientThread and _core, but maybe
> that's better?

Done. I have added accessors for _clientThread and _core and moved this method
onto PauseableHTTPProtocolHandler.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:1057: -
(instancetype)initWithRequest:(NSURLRequest*)request
On 2015/06/11 08:56:19, droger wrote:
> Is this constructor useful? Is it only here for the DCHECK?

It is not. The parent class duplicates the DCHECK anyway, so I've removed it.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy.h (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy.h:18: // Called from the client thread.
On 2015/06/11 09:01:21, droger wrote:
> On 2015/06/11 08:58:36, droger wrote:
> > On 2015/06/11 08:56:19, droger wrote:
> > > Maybe remove this "Called from the client thread." and add a comment for
the
> > > whole class saying that:
> > > - methods inherited from CRNNetworkClientProtocol can be called from any
> > thread
> > > (but all on the same)
> > > - the other methods must be called on the client thread.
> > 
> > Actually I'm not sure if all the methods of CRNNetworkClientProtocol have to
> be
> > called from the same thread or not. For sure there aren't any checks that
they
> > indeed are.
> 
> Looking at the code I would think that in practice they are all called from
the
> same thread, and other CRNNetworkClientProtocol have DCHECKs for that (see for
> example CRNForwardingNetworkClient).
> Digging into this may be out of scope of your work though.

Documented, at least.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm (right):

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:92:
DCHECK(_queuedInvocations.get().count == 0);
On 2015/06/11 08:56:19, droger wrote:
> I think it's expected that the queue may not be empty. For example in the case
> where iOS cancels or pauses the request, but the network stack keeps loading
> data which enqueues invocations. And then the system destroys the request.
> 
> Or did I misunderstood?
> 
> 
> I would just do:
> _queuedInvocations.reset();
> to drop all the pending invocations.

Done.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:98: while
(!_paused && _queuedInvocations.get().count > 0) {
On 2015/06/11 08:56:20, droger wrote:
> I don't understand this. Do you expect _paused or the queue to be mutated
during
> the iteration?

Yes, that is the crux of it. A callback can lead the system stack to pause the
request again, after which we cannot keep delivering invocations.

> I would have expected a code like:
> 
> DCHECK(!_paused);
> for (NSInvocation* invocation in _queuedInvocations) {
>   [inv invoke];
> }
> _queuedInvocations.reset();
> 
> 
> What did I miss?

I'll put an explanatory comment.

https://codereview.chromium.org/1142383006/diff/60001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:132: [inv retain];
On 2015/06/11 08:56:20, droger wrote:
> Can we remove this retain and the corresponding releases in
> runInvocationQueueOnClientThread and invokeOnClientThread?
> I would expect performSelector to do it automatically.

Done.

[Elly Fong-Jones, 2015-06-11 13:49:35]

ptal?

[droger, 2015-06-11 15:45:57]

https://codereview.chromium.org/1142383006/diff/80001/ios/crnet/crnet_consume...
File ios/crnet/crnet_consumer/main.m (right):

https://codereview.chromium.org/1142383006/diff/80001/ios/crnet/crnet_consume...
ios/crnet/crnet_consumer/main.m:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Did you run the formatter on the file?

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:871: @interface DeferredCancellation :
NSObject {}
Remove {}

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:871: @interface DeferredCancellation :
NSObject {}
Add comments.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:1079: DeferredCancellation* cancellation =
<rant>
It seems overkill to create a class for that, but I looked around and I didn't
find a way to do something else. In particular, it would be helpful if we could
just execute a block, but unfortunately it seems that there is no way to do that
with the standard library. We could write the generic code ourselves (taking a
block and running it on the given thread and modes), but it's probably not worth
it for only one use case.
</rant>

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm (right):

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:99: //  drained
again the next time the system network stack calls |resume|.
As discussed offline, maybe mention explicitly that |pause| can be called during
the execution of this method.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:101: NSInvocation*
inv = [_queuedInvocations objectAtIndex:0];
Nit: call this variable |invocation|. The usual style is to avoid abbreviations.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:108: [inv
release];
Use a scoped_nsobject instead of manual retain/release.
Maybe an alternative would be to invoke before removing from the array.

[Elly Fong-Jones, 2015-06-12 15:03:00]

https://codereview.chromium.org/1142383006/diff/80001/ios/crnet/crnet_consume...
File ios/crnet/crnet_consumer/main.m (right):

https://codereview.chromium.org/1142383006/diff/80001/ios/crnet/crnet_consume...
ios/crnet/crnet_consumer/main.m:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2015/06/11 15:45:56, droger wrote:
> Did you run the formatter on the file?

I just ran 'git cl format' and it looks like it does not touch this part of the
file. Should I update the copyright date? I think we are not supposed to.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler.mm (right):

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:871: @interface DeferredCancellation :
NSObject {}
On 2015/06/11 15:45:56, droger wrote:
> Remove {}

Done.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:871: @interface DeferredCancellation :
NSObject {}
On 2015/06/11 15:45:56, droger wrote:
> Add comments.

Done.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler.mm:1079: DeferredCancellation* cancellation =
On 2015/06/11 15:45:57, droger wrote:
> <rant>
> It seems overkill to create a class for that, but I looked around and I didn't
> find a way to do something else. In particular, it would be helpful if we
could
> just execute a block, but unfortunately it seems that there is no way to do
that
> with the standard library. We could write the generic code ourselves (taking a
> block and running it on the given thread and modes), but it's probably not
worth
> it for only one use case.
> </rant>

Yep.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
File ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm (right):

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:99: //  drained
again the next time the system network stack calls |resume|.
On 2015/06/11 15:45:57, droger wrote:
> As discussed offline, maybe mention explicitly that |pause| can be called
during
> the execution of this method.

Done.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:101: NSInvocation*
inv = [_queuedInvocations objectAtIndex:0];
On 2015/06/11 15:45:57, droger wrote:
> Nit: call this variable |invocation|. The usual style is to avoid
abbreviations.

Done.

https://codereview.chromium.org/1142383006/diff/80001/ios/net/crn_http_protoc...
ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm:108: [inv
release];
On 2015/06/11 15:45:57, droger wrote:
> Use a scoped_nsobject instead of manual retain/release.
> Maybe an alternative would be to invoke before removing from the array.

Done.

[droger, 2015-06-12 15:13:15]

lgtm

[Elly Fong-Jones, 2015-06-12 15:13:56]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2015-06-12 15:14:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142383006/120001

[commit-bot: I haz the power, 2015-06-12 15:30:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-12 15:31:13]

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