Remove OwnedInterface

Remove OwnedInterface

Use a StrongBinding in SearchProviderInstallState in place of OwnedInterface.

Committed: https://crrev.com/03613d0da1fca9533eafdcbeae746e2b94b660fb
Cr-Commit-Position: refs/heads/master@{#407719}

[tibell, 2016-07-22 04:52:44]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 04:53:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 04:55:07]

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

[commit-bot: I haz the power, 2016-07-22 04:55:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tibell, 2016-07-22 04:59:34]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 05:00:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 05:21:25]

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

[commit-bot: I haz the power, 2016-07-22 05:21:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tibell, 2016-07-22 05:59:23]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 05:59:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-07-22 06:00:01]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-07-22 06:02:17]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 06:02:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 06:04:42]

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

[commit-bot: I haz the power, 2016-07-22 06:04:43]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[tibell, 2016-07-22 06:19:31]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 06:19:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 06:53:15]

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

[commit-bot: I haz the power, 2016-07-22 06:53:16]

Dry run: This issue passed the CQ dry run.

[tibell, 2016-07-25 00:27:32]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 00:27:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-07-25 00:30:28]

tibell@chromium.org changed reviewers:
+ jam@chromium.org

[tibell, 2016-07-25 00:30:28]

As discussed on chromium-mojo@ lets remove this for now.

[tibell, 2016-07-25 00:31:03]

Description was changed from

==========
Use a StrongBinding in SearchProviderInstallState
==========

to

==========
Remove OwnedInterface

Use a StrongBinding in SearchProviderInstallState in place of OwnedInterface.
==========

[commit-bot: I haz the power, 2016-07-25 01:35:27]

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

[commit-bot: I haz the power, 2016-07-25 01:35:29]

Dry run: This issue passed the CQ dry run.

[jam, 2016-07-25 16:56:07]

lgtm, thanks

[dcheng, 2016-07-25 17:01:33]

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

[dcheng, 2016-07-25 17:01:34]

LGTM as well.

https://codereview.chromium.org/2170313002/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.cc
(right):

https://codereview.chromium.org/2170313002/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:47:
base::Unretained(new SearchProviderInstallStateImpl(
I would personally prefer a small comment that mentions that BindOnIOThread
takes ownership of the impl object.

https://codereview.chromium.org/2170313002/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:49:
base::Passed(std::move(request))));
Nit: base::Passed(&request)

[tibell, 2016-07-26 00:40:24]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 00:41:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-07-26 00:42:03]

https://codereview.chromium.org/2170313002/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.cc
(right):

https://codereview.chromium.org/2170313002/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:47:
base::Unretained(new SearchProviderInstallStateImpl(
On 2016/07/25 17:01:33, dcheng wrote:
> I would personally prefer a small comment that mentions that BindOnIOThread
> takes ownership of the impl object.

Done.

https://codereview.chromium.org/2170313002/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:49:
base::Passed(std::move(request))));
On 2016/07/25 17:01:33, dcheng wrote:
> Nit: base::Passed(&request)

Done.

[Sam McNally, 2016-07-26 01:46:33]

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

[Sam McNally, 2016-07-26 01:46:34]

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.cc
(right):

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:23: :
provider_data_(TemplateURLServiceFactory::GetForProfile(profile),
TemplateURLService is a KeyedService stored as a raw pointer in
|provider_data_|, which uses it without any lifetime checks. It's accessed when
handling an IPC so it could be a problem, depending on what happens during
shutdown.

[commit-bot: I haz the power, 2016-07-26 01:58:58]

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

[commit-bot: I haz the power, 2016-07-26 01:58:59]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-26 02:28:33]

leon.han@intel.com changed reviewers:
+ leon.han@intel.com

[leonhsl(Using Gerrit), 2016-07-26 02:28:34]

Hi, sorry I'm not a reviewer but as I'm also doing the similar work at
https://codereview.chromium.org/2063683002/, I want to make clear of something.
Maybe a silly question.. Thanks in advance.

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.cc
(right):

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:48:
base::Unretained(new SearchProviderInstallStateImpl(
Why don't we do new/bind together on IO thread? If we created the impl instance
here, then before it is bound on IO thread later, no one owns it.

[tibell, 2016-07-26 02:34:06]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-07-26 02:34:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2170313002/#ps120001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-07-26 02:34:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-07-26 02:34:23]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-07-26 02:40:48]

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.cc
(right):

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:23: :
provider_data_(TemplateURLServiceFactory::GetForProfile(profile),
On 2016/07/26 01:46:34, Sam McNally wrote:
> TemplateURLService is a KeyedService stored as a raw pointer in
> |provider_data_|, which uses it without any lifetime checks. It's accessed
when
> handling an IPC so it could be a problem, depending on what happens during
> shutdown.

dcheng@ is going to try to delete this code. I will simply wait until that's
done and just keep the OwnerInterface removal in this CL. That seems like less
work than actually trying to figure out if this is safe. :(

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:48:
base::Unretained(new SearchProviderInstallStateImpl(
On 2016/07/26 02:28:34, leonhsl wrote:
> Why don't we do new/bind together on IO thread? If we created the impl
instance
> here, then before it is bound on IO thread later, no one owns it.

The fields in the class need to be initialized on the UI thread (that's why
there's a DCHECK_CURRENTLY_ON in the constructor).

[jam, 2016-07-26 05:25:36]

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.cc
(right):

https://codereview.chromium.org/2170313002/diff/120001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:23: :
provider_data_(TemplateURLServiceFactory::GetForProfile(profile),
On 2016/07/26 01:46:34, Sam McNally wrote:
> TemplateURLService is a KeyedService stored as a raw pointer in
> |provider_data_|, which uses it without any lifetime checks. It's accessed
when
> handling an IPC so it could be a problem, depending on what happens during
> shutdown.

This is safe, check out the implementation of provider_data_. It posts a task to
the UI thread to call it.

However when profiles are destructed (i.e. when keyed services are destructed),
the main message loop has stopped running so this task posted from the IO loop
wouldn't run.

[tibell, 2016-07-26 05:53:29]

The CQ bit was checked by tibell@chromium.org

[commit-bot: I haz the power, 2016-07-26 05:53:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 05:56:54]

Description was changed from

==========
Remove OwnedInterface

Use a StrongBinding in SearchProviderInstallState in place of OwnedInterface.
==========

to

==========
Remove OwnedInterface

Use a StrongBinding in SearchProviderInstallState in place of OwnedInterface.
==========

[commit-bot: I haz the power, 2016-07-26 05:56:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-26 05:58:54]

Description was changed from

==========
Remove OwnedInterface

Use a StrongBinding in SearchProviderInstallState in place of OwnedInterface.
==========

to

==========
Remove OwnedInterface

Use a StrongBinding in SearchProviderInstallState in place of OwnedInterface.

Committed: https://crrev.com/03613d0da1fca9533eafdcbeae746e2b94b660fb
Cr-Commit-Position: refs/heads/master@{#407719}
==========

[commit-bot: I haz the power, 2016-07-26 05:58:56]

Patchset 7 (id:??) landed as
https://crrev.com/03613d0da1fca9533eafdcbeae746e2b94b660fb
Cr-Commit-Position: refs/heads/master@{#407719}