Eliminate OmniboxMetricsProvider listening to notification

Eliminate OmniboxMetricsProvider listening to notification

OmniboxMetricsProvider is targeted for sharing with iOS, which means that its
use of //content needs to be eliminated. This is a little tricky, as it
currently listens for the notification that a URL was opend from the omnibox
from all Profiles.

To move away from listening for this notification, this CL makes use of
OmniboxEventGlobalTracker (previously an //ios-specific class).
OmniboxEditModel calls out to the OmniboxEventGlobalTracker singleton when a
URL is opened from the omnibox. Listeners interested in receiving notifications
when a URL is opened in *any* OmniboxEditModel register with the
OmniboxEventGlobalTracker.

This CL changes OmniboxMetricsProvider to make use of the new class instead of
listening for the notification.

BUG=508022
TBR=droger

Committed: https://crrev.com/8ee1f8e3a1006de449420c6e0d7bbe0c12b23891
Cr-Commit-Position: refs/heads/master@{#342319}

[blundell, 2015-08-05 15:11:58]

blundell@chromium.org changed reviewers:
+ asvitkine@chromium.org, pkasting@chromium.org

[blundell, 2015-08-05 15:11:58]

Hi Alexei and Peter,

Could you both look over this entire CL? Thanks!

[blundell, 2015-08-06 19:16:23]

friendly ping :)

[Alexei Svitkine (slow), 2015-08-06 19:29:03]

Seems ok to me. I'll defer to Peter's review for the omnibox bits.

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
chrome/browser/metrics/omnibox_metrics_provider.cc:85: base::Unretained(this)));
Nit: Use a weak ptr factory.

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_edit_model.cc:712:
OmniboxEventGlobalTracker::GetInstance()->OnURLOpened(&log);
Seems like you can also eliminate the client_->OnURLOpenedFromOmnibox() call
above by having it use the global.

[Peter Kasting, 2015-08-06 19:58:22]

LGTM

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
chrome/browser/metrics/omnibox_metrics_provider.cc:85: base::Unretained(this)));
On 2015/08/06 19:29:03, Alexei Svitkine wrote:
> Nit: Use a weak ptr factory.

Why is that needed?  Isn't the fact that |subscription_| is a scoped_ptr
sufficient?

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_edit_model.cc:712:
OmniboxEventGlobalTracker::GetInstance()->OnURLOpened(&log);
On 2015/08/06 19:29:03, Alexei Svitkine wrote:
> Seems like you can also eliminate the client_->OnURLOpenedFromOmnibox() call
> above by having it use the global.

I think that would mean that all clients automatically get called for all edit
models, which isn't right.

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_event_global_tracker.h (right):

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_event_global_tracker.h:32:
RegisterCallback(const OnURLOpenedCallback& cb);
Nit: Indent 4?

[blundell, 2015-08-06 20:22:21]

Thanks!

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
chrome/browser/metrics/omnibox_metrics_provider.cc:85: base::Unretained(this)));
On 2015/08/06 19:58:22, Peter Kasting wrote:
> On 2015/08/06 19:29:03, Alexei Svitkine wrote:
> > Nit: Use a weak ptr factory.
> 
> Why is that needed?  Isn't the fact that |subscription_| is a scoped_ptr
> sufficient?

Peter's right: base::CallbackList removes the callback from the list when the
returned subscription is destroyed.

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_edit_model.cc:712:
OmniboxEventGlobalTracker::GetInstance()->OnURLOpened(&log);
On 2015/08/06 19:58:22, Peter Kasting wrote:
> On 2015/08/06 19:29:03, Alexei Svitkine wrote:
> > Seems like you can also eliminate the client_->OnURLOpenedFromOmnibox() call
> > above by having it use the global.
> 
> I think that would mean that all clients automatically get called for all edit
> models, which isn't right.

Peter's correct.

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_event_global_tracker.h (right):

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_event_global_tracker.h:32:
RegisterCallback(const OnURLOpenedCallback& cb);
On 2015/08/06 19:58:22, Peter Kasting wrote:
> Nit: Indent 4?

Done.

[Alexei Svitkine (slow), 2015-08-06 20:27:28]

LGTM

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/1278433002/diff/1/chrome/browser/metrics/omni...
chrome/browser/metrics/omnibox_metrics_provider.cc:85: base::Unretained(this)));
On 2015/08/06 20:22:21, blundell wrote:
> On 2015/08/06 19:58:22, Peter Kasting wrote:
> > On 2015/08/06 19:29:03, Alexei Svitkine wrote:
> > > Nit: Use a weak ptr factory.
> > 
> > Why is that needed?  Isn't the fact that |subscription_| is a scoped_ptr
> > sufficient?
> 
> Peter's right: base::CallbackList removes the callback from the list when the
> returned subscription is destroyed.

Oops, you're right.

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/1278433002/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_edit_model.cc:712:
OmniboxEventGlobalTracker::GetInstance()->OnURLOpened(&log);
On 2015/08/06 20:22:21, blundell wrote:
> On 2015/08/06 19:58:22, Peter Kasting wrote:
> > On 2015/08/06 19:29:03, Alexei Svitkine wrote:
> > > Seems like you can also eliminate the client_->OnURLOpenedFromOmnibox()
call
> > > above by having it use the global.
> > 
> > I think that would mean that all clients automatically get called for all
edit
> > models, which isn't right.
> 
> Peter's correct.

Fair enough. I thought there was just a single client here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

But point taken that it's not the case.

[blundell, 2015-08-07 06:16:57]

blundell@chromium.org changed reviewers:
+ droger@chromium.org

[blundell, 2015-08-07 06:17:07]

TBR=droger for //ios

[blundell, 2015-08-07 06:17:12]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-08-07 06:17:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1278433002/#ps20001
(title: "Response to review")

[commit-bot: I haz the power, 2015-08-07 06:17:19]

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

[commit-bot: I haz the power, 2015-08-07 06:18:37]

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

[commit-bot: I haz the power, 2015-08-07 06:18:38]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[blundell, 2015-08-07 06:23:33]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-08-07 06:23:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1278433002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-07 06:23:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1278433002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1278433002/40001

[blundell, 2015-08-07 06:26:11]

blundell@chromium.org changed reviewers:
+ miguelg@chromium.org

[blundell, 2015-08-07 06:26:12]

miguelg: Can you review //chrome/browser/android? Thanks!

[blundell, 2015-08-07 07:31:48]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-08-07 07:31:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1278433002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-07 07:32:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1278433002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1278433002/60001

[droger, 2015-08-07 07:34:33]

//ios LGTM

[commit-bot: I haz the power, 2015-08-07 08:26:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-07 08:27:27]

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

[sdefresne, 2015-08-07 15:30:09]

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

[sdefresne, 2015-08-07 15:30:09]

https://codereview.chromium.org/1278433002/diff/60001/ios/chrome/ios_chrome.gyp
File ios/chrome/ios_chrome.gyp (left):

https://codereview.chromium.org/1278433002/diff/60001/ios/chrome/ios_chrome.g...
ios/chrome/ios_chrome.gyp:275:
'browser/open_from_clipboard/create_clipboard_recent_content.h',
Why did you remove
browser/open_from_clipboard/create_clipboard_recent_content.{h,mm}?
Can you revert this change?

[blundell, 2015-08-07 15:34:18]

https://codereview.chromium.org/1278433002/diff/60001/ios/chrome/ios_chrome.gyp
File ios/chrome/ios_chrome.gyp (left):

https://codereview.chromium.org/1278433002/diff/60001/ios/chrome/ios_chrome.g...
ios/chrome/ios_chrome.gyp:275:
'browser/open_from_clipboard/create_clipboard_recent_content.h',
On 2015/08/07 15:30:09, sdefresne wrote:
> Why did you remove
> browser/open_from_clipboard/create_clipboard_recent_content.{h,mm}?
> Can you revert this change?

Oops, bad rebase. https://codereview.chromium.org/1274213004/