Add configuration functions for HTTP cache type to Obj-C and C++

Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=680731
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2644583002
Cr-Commit-Position: refs/heads/master@{#444853}
Committed: https://chromium.googlesource.com/chromium/src/+/5ed6b50f4b3448acc783247123e02ed64ff8ba2a

[lilyhoughton1, 2017-01-18 15:52:35]

Description was changed from

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

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

to

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-18 15:54:39]

Description was changed from

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-18 15:54:39]

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

[mef, 2017-01-18 22:39:51]

Looks reasonable, couple of comments.

Please add BUG=680731 to description.

For some reason I didn't get an email, did you mail this for review?

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

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.h:8: #include
"components/cronet/url_request_context_config.h"
Cronet.h is public header, distributed with Cronet framework, but
url_request_context_config.h is not.

We should define ObjC constants or enums here with proper public comments and
map them internally into cronet::URLRequestContextConfig::HttpCacheType.

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.h:34: // Set HTTP Cache type to be used by
CronetEngine.  This method only has  any
nit: remove extra space. Provide more elaborate comment, based on Java API:
https://cs.chromium.org/chromium/src/components/cronet/android/api/src/org/ch...

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.mm:165: gHttpCache = httpCache;
I think this is a good place to map from ObjC to HttpCacheType.

[lilyhoughton, 2017-01-19 15:16:37]

Description was changed from

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=680731
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-19 16:16:56]

On 2017/01/18 22:39:51, mef wrote:
> Looks reasonable, couple of comments.
> 
> Please add BUG=680731 to description.

Done.

> For some reason I didn't get an email, did you mail this for review?

I thought so, but perhaps I forgot.

[lilyhoughton, 2017-01-19 16:17:14]

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

[lilyhoughton, 2017-01-19 16:17:15]

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

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.h:8: #include
"components/cronet/url_request_context_config.h"
On 2017/01/18 22:39:51, mef wrote:
> Cronet.h is public header, distributed with Cronet framework, but
> url_request_context_config.h is not.
> 
> We should define ObjC constants or enums here with proper public comments and
> map them internally into cronet::URLRequestContextConfig::HttpCacheType.

Done.

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.h:34: // Set HTTP Cache type to be used by
CronetEngine.  This method only has  any
On 2017/01/18 22:39:51, mef wrote:
> nit: remove extra space. Provide more elaborate comment, based on Java API:
>
https://cs.chromium.org/chromium/src/components/cronet/android/api/src/org/ch...

Done.

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2644583002/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.mm:165: gHttpCache = httpCache;
On 2017/01/18 22:39:51, mef wrote:
> I think this is a good place to map from ObjC to HttpCacheType.

Done; there may be a more robust way to map the ObjC enum to the C++ one, by
giving the enumerations explicit int values, so that this can be a simple cast
instead of a switch statement?

I don't know which would be clearest/easiest to maintain, though.

[mef, 2017-01-19 20:51:07]

lgtm

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:44: // options
nit: end with period.

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:45: +
(void)setHttpCacheType:(HttpCacheType)http_cache;
nit: s/http_cache/httpCacheType/

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:175: break;
Should we return an error / log something if value is invalid?

[lilyhoughton, 2017-01-19 21:02:52]

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:44: // options
On 2017/01/19 20:51:07, mef wrote:
> nit: end with period.

Done.

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:45: +
(void)setHttpCacheType:(HttpCacheType)http_cache;
On 2017/01/19 20:51:07, mef wrote:
> nit: s/http_cache/httpCacheType/

Done.

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2644583002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:175: break;
On 2017/01/19 20:51:07, mef wrote:
> Should we return an error / log something if value is invalid?

Invalid value means a programming error, as httpCacheType should always be one
of DISABLED, DISK, or MEMORY.  I added an assert just to be sure, though.

[lilyhoughton, 2017-01-19 21:25:08]

The CQ bit was checked by lilyhoughton@chromium.org

[lilyhoughton, 2017-01-19 21:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2644583002/#ps60001
(title: "replace assert with more verbose dcheck")

[commit-bot: I haz the power, 2017-01-19 21:25:31]

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, 2017-01-19 22:04:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484861108379450,
"parent_rev": "2b4d1e2c6c585e420059928c8c3d7ed2a68e4c09", "commit_rev":
"5ed6b50f4b3448acc783247123e02ed64ff8ba2a"}

[commit-bot: I haz the power, 2017-01-19 22:04:41]

Description was changed from

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=680731
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add configuration functions for HTTP cache type to Obj-C and C++
interfaces to Cronet.

BUG=680731
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2644583002
Cr-Commit-Position: refs/heads/master@{#444853}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ed6b50f4b3448acc783247123e0...
==========

[commit-bot: I haz the power, 2017-01-19 22:04:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5ed6b50f4b3448acc783247123e0...

[lilyhoughton, 2017-02-01 16:11:28]

Made some changes to HttpCacheType enum to be a bit more in line with good Obj-C
style.  PTAL

[mef, 2017-02-01 16:54:58]

On 2017/02/01 16:11:28, lilyhoughton wrote:
> Made some changes to HttpCacheType enum to be a bit more in line with good
Obj-C
> style.  PTAL

This looks good to me, but as this CL has landed already you need to create a
new one, based on current ToTT.