[Cronet] Make HistogramManager an abstract class to break dependency on native library.

[Cronet] Make HistogramManager an abstract class to break dependency on native library.

BUG=441466
Committed: https://crrev.com/6ecf654f504da3fe7b64de6529d4d85ee0be01e8
Cr-Commit-Position: refs/heads/master@{#315108}

[mef, 2015-02-06 18:31:36]

mef@chromium.org changed reviewers:
+ clm@google.com, ebravo@google.com, xunjieli@chromium.org

[mef, 2015-02-06 18:31:37]

PTAL.

[xunjieli, 2015-02-06 19:05:01]

I feel that having an abstract HistogramManager class might confuse ppl. What
about having a getter in UrlRequestContext.java? so we can have reflection stuff
all in one place, and it's easier to figure out the motive.

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_histogram_manager.h (right):

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_histogram_manager.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit: 2015?

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_histogram_manager.h:5: #ifndef
COMPONENTS_CRONET_ANDROID_CRONET_CRONET_HISTOGRAM_MANAGER_H_
s/COMPONENTS_CRONET_ANDROID_CRONET_CRONET_HISTOGRAM_MANAGER_H_/COMPONENTS_CRONET_ANDROID_CRONET_HISTOGRAM_MANAGER_H_

too many cronet :)

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/ja...
File components/cronet/android/java/src/org/chromium/net/HistogramManager.java
(right):

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/HistogramManager.java:11: *
Controls UMA histograms.
nit: Maybe add a comment saying that this class is used to construct a
CronetHistogramManger if  native library is loaded, and return null otherwise?

[mef, 2015-02-06 19:28:56]

I'm not a big fan of reflection, but I see two issues with adding it to
UrlRequestContext:

1. We will have to add it to both new and old API.

2. Having getter on UrlRequestContext would make an impression that
HistogramManager is context-specific, when in fact it is not.

WDYT?

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_histogram_manager.h (right):

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_histogram_manager.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/06 19:05:01, xunjieli wrote:
> nit: 2015?

Done.

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_histogram_manager.h:5: #ifndef
COMPONENTS_CRONET_ANDROID_CRONET_CRONET_HISTOGRAM_MANAGER_H_
On 2015/02/06 19:05:01, xunjieli wrote:
>
s/COMPONENTS_CRONET_ANDROID_CRONET_CRONET_HISTOGRAM_MANAGER_H_/COMPONENTS_CRONET_ANDROID_CRONET_HISTOGRAM_MANAGER_H_
> 
> too many cronet :)

Hmm, it means that previously guard didn't match the file path. Done.

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/ja...
File components/cronet/android/java/src/org/chromium/net/HistogramManager.java
(right):

https://codereview.chromium.org/904033004/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/HistogramManager.java:11: *
Controls UMA histograms.
On 2015/02/06 19:05:01, xunjieli wrote:
> nit: Maybe add a comment saying that this class is used to construct a
> CronetHistogramManger if  native library is loaded, and return null otherwise?

Done. I'll add a test that it handles 'no native library' case correctly.

[xunjieli, 2015-02-06 19:36:31]

On 2015/02/06 19:28:56, mef wrote:
> I'm not a big fan of reflection, but I see two issues with adding it to
> UrlRequestContext:
> 
> 1. We will have to add it to both new and old API.
> 2. Having getter on UrlRequestContext would make an impression that
> HistogramManager is context-specific, when in fact it is not.
> 
Agreed, if HistogramManager is not context-specific, putting it in
UrlRequestContext will be confusing.
Don't have a better solution. So i'll lgtm. Might worth waiting for clm or
ebravo to give their perspective.

[chromium-reviews, 2015-02-06 19:51:53]

On Fri, Feb 6, 2015 at 11:36 AM, <xunjieli@chromium.org> wrote:

> On 2015/02/06 19:28:56, mef wrote:
>
>> I'm not a big fan of reflection, but I see two issues with adding it to
>> UrlRequestContext:
>>
>
>  1. We will have to add it to both new and old API.
>> 2. Having getter on UrlRequestContext would make an impression that
>> HistogramManager is context-specific, when in fact it is not.
>>
>
> Agreed, if HistogramManager is not context-specific, putting it in
> UrlRequestContext will be confusing.
> Don't have a better solution. So i'll lgtm. Might worth waiting for clm or
> ebravo to give their perspective.
>

Lets go ahead as is

>
>
>
>
>
> https://codereview.chromium.org/904033004/
>



-- 
Regards

Eduardo Bravo

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mef, 2015-02-06 20:03:54]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2015-02-06 20:04:44]

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

[commit-bot: I haz the power, 2015-02-06 21:21:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-06 21:22:11]

Patchset 2 (id:??) landed as
https://crrev.com/6ecf654f504da3fe7b64de6529d4d85ee0be01e8
Cr-Commit-Position: refs/heads/master@{#315108}