Create OmniboxMetricsProvider class.

Create OmniboxMetricsProvider class.

BUG=374226
R=isherman@chromium.org

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

[Alexei Svitkine (slow), 2014-05-20 14:59:59]

Ilya, can you take an initial look?

This is still missing the other half of the CL that removes the corresponding
code from metrics_service.cc and metrics_log.cc (which I'll make the changes to
once the Provider interface cl lands, since that CL touches those files too).

[Ilya Sherman, 2014-05-20 15:26:49]

Was there any test code for omnibox events that could move over as well?

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
chrome/browser/metrics/omnibox_metrics_provider.cc:81: // Note: Default case
necessary due to NUM_TYPES enum entry.
Please instead have a "case NUM_TYPES: NOTREACHED()".

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
chrome/browser/metrics/omnibox_metrics_provider.cc:143:
RecordOmniboxOpenedURL(*content::Details<OmniboxLog>(details).ptr());
nit: It's probably reasonable to just inline this method.

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
File chrome/browser/metrics/omnibox_metrics_provider.h (right):

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
chrome/browser/metrics/omnibox_metrics_provider.h:45:
metrics::ChromeUserMetricsExtension omnibox_events_cache;
Can we use just the ominbox portion of the protobuf?

[Alexei Svitkine (slow), 2014-05-20 15:49:25]

Thanks, now updated with changes to the other files too.

I've kept MetricsService listening to the notification because it also uses it
to get out of Idle mode. So when we move that to the client, it can also listen
for it.

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
chrome/browser/metrics/omnibox_metrics_provider.cc:81: // Note: Default case
necessary due to NUM_TYPES enum entry.
On 2014/05/20 15:26:49, Ilya Sherman wrote:
> Please instead have a "case NUM_TYPES: NOTREACHED()".

Done.

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
chrome/browser/metrics/omnibox_metrics_provider.cc:143:
RecordOmniboxOpenedURL(*content::Details<OmniboxLog>(details).ptr());
On 2014/05/20 15:26:49, Ilya Sherman wrote:
> nit: It's probably reasonable to just inline this method.

I find having the separate method cleaner (i.e. it's clear that it's getting its
input from the param vs. this one which has non-very-sensical notification
observer params).

Plus, now I've added the incognito check here, which I forgot on the previous
patchset.

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
File chrome/browser/metrics/omnibox_metrics_provider.h (right):

https://codereview.chromium.org/293013002/diff/60001/chrome/browser/metrics/o...
chrome/browser/metrics/omnibox_metrics_provider.h:45:
metrics::ChromeUserMetricsExtension omnibox_events_cache;
On 2014/05/20 15:26:49, Ilya Sherman wrote:
> Can we use just the ominbox portion of the protobuf?

It's a repeated field, so not easily. (Unless we actually use the internal proto
RepeatedType collection...)

[Alexei Svitkine (slow), 2014-05-20 16:01:01]

Sorry, forgot to answer this:

"Was there any test code for omnibox events that could move over as well?"

There's no existing test code for this, alas.

[Ilya Sherman, 2014-05-20 16:02:26]

LGTM with a nit.  Thanks :)

https://codereview.chromium.org/293013002/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/293013002/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/omnibox_metrics_provider.cc:146: if
(!chrome::IsOffTheRecordSessionActive())
nit: Please copy over the comment -- it has a suggestion on how to improve on
this in the future.

[Alexei Svitkine (slow), 2014-05-20 16:10:07]

https://codereview.chromium.org/293013002/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/293013002/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/omnibox_metrics_provider.cc:146: if
(!chrome::IsOffTheRecordSessionActive())
On 2014/05/20 16:02:26, Ilya Sherman wrote:
> nit: Please copy over the comment -- it has a suggestion on how to improve on
> this in the future.

Done. I reworded the comment a little bit to make it more obvious about the
alternative it's hinting at.

[Alexei Svitkine (slow), 2014-05-20 16:10:12]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 16:10:59]

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

[commit-bot: I haz the power, 2014-05-20 16:42:33]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-20 16:45:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 16:45:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Alexei Svitkine (slow), 2014-05-20 20:39:53]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-20 20:41:24]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 20:43:15]

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

[Alexei Svitkine (slow), 2014-05-20 22:28:29]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 22:30:11]

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

[Alexei Svitkine (slow), 2014-05-21 12:33:45]

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