Convert GetSearchProviderInstallState to Mojo

Convert GetSearchProviderInstallState to Mojo

Committed: https://crrev.com/be180334960710cf63734aeffc8046df6144e197
Cr-Commit-Position: refs/heads/master@{#406782}

[tibell, 2016-06-20 00:39:59]

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

[tibell, 2016-06-20 00:40:00]

Ready for a first look.

[Sam McNally, 2016-06-20 01:27:12]

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
File
chrome/browser/search_engines/search_provider_install_state_message_filter.h
(right):

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:24:
class SearchProviderInstallStateMessageFilter
Rename this to SearchProviderInstallStateImpl and rename the file accordingly.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:46:
void GetInstallState(
// mojom::SearchProviderInstallState override.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:49:
const mojo::Callback<void(search_provider::InstallState)>& callback)
GetInstallStateCallback. Change everywhere.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:65:
// TODO(tibell): Add binding of some sort. Binding on IO thread.
Remove?

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:80:
SearchProviderInstallStateMessageFilterAdapter(
explicit.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:81:
SearchProviderInstallStateMessageFilter* filter);
This could take a unique_ptr.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:84:
SearchProviderInstallStateMessageFilter* Get() { return ptr_.get(); }
get()

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2734:
base::Bind(&startup_metric_utils::StartupMetricHostImpl::Create));
Remove.

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/search_...
File
chrome/browser/search_engines/search_provider_install_state_message_filter.cc
(right):

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.cc:110:
~SearchProviderInstallStateMessageFilterAdapter() {}
= default;

https://codereview.chromium.org/2072613003/diff/150001/chrome/common/search_p...
File chrome/common/search_provider.mojom (right):

https://codereview.chromium.org/2072613003/diff/150001/chrome/common/search_p...
chrome/common/search_provider.mojom:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
Add per-file *.mojom owners to chrome/common/OWNERS. You can copy-paste from
content/common/OWNERS.

[tibell, 2016-06-20 03:19:55]

PTAL

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
File
chrome/browser/search_engines/search_provider_install_state_message_filter.h
(right):

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:24:
class SearchProviderInstallStateMessageFilter
On 2016/06/20 01:27:11, Sam McNally wrote:
> Rename this to SearchProviderInstallStateImpl and rename the file accordingly.

Done.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:46:
void GetInstallState(
On 2016/06/20 01:27:11, Sam McNally wrote:
> // mojom::SearchProviderInstallState override.

Done.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:49:
const mojo::Callback<void(search_provider::InstallState)>& callback)
On 2016/06/20 01:27:11, Sam McNally wrote:
> GetInstallStateCallback. Change everywhere.

Done.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:65:
// TODO(tibell): Add binding of some sort. Binding on IO thread.
On 2016/06/20 01:27:11, Sam McNally wrote:
> Remove?

Done.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:80:
SearchProviderInstallStateMessageFilterAdapter(
On 2016/06/20 01:27:11, Sam McNally wrote:
> explicit.

Done.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:81:
SearchProviderInstallStateMessageFilter* filter);
On 2016/06/20 01:27:11, Sam McNally wrote:
> This could take a unique_ptr.

Done.

https://codereview.chromium.org/2072613003/diff/130001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.h:84:
SearchProviderInstallStateMessageFilter* Get() { return ptr_.get(); }
On 2016/06/20 01:27:11, Sam McNally wrote:
> get()

Done.

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2734:
base::Bind(&startup_metric_utils::StartupMetricHostImpl::Create));
On 2016/06/20 01:27:11, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/search_...
File
chrome/browser/search_engines/search_provider_install_state_message_filter.cc
(right):

https://codereview.chromium.org/2072613003/diff/150001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_message_filter.cc:110:
~SearchProviderInstallStateMessageFilterAdapter() {}
On 2016/06/20 01:27:11, Sam McNally wrote:
> = default;

Done.

https://codereview.chromium.org/2072613003/diff/150001/chrome/common/search_p...
File chrome/common/search_provider.mojom (right):

https://codereview.chromium.org/2072613003/diff/150001/chrome/common/search_p...
chrome/common/search_provider.mojom:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/06/20 01:27:11, Sam McNally wrote:
> Add per-file *.mojom owners to chrome/common/OWNERS. You can copy-paste from
> content/common/OWNERS.

Done.

[tibell, 2016-06-20 03:54:44]

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

[tibell, 2016-06-20 03:54:45]

Hi!

We've started converting some old Chrome IPCs to Mojo. This is one of those
conversions. :)

[Sam McNally, 2016-06-20 08:05:18]

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:77: class
SearchProviderInstallStateImplAdapter
Can this move into the .cc file?

[sky, 2016-06-20 15:20:26]

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

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:46:
host->GetServiceRegistry()->AddService<mojom::SearchProviderInstallState>(
I'm not familiar with the threading requirements of this class. What are they?
What threads do this code need to live on?

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:38:
~SearchProviderInstallStateImpl() override;
nit: move beneath constructor (see style guide).

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:67: static
const char kRenderProcessHostKey[];
should be first (see style guide).

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
File chrome/common/search_provider.mojom (right):

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:5: module mojom;
Are we really going with a module of 'mojom' for these files? I would expect a
prefix, perhaps 'chrome.mojom'.

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:17: url.mojom.Url page_url,
Document what page_url and inquiry_url are.

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:19: InstallState install);
nit: this wrapping is a bit awkward. Maybe move 'InstallState install' onto the
previous line.

[tibell, 2016-06-21 01:28:06]

Description was changed from

==========
Convert GetSearchProviderInstallState to Mojo
==========

to

==========
Convert GetSearchProviderInstallState to Mojo

TEST=browser_tests
--gtest_filter=SearchProviderTest.TestIsSearchProviderInstalled
==========

[tibell, 2016-06-21 01:31:32]

PTAL

levin, could you please help with the documentation of the page_url and
inquiry_url args to GetInstallState?

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

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:46:
host->GetServiceRegistry()->AddService<mojom::SearchProviderInstallState>(
On 2016/06/20 15:20:26, sky wrote:
> I'm not familiar with the threading requirements of this class. What are they?
> What threads do this code need to live on?

I'm just preserving the same threading behavior as we had before the switch to
Mojo. Messages arrive and are handled on the IO thread. There are existing
DCHECKs in the code that make sure we're running things on the right threads.

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:38:
~SearchProviderInstallStateImpl() override;
On 2016/06/20 15:20:26, sky wrote:
> nit: move beneath constructor (see style guide).

Done.

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:67: static
const char kRenderProcessHostKey[];
On 2016/06/20 15:20:26, sky wrote:
> should be first (see style guide).

Done.

Made it an implementation detail of the .cc file instead.

https://codereview.chromium.org/2072613003/diff/250001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:77: class
SearchProviderInstallStateImplAdapter
On 2016/06/20 08:05:18, Sam McNally wrote:
> Can this move into the .cc file?

Done.

It can be now after I moved the construction into the factory method (before it
was done in chrome_content_browser_client.cc).

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
File chrome/common/search_provider.mojom (right):

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:5: module mojom;
On 2016/06/20 15:20:26, sky wrote:
> Are we really going with a module of 'mojom' for these files? I would expect a
> prefix, perhaps 'chrome.mojom'.

Done.

The other chrome/ mojom files (except one I think) do use the plain mojom
namespace and the C++ code uses no namespace. Better start late than never to
improve things I guess, even if it gets a bit inconsistent for a while. :)

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:17: url.mojom.Url page_url,
On 2016/06/20 15:20:26, sky wrote:
> Document what page_url and inquiry_url are.

I've just spent some time figuring it out (it's not documented from what I can
tell) by reading the test
(https://cs.chromium.org/chromium/src/chrome/browser/external_extension_browse...)
and the implementation. I'm still not 100% sure exactly what the arguments mean.
I believe the second one is the search provider host (e.g. www.google.com) and
the first is the requesting page, but since I'm not sure and this seems security
related I'd rather not guess.

I've CC:ed levin@ for a clarification.

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:19: InstallState install);
On 2016/06/20 15:20:26, sky wrote:
> nit: this wrapping is a bit awkward. Maybe move 'InstallState install' onto
the
> previous line.

Done.

[tibell, 2016-06-21 01:32:41]

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

[tibell, 2016-06-21 01:35:37]

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

[tibell, 2016-06-21 01:35:38]

sievers@ for DEPS
dcheng@ for render_messages.h

[levin, 2016-06-21 01:48:43]

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

[levin, 2016-06-21 01:48:45]

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
File chrome/common/search_provider.mojom (right):

https://codereview.chromium.org/2072613003/diff/250001/chrome/common/search_p...
chrome/common/search_provider.mojom:17: url.mojom.Url page_url,
On 2016/06/21 01:31:32, tibell wrote:
> On 2016/06/20 15:20:26, sky wrote:
> > Document what page_url and inquiry_url are.
> 
> I've just spent some time figuring it out (it's not documented from what I can
> tell) by reading the test
>
(https://cs.chromium.org/chromium/src/chrome/browser/external_extension_browse...)
> and the implementation. I'm still not 100% sure exactly what the arguments
mean.
> I believe the second one is the search provider host (e.g.
http://www.google.com) and
> the first is the requesting page, but since I'm not sure and this seems
security
> related I'd rather not guess.
> 
> I've CC:ed levin@ for a clarification.

You're basically correct except that the second is a url (not just a host --
you'd need at least a security origin (aka scheme plus host plus port) to be
secure), and it's checked over here:
https://cs.chromium.org/chromium/src/chrome/browser/search_engines/search_pro...


You can see where this is called:

https://cs.chromium.org/chromium/src/chrome/renderer/external_extension.cc?rc...

Hope that helps.

[levin, 2016-06-21 01:48:47]

levin@chromium.org changed reviewers:
- levin@chromium.org

[tibell, 2016-06-21 02:50:57]

On 2016/06/21 01:48:47, levin wrote:

Thanks levin! I've updated the docs based on your comments.

[sky, 2016-06-21 16:11:01]

LGTM

[no sievers, 2016-06-21 18:42:07]

On 2016/06/21 01:35:38, tibell wrote:
> sievers@ for DEPS
> dcheng@ for render_messages.h

lgtm

[dcheng, 2016-06-21 19:14:55]

This is a lot of boilerplate to register one service. Is every other mojo
service at the RenderProcessHost level going to have to do this?

https://codereview.chromium.org/2072613003/diff/310001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/310001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:11: #include
"base/supports_user_data.h"
Move this include to the .cc

https://codereview.chromium.org/2072613003/diff/310001/chrome/common/install_...
File chrome/common/install_state_traits.h (right):

https://codereview.chromium.org/2072613003/diff/310001/chrome/common/install_...
chrome/common/install_state_traits.h:8:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(search_provider::InstallState,
Can we just define the enum in mojom, remove the old one, and use the mojo one
everywhere?

[tibell, 2016-06-23 01:23:03]

Description was changed from

==========
Convert GetSearchProviderInstallState to Mojo

TEST=browser_tests
--gtest_filter=SearchProviderTest.TestIsSearchProviderInstalled
==========

to

==========
Convert GetSearchProviderInstallState to Mojo
==========

[tibell, 2016-06-23 02:47:17]

> This is a lot of boilerplate to register one service. Is every other mojo
> service at the RenderProcessHost level going to have to do this?

For now yes, but we're thinking about ways to factor out some helper code.
However, there'll always be a bit more setup for Mojo given that it's connection
oriented, unlike Chrome IPC.

https://codereview.chromium.org/2072613003/diff/310001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/310001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:11: #include
"base/supports_user_data.h"
On 2016/06/21 19:14:55, dcheng wrote:
> Move this include to the .cc

Done.

https://codereview.chromium.org/2072613003/diff/310001/chrome/common/install_...
File chrome/common/install_state_traits.h (right):

https://codereview.chromium.org/2072613003/diff/310001/chrome/common/install_...
chrome/common/install_state_traits.h:8:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(search_provider::InstallState,
On 2016/06/21 19:14:55, dcheng wrote:
> Can we just define the enum in mojom, remove the old one, and use the mojo one
> everywhere?

Done.

[Sam McNally, 2016-06-23 03:05:36]

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

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:14: #include
"chrome/common/render_messages.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:8: #include
"base/callback.h"
callback_forward.h

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:11: #include
"chrome/browser/profiles/profile.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:13: #include
"chrome/common/search_provider.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:16: #include
"content/public/browser/render_process_host.h"
Remove and forward declare RenderProcessHost instead.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:17: #include
"content/public/common/service_registry.h"
Move this into the .cc file.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
File chrome/renderer/external_extension.cc (right):

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:10: #include
"chrome/common/render_messages.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:11: #include
"chrome/common/search_provider.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:13: #include
"content/public/common/service_registry.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:15: #include
"content/public/renderer/render_view.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:19: #include
"third_party/WebKit/public/web/WebView.h"
Remove.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:106: RenderView* render_view =
GetRenderView();
Remove this and GetRenderView().

[tibell, 2016-06-23 03:44:39]

PTAL

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

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:14: #include
"chrome/common/render_messages.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_state_impl.h (right):

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:8: #include
"base/callback.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> callback_forward.h

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:11: #include
"chrome/browser/profiles/profile.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:13: #include
"chrome/common/search_provider.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:16: #include
"content/public/browser/render_process_host.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Remove and forward declare RenderProcessHost instead.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.h:17: #include
"content/public/common/service_registry.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Move this into the .cc file.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
File chrome/renderer/external_extension.cc (right):

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:10: #include
"chrome/common/render_messages.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:11: #include
"chrome/common/search_provider.h"
On 2016/06/23 03:05:36, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:13: #include
"content/public/common/service_registry.h"
On 2016/06/23 03:05:35, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:15: #include
"content/public/renderer/render_view.h"
On 2016/06/23 03:05:36, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:19: #include
"third_party/WebKit/public/web/WebView.h"
On 2016/06/23 03:05:36, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2072613003/diff/330001/chrome/renderer/extern...
chrome/renderer/external_extension.cc:106: RenderView* render_view =
GetRenderView();
On 2016/06/23 03:05:36, Sam McNally wrote:
> Remove this and GetRenderView().

Done.

[Sam McNally, 2016-06-23 03:48:04]

lgtm

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

https://codereview.chromium.org/2072613003/diff/370001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:43:
content::BrowserThread::DeleteOnIOThread> filter)
filter?

[tibell, 2016-06-27 00:48:39]

dcheng can you please take another look? Thanks!

[tibell, 2016-06-29 03:44:30]

On 2016/06/27 00:48:39, tibell wrote:
> dcheng can you please take another look? Thanks!

dcheng, friendly ping. :)

[dcheng, 2016-07-04 06:46:29]

I'm honestly not entirely comfortable with this CL, because I think the
ownership/lifetime semantics are just too complicated. I've left some feedback
on ways to clarify this a bit, but I think there are two alternatives are also
worth considering.

1. Just use a std::unique_ptr<...> member on ChromeContentBrowserClient
directly.
2. Since (1) is probably undesirable from a layering perspective, subclass
ChromeContentBrowserClientPart instead.

This changes the ownership model a bit: now, there's no longer one per RPH, it's
one per browser process. This doesn't actually seem problematic to me, but the
ownership model of GoogleURLObserver (which is RenderProcessHostObserver) is
unclear to me. If this doesn't actually work, it would be really nice to have
some more comments on GoogleURLObserver detailing why the simpler at face value
approach doesn't work.

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:952:
SearchProviderInstallStateImpl::InstallService(host);
Nit: Let's give mojo stuff it's own distinct section.

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

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:23: class
SearchProviderInstallStateImplAdapter
Please add some more comments describing how this all works together. A lot of
things are underspecified during teardown, so it's not obvious that this is
safe: for example, is it possible there are still pending requests to create
this interface when we've already queued the impl to be destroyed on the IO
thread? I would suggest:
- mentioning that this particular class is just an adapter which is created /
destroyed on the UI thread and has its lifetime associated with the RPH
- the actual impl must be constructed on the ui thread but must otherwise be
used (and destroyed on the io thread)
- other stuff?

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:27:
std::unique_ptr<SearchProviderInstallStateImpl,
I'd recommend just constructing SearchProviderInstallStateImpl directly in the
ctor to avoid passing this verbose name around. It'll also avoid the need to
explicitly spell out the typename on line 72.

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:47: static
const char kRenderProcessHostKey[] = "kRenderProcessHostKey";
The key should be specific to this SupportsUserData type (i.e. it should mention
'search provider')

[dcheng, 2016-07-04 07:42:48]

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:952:
SearchProviderInstallStateImpl::InstallService(host);
On 2016/07/04 06:46:29, dcheng wrote:
> Nit: Let's give mojo stuff it's own distinct section.

Actually it looks like there's already a specific callback for registering mojo
interfaces:
https://cs.chromium.org/chromium/src/content/public/browser/content_browser_c...

So let's use that (regardless of whether we use the
ChromeContentBrowserClientPart implementation strategy or not)

[tibell, 2016-07-05 04:10:41]

Thanks for reviewing!

On 2016/07/04 06:46:29, dcheng wrote:
> I'm honestly not entirely comfortable with this CL, because I think the
> ownership/lifetime semantics are just too complicated.

That's good feedback. Lets get it to a state we're both happy with. :) Let me
explain what I'm trying to achieve here, which is more than just trying convert
this particular code to Mojo.

We need to convert a lot of messages, in the order of 1,000, before we've
completely switched from Chrome IPC to Mojo. We'll eventually ask Chrome
developers to help us with that, but before we do  the Mojo team is doing some
"test" conversions that will let us

 * improve Mojo to make conversions easier and
 * provide docs on how to do the conversion in a number of common scenarios.

Right now (e.g. in this CL) I'm trying to make sure that developers don't have
to carefully rethink their ownership structures or otherwise do larger, risky
rewrites of their code to switch to Mojo. Having done two larger-scale
migrations inside Google, I don't believe coupling migrations with large
rewrites is a good idea.

As for this CL, I *am* trying to avoid changing the ownership structure by
having the RPH own the service impl, just like it owned the message filter
before. This makes sure than any state the impl relies on (the Profile in this
case) still outlives it. The mechanism I used here was "user data" but we could
do it more explicitly (i.e. with a similar implementation to message filters),
if desired.

> I've left some feedback
> on ways to clarify this a bit, but I think there are two alternatives are also
> worth considering.
> 
> 1. Just use a std::unique_ptr<...> member on ChromeContentBrowserClient
> directly.
> 2. Since (1) is probably undesirable from a layering perspective, subclass
> ChromeContentBrowserClientPart instead.
> 
> This changes the ownership model a bit: now, there's no longer one per RPH,
it's
> one per browser process. This doesn't actually seem problematic to me, but the
> ownership model of GoogleURLObserver (which is RenderProcessHostObserver) is
> unclear to me. If this doesn't actually work, it would be really nice to have
> some more comments on GoogleURLObserver detailing why the simpler at face
value
> approach doesn't work.

We need (at least) one impl per Profile so one per browser process won't do. We
could try to tie the implementation to the profile (e.g. using browser context
keyed services), but we'd get back into having to rethink the ownership
structure, which I want to avoid.

[tibell, 2016-07-05 04:14:14]

It's worth adding that this CL happens to be a bit more complicated than the
existing Mojo services registered in
RenderProcessHostImpl::RegisterMojoInterfaces, because the service impl is in
chrome/ and not in content/, unlike the other service impls already registered.
This means that we can't just stick the service impl on the RPH (or an object it
already owns), to manage ownership.

[tibell, 2016-07-12 05:06:08]

dcheng, any more thoughts on this? Do you consider this CL blocked until we've
resolved the ownership thread on chromium-mojo@?

[dcheng, 2016-07-12 05:22:59]

On 2016/07/12 05:06:08, tibell wrote:
> dcheng, any more thoughts on this? Do you consider this CL blocked until we've
> resolved the ownership thread on chromium-mojo@?

Yes, I'd like to see a better answer for this. Some of this complexity does seem
unavoidable, but the current status quo of binding reused impls as
base::Unretained is not a great precedent to be setting.

[tibell, 2016-07-15 00:47:08]

PTAL. I took a new, different stab at this.

 * Impls are now owned by the RPH through a collection of (type erased) impls,
called owned_service_impls_.
 * The user no longer has to deal with separate registration (and the associated
Unretained logic). Lifetime and registration is now encapsulated in the RPH.
 * I believe all existing message filters should be able to use the AddService
pattern when converted.

We might need to bikeshed the name of AddService. Do we still call things
services, or is everything an interface (impl)? :)

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:952:
SearchProviderInstallStateImpl::InstallService(host);
On 2016/07/04 07:42:48, dcheng wrote:
> On 2016/07/04 06:46:29, dcheng wrote:
> > Nit: Let's give mojo stuff it's own distinct section.
> 
> Actually it looks like there's already a specific callback for registering
mojo
> interfaces:
>
https://cs.chromium.org/chromium/src/content/public/browser/content_browser_c...
> 
> So let's use that (regardless of whether we use the
> ChromeContentBrowserClientPart implementation strategy or not)

Now when registration is encapsulated (see AddService on RPH) I think this makes
less sense. What do you think?

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

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:23: class
SearchProviderInstallStateImplAdapter
On 2016/07/04 06:46:29, dcheng wrote:
> Please add some more comments describing how this all works together. A lot of
> things are underspecified during teardown, so it's not obvious that this is
> safe: for example, is it possible there are still pending requests to create
> this interface when we've already queued the impl to be destroyed on the IO
> thread? I would suggest:
> - mentioning that this particular class is just an adapter which is created /
> destroyed on the UI thread and has its lifetime associated with the RPH
> - the actual impl must be constructed on the ui thread but must otherwise be
> used (and destroyed on the io thread)
> - other stuff?

This class is now gone. All ownership/registration is now done in the RPH.

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:27:
std::unique_ptr<SearchProviderInstallStateImpl,
On 2016/07/04 06:46:29, dcheng wrote:
> I'd recommend just constructing SearchProviderInstallStateImpl directly in the
> ctor to avoid passing this verbose name around. It'll also avoid the need to
> explicitly spell out the typename on line 72.

Acknowledged.

https://codereview.chromium.org/2072613003/diff/390001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:47: static
const char kRenderProcessHostKey[] = "kRenderProcessHostKey";
On 2016/07/04 06:46:29, dcheng wrote:
> The key should be specific to this SupportsUserData type (i.e. it should
mention
> 'search provider')

No longer used.

[tibell, 2016-07-20 00:40:34]

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

[commit-bot: I haz the power, 2016-07-20 00:41:18]

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-20 02:04:28]

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

[commit-bot: I haz the power, 2016-07-20 02:04:29]

Dry run: This issue passed the CQ dry run.

[tibell, 2016-07-20 02:52:08]

tibell@chromium.org changed reviewers:
+ sievers. mbarbella@chromium.org
- dcheng@chromium.org, sievers@chromium.org

[tibell, 2016-07-20 02:52:09]

+mbarbella for security review

[Martin Barbella, 2016-07-20 16:58:06]

On 2016/07/20 02:52:09, tibell wrote:
> +mbarbella for security review

mojom/render_messages.h lgtm

[tibell, 2016-07-21 00:20:36]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-07-21 00:20:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sievers@chromium.org, sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2072613003/#ps570001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-21 00:21:08]

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-21 01:03:24]

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

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

Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[tibell, 2016-07-21 04:55:19]

tibell@chromium.org changed reviewers:
+ mbarbella@chromium.org, sievers@chromium.org
- sievers. mbarbella@chromium.org

[tibell, 2016-07-21 04:56:30]

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

[commit-bot: I haz the power, 2016-07-21 04:56:38]

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-21 05:23:24]

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

[commit-bot: I haz the power, 2016-07-21 05:23:40]

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-21 05:42:58]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-07-21 05:43:02]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-07-21 05:43:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
mbarbella@chromium.org, sky@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2072613003/#ps610001
(title: "Merge")

[tibell, 2016-07-21 05:43:09]

The CQ bit was checked by tibell@chromium.org

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

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-21 06:22:23]

Committed patchset #32 (id:610001)

[commit-bot: I haz the power, 2016-07-21 06:23:50]

Description was changed from

==========
Convert GetSearchProviderInstallState to Mojo
==========

to

==========
Convert GetSearchProviderInstallState to Mojo

Committed: https://crrev.com/be180334960710cf63734aeffc8046df6144e197
Cr-Commit-Position: refs/heads/master@{#406782}
==========

[commit-bot: I haz the power, 2016-07-21 06:23:52]

Patchset 32 (id:??) landed as
https://crrev.com/be180334960710cf63734aeffc8046df6144e197
Cr-Commit-Position: refs/heads/master@{#406782}

[yzshen1, 2016-07-21 15:42:36]

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

[yzshen1, 2016-07-21 15:42:38]

(A drive-by comment. I didn't review the whole CL.)

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

https://codereview.chromium.org/2072613003/diff/610001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:43: void
InstallService2(
It seems this is not used anywhere?

[tibell, 2016-07-22 00:19:53]

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

https://codereview.chromium.org/2072613003/diff/610001/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_state_impl.cc:43: void
InstallService2(
On 2016/07/21 15:42:38, yzshen1 wrote:
> It seems this is not used anywhere?

Good catch.

Fixed in https://codereview.chromium.org/2169243002.