Cronet for iOS with C API for GRPC support.

Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
- Define CronetBidirectionalStream C++ class based on Android CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=601972
Committed: https://crrev.com/06eab3e8e64eacef30f15299ee7c3ad32df2840b
Cr-Commit-Position: refs/heads/master@{#387467}

[mef, 2016-04-03 19:25:51]

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

[mef, 2016-04-03 19:25:52]

Hi Andrei, PTAL. 

I've rebased my CL onto your net_small CL, but I'm not sure why it still shows
differences in net and url.

thanks,
-m

[mef, 2016-04-05 16:32:58]

Description was changed from

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
 - Remove ICU dependencies (no proper replacement yet).
- Remove FTP, FILE and Web Socket support.
- Rename CrNet -> CronetEngine.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=

patch from issue 1759293002 at patchset 540001
(http://crrev.com/1759293002#ps540001)
==========

to

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
 - Remove ICU dependencies (no proper replacement yet).
- Remove FTP, FILE and Web Socket support.
- Rename CrNet -> CronetEngine.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=

patch from issue 1759293002 at patchset 540001
(http://crrev.com/1759293002#ps540001)
==========

[mef, 2016-04-05 16:32:58]

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

[kapishnikov, 2016-04-06 17:33:26]

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_engine.h (right):

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.h:10: @interface CronetEngine : NSObject
Should we consider making all CronetEngine methods non-static? We can use the
singleton pattern if we do not want the client to create multiple instances of
CronetEngine. It may help with testing and moving to multi-engine support in the
future.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.h:31: + (void)install;
In CrNet we use install() method to install NSUrlConnection protocol handler, so
the name of the method corresponds to the action. In CronetEngine we don't
install the handler. Should we rename the method to something like start()?

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.h:46: + (NSString*)userAgent;
This can be an Objective C property. Otherwise, I would rename it to
getUserAgent().

@property (nonatomic, readonly) NSString *userAgent;

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_engine.mm (right):

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:13: static BOOL g_http2_enabled = YES;
The convention is to use mixed case to delimit words in the variable names. It
would be great to convert them to instance variables and start the names with
underscore.
https://engdoc.corp.google.com/eng/doc/objcguide.xml?cl=head#Variable_Names

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:20: +
(void)setHttp2Enabled:(BOOL)http2Enabled {
We could add the verification that the method is not called after the engine was
installed/started. It least we should log an error message. Same for all
setters.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:56: if (g_chrome_net && [fileName
length]) {
It would be great to log full path to the netlog file, so it can be easily
found. Also, if somebody calls startNetLogToFile but it cannot be started, we
should at least log an error.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:63: return g_chrome_net->StopNetLog();
The method return type is void. Does this line generate a warning?

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:43: //
SetHTTPProtocolHandlerRegistered.
I think the comments should not mention the handler.

[xunjieli, 2016-04-06 21:27:20]

Very sorry about the delay! I am planning to get to this tomorrow. But it will
probably take me some time, since the iOS bits are very foreign to me.

[mef, 2016-04-06 21:31:15]

On 2016/04/06 21:27:20, xunjieli wrote:
> Very sorry about the delay! I am planning to get to this tomorrow. But it will
> probably take me some time, since the iOS bits are very foreign to me.

No problem, I didn't really get to work on it today either.
FWIW, most interesting bits are in .cc files (the test is .mm, but it is mostly
normal C/C++).

[xunjieli, 2016-04-08 14:43:16]

https://codereview.chromium.org/1858483002/diff/40001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet.gypi#...
components/cronet.gypi:792: '-DLOGGING=1',
Does this mean that we will have verbose logging by default?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet.gypi#...
components/cronet.gypi:844: # TODO(crbug.com/571217): Disabled warning-as-error
option until
This bug is already fixed.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:7: #include <stdbool.h>
Why do we need to include stdbool?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:14: #include
"base/macros.h"
not needed?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:27: #include
"net/http/http_transaction_factory.h"
Not needed?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:79: new
net::WrappedIOBuffer(buffer));
Should we write to |read_buffer_| here, and use that member variable directly
on network thread? DCHECK to make sure it is not null.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:94: new
net::WrappedIOBuffer(buffer));
Should we write to |write_buffer_| here, and use that member variable directly
on network thread? DCHECK to make sure it is not null.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.h (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.h:85: * States of
BidirectionalStream are tracked in read_state_ and write_state_.
nit: add pipes? |read_state_| and |write_state_|

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.h:91: */
All the comments should start with double slash? instead of the Java style?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_c_for_grpc.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_c_for_grpc.cc:264: int
cronet_bidirectional_stream_destroy(cronet_bidirectional_stream* stream) {
Theses functions are in global namespace. Do we need a cronet namespace?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:252: main_context_.reset(new
net::URLRequestContext);
Can we use net::URLRequestContextBuilder?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:27: }
//  namespace net

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:97: void
StartNetLogInternal(base::FilePath::StringType file_name,
const base::FilePath::StringType& ?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:101: // Returns the
HttpNeteworkSession object from the passed in
typo in HttpNetworkSession

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:106: // Helper method that closes all
current http2 sessions on the network IO
HTTP/2

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:94:
std::map<std::string, std::string> request_headers;
#include <string> <map> <list>?

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:146:
DCHECK(stream == this->stream);
DCHECK_EQ

[mef, 2016-04-08 15:13:01]

Thanks, PTAL. I've renamed cronet_engine.* -> Cronet.*

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_engine.h (right):

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.h:10: @interface CronetEngine : NSObject
On 2016/04/06 17:33:25, kapishnikov wrote:
> Should we consider making all CronetEngine methods non-static? We can use the
> singleton pattern if we do not want the client to create multiple instances of
> CronetEngine. It may help with testing and moving to multi-engine support in
the
> future.

Yes. For the purpose of this CL I kept it close to original CrNet interface, but
we should definitely refactor this afterwards.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.h:31: + (void)install;
On 2016/04/06 17:33:25, kapishnikov wrote:
> In CrNet we use install() method to install NSUrlConnection protocol handler,
so
> the name of the method corresponds to the action. In CronetEngine we don't
> install the handler. Should we rename the method to something like start()?

Done.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.h:46: + (NSString*)userAgent;
On 2016/04/06 17:33:25, kapishnikov wrote:
> This can be an Objective C property. Otherwise, I would rename it to
> getUserAgent().
> 
> @property (nonatomic, readonly) NSString *userAgent;

Done.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_engine.mm (right):

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:13: static BOOL g_http2_enabled = YES;
On 2016/04/06 17:33:25, kapishnikov wrote:
> The convention is to use mixed case to delimit words in the variable names. It
> would be great to convert them to instance variables and start the names with
> underscore.
> https://engdoc.corp.google.com/eng/doc/objcguide.xml?cl=head#Variable_Names

Done. I'd do instance variables in another CL.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:20: +
(void)setHttp2Enabled:(BOOL)http2Enabled {
On 2016/04/06 17:33:25, kapishnikov wrote:
> We could add the verification that the method is not called after the engine
was
> installed/started. It least we should log an error message. Same for all
> setters.

Done.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:56: if (g_chrome_net && [fileName
length]) {
On 2016/04/06 17:33:25, kapishnikov wrote:
> It would be great to log full path to the netlog file, so it can be easily
> found. Also, if somebody calls startNetLogToFile but it cannot be started, we
> should at least log an error.

Full path is logged in CronetEnvironment::StartNetLogInternal. I'll add error
logs there as well.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_engine.mm:63: return g_chrome_net->StopNetLog();
On 2016/04/06 17:33:25, kapishnikov wrote:
> The method return type is void. Does this line generate a warning?

Done.

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:43: //
SetHTTPProtocolHandlerRegistered.
On 2016/04/06 17:33:25, kapishnikov wrote:
> I think the comments should not mention the handler.

Done.

[mef, 2016-04-08 16:43:03]

Thanks, PTAL.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet.gypi#...
components/cronet.gypi:792: '-DLOGGING=1',
On 2016/04/08 14:43:15, xunjieli wrote:
> Does this mean that we will have verbose logging by default? 

Good question. I carried this over from cronet_static.gypi, but I'm not sure
what it does. Removed.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet.gypi#...
components/cronet.gypi:844: # TODO(crbug.com/571217): Disabled warning-as-error
option until
On 2016/04/08 14:43:15, xunjieli wrote:
> This bug is already fixed.

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:7: #include <stdbool.h>
On 2016/04/08 14:43:15, xunjieli wrote:
> Why do we need to include stdbool?

I think this is carry-over from when it used to be an .mm. Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:14: #include
"base/macros.h"
On 2016/04/08 14:43:15, xunjieli wrote:
> not needed?

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:27: #include
"net/http/http_transaction_factory.h"
On 2016/04/08 14:43:15, xunjieli wrote:
> Not needed?

Needed.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:79: new
net::WrappedIOBuffer(buffer));
On 2016/04/08 14:43:15, xunjieli wrote:
> Should we write to |read_buffer_| here, and use that member variable directly
> on network thread? DCHECK to make sure it is not null.

I don't think so. By accessing |read_buffer_| only on the network thread we
avoid the need to lock/synchronize the access.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:94: new
net::WrappedIOBuffer(buffer));
On 2016/04/08 14:43:15, xunjieli wrote:
> Should we write to |write_buffer_| here, and use that member variable directly
> on network thread? DCHECK to make sure it is not null.

ditto.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.h (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.h:85: * States of
BidirectionalStream are tracked in read_state_ and write_state_.
On 2016/04/08 14:43:15, xunjieli wrote:
> nit: add pipes? |read_state_| and |write_state_|

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.h:91: */
On 2016/04/08 14:43:15, xunjieli wrote:
> All the comments should start with double slash? instead of the Java style?

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_c_for_grpc.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_c_for_grpc.cc:264: int
cronet_bidirectional_stream_destroy(cronet_bidirectional_stream* stream) {
On 2016/04/08 14:43:15, xunjieli wrote:
> Theses functions are in global namespace. Do we need a cronet namespace?

C doesn't support namespaces. That's why they have long function names. :)

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:252: main_context_.reset(new
net::URLRequestContext);
On 2016/04/08 14:43:15, xunjieli wrote:
> Can we use net::URLRequestContextBuilder?

Yes, and we totally should, just not in this CL. This is based on CrNet/iOS to
minimize changes.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:27: }
On 2016/04/08 14:43:16, xunjieli wrote:
> //  namespace net

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:97: void
StartNetLogInternal(base::FilePath::StringType file_name,
On 2016/04/08 14:43:15, xunjieli wrote:
> const base::FilePath::StringType& ?

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:101: // Returns the
HttpNeteworkSession object from the passed in
On 2016/04/08 14:43:16, xunjieli wrote:
> typo in HttpNetworkSession

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:106: // Helper method that closes all
current http2 sessions on the network IO
On 2016/04/08 14:43:16, xunjieli wrote:
> HTTP/2

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:94:
std::map<std::string, std::string> request_headers;
On 2016/04/08 14:43:16, xunjieli wrote:
> #include <string> <map> <list>?

Done.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:146:
DCHECK(stream == this->stream);
On 2016/04/08 14:43:16, xunjieli wrote:
> DCHECK_EQ

Done.

[xunjieli, 2016-04-08 17:46:16]

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:27: #include
"net/http/http_transaction_factory.h"
On 2016/04/08 16:43:03, mef wrote:
> On 2016/04/08 14:43:15, xunjieli wrote:
> > Not needed?
> 
> Needed.

Acknowledged.

https://codereview.chromium.org/1858483002/diff/40001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:79: new
net::WrappedIOBuffer(buffer));
On 2016/04/08 16:43:02, mef wrote:
> On 2016/04/08 14:43:15, xunjieli wrote:
> > Should we write to |read_buffer_| here, and use that member variable
directly
> > on network thread? DCHECK to make sure it is not null.
> 
> I don't think so. By accessing |read_buffer_| only on the network thread we
> avoid the need to lock/synchronize the access.

Acknowledged.

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

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:5: #import "components/cronet/ios/Cronet.h"
newbie question: Why is this file is named as Cronet with a capitalized C?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:7: #include <vector>
not needed?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:15: static cronet::CronetEnvironment* gChromeNet
= NULL;
Can we add a comment here to say why we have a global CronetEnvironment and
whether we plan to support multiple CronetEnvironment in the future?

Can we use base::LazyInstance<cronet::CronetEnvironment>::Leaky ?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_bidirectional_stream.h (right):

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_bidirectional_stream.h:17: }
nit:  "// namespace net"

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

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:121: #pragma
unused(main_message_loop)
Can we get rid of this and add a base::MessageLoop* g_main_message_loop to the
top of the file?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:136: void
CronetEnvironment::StartNetLogInternal(
StartNetLog(OnNetworkThread) ?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:173: void
CronetEnvironment::StopNetLogInternal() {
StopNetLogOnNetworkThread?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:271: // objects.  See
http://crbug.com/523858.
I still feel we should convert to using net::URLRequestContextBuilder, and not
leave these unowned objects around.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:318: for (auto quic_hint :
quic_hints_) {
const auto& ?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:338: //                See
https://crbug.com/523858.
I think we should fix these leaky objects. Does converting to using
net::URLRequestContextBuilder changes a lot of things?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:32: static bool
initialized = false;
Why is this a static bool?

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

Thanks, PTAL.

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

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:5: #import "components/cronet/ios/Cronet.h"
On 2016/04/08 17:46:15, xunjieli wrote:
> newbie question: Why is this file is named as Cronet with a capitalized C?

In my integration prototype I've noticed that all objC modules use CamelCase
instead of lower_case naming.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:7: #include <vector>
On 2016/04/08 17:46:15, xunjieli wrote:
> not needed?

Done.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:15: static cronet::CronetEnvironment* gChromeNet
= NULL;
On 2016/04/08 17:46:15, xunjieli wrote:
> Can we add a comment here to say why we have a global CronetEnvironment and
> whether we plan to support multiple CronetEnvironment in the future?
Added comment.

> Can we use base::LazyInstance<cronet::CronetEnvironment>::Leaky ?
Maybe, what would be the advantage?

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_bidirectional_stream.h (right):

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_bidirectional_stream.h:17: }
On 2016/04/08 17:46:15, xunjieli wrote:
> nit:  "// namespace net"

Done.

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

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:121: #pragma
unused(main_message_loop)
On 2016/04/08 17:46:15, xunjieli wrote:
> Can we get rid of this and add a base::MessageLoop* g_main_message_loop to the
> top of the file?

Done.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:136: void
CronetEnvironment::StartNetLogInternal(
On 2016/04/08 17:46:15, xunjieli wrote:
> StartNetLog(OnNetworkThread) ?

Done.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:173: void
CronetEnvironment::StopNetLogInternal() {
On 2016/04/08 17:46:15, xunjieli wrote:
> StopNetLogOnNetworkThread?

Done.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:271: // objects.  See
http://crbug.com/523858.
On 2016/04/08 17:46:15, xunjieli wrote:
> I still feel we should convert to using net::URLRequestContextBuilder, and not
> leave these unowned objects around.

I agree, however I would like to unify CronetEngine with
CronetURLRequestContext{Adapter}, so the C++ API could share the net context
with Java API on Android.
Changing this now doesn't seem as a move in the right direction.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:318: for (auto quic_hint :
quic_hints_) {
On 2016/04/08 17:46:15, xunjieli wrote:
> const auto& ?

Done.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:338: //                See
https://crbug.com/523858.
On 2016/04/08 17:46:15, xunjieli wrote:
> I think we should fix these leaky objects. Does converting to using
> net::URLRequestContextBuilder changes a lot of things?

Yes, and I don't know. I'll try it out when/if I have a chance.

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:32: static bool
initialized = false;
On 2016/04/08 17:46:15, xunjieli wrote:
> Why is this a static bool?

It's a kludge because I'm having troubles with SetUp being called multiple times
during the test, and quic test server not shutting down / restarting gracefully.

[kapishnikov, 2016-04-08 23:54:18]

https://codereview.chromium.org/1858483002/diff/120001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1858483002/diff/120001/components/cronet.gypi...
components/cronet.gypi:810: 'target_name': 'cronet_test',
Should we rename the target to |cronet_ios_test| to be clear that it is  for iOS
only. Even better, if we can have a single target name for iOS and Android. What
if we rename this target and |cronet_test_instrumentation_apk| to
|cronet_test_app|?

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

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:50: + (NSString*)getUserAgent;
Nit: space between 'NSString' and '*'

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

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:60: std::string partial_user_agent =
base::SysNSStringToUTF8(gUserAgent);
Local variables should use mixed case to delimit words:
https://engdoc.corp.google.com/eng/doc/objcguide.xml?cl=head#Variable_Names

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:77: [self installInternal];
cronet::CronetEnvironment::Initialize() states that it should be invoked on the
main thread; therefore, [self installInternal] should be invoked on the main
thread too. Should we replace this with:

dispatch_once(&onceToken, ^{
  dispatch_sync(dispatch_get_main_queue(), ^(void) {
    [self installInternal];
  });
});

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:81: +
(void)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes {
Nit: space after 'NSString'

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
File components/cronet/tools/cr_cronet.py (right):

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
components/cronet/tools/cr_cronet.py:20: test_target = 'cronet_test'
It is better if the name stays the same (see the comment in cronet.gypi).

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
components/cronet/tools/cr_cronet.py:21: out_dir_suffix = '-iphonesimulator'
Should we add the ability to build for an iOS phisical device too?

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
components/cronet/tools/cr_cronet.py:115: if (options.command=='build-test'):
Should we add 'build-test' for iOS?

[kapishnikov, 2016-04-09 00:02:11]

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

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:77: [self installInternal];
On 2016/04/08 23:54:18, kapishnikov wrote:
> cronet::CronetEnvironment::Initialize() states that it should be invoked on
the
> main thread; therefore, [self installInternal] should be invoked on the main
> thread too. Should we replace this with:
> 
> dispatch_once(&onceToken, ^{
>   dispatch_sync(dispatch_get_main_queue(), ^(void) {
>     [self installInternal];
>   });
> });

Actually it will cause the deadlock if the method is called on the main thread.
So, it should be something like this:

dispatch_once(&onceToken, ^{
  if (![NSThread isMainThread]) {
    dispatch_sync(dispatch_get_main_queue(), ^(void) {
      [self installInternal];
    });
  } else {
    [self installInternal];
  }
});

[mef, 2016-04-09 00:52:29]

Thanks, PTAL when you have a chance.

https://codereview.chromium.org/1858483002/diff/120001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1858483002/diff/120001/components/cronet.gypi...
components/cronet.gypi:810: 'target_name': 'cronet_test',
On 2016/04/08 23:54:18, kapishnikov wrote:
> Should we rename the target to |cronet_ios_test| to be clear that it is  for
iOS
> only. Even better, if we can have a single target name for iOS and Android.
What
> if we rename this target and |cronet_test_instrumentation_apk| to
> |cronet_test_app|?

I like the idea, I'm not sure about the name. 
On android there is cronet_test_apk and cronet_test_instrumentation_apk which
drives it. 
Maybe |cronet_tester|? Also, running them is also different...

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

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:50: + (NSString*)getUserAgent;
On 2016/04/08 23:54:18, kapishnikov wrote:
> Nit: space between 'NSString' and '*'

I've used 'git cl format components/cronet', and disagrees. :)

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

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:60: std::string partial_user_agent =
base::SysNSStringToUTF8(gUserAgent);
On 2016/04/08 23:54:18, kapishnikov wrote:
> Local variables should use mixed case to delimit words:
> https://engdoc.corp.google.com/eng/doc/objcguide.xml?cl=head#Variable_Names

Done.

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:77: [self installInternal];
On 2016/04/09 00:02:11, kapishnikov wrote:
> On 2016/04/08 23:54:18, kapishnikov wrote:
> > cronet::CronetEnvironment::Initialize() states that it should be invoked on
> the
> > main thread; therefore, [self installInternal] should be invoked on the main
> > thread too. Should we replace this with:
> > 
> > dispatch_once(&onceToken, ^{
> >   dispatch_sync(dispatch_get_main_queue(), ^(void) {
> >     [self installInternal];
> >   });
> > });
> 
> Actually it will cause the deadlock if the method is called on the main
thread.
> So, it should be something like this:
> 
> dispatch_once(&onceToken, ^{
>   if (![NSThread isMainThread]) {
>     dispatch_sync(dispatch_get_main_queue(), ^(void) {
>       [self installInternal];
>     });
>   } else {
>     [self installInternal];
>   }
> });

Great point! I thought about it, but didn't know how to express it correctly. :)
Done.

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:81: +
(void)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes {
On 2016/04/08 23:54:18, kapishnikov wrote:
> Nit: space after 'NSString'

Acknowledged. git cl format.

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
File components/cronet/tools/cr_cronet.py (right):

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
components/cronet/tools/cr_cronet.py:20: test_target = 'cronet_test'
On 2016/04/08 23:54:18, kapishnikov wrote:
> It is better if the name stays the same (see the comment in cronet.gypi).

Yes, although its execution is different anyway.

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
components/cronet/tools/cr_cronet.py:21: out_dir_suffix = '-iphonesimulator'
On 2016/04/08 23:54:18, kapishnikov wrote:
> Should we add the ability to build for an iOS phisical device too?

Done.

https://codereview.chromium.org/1858483002/diff/120001/components/cronet/tool...
components/cronet/tools/cr_cronet.py:115: if (options.command=='build-test'):
On 2016/04/08 23:54:18, kapishnikov wrote:
> Should we add 'build-test' for iOS?

Done.

[mef, 2016-04-09 00:58:35]

Description was changed from

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
 - Remove ICU dependencies (no proper replacement yet).
- Remove FTP, FILE and Web Socket support.
- Rename CrNet -> CronetEngine.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=

patch from issue 1759293002 at patchset 540001
(http://crrev.com/1759293002#ps540001)
==========

to

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=601972
==========

[xunjieli, 2016-04-11 13:42:05]

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

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:15: static cronet::CronetEnvironment* gChromeNet
= NULL;
On 2016/04/08 20:40:43, mef wrote:
> On 2016/04/08 17:46:15, xunjieli wrote:
> > Can we add a comment here to say why we have a global CronetEnvironment and
> > whether we plan to support multiple CronetEnvironment in the future?
> Added comment.
> 
> > Can we use base::LazyInstance<cronet::CronetEnvironment>::Leaky ?
> Maybe, what would be the advantage?

See:
http://dev.chromium.org/developers/coding-style/important-abstractions-and-da...

The advantage of using LazyInstance::Leaky is that it is completely thread-safe,
so we will have exactly one gChromeNet even if initialize is called by two
different threads). There are other benefits like not slowing startup and
cleaning up during shutdown.

[mef, 2016-04-13 11:24:49]

Thanks, PTAL.

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

https://codereview.chromium.org/1858483002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:15: static cronet::CronetEnvironment* gChromeNet
= NULL;
On 2016/04/11 13:42:05, xunjieli wrote:
> On 2016/04/08 20:40:43, mef wrote:
> > On 2016/04/08 17:46:15, xunjieli wrote:
> > > Can we add a comment here to say why we have a global CronetEnvironment
and
> > > whether we plan to support multiple CronetEnvironment in the future?
> > Added comment.
> > 
> > > Can we use base::LazyInstance<cronet::CronetEnvironment>::Leaky ?
> > Maybe, what would be the advantage?
> 
> See:
>
http://dev.chromium.org/developers/coding-style/important-abstractions-and-da...
> 
> The advantage of using LazyInstance::Leaky is that it is completely
thread-safe,
> so we will have exactly one gChromeNet even if initialize is called by two
> different threads). There are other benefits like not slowing startup and
> cleaning up during shutdown.

Done.

[xunjieli, 2016-04-13 15:28:24]

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:20:
base::LazyInstance<scoped_ptr<cronet::CronetEnvironment>>::Leaky gChromeNet =
Hmm.. There isn't an example of using LazyInstance with scoped_ptr in code
search.

Should this be just base::LazyInstance<cronet::CronetEngine>::Leaky ?

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:76: int count);
nit: Maybe use |bytes_read|, which is a little more descriptive? I didn't know
what |count|
is until I read the docs.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:83: const char* data);
Is this char* useful without knowing how long the data is?

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:179: int count,
nit: Maybe use |buffer_length|, which is a little more descriptive? I didn't
know what |count| is until I read the docs.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:18: #include
"base/mac/scoped_block.h"
Not used?

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:29: #include
"net/base/sdch_manager.h"
Not used?

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:42: #include "net/sdch/sdch_owner.h"
Not used?

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:117: base::CommandLine::Init(0,
nullptr);
Why do we call base::CommandLine?

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:40: static void Initialize();
Why is this method static?
Why do we have both Initialize() and Install()? It isn't clear what the
differences are.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:46: // Installs this Cronet
environment. Can only be called once.
Can install() be called on any thread? Maybe document the requirement?

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:115: static CronetEnvironment*
chrome_net_;
Why is this "static"?

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
File components/cronet/ios/test/quic_test_server.cc (right):

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/test/quic_test_server.cc:24: //
components/cronet/android/test/assets/test/quic_data/simple.txt).
nit: simple.txt isn't used.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/test/quic_test_server.cc:41: file_dir.value());
If we can't serve files out of disk, can we add some simple response to
QuicInMemoryCache and make sure we get something that is not 404?

QuicInMemoryCache::GetInstance()::AddResponse can add a response with trailers
as well.

[mef, 2016-04-14 14:07:24]

Thanks! I'll add better responses to QuicServer and corresponding tests in the
next patch.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:20:
base::LazyInstance<scoped_ptr<cronet::CronetEnvironment>>::Leaky gChromeNet =
On 2016/04/13 15:28:23, xunjieli wrote:
> Hmm.. There isn't an example of using LazyInstance with scoped_ptr in code
> search.

There are a few with scoped_refptr though, for example static
base::LazyInstance<scoped_refptr<MemoryInfoProvider> > provider_;
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

> Should this be just base::LazyInstance<cronet::CronetEngine>::Leaky ?

Can't use LazyInstance<cronet::CronetEngine> because we need custom constructor,
and also most setters are only applicable until singleton is created.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:76: int count);
On 2016/04/13 15:28:24, xunjieli wrote:
> nit: Maybe use |bytes_read|, which is a little more descriptive? I didn't know
> what |count|
> is until I read the docs. 

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:83: const char* data);
On 2016/04/13 15:28:23, xunjieli wrote:
> Is this char* useful without knowing how long the data is?

I think so, it lets the app know that buffer is ready to be freed/reused.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:18: #include
"base/mac/scoped_block.h"
On 2016/04/13 15:28:24, xunjieli wrote:
> Not used?

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:29: #include
"net/base/sdch_manager.h"
On 2016/04/13 15:28:24, xunjieli wrote:
> Not used?

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:42: #include "net/sdch/sdch_owner.h"
On 2016/04/13 15:28:24, xunjieli wrote:
> Not used?

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:117: base::CommandLine::Init(0,
nullptr);
On 2016/04/13 15:28:24, xunjieli wrote:
> Why do we call base::CommandLine?

I don't really know. It was this way in CrNet.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
File components/cronet/ios/test/quic_test_server.cc (right):

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/test/quic_test_server.cc:24: //
components/cronet/android/test/assets/test/quic_data/simple.txt).
On 2016/04/13 15:28:24, xunjieli wrote:
> nit: simple.txt isn't used.

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/test/quic_test_server.cc:41: file_dir.value());
On 2016/04/13 15:28:24, xunjieli wrote:
> If we can't serve files out of disk, can we add some simple response to
> QuicInMemoryCache and make sure we get something that is not 404?
> 
> QuicInMemoryCache::GetInstance()::AddResponse can add a response with trailers
> as well.

Great idea! I'll add that and more tests in the next patch.

[mef, 2016-04-14 14:07:26]

Thanks! I'll add better responses to QuicServer and corresponding tests in the
next patch.

[kapishnikov, 2016-04-14 15:16:55]

LGTM

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

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:29: void CheckNotStarted() {
We could declare this method as:
+ (void)checkNotStarted {
    CHECK(gChromeNet == NULL) << "Cronet is already started.";
}
, and move it into the implementation part.

[xunjieli, 2016-04-14 15:43:39]

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:20:
base::LazyInstance<scoped_ptr<cronet::CronetEnvironment>>::Leaky gChromeNet =
On 2016/04/14 14:07:24, mef wrote:
> On 2016/04/13 15:28:23, xunjieli wrote:
> > Hmm.. There isn't an example of using LazyInstance with scoped_ptr in code
> > search.
> 
> There are a few with scoped_refptr though, for example static
> base::LazyInstance<scoped_refptr<MemoryInfoProvider> > provider_;
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> > Should this be just base::LazyInstance<cronet::CronetEngine>::Leaky ?
> 
> Can't use LazyInstance<cronet::CronetEngine> because we need custom
constructor,
> and also most setters are only applicable until singleton is created.

I see. base/lazy_instance.h doesn't mention this use case or the scoped_refptr
case. Is it possible to move partialUserAgent as a setter? That way, we can use
the default constructor?

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
File components/cronet/ios/cronet_c_for_grpc.h (right):

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:179: int count,
nit: Maybe use |buffer_length|, which is a little more descriptive? I didn't
know what |count| is until I read the docs.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:40: static void Initialize();
Why is this method static?
Why do we have both Initialize() and Install()? It isn't clear what the
differences are.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:46: // Installs this Cronet
environment. Can only be called once.
Can install() be called on any thread? Maybe document the requirement?

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:115: static CronetEnvironment*
chrome_net_;
Why is this "static"?

[kapishnikov, 2016-04-14 15:54:57]

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

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:63: + (void)installInternal {
Rename this method to startInternal() ?

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:46: // Installs this Cronet
environment. Can only be called once.
On 2016/04/14 15:43:39, xunjieli wrote:
> Can install() be called on any thread? Maybe document the requirement?

Btw, should we rename Install() => Start() ?

[mef, 2016-04-14 21:17:36]

Thanks! I've addressed most of the comments, but I still need to add more tests
with quic responses.

I'm going to land this now as it doesn't affect anything outside of cronet/ios,
and kapishnikov@ will be able to fix any issues reported by grpc team.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:20:
base::LazyInstance<scoped_ptr<cronet::CronetEnvironment>>::Leaky gChromeNet =
On 2016/04/14 15:43:39, xunjieli wrote:
> On 2016/04/14 14:07:24, mef wrote:
> > On 2016/04/13 15:28:23, xunjieli wrote:
> > > Hmm.. There isn't an example of using LazyInstance with scoped_ptr in code
> > > search.
> > 
> > There are a few with scoped_refptr though, for example static
> > base::LazyInstance<scoped_refptr<MemoryInfoProvider> > provider_;
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > 
> > > Should this be just base::LazyInstance<cronet::CronetEngine>::Leaky ?
> > 
> > Can't use LazyInstance<cronet::CronetEngine> because we need custom
> constructor,
> > and also most setters are only applicable until singleton is created.
> 
> I see. base/lazy_instance.h doesn't mention this use case or the scoped_refptr
> case. Is it possible to move partialUserAgent as a setter? That way, we can
use
> the default constructor?
It is possible, I'm just not sure whether it is worth the trouble.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:179: int count,
On 2016/04/13 15:28:23, xunjieli wrote:
> nit: Maybe use |buffer_length|, which is a little more descriptive? I didn't
> know what |count| is until I read the docs. 

Done.

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

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:40: static void Initialize();
On 2016/04/13 15:28:24, xunjieli wrote:
> Why is this method static?
> Why do we have both Initialize() and Install()? It isn't clear what the
> differences are.

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:46: // Installs this Cronet
environment. Can only be called once.
On 2016/04/13 15:28:24, xunjieli wrote:
> Can install() be called on any thread? Maybe document the requirement?

Done.

https://codereview.chromium.org/1858483002/diff/200001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:115: static CronetEnvironment*
chrome_net_;
On 2016/04/13 15:28:24, xunjieli wrote:
> Why is this "static"?

Done.

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

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:29: void CheckNotStarted() {
On 2016/04/14 15:16:55, kapishnikov wrote:
> We could declare this method as:
> + (void)checkNotStarted {
>     CHECK(gChromeNet == NULL) << "Cronet is already started.";
> }
> , and move it into the implementation part.

Acknowledged.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:63: + (void)installInternal {
On 2016/04/14 15:54:57, kapishnikov wrote:
> Rename this method to startInternal() ?

Done.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
File components/cronet/ios/cronet_c_for_grpc.h (right):

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.h:179: int count,
On 2016/04/14 15:43:39, xunjieli wrote:
> nit: Maybe use |buffer_length|, which is a little more descriptive? I didn't
> know what |count| is until I read the docs. 

Done.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:46: // Installs this Cronet
environment. Can only be called once.
On 2016/04/14 15:54:57, kapishnikov wrote:
> On 2016/04/14 15:43:39, xunjieli wrote:
> > Can install() be called on any thread? Maybe document the requirement?
> 
> Btw, should we rename Install() => Start() ?

Done.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:46: // Installs this Cronet
environment. Can only be called once.
On 2016/04/14 15:43:39, xunjieli wrote:
> Can install() be called on any thread? Maybe document the requirement?

Done.

https://codereview.chromium.org/1858483002/diff/220001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:115: static CronetEnvironment*
chrome_net_;
On 2016/04/14 15:43:39, xunjieli wrote:
> Why is this "static"?

Done.

[mef, 2016-04-14 21:33:09]

The CQ bit was checked by mef@chromium.org

[mef, 2016-04-14 21:33:10]

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/1858483002/#ps280001
(title: "Address comments, bundle libboringssl.a into libcronet.a")

[commit-bot: I haz the power, 2016-04-14 21:34:01]

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

[commit-bot: I haz the power, 2016-04-14 22:45:58]

Description was changed from

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=601972
==========

to

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=601972
==========

[commit-bot: I haz the power, 2016-04-14 22:45:59]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-04-14 22:48:01]

Description was changed from

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=601972
==========

to

==========
Cronet for iOS with C API for GRPC support.

- Fork CrNet to preserve current functionality for existing consumers.
- Define CronetBidirectionalStream C++ class based on Android
CronetBidirectionalStreamAdapter.
- Define cronet_bidirectional_stream* C interface.

BUG=601972

Committed: https://crrev.com/06eab3e8e64eacef30f15299ee7c3ad32df2840b
Cr-Commit-Position: refs/heads/master@{#387467}
==========

[commit-bot: I haz the power, 2016-04-14 22:48:02]

Patchset 15 (id:??) landed as
https://crrev.com/06eab3e8e64eacef30f15299ee7c3ad32df2840b
Cr-Commit-Position: refs/heads/master@{#387467}