Convert instant search messages to Mojo

Convert instant search messages to Mojo

Instead of mirroring browser state into a RenderViewObserver we use a RenderFrameObserver on the main frame. This is because Mojo, by design, doesn't communicate with RenderViews. Also, the data is eventually used from the main frame.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=653373
Review-Url: https://codereview.chromium.org/2086223002
Cr-Commit-Position: refs/heads/master@{#443838}
Committed: https://chromium.googlesource.com/chromium/src/+/e3df8cd8ac45539711b1f0b6a00420f87d09d279

[tibell, 2016-06-23 01:21:42]

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

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

Description was changed from

==========
Convert instant search messages to Mojo

TEST=unit_tests
==========

to

==========
Convert instant search messages to Mojo
==========

[tibell, 2016-06-24 00:32:11]

This is now ready for review.

[Sam McNally, 2016-06-24 02:51:16]

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:178: "//content/public/common",
I don't think this is necessary anymore.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:16: #include
"content/public/common/service_registry.h"
Remove.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:167: bool
SearchIPCRouter::ShouldHandleMessage() const {
Check this in RenderFrameCreated instead.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:18: #include
"content/public/browser/render_frame_host.h"
Forward declare RenderFrameHost and move this into the .cc file.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:120: void
RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
// Overridden from contents::WebContentsObserver:

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router_unittest.cc:326:
mojo::String::From(base::ASCIIToUTF16("Server")));
Remove the base::ASCIIToUTF16()

https://codereview.chromium.org/2086223002/diff/40001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.cc:28: #include
"content/public/common/service_registry.h"
Remove.

[tibell, 2016-06-24 03:55:51]

PTAL

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:178: "//content/public/common",
On 2016/06/24 02:51:15, Sam McNally wrote:
> I don't think this is necessary anymore.

Done.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:16: #include
"content/public/common/service_registry.h"
On 2016/06/24 02:51:15, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:167: bool
SearchIPCRouter::ShouldHandleMessage() const {
On 2016/06/24 02:51:15, Sam McNally wrote:
> Check this in RenderFrameCreated instead.

Done.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:18: #include
"content/public/browser/render_frame_host.h"
On 2016/06/24 02:51:16, Sam McNally wrote:
> Forward declare RenderFrameHost and move this into the .cc file.

Done.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:120: void
RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
On 2016/06/24 02:51:15, Sam McNally wrote:
> // Overridden from contents::WebContentsObserver:

Done.

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router_unittest.cc:326:
mojo::String::From(base::ASCIIToUTF16("Server")));
On 2016/06/24 02:51:16, Sam McNally wrote:
> Remove the base::ASCIIToUTF16()

Done.

https://codereview.chromium.org/2086223002/diff/40001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/2086223002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.cc:28: #include
"content/public/common/service_registry.h"
On 2016/06/24 02:51:16, Sam McNally wrote:
> Remove.

Done.

[tibell, 2016-06-24 03:57:41]

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

[tibell, 2016-06-24 03:57:43]

This is another Chrome IPC to Mojo conversion. As a reminder, we're doing a few
of these to iron out any issues with converting to Mojo, so we can make that
simpler before we ask non-Mojo team members to do it.

[tibell, 2016-06-24 04:16:56]

https://codereview.chromium.org/2086223002/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (left):

https://codereview.chromium.org/2086223002/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router_unittest.cc:207:
TEST_F(SearchIPCRouterTest, IgnoreMessagesFromNonInstantRenderers) {
This test (and the one further below) make less sense once you switch to Mojo,
because Mojo doesn't accept random messages without an existing connection. The
code as now written doesn't accept connections at all from non-instant renderers
(but see comment further down about potentially exercising the connection
logic).

[tibell, 2016-06-27 00:45:56]

PTAL

I had to revert the change to ignore incomming connections instead of incoming
messages (as suggested by sammc), because this causes tests that try to connect
to hang and fail.

[tibell, 2016-06-29 03:43:50]

On 2016/06/27 00:45:56, tibell wrote:
> PTAL
> 
> I had to revert the change to ignore incomming connections instead of incoming
> messages (as suggested by sammc), because this causes tests that try to
connect
> to hang and fail.

Friendly ping. This is now ready for review.

[Sam McNally, 2016-06-29 05:55:37]

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:167: bool
SearchIPCRouter::ShouldHandleMessage() const {
I think this needs to drop existing connections if there's a
cross-render-process navigation. Otherwise there could be lingering connections
from the old renderer that would have been ignored otherwise.

Have you tried checking InstantService::IsInstantProcess() for the
RenderFrameHost's process in RenderFrameCreated()?

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:265:
!IsProviderValid(provider.To<base::string16>()))
Add {}

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:284:
!IsProviderValid(provider.To<base::string16>()))
Add {}

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:233:
mojo::BindingSet<chrome::mojom::Instant> binding_set_;
BindingSets are usually named bindings_.

[tibell, 2016-07-07 03:49:23]

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:167: bool
SearchIPCRouter::ShouldHandleMessage() const {
On 2016/06/29 05:55:37, Sam McNally wrote:
> I think this needs to drop existing connections if there's a
> cross-render-process navigation. Otherwise there could be lingering
connections
> from the old renderer that would have been ignored otherwise.
> 
> Have you tried checking InstantService::IsInstantProcess() for the
> RenderFrameHost's process in RenderFrameCreated()?

I need to give this one some more thought. As per our offline discussion there
might be a race with the frame navigation messages as Mojo doesn't provide FIFO
ordering w.r.t. those messages. We could potentially work around this, but that
seems wrong. Perhaps we're better of waiting for FIFO support between Mojo and
Chrome IPC.

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:265:
!IsProviderValid(provider.To<base::string16>()))
On 2016/06/29 05:55:37, Sam McNally wrote:
> Add {}

Done.

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:284:
!IsProviderValid(provider.To<base::string16>()))
On 2016/06/29 05:55:37, Sam McNally wrote:
> Add {}

Done.

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:233:
mojo::BindingSet<chrome::mojom::Instant> binding_set_;
On 2016/06/29 05:55:37, Sam McNally wrote:
> BindingSets are usually named bindings_.

Done.

[tibell, 2016-07-07 05:36:02]

PTAL

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.cc:167: bool
SearchIPCRouter::ShouldHandleMessage() const {
On 2016/07/07 03:49:23, tibell wrote:
> On 2016/06/29 05:55:37, Sam McNally wrote:
> > I think this needs to drop existing connections if there's a
> > cross-render-process navigation. Otherwise there could be lingering
> connections
> > from the old renderer that would have been ignored otherwise.
> > 
> > Have you tried checking InstantService::IsInstantProcess() for the
> > RenderFrameHost's process in RenderFrameCreated()?
> 
> I need to give this one some more thought. As per our offline discussion there
> might be a race with the frame navigation messages as Mojo doesn't provide
FIFO
> ordering w.r.t. those messages. We could potentially work around this, but
that
> seems wrong. Perhaps we're better of waiting for FIFO support between Mojo and
> Chrome IPC.

I went with the easier to reason about approach of simply disallowing any
connections from non-Instant processes.

[dcheng, 2016-07-08 06:53:15]

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:211: const base::TimeDelta& time)
{
I haven't looked through the entire CL yet, but is there a way to tell mojom
that some things should be passed by value?

[dcheng, 2016-07-08 07:22:27]

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:211: const base::TimeDelta& time)
{
On 2016/07/08 06:53:15, dcheng wrote:
> I haven't looked through the entire CL yet, but is there a way to tell mojom
> that some things should be passed by value?

Ah there is. I'll fix it.

[tibell, 2016-07-11 00:53:03]

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:211: const base::TimeDelta& time)
{
On 2016/07/08 07:22:27, dcheng wrote:
> On 2016/07/08 06:53:15, dcheng wrote:
> > I haven't looked through the entire CL yet, but is there a way to tell mojom
> > that some things should be passed by value?
> 
> Ah there is. I'll fix it.

Acknowledged.

[dcheng, 2016-07-11 12:29:29]

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

[dcheng, 2016-07-11 12:29:30]

I like this way of scoping instant messages to instant processes: it definitely
seems simpler. However, I still have some concerns about the clarify of impl
lifetimes with Mojo though: at the very least, we need some sort of
documentation about what makes this safe. It would be even better if we could
figure out a way not to use base::Unretained =)

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/140001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:44:
base::Bind(&SearchIPCRouter::Bind, base::Unretained(this)));
It's difficult for me to understand how this is safe: this is /probably/ safe,
but it's very hard to be sure (this goes back to the thread on chromium-dev):
what guarantee does Mojo provide me that the InterfaceRegistry won't end up
outliving the WebContents (since SearchIPCRouter is ultimately owned by an
object that's a UserData of WebContents)

[tibell, 2016-07-20 05:35:42]

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

[commit-bot: I haz the power, 2016-07-20 05:36:02]

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 06:20:29]

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

[commit-bot: I haz the power, 2016-07-20 06:20:30]

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-08-26 21:03:38]

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

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

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-08-26 21:08:51]

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

[commit-bot: I haz the power, 2016-08-26 21:08:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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-09-23 05:04:06]

Rebase

[tibell, 2016-09-23 05:04:14]

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

[commit-bot: I haz the power, 2016-09-23 05:04:23]

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-09-23 05:42:33]

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

[commit-bot: I haz the power, 2016-09-23 05:42:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tibell, 2016-09-30 04:41:47]

Rewrite using WebContentsObserver Mojo support

[tibell, 2016-09-30 04:41:54]

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

[commit-bot: I haz the power, 2016-09-30 04:42:11]

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-09-30 04:45:39]

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

[commit-bot: I haz the power, 2016-09-30 04:45:40]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tibell, 2016-09-30 04:47:22]

tibell@chromium.org changed reviewers:
+ kmadhusu@chromium.org, rockot@chromium.org

[tibell, 2016-09-30 04:47:23]

This is now ready for review.

I've substantially improved the CL by using new support for using Mojo in
WebContentsObservers and I've also simplified mocking of mojom interfaces (see
changes in associated_interface_ptr.h).

kmadhusu@: Can you please review this from a search perspective. Especially look
at the naming of the interfaces/methods in instant.mojom. I'm happy to change
them. Also if you could suggest some docs that would be great. All the messages
in render_messages.h weren't documented.

rockot@/sammc@: Please spend some extra attention on associated_interface_ptr.h.

[tibell, 2016-09-30 04:53:23]

Fix visibility rules on Android etc

[tibell, 2016-09-30 04:53:32]

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

[commit-bot: I haz the power, 2016-09-30 04:53:47]

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-09-30 04:57:10]

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

[commit-bot: I haz the power, 2016-09-30 04:57:11]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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-09-30 05:15:28]

Add missing interface impl registration

[tibell, 2016-09-30 05:15:35]

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

[commit-bot: I haz the power, 2016-09-30 05:15:45]

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-09-30 05:19:07]

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

[commit-bot: I haz the power, 2016-09-30 05:19:08]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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-09-30 05:44:02]

Add missing build dep

[tibell, 2016-09-30 05:44:14]

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

[commit-bot: I haz the power, 2016-09-30 05:44:28]

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-09-30 06:37:17]

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

[commit-bot: I haz the power, 2016-09-30 06:37:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-10-03 18:08:42]

Awesome, associated interface usage and mojo/public LGTM

https://codereview.chromium.org/2086223002/diff/280001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/associated_interface_ptr.h (right):

https://codereview.chromium.org/2086223002/diff/280001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/associated_interface_ptr.h:243: // through a message
pipe endpoint.
nit: Also worth noting that this interface is never actually associated with any
other interface, so it's not appropriate for testing interactions over > 1
interface.

[tibell, 2016-10-04 06:06:48]

Ignore too-early messages

[tibell, 2016-10-04 06:07:02]

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

[commit-bot: I haz the power, 2016-10-04 06:07:13]

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-10-04 07:09:01]

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

[commit-bot: I haz the power, 2016-10-04 07:09:03]

Dry run: This issue passed the CQ dry run.

[tibell, 2016-10-04 23:05:56]

tibell@chromium.org changed reviewers:
+ treib@chromium.org
- kmadhusu@chromium.org

[tibell, 2016-10-04 23:06:21]

tibell@chromium.org changed reviewers:
- creis@chromium.org

[dcheng, 2016-10-05 00:44:19]

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:33:
mojo::GetProxyForTesting(&search_box_);
Sorry, I don't quite get this comment; can you help me understand what it means?
To me, it seems unusual to see testing (?) code here. Why would we be processing
search IPCs before we have a render frame? Is this something triggered by tests?

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:135: if
(!IsRenderedInInstantProcess())
Would it be possible to get rid of this check? My understanding is that with
mojo, interfaces are associated (securely?) in some way with their endpoints,
which means that a rogue renderer couldn't just start sending search IPCs, as it
wouldn't have the right message pipe.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:242: chrome::mojom::SearchBox*
search_box_for_testing_;
How hard would it be to not have these two separate fields? I feel like this is
potentially trouble if someone just uses search_box_ directly by accident.

Can unit tests pass in the thing with PassInterface() and just retain the raw
pointer themselves? In general, my preference is to place the test burden on
tests where possible (my current enemy is "this is null for testing" :-)

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
chrome/common/instant.mojom:19: interface Instant {
Nit: it would be helpful to me to have some more comments on these interfaces.
In particular, it'd be useful to know that Instant is the privileged interface
and SearchBox is not without needing to examine the interface methods.

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/render_m...
chrome/common/render_messages.h:14: #include
"chrome/common/instant_type_traits.h"
Slightly surprising to me that we still need to include this here. I guess that
it's to force generation of the appropriate ParamTraits?

[Sam McNally, 2016-10-05 01:34:09]

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_tab_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:109:
search_tab()->DetermineIfPageSupportsInstant();
I think this needs a FlushForTesting().

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:120:
EXPECT_CALL(*mock_search_box(), DetermineIfPageSupportsInstant()).Times(1);
I think this is relying on RenderViewHostTestHarness::TearDown() running the
message loop before the mock is destroyed in the destructor. Please ensure that
the mock verifies earlier than that (clearing in InstantTabTest::TearDown()
seems OK).

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:217:
GetSearchIPCRouter().FocusOmnibox(GetSearchIPCRouterSeqNo(),
This is also relying on a base class TearDown().

https://codereview.chromium.org/2086223002/diff/300001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:122: "//mojo/common",
What's this for?

[Marc Treib, 2016-10-05 08:13:48]

Some drive-by nits :)

Also, is there a tracking bug for this that you could link in the description?

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_search_prerenderer_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:91: explicit
DummyPrerenderContentsFactory(bool call_did_finish_load,
Doesn't need to be explicit anymore.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_tab_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:92:
EXPECT_CALL(*mock_search_box(), DetermineIfPageSupportsInstant()).Times(1);
nit: ".Times(1)" doesn't do anything, might as well leave it out.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:121: void
RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
Can this be private?

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:164: // chrome::mojom::Instant:
Also here: Can all these be private?

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:242: chrome::mojom::SearchBox*
search_box_for_testing_;
On 2016/10/05 00:44:18, dcheng wrote:
> How hard would it be to not have these two separate fields? I feel like this
is
> potentially trouble if someone just uses search_box_ directly by accident.
> 
> Can unit tests pass in the thing with PassInterface() and just retain the raw
> pointer themselves? In general, my preference is to place the test burden on
> tests where possible (my current enemy is "this is null for testing" :-)

+1, I'd much prefer a single member here

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_tab_helper.h:92: SearchIPCRouter& ipc_router() {
return ipc_router_; }
Does this have to be public? If it's only used for tests, I generally prefer
"friend" declarations.

[tibell, 2016-10-06 04:04:33]

Address review comments

[tibell, 2016-10-06 04:04:35]

Description was changed from

==========
Convert instant search messages to Mojo
==========

to

==========
Convert instant search messages to Mojo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tibell, 2016-10-06 04:05:15]

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

[tibell, 2016-10-06 04:05:27]

Description was changed from

==========
Convert instant search messages to Mojo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert instant search messages to Mojo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=653373
==========

[commit-bot: I haz the power, 2016-10-06 04:05:35]

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

[tibell, 2016-10-06 04:06:44]

PTAL

treib@ could you take a look at the mojom file and help add some comments for
the methods there. I just copied them from render_messages.h but someone who has
a better understanding of what the code actually is doing would be a better
person to document the interface.

Thanks!

https://codereview.chromium.org/2086223002/diff/280001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/associated_interface_ptr.h (right):

https://codereview.chromium.org/2086223002/diff/280001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/associated_interface_ptr.h:243: // through a message
pipe endpoint.
On 2016/10/03 18:08:41, Ken Rockot wrote:
> nit: Also worth noting that this interface is never actually associated with
any
> other interface, so it's not appropriate for testing interactions over > 1
> interface.

As per our offline discussion I've renamed this function to GetIsolatedProxy and
improved its documentation. It can now be used both in testing and as a
replacement for GetDummyProxyForTesting (which I have deleted).

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_search_prerenderer_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:91: explicit
DummyPrerenderContentsFactory(bool call_did_finish_load,
On 2016/10/05 08:13:48, Marc Treib wrote:
> Doesn't need to be explicit anymore.

Done.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_tab_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:92:
EXPECT_CALL(*mock_search_box(), DetermineIfPageSupportsInstant()).Times(1);
On 2016/10/05 08:13:48, Marc Treib wrote:
> nit: ".Times(1)" doesn't do anything, might as well leave it out.

Done.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:109:
search_tab()->DetermineIfPageSupportsInstant();
On 2016/10/05 01:34:09, Sam McNally wrote:
> I think this needs a FlushForTesting().

Done.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:120:
EXPECT_CALL(*mock_search_box(), DetermineIfPageSupportsInstant()).Times(1);
On 2016/10/05 01:34:09, Sam McNally wrote:
> I think this is relying on RenderViewHostTestHarness::TearDown() running the
> message loop before the mock is destroyed in the destructor. Please ensure
that
> the mock verifies earlier than that (clearing in InstantTabTest::TearDown()
> seems OK).

Done.

We now run the event loop manually. A bit more noisy, but more robust.

Also, as discussed offline, I make sure that the mock gets verified at the end
of the tests so to make sure we don't rely on TearDown in any of the super
classes.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:33:
mojo::GetProxyForTesting(&search_box_);
On 2016/10/05 00:44:18, dcheng wrote:
> Sorry, I don't quite get this comment; can you help me understand what it
means?
> To me, it seems unusual to see testing (?) code here. Why would we be
processing
> search IPCs before we have a render frame? Is this something triggered by
tests?

(The function has now been renamed as it had non-test uses.)

The old Chrome IPC code definitely sends messages before RenderFrameCreated is
called in browser tests (that aren't testing this class or its users
intentionally). I do not know why.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:135: if
(!IsRenderedInInstantProcess())
On 2016/10/05 00:44:18, dcheng wrote:
> Would it be possible to get rid of this check? My understanding is that with
> mojo, interfaces are associated (securely?) in some way with their endpoints,
> which means that a rogue renderer couldn't just start sending search IPCs, as
it
> wouldn't have the right message pipe.

This check is necessary today as we have one router in every process (even
non-instant search ones). Therefore we need to reject messages for some of these
routers.

I've added a TODO in IsRenderedInInstantProcess sketching out what this might
look like if we did a bigger rethink of how the SearchIPCRouter works.

I don't think we want to do the change I describe in the TODO in this CL. In
general, I don't think we want to do larger changes like that when converting,
but rather as a separate step. That keeps the migration manageable, allows us to
be reasonably comfortable we didn't break things (by mimicking old behavior),
and makes migrations feasible to do for members of the Mojo team and
non-original authors of the code in general.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:121: void
RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
On 2016/10/05 08:13:48, Marc Treib wrote:
> Can this be private?

Done.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:164: // chrome::mojom::Instant:
On 2016/10/05 08:13:48, Marc Treib wrote:
> Also here: Can all these be private?

No, because the tests call these to simulate incoming IPCs.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:242: chrome::mojom::SearchBox*
search_box_for_testing_;
On 2016/10/05 08:13:48, Marc Treib wrote:
> On 2016/10/05 00:44:18, dcheng wrote:
> > How hard would it be to not have these two separate fields? I feel like this
> is
> > potentially trouble if someone just uses search_box_ directly by accident.
> > 
> > Can unit tests pass in the thing with PassInterface() and just retain the
raw
> > pointer themselves? In general, my preference is to place the test burden on
> > tests where possible (my current enemy is "this is null for testing" :-)
> 
> +1, I'd much prefer a single member here

The issue here is that RenderFrameCreated changes the pointer (it needs to) and
that method will get called in tests, behind our backs, ruining our attempt to
change the real pointer.

Imagine we had a `SearchBoxAssociatedPtr* search_box()` method on
SearchIPCRouter to let us change search_box_ptr_ instead of having a second,
test pointer. Example test code:

  ConnectToMockSearchBox(router()->search_box());
  NavigateAndCommitActiveTab(...);  // RenderFrameCreated called
                                    // Message gets sent to the wrong place

We could instead of having set_search_box_for_testing have a
set_search_box_factory method that lets us inject a factory that gets used in
RenderFrameCreated, both in testing and in prod. The code gets a bit more
complicated to support this factory but it would let us change the factory in
tests and have RenderFrameCreated do the right thing in tests.

Thoughts?

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:217:
GetSearchIPCRouter().FocusOmnibox(GetSearchIPCRouterSeqNo(),
On 2016/10/05 01:34:09, Sam McNally wrote:
> This is also relying on a base class TearDown().

Done.

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_tab_helper.h:92: SearchIPCRouter& ipc_router() {
return ipc_router_; }
On 2016/10/05 08:13:48, Marc Treib wrote:
> Does this have to be public? If it's only used for tests, I generally prefer
> "friend" declarations.

Unfortunately it's used from classes in anonymous namespaces in tests so it
can't be friended. :/

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
chrome/common/instant.mojom:19: interface Instant {
On 2016/10/05 00:44:18, dcheng wrote:
> Nit: it would be helpful to me to have some more comments on these interfaces.
> In particular, it'd be useful to know that Instant is the privileged interface
> and SearchBox is not without needing to examine the interface methods.

I'm hoping that treib@ could help here, as I don't actually understand the
details of the search interface. The comments here were transferred over from
render_messages.h.

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/render_m...
chrome/common/render_messages.h:14: #include
"chrome/common/instant_type_traits.h"
On 2016/10/05 00:44:18, dcheng wrote:
> Slightly surprising to me that we still need to include this here. I guess
that
> it's to force generation of the appropriate ParamTraits?

Yes.

https://codereview.chromium.org/2086223002/diff/300001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:122: "//mojo/common",
On 2016/10/05 01:34:09, Sam McNally wrote:
> What's this for?

Done.

I used to use type converters.

[commit-bot: I haz the power, 2016-10-06 04:24:35]

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

[commit-bot: I haz the power, 2016-10-06 04:24:36]

Dry run: Exceeded global retry quota

[tibell, 2016-10-06 04:35:26]

Merge

[tibell, 2016-10-06 04:35:34]

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

[commit-bot: I haz the power, 2016-10-06 04:35:57]

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

[tibell, 2016-10-06 05:54:46]

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

[tibell, 2016-10-06 05:55:21]

On 2016/10/06 04:35:26, tibell wrote:
> Merge

+jam@ for trivial changes in render_frame_host_impl.cc and
render_process_host_impl.cc.

[Sam McNally, 2016-10-06 06:02:47]

lgtm

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:279: // TODO: Instead of rejecting
messages check at connection time if we want to
TODO(...)

[Ken Rockot(use gerrit already), 2016-10-06 06:13:04]

FWIW new mojo/ changes LGTM

[commit-bot: I haz the power, 2016-10-06 06:26:59]

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

[commit-bot: I haz the power, 2016-10-06 06:27:00]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[dcheng, 2016-10-06 07:56:57]

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:242: chrome::mojom::SearchBox*
search_box_for_testing_;
On 2016/10/06 04:06:43, tibell wrote:
> On 2016/10/05 08:13:48, Marc Treib wrote:
> > On 2016/10/05 00:44:18, dcheng wrote:
> > > How hard would it be to not have these two separate fields? I feel like
this
> > is
> > > potentially trouble if someone just uses search_box_ directly by accident.
> > > 
> > > Can unit tests pass in the thing with PassInterface() and just retain the
> raw
> > > pointer themselves? In general, my preference is to place the test burden
on
> > > tests where possible (my current enemy is "this is null for testing" :-)
> > 
> > +1, I'd much prefer a single member here
> 
> The issue here is that RenderFrameCreated changes the pointer (it needs to)
and
> that method will get called in tests, behind our backs, ruining our attempt to
> change the real pointer.
> 
> Imagine we had a `SearchBoxAssociatedPtr* search_box()` method on
> SearchIPCRouter to let us change search_box_ptr_ instead of having a second,
> test pointer. Example test code:
> 
>   ConnectToMockSearchBox(router()->search_box());
>   NavigateAndCommitActiveTab(...);  // RenderFrameCreated called
>                                     // Message gets sent to the wrong place
> 
> We could instead of having set_search_box_for_testing have a
> set_search_box_factory method that lets us inject a factory that gets used in
> RenderFrameCreated, both in testing and in prod. The code gets a bit more
> complicated to support this factory but it would let us change the factory in
> tests and have RenderFrameCreated do the right thing in tests.
> 
> Thoughts?

Hmm, so I guess things get weird because this is a unit test.

I don't know how hard it is, but it looks like some of the mock content hosts
already allow mocking out the interface provider (for example,
MockRenderProcessHost). Maybe it would be best if the InterfaceProvider* on
RenderFrameHost could be hooked in unit tests? WDYT?

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:33:
mojo::GetIsolatedProxy(&search_box_);
Can you put a TODO to fix this? Maybe treib@ or someone will have time to look
at this.

It's pretty weird that we're calling into this before there's a corresponding
RenderFrame to talk to.

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:282: // the RenderView.
Ideally, we would just reject it when the remote end makes an interface request
and before we bind it, rather than having each method reject it individually. Is
that still not practical to do here?

[Marc Treib, 2016-10-06 11:03:21]

A few more comments - unfortunately, I'm not really familiar enough (yet) with
all the SearchBox/Instant IPC stuff to help much, sorry :(

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
chrome/common/instant.mojom:19: interface Instant {
On 2016/10/06 04:06:43, tibell wrote:
> On 2016/10/05 00:44:18, dcheng wrote:
> > Nit: it would be helpful to me to have some more comments on these
interfaces.
> > In particular, it'd be useful to know that Instant is the privileged
interface
> > and SearchBox is not without needing to examine the interface methods.
> 
> I'm hoping that treib@ could help here, as I don't actually understand the
> details of the search interface. The comments here were transferred over from
> render_messages.h.

I inherited this code, and am also not too familiar yet with all the details...
The comments on the methods here actually look quite reasonable to me.

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
chrome/common/instant.mojom:37: LogEvent(
nit: remove the extra line break

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:41: &search_box_);
Can this happen more than once per SearchIPCRouter instance? If not, can you
DCHECK that search_box_ is "null" before (whatever exactly the GetIsolatedProxy
above returns)?
Should search_box_ get cleared in RenderFrameDeleted?

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:185: // Used by unit tests.
nitty nit: comment isn't necessary, the "for_testing" suffix says that already
(and AFAIK is actually checked by presubmits)

https://codereview.chromium.org/2086223002/diff/340001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:56: // Sends ChromeViewHostMsg_LogEvent to
the browser.
Please update all the comments here to no longer reference the old messages.

[jam, 2016-10-06 15:40:49]

On 2016/10/06 05:55:21, tibell wrote:
> On 2016/10/06 04:35:26, tibell wrote:
> > Merge
> 
> +jam@ for trivial changes in render_frame_host_impl.cc and
> render_process_host_impl.cc.

lgtm

[tibell, 2016-10-13 05:30:25]

Address more review comments (required merge)

[tibell, 2016-10-13 05:31:13]

PTAL

I've spent a couple of days trying to figure out the best way to inject mocks
for testing. After some initial misadventures trying to override the
InterfaceProvider in tests, which ended up with lots of issues both related to
running message loops to propagate messages and to actually having the tests use
a testing-compatible InterfaceProvider, I've settled on something simpler using
a simple, overridable factory.

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
chrome/common/instant.mojom:19: interface Instant {
On 2016/10/06 11:03:21, Marc Treib wrote:
> On 2016/10/06 04:06:43, tibell wrote:
> > On 2016/10/05 00:44:18, dcheng wrote:
> > > Nit: it would be helpful to me to have some more comments on these
> interfaces.
> > > In particular, it'd be useful to know that Instant is the privileged
> interface
> > > and SearchBox is not without needing to examine the interface methods.
> > 
> > I'm hoping that treib@ could help here, as I don't actually understand the
> > details of the search interface. The comments here were transferred over
from
> > render_messages.h.
> 
> I inherited this code, and am also not too familiar yet with all the
details...
> The comments on the methods here actually look quite reasonable to me. 

Acknowledged.

https://codereview.chromium.org/2086223002/diff/300001/chrome/common/instant....
chrome/common/instant.mojom:37: LogEvent(
On 2016/10/06 11:03:21, Marc Treib wrote:
> nit: remove the extra line break

Done.

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:33:
mojo::GetIsolatedProxy(&search_box_);
On 2016/10/06 07:56:57, dcheng wrote:
> Can you put a TODO to fix this? Maybe treib@ or someone will have time to look
> at this.
> 
> It's pretty weird that we're calling into this before there's a corresponding
> RenderFrame to talk to.

Done.

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:41: &search_box_);
On 2016/10/06 11:03:21, Marc Treib wrote:
> Can this happen more than once per SearchIPCRouter instance? If not, can you
> DCHECK that search_box_ is "null" before (whatever exactly the
GetIsolatedProxy
> above returns)?
> Should search_box_ get cleared in RenderFrameDeleted?

I've completely changed how this work. We really only need to connect to the
main frame I believe. Is that right?

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:279: // TODO: Instead of rejecting
messages check at connection time if we want to
On 2016/10/06 06:02:47, Sam McNally wrote:
> TODO(...)

Done.

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:282: // the RenderView.
On 2016/10/06 07:56:57, dcheng wrote:
> Ideally, we would just reject it when the remote end makes an interface
request
> and before we bind it, rather than having each method reject it individually.
Is
> that still not practical to do here?

It's difficult so I left a TODO.

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:185: // Used by unit tests.
On 2016/10/06 11:03:21, Marc Treib wrote:
> nitty nit: comment isn't necessary, the "for_testing" suffix says that already
> (and AFAIK is actually checked by presubmits)

Done.

https://codereview.chromium.org/2086223002/diff/340001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/2086223002/diff/340001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:56: // Sends ChromeViewHostMsg_LogEvent to
the browser.
On 2016/10/06 11:03:21, Marc Treib wrote:
> Please update all the comments here to no longer reference the old messages.

Done.

[tibell, 2016-10-13 05:31:21]

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

[commit-bot: I haz the power, 2016-10-13 05:31:45]

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-10-13 05:34:20]

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

[commit-bot: I haz the power, 2016-10-13 05:34:22]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-10-13 06:20:02]

Merge

[tibell, 2016-10-13 06:20:12]

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

[commit-bot: I haz the power, 2016-10-13 06:20:31]

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-10-13 08:02:38]

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

[commit-bot: I haz the power, 2016-10-13 08:02:39]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Marc Treib, 2016-10-13 09:04:50]

lgtm

Thanks! search lgtm with a few more comments and questions (most of which are
probably caused by my infamiliarity with Mojo :) )

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_tab_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:49: MOCK_METHOD0(GetSearchBox,
chrome::mojom::SearchBox*(void));
nit: You don't need the flexibility of a Mock here, might as well just make a
fake (i.e. take a SearchBox* in the ctor and just return that from
GetSearchBox).

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:37: &search_box_);
Does this need to happen on every GetSearchBox call, or would it be enough to
only do it the first time?

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:173: // chrome::mojom::Instant:
Does the chrome::mojom::Instant implementation have to be public?

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:94:
MOCK_METHOD0(GetSearchBox, chrome::mojom::SearchBox*(void));
nit: Like in the other test, a simple fake would do.

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_tab_helper.h:92: SearchIPCRouter& ipc_router() {
return ipc_router_; }
If this has to stay public, could you rename it to ipc_router_for_testing?

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_tab_helper_unittest.cc:75:
MOCK_METHOD0(GetSearchBox, chrome::mojom::SearchBox*(void));
Once more: Fake :)
You could also put the fake factory next to the MockSearchBox definition, so you
don't have to repeat it in all the tests.

https://codereview.chromium.org/2086223002/diff/380001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/common/instant....
chrome/common/instant.mojom:34: SearchBoxUndoMostVisitedDeletion(int32
page_seq_no, url.mojom.Url url);
Not your doing, but: Would you mind removing the "SearchBox" prefix from these
three? I don't see what that's doing there...
Or if you don't want to rename things in this CL (which is fine), just put a
TODO(treib) here. Thanks!

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:122: // Sends
ChromeViewHostMsg_StopCapturingKeyStrokes to the browser.
nit: Missed two ChromeViewHostMsg here

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:126: // browser.
nit: fits on previous line

[tibell, 2016-10-13 23:29:26]

Address treib's review comments

[tibell, 2016-10-13 23:29:36]

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

[tibell, 2016-10-13 23:29:45]

Thanks for reviewing!

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_tab_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:49: MOCK_METHOD0(GetSearchBox,
chrome::mojom::SearchBox*(void));
On 2016/10/13 09:04:50, Marc Treib wrote:
> nit: You don't need the flexibility of a Mock here, might as well just make a
> fake (i.e. take a SearchBox* in the ctor and just return that from
> GetSearchBox).

I did initially create a fake, but it's more verbose (see below) and either has
to be duplicated for all four test files or be shared by moving it into its own
little library. I settled on this as the least verbose option. Note that
ON_CALL, which I use to return the MockSearchBox doesn't set up any expectations
on the mock.

Here's a manual fake for comparison:

class SearchBoxClientFactoryFake
    : public SearchIPCRouter::SearchBoxClientFactory {
 public:
  // |mock| must outlive this object.
  SearchBoxClientFactoryFake(MockSearchBox* mock) : mock_(mock) {}
  chrome::mojom::SearchBox* GetSearchBox() override { return mock_; }

 private:
  chrome::mojom::SearchBox* mock_;

  DISALLOW_COPY_AND_ASSIGN(SearchBoxClientFactoryFake);
};

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:37: &search_box_);
On 2016/10/13 09:04:50, Marc Treib wrote:
> Does this need to happen on every GetSearchBox call, or would it be enough to
> only do it the first time?

Done.

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:173: // chrome::mojom::Instant:
On 2016/10/13 09:04:50, Marc Treib wrote:
> Does the chrome::mojom::Instant implementation have to be public?

Yes, since the tests call these methods directly. We could friend the 6 test
classes perhaps and make these private but it doesn't really offer much
protection as they can still be accessed through a chrome::mojom::Instant*.

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router_unittest.cc:94:
MOCK_METHOD0(GetSearchBox, chrome::mojom::SearchBox*(void));
On 2016/10/13 09:04:50, Marc Treib wrote:
> nit: Like in the other test, a simple fake would do.

Acknowledged.

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_tab_helper.h:92: SearchIPCRouter& ipc_router() {
return ipc_router_; }
On 2016/10/13 09:04:50, Marc Treib wrote:
> If this has to stay public, could you rename it to ipc_router_for_testing?

Done.

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_tab_helper_unittest.cc:75:
MOCK_METHOD0(GetSearchBox, chrome::mojom::SearchBox*(void));
On 2016/10/13 09:04:50, Marc Treib wrote:
> Once more: Fake :)
> You could also put the fake factory next to the MockSearchBox definition, so
you
> don't have to repeat it in all the tests.

Acknowledged.

I did consider doing exactly that, but then I thought that wasn't really the
right place (this is a fake for the browser side, not the render side). Then I
considered creating a brand new .h file to put it in and create a build target
for that. At that point I felt it was all a bit verbose for what could be
achieved more succinctly like this.

https://codereview.chromium.org/2086223002/diff/380001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/common/instant....
chrome/common/instant.mojom:34: SearchBoxUndoMostVisitedDeletion(int32
page_seq_no, url.mojom.Url url);
On 2016/10/13 09:04:50, Marc Treib wrote:
> Not your doing, but: Would you mind removing the "SearchBox" prefix from these
> three? I don't see what that's doing there...
> Or if you don't want to rename things in this CL (which is fine), just put a
> TODO(treib) here. Thanks!

Added a TODO. :)

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:122: // Sends
ChromeViewHostMsg_StopCapturingKeyStrokes to the browser.
On 2016/10/13 09:04:50, Marc Treib wrote:
> nit: Missed two ChromeViewHostMsg here

These are different messages not part of the search interface! (They probably
should be mojo-ified at some point though.)

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:126: // browser.
On 2016/10/13 09:04:50, Marc Treib wrote:
> nit: fits on previous line

Done.

[commit-bot: I haz the power, 2016-10-13 23:30:00]

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-10-14 01:08:29]

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

[commit-bot: I haz the power, 2016-10-14 01:08:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2016-10-14 04:06:49]

CCing alexmos@ as there's some site-isolation related test failures and perhaps
he has an idea of what to do. :) If not I will try to understand what's going on
on Monday.

[tibell, 2016-10-14 05:36:21]

Convert SearchBox to a RenderFrameObserver to support site isolation

[tibell, 2016-10-14 05:36:57]

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

[commit-bot: I haz the power, 2016-10-14 05:37:11]

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

[tibell, 2016-10-14 05:38:59]

This needs another look.

In order to try make the Mojo impl work with site isolation I've converted the
SearchBox from a RenderViewObserver to a RenderFrameObserver. Not doing so cause
us to be unable to correctly observe frame creation
(RenderViewObserver::FrameCreated wouldn't be called when running
--site-per-process and thus we couldn't connect to the SearchIPCRouter).

[commit-bot: I haz the power, 2016-10-14 06:29:02]

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

[commit-bot: I haz the power, 2016-10-14 06:29:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2016-10-14 06:44:29]

mojo lgtm with nits

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:26:
SearchBoxClientFactoryImpl(content::WebContents* web_contents)
Nit: explicit

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:50: if (id != last_connected_rfh_)
{
This is a bit surprising to me (though I think this is actually the only correct
way to write this atm).

1) Naively, I would have expected that simply checking if the interface is bound
would have been sufficient.

2) However, this isn't good enough due to process swapping: we don't wait for
the old process to unload before starting to load the new document. If we simply
waited for the interface to be unbound before rebinding, we'd have a new process
with already committed data that might not be properly connected.

3) However, this results in interesting semantics: normally, we explicitly allow
IPCs from the process being swapped out to be processed. But with this, we'll be
ignoring everything from the old process.

I think this CL is fine as-is, but the complexities here probably warrant a
followup. I'll chat with creis@ and nasko@ to see what thoughts they have here
as well.

(Probably, the right fix for this is to make this all be frame-associated rather
than WebContents associated?)

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:114: // Creates
chrome::mojom::SearchBox connections on request.
So this is OK for now, but I think there would be a lot of value in letting us
inject / override interface requests from RFH / etc for testing. Can we file a
bug for that?

[Marc Treib, 2016-10-14 08:58:21]

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_tab_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_tab_unittest.cc:49: MOCK_METHOD0(GetSearchBox,
chrome::mojom::SearchBox*(void));
On 2016/10/13 23:29:44, tibell wrote:
> On 2016/10/13 09:04:50, Marc Treib wrote:
> > nit: You don't need the flexibility of a Mock here, might as well just make
a
> > fake (i.e. take a SearchBox* in the ctor and just return that from
> > GetSearchBox).
> 
> I did initially create a fake, but it's more verbose (see below) and either
has
> to be duplicated for all four test files or be shared by moving it into its
own
> little library. I settled on this as the least verbose option. Note that
> ON_CALL, which I use to return the MockSearchBox doesn't set up any
expectations
> on the mock.
> 
> Here's a manual fake for comparison:
> 
> class SearchBoxClientFactoryFake
>     : public SearchIPCRouter::SearchBoxClientFactory {
>  public:
>   // |mock| must outlive this object.
>   SearchBoxClientFactoryFake(MockSearchBox* mock) : mock_(mock) {}
>   chrome::mojom::SearchBox* GetSearchBox() override { return mock_; }
> 
>  private:
>   chrome::mojom::SearchBox* mock_;
> 
>   DISALLOW_COPY_AND_ASSIGN(SearchBoxClientFactoryFake);
> };

Yep, that's about what I would expect. The class itself is slightly more
verbose, but you save a few ON_CALL lines.
But more importantly, it's just more honest: This is effectively a fake, not a
mock, so I'd prefer to call it that.

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.h:173: // chrome::mojom::Instant:
On 2016/10/13 23:29:44, tibell wrote:
> On 2016/10/13 09:04:50, Marc Treib wrote:
> > Does the chrome::mojom::Instant implementation have to be public?
> 
> Yes, since the tests call these methods directly. We could friend the 6 test
> classes perhaps and make these private but it doesn't really offer much
> protection as they can still be accessed through a chrome::mojom::Instant*.

I'm not worried about protection, but about making it easy to understand how the
class is supposed to be used. And these aren't really part of the public
interface - noone who uses this class is supposed to call them directly.
So I'd prefer to make them private and friend the tests, or even better, have
the tests cast to chrome::mojom::Instant in order to call them. (I always find
it preferable to put the burden on the tests, rather than on the production
code.)

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/2086223002/diff/380001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.h:122: // Sends
ChromeViewHostMsg_StopCapturingKeyStrokes to the browser.
On 2016/10/13 23:29:44, tibell wrote:
> On 2016/10/13 09:04:50, Marc Treib wrote:
> > nit: Missed two ChromeViewHostMsg here
> 
> These are different messages not part of the search interface! (They probably
> should be mojo-ified at some point though.)

But this class doesn't (directly) send them anymore, right?

https://codereview.chromium.org/2086223002/diff/420001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/2086223002/diff/420001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.cc:20: #include
"chrome/common/render_messages.h"
Can this be removed now? :)

[dcheng, 2016-10-14 23:43:37]

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/400001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:50: if (id != last_connected_rfh_)
{
On 2016/10/14 06:44:29, dcheng wrote:
> This is a bit surprising to me (though I think this is actually the only
correct
> way to write this atm).
> 
> 1) Naively, I would have expected that simply checking if the interface is
bound
> would have been sufficient.
> 
> 2) However, this isn't good enough due to process swapping: we don't wait for
> the old process to unload before starting to load the new document. If we
simply
> waited for the interface to be unbound before rebinding, we'd have a new
process
> with already committed data that might not be properly connected.
> 
> 3) However, this results in interesting semantics: normally, we explicitly
allow
> IPCs from the process being swapped out to be processed. But with this, we'll
be
> ignoring everything from the old process.
> 
> I think this CL is fine as-is, but the complexities here probably warrant a
> followup. I'll chat with creis@ and nasko@ to see what thoughts they have here
> as well.
> 
> (Probably, the right fix for this is to make this all be frame-associated
rather
> than WebContents associated?)

Actually, nasko@ points out WebContentsObserver::RenderFrameHostChanged(), so
perhaps we can use that here instead?

[tibell, 2016-10-19 04:30:58]

Merge

[tibell, 2016-10-24 00:06:49]

Merge

[tibell, 2016-10-24 00:18:08]

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

[commit-bot: I haz the power, 2016-10-24 00:18: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-10-24 01:17:04]

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

[commit-bot: I haz the power, 2016-10-24 01:17:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Marc Treib, 2016-11-22 12:52:04]

Ping - what's the status of this CL?

[tibell, 2016-11-24 23:18:29]

On 2016/11/22 12:52:04, Marc Treib wrote:
> Ping - what's the status of this CL?

It's currently stuck on some test failures and I got distracted with other
things. I'll try to get back to the CL when I can.

[Marc Treib, 2016-11-25 10:32:30]

On 2016/11/24 23:18:29, tibell wrote:
> On 2016/11/22 12:52:04, Marc Treib wrote:
> > Ping - what's the status of this CL?
> 
> It's currently stuck on some test failures and I got distracted with other
> things. I'll try to get back to the CL when I can.

Do you have some (rough) timeline?
Background: I have some other changes planned to this IPC code, and I'd prefer
to do them directly in the new Mojo world. Also that would save you some more
merging ;)

[tibell, 2016-11-27 22:34:07]

On 2016/11/25 10:32:30, Marc Treib wrote:
> On 2016/11/24 23:18:29, tibell wrote:
> > On 2016/11/22 12:52:04, Marc Treib wrote:
> > > Ping - what's the status of this CL?
> > 
> > It's currently stuck on some test failures and I got distracted with other
> > things. I'll try to get back to the CL when I can.
> 
> Do you have some (rough) timeline?
> Background: I have some other changes planned to this IPC code, and I'd prefer
> to do them directly in the new Mojo world. Also that would save you some more
> merging ;)

Not this week and I'll be out the following week. If that's longer than you need
please go ahead and make the changes in the old IPC and I'll merge (unless you
want to take over this CL).

[Marc Treib, 2016-11-28 15:49:38]

On 2016/11/27 22:34:07, tibell wrote:
> On 2016/11/25 10:32:30, Marc Treib wrote:
> > On 2016/11/24 23:18:29, tibell wrote:
> > > On 2016/11/22 12:52:04, Marc Treib wrote:
> > > > Ping - what's the status of this CL?
> > > 
> > > It's currently stuck on some test failures and I got distracted with other
> > > things. I'll try to get back to the CL when I can.
> > 
> > Do you have some (rough) timeline?
> > Background: I have some other changes planned to this IPC code, and I'd
prefer
> > to do them directly in the new Mojo world. Also that would save you some
more
> > merging ;)
> 
> Not this week and I'll be out the following week. If that's longer than you
need
> please go ahead and make the changes in the old IPC and I'll merge (unless you
> want to take over this CL).

Alright, thanks! We'll see how far I get; I probably won't have much time this
week either. In any case, my changes won't be very large (some added/removed
members in InstantMostVisitedItem), so merging should be straightforward enough.

[Marc Treib, 2016-12-12 16:14:58]

On 2016/11/28 15:49:38, Marc Treib wrote:
> On 2016/11/27 22:34:07, tibell wrote:
> > On 2016/11/25 10:32:30, Marc Treib wrote:
> > > On 2016/11/24 23:18:29, tibell wrote:
> > > > On 2016/11/22 12:52:04, Marc Treib wrote:
> > > > > Ping - what's the status of this CL?
> > > > 
> > > > It's currently stuck on some test failures and I got distracted with
other
> > > > things. I'll try to get back to the CL when I can.
> > > 
> > > Do you have some (rough) timeline?
> > > Background: I have some other changes planned to this IPC code, and I'd
> prefer
> > > to do them directly in the new Mojo world. Also that would save you some
> more
> > > merging ;)
> > 
> > Not this week and I'll be out the following week. If that's longer than you
> need
> > please go ahead and make the changes in the old IPC and I'll merge (unless
you
> > want to take over this CL).
> 
> Alright, thanks! We'll see how far I get; I probably won't have much time this
> week either. In any case, my changes won't be very large (some added/removed
> members in InstantMostVisitedItem), so merging should be straightforward
enough.

Update: I've landed 3 CLs which touched the searchbox IPC code:
https://codereview.chromium.org/2525653002
https://codereview.chromium.org/2544883006
https://codereview.chromium.org/2532103002

All of them should be straightforward to integrate here. Let me know if I can
help with anything!

[tibell, 2017-01-03 04:01:14]

Merge

[tibell, 2017-01-03 04:01:18]

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

[commit-bot: I haz the power, 2017-01-03 04:01:33]

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, 2017-01-03 04:22:23]

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

[commit-bot: I haz the power, 2017-01-03 04:22:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2017-01-03 04:27:31]

I've (partly) swapped back the problem the originally got this CL stuck into my
head. I'm going to try to write it down here:

The core issue is that there isn't a Mojo channel between RenderViews and
WebContents/RenderViewHost. Since RenderViews are supposed to go away we didn't
want to encourage people to use them as a place to host Mojo services. The
SearchBox code, prior to this change, is a RenderViewObserver and I had to
change that to work with Mojo (by making SearchBox a RenderFrameObserver
instead). This means that this change wasn't trivial and required some real
refactoring.

The issue I then ran into (and which I haven't fully solved) is that some tests
start failing and I'm still investigating (and have partly forgotten) why. What
could help is a better understanding of what the (old) code tries to achieve.
Here's my understanding, perhaps you could tell me if I got it right. I'm using
a chance of most visited items as an example:

Browser:
 * InstantService detects (somehow?) a changed in most visited items.
 * InstantService calls SearchTabHelper::MostVisitedItemsChanged.
 * SearchTabHelper::MostVisitedItemsChanged calls
SearchIPCRouter::SendMostVisitedItems.
 * SearchIPCRouter::SendMostVisitedItems sends a
ChromeViewMsg_SearchBoxMostVisitedItemsChanged IPC message to the renderer.
Renderer:
 * SearchBox receives the IPC message and calls
SearchBoxExtension::DispatchMostVisitedChanged *on the main frame*.
 * SearchBoxExtension calls some JS to notify it of the change and the JS calls
back (?) to fetch the items, which have been received in the IPC and cached in
the SearchBox.

My understanding here is that 1) only the main frame matters here and it's the
only frame that actually cares about changes to the most visited items and 2)
most the work going on above is just to cache the most visited items in the
renderer. Is that right?

[tibell, 2017-01-03 04:31:04]

Fix missed GetIsolatedProxy rename

[tibell, 2017-01-03 04:31:09]

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

[commit-bot: I haz the power, 2017-01-03 04:31: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, 2017-01-03 04:53:07]

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

[commit-bot: I haz the power, 2017-01-03 04:53:09]

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, 2017-01-03 04:57:41]

Remove left-over debug logging

[tibell, 2017-01-03 04:57:46]

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

[commit-bot: I haz the power, 2017-01-03 04:57:59]

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, 2017-01-03 05:39:49]

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

[commit-bot: I haz the power, 2017-01-03 05:39:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2017-01-11 06:42:56]

Only create SearchBox for main frame

[tibell, 2017-01-11 06:43:06]

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

[commit-bot: I haz the power, 2017-01-11 06:43:26]

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, 2017-01-11 07:48:19]

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

[commit-bot: I haz the power, 2017-01-11 07:48:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tibell, 2017-01-11 23:48:36]

We must have a SearchBox for all frames

[tibell, 2017-01-11 23:48:41]

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

[commit-bot: I haz the power, 2017-01-11 23:50:06]

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

[tibell, 2017-01-12 02:25:52]

Again only have a SearchBox from the main frame, but make sure it's accessed
from the extension

[tibell, 2017-01-12 02:25:58]

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

[commit-bot: I haz the power, 2017-01-12 02:26:29]

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, 2017-01-12 02:38:10]

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

[commit-bot: I haz the power, 2017-01-12 02:38:12]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tibell, 2017-01-12 03:55:48]

I think I have this all figured out now. There are future improvements we could
do (e.g. get rid of SearchIPCRouter altogether and expose a mojo service much
closure to the InstantService object and have the frame connect to that service
instead of having the browser "connect" to the renderer, like it does today.)

[tibell, 2017-01-12 03:57:09]

Description was changed from

==========
Convert instant search messages to Mojo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=653373
==========

to

==========
Convert instant search messages to Mojo

* Instead of mirror browser state into a RenderViewObserver, we use a
RenderFrameObserver on the main frame. This is because Mojo, by design, doesn't
communicate with RenderViews.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=653373
==========

[tibell, 2017-01-12 03:57:55]

Description was changed from

==========
Convert instant search messages to Mojo

* Instead of mirror browser state into a RenderViewObserver, we use a
RenderFrameObserver on the main frame. This is because Mojo, by design, doesn't
communicate with RenderViews.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=653373
==========

to

==========
Convert instant search messages to Mojo

Instead of mirroring browser state into a RenderViewObserver we use a
RenderFrameObserver on the main frame. This is because Mojo, by design, doesn't
communicate with RenderViews. Also, the data is eventually used from the main
frame.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=653373
==========

[tibell, 2017-01-12 06:02:09]

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

[commit-bot: I haz the power, 2017-01-12 06:02:43]

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, 2017-01-12 07:09:45]

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

[commit-bot: I haz the power, 2017-01-12 07:09:47]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-01-12 21:17:19]

Mojo bits still LGTM. Do you want specific people to do another review pass?

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:640: "//chrome/common:instant_mojom",
nit: use ":foo_target" for file-local targets

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/instant....
chrome/common/instant.mojom:20: interface Instant {
nit: WDYT about InstantSearch instead of just Instant? That would also go for
other relevant names (e.g. instant_mojom -> instant_search_mojom,
instant_type_traits -> instant_search_type_traits)? I only suggest this because
for someone who doesn't know better, "instant" looks like just another adjective
in many of these cases, rather than the name of a feature.

[tibell, 2017-01-12 23:53:06]

Address rockot's review comments

[tibell, 2017-01-12 23:53:11]

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

[tibell, 2017-01-12 23:54:22]

Marc, could you please take another look to make sure I didn't break any of your
recent changes when I merged? Otherwise I think this is good to go.

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:640: "//chrome/common:instant_mojom",
On 2017/01/12 21:17:19, Ken Rockot wrote:
> nit: use ":foo_target" for file-local targets

Done.

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/instant....
File chrome/common/instant.mojom (right):

https://codereview.chromium.org/2086223002/diff/580001/chrome/common/instant....
chrome/common/instant.mojom:20: interface Instant {
On 2017/01/12 21:17:19, Ken Rockot wrote:
> nit: WDYT about InstantSearch instead of just Instant? That would also go for
> other relevant names (e.g. instant_mojom -> instant_search_mojom,
> instant_type_traits -> instant_search_type_traits)? I only suggest this
because
> for someone who doesn't know better, "instant" looks like just another
adjective
> in many of these cases, rather than the name of a feature.

I will do a rename in a follow up (to make this review easier to follow).

[commit-bot: I haz the power, 2017-01-12 23:55:26]

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

[Sam McNally, 2017-01-13 00:04:08]

lgtm

https://codereview.chromium.org/2086223002/diff/580001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_search_prerenderer_unittest.cc (right):

https://codereview.chromium.org/2086223002/diff/580001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:59:
MOCK_METHOD0(GetSearchBox, chrome::mojom::SearchBox*(void));
Is the void param necessary? If not, fix everywhere.

https://codereview.chromium.org/2086223002/diff/580001/chrome/browser/ui/sear...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/2086223002/diff/580001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:23: // TODO(tibell): Instead of
rejecting messages check at connection time if we
This comment feels a bit out of place now.

https://codereview.chromium.org/2086223002/diff/580001/chrome/browser/ui/sear...
chrome/browser/ui/search/search_ipc_router.cc:25: // world, the
IsRenderedInInstantProcess check will have to be done, as it's
will have to be done?

[commit-bot: I haz the power, 2017-01-13 01:42:11]

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

[commit-bot: I haz the power, 2017-01-13 01:42:14]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-01-13 17:27:06]

On 2017/01/12 23:54:22, tibell wrote:
> Marc, could you please take another look to make sure I didn't break any of
your
> recent changes when I merged? Otherwise I think this is good to go.

Still lgtm, thanks!

[tibell, 2017-01-15 23:45:37]

The CQ bit was checked by tibell@chromium.org

[tibell, 2017-01-15 23:45:37]

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

[commit-bot: I haz the power, 2017-01-15 23:45:53]

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, 2017-01-16 01:24:25]

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

[commit-bot: I haz the power, 2017-01-16 01:24:28]

Failed to apply patch for chrome/browser/ui/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: chrome/browser/ui/BUILD.gn:474
  error: chrome/browser/ui/BUILD.gn: patch does not apply

Patch:       chrome/browser/ui/BUILD.gn
Index: chrome/browser/ui/BUILD.gn
diff --git a/chrome/browser/ui/BUILD.gn b/chrome/browser/ui/BUILD.gn
index
e09d63307bd8f0a0e95cc029566f8ccf6010de00..78ef366f6a1103661334b5b94f5a90746406bc22
100644
--- a/chrome/browser/ui/BUILD.gn
+++ b/chrome/browser/ui/BUILD.gn
@@ -474,6 +474,7 @@ split_static_library("ui") {
     "//chrome/browser/ui/webui/plugins:mojo_bindings",
     "//chrome/browser/ui/webui/usb_internals:mojo_bindings",
     "//chrome/common",
+    "//chrome/common:instant_mojom",
     "//chrome/common/net",
     "//chrome/installer/util:with_no_strings",
     "//components/app_modal",

[tibell, 2017-01-16 01:48:58]

Merge

[tibell, 2017-01-16 01:49:26]

The CQ bit was checked by tibell@chromium.org

[tibell, 2017-01-16 01:49:26]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
rockot@chromium.org, jam@chromium.org, dcheng@chromium.org, sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2086223002/#ps620001
(title: "Merge")

[commit-bot: I haz the power, 2017-01-16 01:49:39]

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, 2017-01-16 03:21:19]

CQ is committing da patch.

Bot data: {"patchset_id": 620001, "attempt_start_ts": 1484531366068140,
"parent_rev": "dc7d397153bcbccbc21e12f2ea3568f594699220", "commit_rev":
"e3df8cd8ac45539711b1f0b6a00420f87d09d279"}

[commit-bot: I haz the power, 2017-01-16 03:21:48]

Description was changed from

==========
Convert instant search messages to Mojo

Instead of mirroring browser state into a RenderViewObserver we use a
RenderFrameObserver on the main frame. This is because Mojo, by design, doesn't
communicate with RenderViews. Also, the data is eventually used from the main
frame.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=653373
==========

to

==========
Convert instant search messages to Mojo

Instead of mirroring browser state into a RenderViewObserver we use a
RenderFrameObserver on the main frame. This is because Mojo, by design, doesn't
communicate with RenderViews. Also, the data is eventually used from the main
frame.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=653373

Review-Url: https://codereview.chromium.org/2086223002
Cr-Commit-Position: refs/heads/master@{#443838}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3df8cd8ac45539711b1f0b6a004...
==========

[commit-bot: I haz the power, 2017-01-16 03:21:51]

Committed patchset #32 (id:620001) as
https://chromium.googlesource.com/chromium/src/+/e3df8cd8ac45539711b1f0b6a004...

[tibell, 2017-02-07 00:27:15]

Patchset #33 (id:640001) has been deleted