Expose data reduction proxy auth token for internal builds.

Expose data reduction proxy auth token for internal builds.

Provide a method in google_api_keys to expose data reduction proxy auth
token to non-mobile builds.

BUG=412884
Committed: https://crrev.com/033fb6ec8e164d0ee17da4b575dc103785cb4722
Cr-Commit-Position: refs/heads/master@{#299736}

[Not at Google. Contact bengr, 2014-10-07 21:53:17]

kundaji@chromium.org changed reviewers:
+ bengr@chromium.org

[Not at Google. Contact bengr, 2014-10-07 21:53:57]

bengr:
  components/data_reduction_proxy/*

[bengr, 2014-10-10 00:26:07]

https://codereview.chromium.org/635013003/diff/20001/components/data_reductio...
File components/data_reduction_proxy/DEPS (right):

https://codereview.chromium.org/635013003/diff/20001/components/data_reductio...
components/data_reduction_proxy/DEPS:4: "+google_apis",
Does this dependency include a lot of code? I'm worried that some of the leaner
platforms (e.g., Webview) will grow substantially with such a change.

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
File components/data_reduction_proxy/DEPS (right):

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/DEPS:4: "+google_apis",
I am ok with this *only* if this does not significantly increase the code size
for data reduction proxy clients that do not already use google_apis. Please
verify that this is the case by measuring the code size of cronet before and
after this change (you'll have to add data reduction proxy objects to cronet).
If webview does not currently use google_apis, you could measure the code size
of it instead. It already has data reduction proxy support.

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:220:
#if defined(SPDY_PROXY_AUTH_VALUE)
Is this ever defined outside of iOS and Android? If not, should you be more
explicit:

#if defined(OS_ANDROID) || defined(OS_IOS)
#if defined(SPDY_PROXY_AUTH_VALUE)
...
#endif
#else
 k = google_apis::...
#endif

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:225:
#if !defined(OS_ANDROID) && !defined(OS_IOS)
Presuming this doesn't bloat WebView or Cronet, can we use this approach on all
platforms?

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:226:
// On Android and iOS this value is obtained from supplement.gypi. We avoid
I wouldn't describe how internal code works. Instead, add a comment above line
220 that says:

// Android and iOS get the default key from a preprocessor constant. All other
platforms
// get the key from google_apis.

[Not at Google. Contact bengr, 2014-10-10 17:09:07]

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
File components/data_reduction_proxy/DEPS (right):

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/DEPS:4: "+google_apis",
On 2014/10/10 00:26:06, bengr1 wrote:
> I am ok with this *only* if this does not significantly increase the code size
> for data reduction proxy clients that do not already use google_apis. Please
> verify that this is the case by measuring the code size of cronet before and
> after this change (you'll have to add data reduction proxy objects to cronet).
> If webview does not currently use google_apis, you could measure the code size
> of it instead. It already has data reduction proxy support.

The size of libgoogle_apis.a is 1.4MB. I would expect that this would be the
size increase when it is linked in. Is this too high?

[bengr, 2014-10-10 18:35:06]

On 2014/10/10 17:09:07, kundaji wrote:
>
https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
> File components/data_reduction_proxy/DEPS (right):
> 
>
https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
> components/data_reduction_proxy/DEPS:4: "+google_apis",
> On 2014/10/10 00:26:06, bengr1 wrote:
> > I am ok with this *only* if this does not significantly increase the code
size
> > for data reduction proxy clients that do not already use google_apis. Please
> > verify that this is the case by measuring the code size of cronet before and
> > after this change (you'll have to add data reduction proxy objects to
cronet).
> > If webview does not currently use google_apis, you could measure the code
size
> > of it instead. It already has data reduction proxy support.
> 
> The size of libgoogle_apis.a is 1.4MB. I would expect that this would be the
> size increase when it is linked in. Is this too high?

As discussed offline, I am ok with dependency so long as google_apis is not
linked in to the data reduction proxy on any iOS or Android client.

[Not at Google. Contact bengr, 2014-10-10 18:55:44]

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
File components/data_reduction_proxy/DEPS (right):

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/DEPS:4: "+google_apis",
On 2014/10/10 00:26:06, bengr1 wrote:
> I am ok with this *only* if this does not significantly increase the code size
> for data reduction proxy clients that do not already use google_apis. Please
> verify that this is the case by measuring the code size of cronet before and
> after this change (you'll have to add data reduction proxy objects to cronet).
> If webview does not currently use google_apis, you could measure the code size
> of it instead. It already has data reduction proxy support.

Unable to build the cronet since it gives random errors. Not sure how stable
that target is. I made the gyp and gn rules so that the dependency to google_api
is only for non-mobile.

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:220:
#if defined(SPDY_PROXY_AUTH_VALUE)
On 2014/10/10 00:26:07, bengr1 wrote:
> Is this ever defined outside of iOS and Android? If not, should you be more
> explicit:
> 
> #if defined(OS_ANDROID) || defined(OS_IOS)
> #if defined(SPDY_PROXY_AUTH_VALUE)
> ...
> #endif
> #else
>  k = google_apis::...
> #endif

Done.

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:225:
#if !defined(OS_ANDROID) && !defined(OS_IOS)
On 2014/10/10 00:26:07, bengr1 wrote:
> Presuming this doesn't bloat WebView or Cronet, can we use this approach on
all
> platforms?

It will increase the size by about 1.4M. Also the gypi mechanism better and we
might be able to make use of that in the future for all platforms.

https://codereview.chromium.org/635013003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:226:
// On Android and iOS this value is obtained from supplement.gypi. We avoid
On 2014/10/10 00:26:06, bengr1 wrote:
> I wouldn't describe how internal code works. Instead, add a comment above line
> 220 that says:
> 
> // Android and iOS get the default key from a preprocessor constant. All other
> platforms
> // get the key from google_apis.

Done.

[bengr, 2014-10-10 23:36:48]

lgtm with nit.

https://codereview.chromium.org/635013003/diff/430001/google_apis/google_api_...
File google_apis/google_api_keys.cc (right):

https://codereview.chromium.org/635013003/diff/430001/google_apis/google_api_...
google_apis/google_api_keys.cc:187: return "";
return std::string()

[Not at Google. Contact bengr, 2014-10-11 00:00:25]

https://codereview.chromium.org/635013003/diff/430001/google_apis/google_api_...
File google_apis/google_api_keys.cc (right):

https://codereview.chromium.org/635013003/diff/430001/google_apis/google_api_...
google_apis/google_api_keys.cc:187: return "";
On 2014/10/10 23:36:47, bengr1 wrote:
> return std::string()

Done.

[Not at Google. Contact bengr, 2014-10-11 00:01:20]

kundaji@chromium.org changed reviewers:
+ rogerta@chromium.org

[Not at Google. Contact bengr, 2014-10-11 00:02:22]

rogerta:
  google_apis/google_api_keys.cc
  google_apis/google_api_keys.h

bengr:
  components/data_reduction_proxy.gypi
  components/data_reduction_proxy/DEPS
  components/data_reduction_proxy/browser/BUILD.gn

components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc

[Roger Tawa OOO till Jul 10th, 2014-10-15 14:38:27]

lgtm google_apis

Sorry for delay in review.

[Not at Google. Contact bengr, 2014-10-15 18:55:32]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-10-15 18:56:50]

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

[commit-bot: I haz the power, 2014-10-15 19:11:16]

Committed patchset #8 (id:660001)

[commit-bot: I haz the power, 2014-10-15 19:12:10]

Patchset 8 (id:??) landed as
https://crrev.com/033fb6ec8e164d0ee17da4b575dc103785cb4722
Cr-Commit-Position: refs/heads/master@{#299736}