Introduce AndroidMetricsProvider class.

Introduce AndroidMetricsProvider class.

BUG=374219
R=isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272136

[Ilya Sherman, 2014-05-21 09:15:28]

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider_android.cc (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider_android.cc:34:
DCHECK(local_state_);
nit: This seems unnecessary.

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider_android.cc:82: void
AndroidMetricsProvider::ConvertAndroidStabilityPrefsToHistograms(f) {
nit: "(f)" -> "()"

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider_android.h (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider_android.h:36: void
ConvertAndroidStabilityPrefsToHistograms();
nit: WDYT of dropping "Android" from these method names, given that they're
already in an Android-specific class?

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:188: #include
"chrome/browser/metrics/android_metrics_provider_android.h"
nit: Only for Android builds?

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:487: #endif  // defined(OS_ANDROID)
Hmm, why did you move this out of InitializeMetricsState()?  Are you sure that
either location is fine?

https://codereview.chromium.org/294043008/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:1188:
'browser/metrics/android_metrics_provider_android.h',
nit: Perhaps just "metrics_provider_android" or
"stability_metrics_provider_android"?

[blundell, 2014-05-21 09:20:18]

+1 to MetricsStabilityProviderAndroid

[blundell, 2014-05-21 09:20:42]

err, StabilityMetricsProviderAndroid

[Alexei Svitkine (slow), 2014-05-21 09:48:46]

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider_android.cc (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider_android.cc:34:
DCHECK(local_state_);
On 2014/05/21 09:15:28, Ilya Sherman wrote:
> nit: This seems unnecessary.

Done.

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider_android.cc:82: void
AndroidMetricsProvider::ConvertAndroidStabilityPrefsToHistograms(f) {
On 2014/05/21 09:15:28, Ilya Sherman wrote:
> nit: "(f)" -> "()"

Done.

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider_android.h (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider_android.h:36: void
ConvertAndroidStabilityPrefsToHistograms();
On 2014/05/21 09:15:28, Ilya Sherman wrote:
> nit: WDYT of dropping "Android" from these method names, given that they're
> already in an Android-specific class?

SGTM. Done.

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:188: #include
"chrome/browser/metrics/android_metrics_provider_android.h"
On 2014/05/21 09:15:28, Ilya Sherman wrote:
> nit: Only for Android builds?

Done.

https://codereview.chromium.org/294043008/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:487: #endif  // defined(OS_ANDROID)
On 2014/05/21 09:15:28, Ilya Sherman wrote:
> Hmm, why did you move this out of InitializeMetricsState()?  Are you sure that
> either location is fine?

Yes, either location is fine - as long as this happens during start up. The code
that gets run here merely looks at prefs (saved by previous session) and updates
other prefs.

https://codereview.chromium.org/294043008/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/294043008/diff/40001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:1188:
'browser/metrics/android_metrics_provider_android.h',
On 2014/05/21 09:15:28, Ilya Sherman wrote:
> nit: Perhaps just "metrics_provider_android" or
> "stability_metrics_provider_android"?

Changed to android_metrics_provider.cc per offline discussion.

[Ilya Sherman, 2014-05-21 09:50:43]

LGTM, thanks.  BTW, did you mean to have a bug # in the CL description?

[Alexei Svitkine (slow), 2014-05-21 09:52:20]

Oops, CL description updated now, thanks!

[Alexei Svitkine (slow), 2014-05-21 09:56:00]

+thakis for chrome_browser.gypi OWNERS review

(For context, the reason we didn't go with _android.[cc|h] as a suffix is to
keep consistency with out MetricsProviders that have the type at the start and
we felt that android_metrics_provider_android.cc would be too ugly.)

[Alexei Svitkine (slow), 2014-05-21 09:57:38]

On 2014/05/21 09:56:00, Alexei Svitkine wrote:
> +thakis for chrome_browser.gypi OWNERS review
> 
> (For context, the reason we didn't go with _android.[cc|h] as a suffix is to
> keep consistency with out MetricsProviders that have the type at the start and
> we felt that android_metrics_provider_android.cc would be too ugly.)

Err, I forgot that .gypi files don't need an owners review. thakis -> cc

[Ilya Sherman, 2014-05-21 09:58:08]

On 2014/05/21 09:56:00, Alexei Svitkine wrote:
> +thakis for chrome_browser.gypi OWNERS review
> 
> (For context, the reason we didn't go with _android.[cc|h] as a suffix is to
> keep consistency with out MetricsProviders that have the type at the start and
> we felt that android_metrics_provider_android.cc would be too ugly.)

(Specifically, we want the class name to be AndroidMetricsProvider, and we want
the filename to match the class name.)

[Alexei Svitkine (slow), 2014-05-21 13:31:47]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-21 15:03:17]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-21 15:03:17]

The CQ bit was checked by asvitkine@chromium.org

[Nico, 2014-05-21 16:40:57]

cs doesn't find anything for "file:metrics_provider". Can you rename the
existing files? Not being able to use the automatic suffix rules is making
things more confusing than they have to be. (fine to do in a separate commit)

[Alexei Svitkine (slow), 2014-05-21 16:45:48]

On 2014/05/21 16:40:57, Nico wrote:
> cs doesn't find anything for "file:metrics_provider". Can you rename the
> existing files? Not being able to use the automatic suffix rules is making
> things more confusing than they have to be. (fine to do in a separate commit)

So other classes have names like "OmniboxMetricsProvider", "GPUMetricsProvider",
etc. which read better than if we were to name them e.g. MetricsProviderOmnibox.
So we'd rather keep the more readable naming convention.

[Alexei Svitkine (slow), 2014-05-21 16:46:04]

The CQ bit was unchecked by asvitkine@chromium.org

[Nico, 2014-05-21 17:03:02]

Ah ok, makes sense.


On Wed, May 21, 2014 at 9:45 AM, <asvitkine@chromium.org> wrote:

> On 2014/05/21 16:40:57, Nico wrote:
>
>> cs doesn't find anything for "file:metrics_provider". Can you rename the
>> existing files? Not being able to use the automatic suffix rules is making
>> things more confusing than they have to be. (fine to do in a separate
>> commit)
>>
>
> So other classes have names like "OmniboxMetricsProvider",
> "GPUMetricsProvider",
> etc. which read better than if we were to name them e.g.
> MetricsProviderOmnibox.
> So we'd rather keep the more readable naming convention.
>
>
> https://codereview.chromium.org/294043008/
>

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

[Alexei Svitkine (slow), 2014-05-21 23:35:24]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-21 23:35:27]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-21 23:37:10]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-21 23:42:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/294043008/120001

[Alexei Svitkine (slow), 2014-05-22 06:59:34]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-22 07:00:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/294043008/130001

[Alexei Svitkine (slow), 2014-05-22 08:17:49]

Committed patchset #6 manually as r272136 (presubmit successful).