[Cronet] Integrate CrNet functionality into Cronet on iOS.

[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.

BUG=654177
Committed: https://crrev.com/254963db8e5f58c0d1279cf29bb1f7d48043cad2
Cr-Commit-Position: refs/heads/master@{#426609}

[mef, 2016-07-13 20:40:10]

mef@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[mef, 2016-07-13 20:40:10]

FYI, WIP.

[mef, 2016-08-04 20:34:36]

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

[commit-bot: I haz the power, 2016-08-04 20:35:03]

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

[mef, 2016-08-04 20:43:09]

Description was changed from

==========
[Cronet] Add NSURLSession handling to Cronet on iOS.

BUG=
==========

to

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling similar to CrNet.
- Add GetGlobalMetricsDeltas similar to Android API.

BUG=
==========

[mef, 2016-08-04 20:44:06]

Description was changed from

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling similar to CrNet.
- Add GetGlobalMetricsDeltas similar to Android API.

BUG=
==========

to

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.
==========

[commit-bot: I haz the power, 2016-08-04 21:58:11]

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

[commit-bot: I haz the power, 2016-08-04 21:58:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mef, 2016-08-08 20:59:16]

Hi Andrei, PTAL when you have a chance. 

I think it is in a reasonably good shape to verify whether it includes all
functionality needed by current consumers.

[kapishnikov, 2016-08-10 17:47:41]

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
File components/cronet/ios/BUILD.gn (right):

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/BUILD.gn:87: "cronet_c_for_grpc.h",
"cronet_c_for_grpc.h" & "Cronet.h" are included in source_set("cronet_sources").
Should they be listed here as well?

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:36: +
(void)setHostResolverRulesForTesting:(NSString*)hostResolverRulesForTesting;
I wonder if we can somehow hide "...ForTesting" methods. Maybe we can move them
to the subclass?

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:61: // requests done through shared NSURLSession.
This can fit in the previous line.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:70: // Installs Croet into an NSURLSession,
passed in by the caller. Note that this
Croer=>Cronet

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:71: // NSURLSession will share settings with the
sharedSession, which the
The shared session has its own internal configuration. It is not clear, how this
method impacts it. Also, a single NSURLSessionConfiguration can be used for
multiple sessions. Should we say that this method enables Cronet in
NSURLSessionConfiguration so that all sessions created with this configuration
will use the Cronet stack?

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:99: + (NSData*)getGlobalMetricsDeltas;
What is the structure of NSData? What should the user do with it?

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:33: std::unique_ptr<net::CertVerifier>
gMockCertVerifierForTesting;
Can we rely on Objective-C++ ARC instead of std::unique_ptr?

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:45: : getter_(getter), filter_(filter,
base::scoped_policy::RETAIN) {}
RequestFilterBlock is retained by Cronet and at the same time the block supplied
by the user can reference Cronet. This can create a cycle and the objects will
never be released. It is probably okay in this situation. Will it be better if
we do not retain the block and describe it in the setRequestFilterBlock method
documentation?

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:172: [NSURLCache setSharedURLCache:[[[NSURLCache
alloc] init] autorelease]];
Should we remember the old value of NSURLCache in registerHttpProtocolHandler
and restore it here? I am not sure that [[NSURLCache alloc] init]] will produce
the desired object. The proper NSURLCache initializer is:
initWithMemoryCapacity:(NSUInteger)memoryCapacity
          diskCapacity:(NSUInteger)diskCapacity
              diskPath:(NSString *)path
It is probably called with specific parameters somewhere by the system .

[mef, 2016-08-11 21:21:21]

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

[mef, 2016-08-11 21:22:16]

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
File components/cronet/ios/BUILD.gn (right):

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/BUILD.gn:87: "cronet_c_for_grpc.h",
On 2016/08/10 17:47:40, kapishnikov wrote:
> "cronet_c_for_grpc.h" & "Cronet.h" are included in
source_set("cronet_sources").
> Should they be listed here as well?

ios_framework_bundle requires for some reason sources section.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:36: +
(void)setHostResolverRulesForTesting:(NSString*)hostResolverRulesForTesting;
On 2016/08/10 17:47:40, kapishnikov wrote:
> I wonder if we can somehow hide "...ForTesting" methods. Maybe we can move
them
> to the subclass?

Great idea! I've extracted them into Cronet(TestSupport)..

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:61: // requests done through shared NSURLSession.
On 2016/08/10 17:47:40, kapishnikov wrote:
> This can fit in the previous line.

Done.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:70: // Installs Croet into an NSURLSession,
passed in by the caller. Note that this
On 2016/08/10 17:47:40, kapishnikov wrote:
> Croer=>Cronet

Done.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:71: // NSURLSession will share settings with the
sharedSession, which the
On 2016/08/10 17:47:40, kapishnikov wrote:
> The shared session has its own internal configuration. It is not clear, how
this
> method impacts it. Also, a single NSURLSessionConfiguration can be used for
> multiple sessions. Should we say that this method enables Cronet in
> NSURLSessionConfiguration so that all sessions created with this configuration
> will use the Cronet stack?

Done.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.h:99: + (NSData*)getGlobalMetricsDeltas;
On 2016/08/10 17:47:40, kapishnikov wrote:
> What is the structure of NSData? What should the user do with it?

Done.

[commit-bot: I haz the power, 2016-08-11 21:22:29]

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-08-11 23:24:13]

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

[commit-bot: I haz the power, 2016-08-11 23:24:14]

Dry run: This issue passed the CQ dry run.

[kapishnikov, 2016-08-12 17:19:31]

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.h:42: +
(void)setRequestFilterBlock:(RequestFilterBlock)block;
We should mention in the doc whether the method keeps the strong or weak
reference to the block.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.h:62: // Installs Cronet into Cronet into
NSURLSessionConfiguration so that all
Remove repeating "Cronet into"

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.h:64: // Note that this NSURLSession will share
Cronet settings with the
Maybe we should mention that Cronet settings are global and are shared between
all NSURLSessions & NSURLConnections that use the Cronet stack.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:45: : getter_(getter), filter_(filter,
base::scoped_policy::RETAIN) {}
As previously noted, RETAIN can potentially create a cycle.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:172: [NSURLCache setSharedURLCache:[[[NSURLCache
alloc] init] autorelease]];
As noted before, [[NSURLCache alloc] init] may not result in the same NSURLCache
that was set before registering the handler.

[mef, 2016-08-26 14:58:07]

Thanks, PTAL.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:33: std::unique_ptr<net::CertVerifier>
gMockCertVerifierForTesting;
On 2016/08/10 17:47:41, kapishnikov wrote:
> Can we rely on Objective-C++ ARC instead of std::unique_ptr? 

moved into CronetTestSupport.mm, changed to bool.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:45: : getter_(getter), filter_(filter,
base::scoped_policy::RETAIN) {}
On 2016/08/10 17:47:41, kapishnikov wrote:
> RequestFilterBlock is retained by Cronet and at the same time the block
supplied
> by the user can reference Cronet. This can create a cycle and the objects will
> never be released. It is probably okay in this situation. Will it be better if
> we do not retain the block and describe it in the setRequestFilterBlock method
> documentation?

Now allow to call setFilterBlock multiple times to release previous block.

https://codereview.chromium.org/2146643002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:172: [NSURLCache setSharedURLCache:[[[NSURLCache
alloc] init] autorelease]];
On 2016/08/10 17:47:41, kapishnikov wrote:
> Should we remember the old value of NSURLCache in registerHttpProtocolHandler
> and restore it here? I am not sure that [[NSURLCache alloc] init]] will
produce
> the desired object. The proper NSURLCache initializer is:
> initWithMemoryCapacity:(NSUInteger)memoryCapacity
>           diskCapacity:(NSUInteger)diskCapacity
>               diskPath:(NSString *)path
> It is probably called with specific parameters somewhere by the system .
> 

Done.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.h:42: +
(void)setRequestFilterBlock:(RequestFilterBlock)block;
On 2016/08/12 17:19:31, kapishnikov wrote:
> We should mention in the doc whether the method keeps the strong or weak
> reference to the block. 

Done. Changed it to reset block after start, so it could be released.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.h:62: // Installs Cronet into Cronet into
NSURLSessionConfiguration so that all
On 2016/08/12 17:19:31, kapishnikov wrote:
> Remove repeating "Cronet into"

Done.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.h:64: // Note that this NSURLSession will share
Cronet settings with the
On 2016/08/12 17:19:31, kapishnikov wrote:
> Maybe we should mention that Cronet settings are global and are shared between
> all NSURLSessions & NSURLConnections that use the Cronet stack.

Done.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:45: : getter_(getter), filter_(filter,
base::scoped_policy::RETAIN) {}
On 2016/08/12 17:19:31, kapishnikov wrote:
> As previously noted, RETAIN can potentially create a cycle.

Ack. Changed it to allow setting new (nil) block to release.

https://codereview.chromium.org/2146643002/diff/240001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:172: [NSURLCache setSharedURLCache:[[[NSURLCache
alloc] init] autorelease]];
On 2016/08/12 17:19:31, kapishnikov wrote:
> As noted before, [[NSURLCache alloc] init] may not result in the same
NSURLCache
> that was set before registering the handler.

Done.

[mef, 2016-08-31 17:24:37]

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

[commit-bot: I haz the power, 2016-08-31 17:25:05]

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-08-31 17:28:21]

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

[commit-bot: I haz the power, 2016-08-31 17:28:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mef, 2016-08-31 18:49:38]

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

[commit-bot: I haz the power, 2016-08-31 18:50:00]

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-08-31 20:17:10]

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

[commit-bot: I haz the power, 2016-08-31 20:17:11]

Dry run: This issue passed the CQ dry run.

[mef, 2016-10-11 17:43:05]

mef@chromium.org changed reviewers:
+ lilyhoughton@chromium.org

[mef, 2016-10-17 22:16:50]

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

[commit-bot: I haz the power, 2016-10-17 22:17:38]

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-10-17 23:52:56]

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

[commit-bot: I haz the power, 2016-10-17 23:52:58]

Dry run: This issue passed the CQ dry run.

[mef, 2016-10-18 14:14:28]

Andrei, PTAL.

I've removed CronetTestSupport as we need TestCertVerifier and HostMappingRules
in Cronet.framework to support GRPC testing.

I've also added Resources for Accept-Language, but per our discussion it may be
better to generate a simple mapping from language to Accept-Language and avoid
adding resources at all. I'll be happy to remove this now and deal with in
follow-up CL.

I've also added Http test server and ported over all existing tests for
CrNet.framework. I've also changed Cronet_test to use Cronet.framework instead
of directly linking with Cronet sources.

[mef, 2016-10-18 19:47:35]

mef@chromium.org changed reviewers:
+ sdefresne@chromium.org

[mef, 2016-10-18 19:50:02]

mef@chromium.org changed reviewers:
+ marq@chromium.org
- sdefresne@chromium.org

[mef, 2016-10-18 19:51:03]

Description was changed from

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.
==========

to

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.

BUG=654177
==========

[mef, 2016-10-18 19:51:03]

mef@chromium.org changed reviewers:
+ sdefresne@chromium.org
- marq@chromium.org

[kapishnikov, 2016-10-19 18:28:53]

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:29: // |userAgent| is expected to be the user
agent value sent to remote.
Not sure about the use of 'expected' here. Maybe better: "Sets the user-agent
string to be sent with the requests"?

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:35: // If partial is set to NO, then |userAgent|
value is complete value sent to
partial -> |partial|

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:45: // Set the block used to determine whether or
not Cronet should handle the
Set->Sets

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:46: // request. If this is not set, Cronet will
handle all requests. Cronet retains
this->'the block'

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:59: // and handles all NSURLConnection and
NSURLRequests issued by the app,
Since
1) NSURLRequest is merely a value objects that represents a URL and
2) NSURLSession is not part of NSURLConnection,
I think it is more correct to say that: "Once registered, Cronet intercepts and
handles all requests made through  NSURLConnection and shared NSURLSession.'

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:96: // <a
href=https://developers.google.com/protocol-buffers>protobuf</a>.
Xcode uses markup to specify links: see
https://developer.apple.com/library/content/documentation/Xcode/Reference/xco...

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:101: // differences in metrics collected by
Cronet, since the last call
I think this line should not be here.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:110: + (void)enableTestCertVerifierForTesting;
I remember we discussed moving these methods to TestCronet class that would
inherit from Cronet class. If this idea is viable, maybe we should do it in a
next CL.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:126: acceptLanguages = @"";
Why don't we set @"en-US,en" here? Also, should we log an error if
acceptLanguages is somehow Nil. I guess that can happen if the bundle cannot be
found.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:161: +
(void)setRequestFilterBlock:(RequestFilterBlock)block {
Is this method thread safe? What if there are pending requests that are being
processed? I think we should synchronize SetRequestFilterBlock and
CanHandleRequest inside CronetHttpProtocolHandlerDelegate.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:229: config.protocolClasses = @[
[CRNPauseableHTTPProtocolHandler class] ];
Should we append to the list of protocols rather than replacing it? E.g., a
client may have a custom protocol that handles "foo://". Calling this method
will remove it.

Can we also check that this approach indeed works? The documentation implies
that the default handlers have higher priority: "Prior to handling a request, an
URLSession object searches the default protocols first and then checks your
custom protocols until it finds one capable of handling the specified request. "
I wonder whether "the default protocols" are the ones registered with
[NSURLProtocol registerClass]. If yes, then registerHttpProtocolHandler method
impacts not only the shared NSURLSession but the custom sessions too.

https://developer.apple.com/reference/foundation/nsurlsessionconfiguration/14...

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:272: gEnableTestCertVerifierForTesting = TRUE;
TRUE -> YES

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:282: user_agent_partial_ = false;
Why do we set it here to 'false'? I assume InitializeOnNetworkThread cannot be
called twice.

[sdefresne, 2016-10-19 19:06:14]

What do you want me to review?

[mef, 2016-10-19 20:12:59]

Andrei: PTAL
Sylivain: Could you OWNER-approve use of //ios/net in Cronet.mm and //ios/web in
cronet_enviornment.cc (both added to DEPS)?

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:29: // |userAgent| is expected to be the user
agent value sent to remote.
On 2016/10/19 18:28:53, kapishnikov wrote:
> Not sure about the use of 'expected' here. Maybe better: "Sets the user-agent
> string to be sent with the requests"?

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:35: // If partial is set to NO, then |userAgent|
value is complete value sent to
On 2016/10/19 18:28:53, kapishnikov wrote:
> partial -> |partial|

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:45: // Set the block used to determine whether or
not Cronet should handle the
On 2016/10/19 18:28:53, kapishnikov wrote:
> Set->Sets

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:46: // request. If this is not set, Cronet will
handle all requests. Cronet retains
On 2016/10/19 18:28:53, kapishnikov wrote:
> this->'the block'

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:59: // and handles all NSURLConnection and
NSURLRequests issued by the app,
On 2016/10/19 18:28:53, kapishnikov wrote:
> Since
> 1) NSURLRequest is merely a value objects that represents a URL and
> 2) NSURLSession is not part of NSURLConnection,
> I think it is more correct to say that: "Once registered, Cronet intercepts
and
> handles all requests made through  NSURLConnection and shared NSURLSession.'

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:96: // <a
href=https://developers.google.com/protocol-buffers>protobuf</a>.
On 2016/10/19 18:28:53, kapishnikov wrote:
> Xcode uses markup to specify links: see
>
https://developer.apple.com/library/content/documentation/Xcode/Reference/xco...

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:101: // differences in metrics collected by
Cronet, since the last call
On 2016/10/19 18:28:53, kapishnikov wrote:
> I think this line should not be here.

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.h:110: + (void)enableTestCertVerifierForTesting;
On 2016/10/19 18:28:53, kapishnikov wrote:
> I remember we discussed moving these methods to TestCronet class that would
> inherit from Cronet class. If this idea is viable, maybe we should do it in a
> next CL.

Yes, I would like to revisit that idea if we can create two frameworks.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:126: acceptLanguages = @"";
On 2016/10/19 18:28:53, kapishnikov wrote:
> Why don't we set @"en-US,en" here? Also, should we log an error if
> acceptLanguages is somehow Nil. I guess that can happen if the bundle cannot
be
> found.

My motivation is that if we run into some unknown language it may be better to
omit Accept-Languages completely, so server would use its own preference (e.g.
Mongolian app connecting to Mongolian server).

I also wonder whether some apps would not want Accept-Languages to be determined
automatically at all, and will remove cronet_resources bundle. WDYT?

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:161: +
(void)setRequestFilterBlock:(RequestFilterBlock)block {
On 2016/10/19 18:28:53, kapishnikov wrote:
> Is this method thread safe? What if there are pending requests that are being
> processed? I think we should synchronize SetRequestFilterBlock and
> CanHandleRequest inside CronetHttpProtocolHandlerDelegate.

Good catch! Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:229: config.protocolClasses = @[
[CRNPauseableHTTPProtocolHandler class] ];
On 2016/10/19 18:28:53, kapishnikov wrote:
> Should we append to the list of protocols rather than replacing it? E.g., a
> client may have a custom protocol that handles "foo://". Calling this method
> will remove it.
But wouldn't this require Cronet to handle that custom protocol? Besides, at
this point I'm just trying to integrate existing functionality of CrNet, not
change it.
> 
> Can we also check that this approach indeed works? The documentation implies
> that the default handlers have higher priority: "Prior to handling a request,
an
> URLSession object searches the default protocols first and then checks your
> custom protocols until it finds one capable of handling the specified request.
"
> I wonder whether "the default protocols" are the ones registered with
> [NSURLProtocol registerClass]. If yes, then registerHttpProtocolHandler method
> impacts not only the shared NSURLSession but the custom sessions too.
> 
>
https://developer.apple.com/reference/foundation/nsurlsessionconfiguration/14...
Good question, I think it is deserves additional tests, maybe in a followup CL.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:272: gEnableTestCertVerifierForTesting = TRUE;
On 2016/10/19 18:28:53, kapishnikov wrote:
> TRUE -> YES

Done.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:282: user_agent_partial_ = false;
On 2016/10/19 18:28:53, kapishnikov wrote:
> Why do we set it here to 'false'? I assume InitializeOnNetworkThread cannot be
> called twice.

Good point. Done. I thought about moving it into constructor, but it seems that
BuildUserAgentFromProduct could do something non-trivial. 

Sylvain, do you know if it is ok to call web::BuildUserAgentFromProduct() on any
thread, including main?

[kapishnikov, 2016-10-19 21:12:29]

The change looks good. We can address the open issues in a follow up CL. LGTM

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:126: acceptLanguages = @"";
On 2016/10/19 20:12:59, mef wrote:
> On 2016/10/19 18:28:53, kapishnikov wrote:
> > Why don't we set @"en-US,en" here? Also, should we log an error if
> > acceptLanguages is somehow Nil. I guess that can happen if the bundle cannot
> be
> > found.
> 
> My motivation is that if we run into some unknown language it may be better to
> omit Accept-Languages completely, so server would use its own preference (e.g.
> Mongolian app connecting to Mongolian server).
> 
> I also wonder whether some apps would not want Accept-Languages to be
determined
> automatically at all, and will remove cronet_resources bundle. WDYT?

I think it is a good idea to give the client an option to set an arbitrary
Accept-Language value; otherwise, determine it automatically. Since we package
the resources inside the framework bundle, I think we should not recommend
removing the resources manually. It will definitely break the signature and
Xcode may not like it.

Btw, according to
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.4 ,
Accept-Languages can contain multiple languages with different weights.

https://codereview.chromium.org/2146643002/diff/500001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:229: config.protocolClasses = @[
[CRNPauseableHTTPProtocolHandler class] ];
On 2016/10/19 20:12:59, mef wrote:
> On 2016/10/19 18:28:53, kapishnikov wrote:
> > Should we append to the list of protocols rather than replacing it? E.g., a
> > client may have a custom protocol that handles "foo://". Calling this method
> > will remove it.
> But wouldn't this require Cronet to handle that custom protocol? Besides, at
> this point I'm just trying to integrate existing functionality of CrNet, not
> change it.
> > 
Cronet handles "http://", "https://" and "data://" but not "foo://". Maybe we
should keep any existig protocols handlers added by the client and prepend
Cronet protocol to the front of the list (or back depending on how the order is
determined). Anyways, I think it is a corner case and can be addressed later.

> > Can we also check that this approach indeed works? The documentation implies
> > that the default handlers have higher priority: "Prior to handling a
request,
> an
> > URLSession object searches the default protocols first and then checks your
> > custom protocols until it finds one capable of handling the specified
request.
> "
> > I wonder whether "the default protocols" are the ones registered with
> > [NSURLProtocol registerClass]. If yes, then registerHttpProtocolHandler
method
> > impacts not only the shared NSURLSession but the custom sessions too.
> > 
> >
>
https://developer.apple.com/reference/foundation/nsurlsessionconfiguration/14...
> Good question, I think it is deserves additional tests, maybe in a followup
CL.

Sounds good.

[sdefresne, 2016-10-19 22:02:43]

I won't have time to review today. Can you ping me if I have not reviewed
tomorrow (let's say in 20h)?

[mef, 2016-10-19 22:23:03]

On 2016/10/19 22:02:43, sdefresne wrote:
> I won't have time to review today. Can you ping me if I have not reviewed
> tomorrow (let's say in 20h)?

Sounds good, thanks a lot!

[sdefresne, 2016-10-20 19:00:43]

> Sylvain: Could you OWNER-approve use of //ios/net in Cronet.mm and //ios/web
in
> cronet_enviornment.cc (both added to DEPS)?

lgtm

https://codereview.chromium.org/2146643002/diff/540001/components/cronet/ios/...
File components/cronet/ios/BUILD.gn (right):

https://codereview.chromium.org/2146643002/diff/540001/components/cronet/ios/...
components/cronet/ios/BUILD.gn:78: "Resources/Localization/am.lproj",
Can you use //ios/chrome/tools/strings/generate_localizable_strings.gni to
generate those files? The code has no dependency on ios/chrome, so it could be
moved to //ios/tools or some other place that can be used by this component if
necessary.

Probably better to do in a follow-up CL though.

[mef, 2016-10-20 19:30:08]

On 2016/10/20 19:00:43, sdefresne wrote:
> > Sylvain: Could you OWNER-approve use of //ios/net in Cronet.mm and //ios/web
> in
> > cronet_enviornment.cc (both added to DEPS)?
> 
> lgtm
> 
>
https://codereview.chromium.org/2146643002/diff/540001/components/cronet/ios/...
> File components/cronet/ios/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2146643002/diff/540001/components/cronet/ios/...
> components/cronet/ios/BUILD.gn:78: "Resources/Localization/am.lproj",
> Can you use //ios/chrome/tools/strings/generate_localizable_strings.gni to
> generate those files? The code has no dependency on ios/chrome, so it could be
> moved to //ios/tools or some other place that can be used by this component if
> necessary.
> 
> Probably better to do in a follow-up CL though.

Thanks a lot! I've filed crbug.com/655689 to improve it. 
Ideally I would like to avoid using string resources for this as we could use a
simple table lookup.

[mef, 2016-10-20 20:30:16]

The CQ bit was checked by mef@chromium.org

[mef, 2016-10-20 20:30:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2146643002/#ps560001
(title: "Remove cronet_test_bundle_data target and use data bundled with
net_test_support.")

[commit-bot: I haz the power, 2016-10-20 20:30:52]

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-10-20 21:48:33]

Description was changed from

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.

BUG=654177
==========

to

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.

BUG=654177
==========

[commit-bot: I haz the power, 2016-10-20 21:48:39]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2016-10-21 13:23:12]

Description was changed from

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.

BUG=654177
==========

to

==========
[Cronet] Integrate CrNet functionality into Cronet on iOS.

- Add NSURLSession handling to match CrNet.
- Add GetGlobalMetricsDeltas to match Android API.
- Add tests for new functionality.

BUG=654177

Committed: https://crrev.com/254963db8e5f58c0d1279cf29bb1f7d48043cad2
Cr-Commit-Position: refs/heads/master@{#426609}
==========

[commit-bot: I haz the power, 2016-10-21 13:23:13]

Patchset 29 (id:??) landed as
https://crrev.com/254963db8e5f58c0d1279cf29bb1f7d48043cad2
Cr-Commit-Position: refs/heads/master@{#426609}