Migrated ChromeViewHostMsg_PageHasOSDD to Mojo.

Migrated ChromeViewHostMsg_PageHasOSDD to Mojo.

Tested by manually loading two search engines (DuckDuckGo and Ask) and
logging the input to AssociateURLFetcherWithWebContents, which was found
to be the same before and after the change.

BUG=577685
Review-Url: https://codereview.chromium.org/2746293005
Cr-Commit-Position: refs/heads/master@{#458647}
Committed: https://chromium.googlesource.com/chromium/src/+/e3cf9093ddcaa466a5cf7153745e37d579679ab7

[martis, 2017-03-14 22:55:59]

Description was changed from

==========
Migrated ChromeViewHostMsg_PageHasOSDD to Mojo.

Tested by manually loading two search engines (DuckDuckGo and Ask) and logging
the input to AssociateURLFetcherWithWebContents, which was found to be the same
before and after the change.

BUG=577685
==========

to

==========
Migrated ChromeViewHostMsg_PageHasOSDD to Mojo.

Tested by manually loading two search engines (DuckDuckGo and Ask) and
logging the input to AssociateURLFetcherWithWebContents, which was found
to be the same before and after the change.

BUG=577685
==========

[martis, 2017-03-14 22:55:59]

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

[Sam McNally, 2017-03-15 00:38:49]

Rietveld doesn't send emails about reviewer changes so you'll need to mail a
comment when adding reviewers.

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
SearchEngineTabHelper();
Is this necessary?

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/search_engine_tab_helper.h:44:
mojo::Binding<chrome::mojom::OSDDHandler> osdd_handler_binding_;
A mojo::Binding only supports a single connection; mojo::BindingSet supports
multiple connections, but in this case you want a
content::WebContentsFrameBindingSet<chrome::mojom::OSDDHandler>. That supports
multiple connections like a BindingSet, but also handles interface registration.
You'll want to add a check similar to the one in
https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind...
to only accept messages from the main frame.

You'll need to use GetRemoteAssociatedInterfaces() on the renderer side since
WebContentsFrameBindingSet uses an associated binding.

https://codereview.chromium.org/2746293005/diff/20001/chrome/common/osdd_hand...
File chrome/common/osdd_handler.mojom (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/common/osdd_hand...
chrome/common/osdd_handler.mojom:12: OnPageHasOSDD(url.mojom.Url page_url,
url.mojom.Url osdd_url);
We usually don't include "On" in mojo method names except for observer
interfaces.

I would also spell out OpenSearchDescriptionDocument instead of OSDD.

https://codereview.chromium.org/2746293005/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_frame_observer.cc (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_frame_observer.cc:253:
mojo::MakeRequest(&osdd_handler));
GetInterface() has an overload that calls MakeRequest internally.

[martis, 2017-03-15 05:00:28]

On 2017/03/15 00:38:49, Sam McNally wrote:
> Rietveld doesn't send emails about reviewer changes so you'll need to mail a
> comment when adding reviewers.
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
> SearchEngineTabHelper();
> Is this necessary?
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> chrome/browser/ui/search_engines/search_engine_tab_helper.h:44:
> mojo::Binding<chrome::mojom::OSDDHandler> osdd_handler_binding_;
> A mojo::Binding only supports a single connection; mojo::BindingSet supports
> multiple connections, but in this case you want a
> content::WebContentsFrameBindingSet<chrome::mojom::OSDDHandler>. That supports
> multiple connections like a BindingSet, but also handles interface
registration.
> You'll want to add a check similar to the one in
>
https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind...
> to only accept messages from the main frame.
> 
> You'll need to use GetRemoteAssociatedInterfaces() on the renderer side since
> WebContentsFrameBindingSet uses an associated binding.
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/common/osdd_hand...
> File chrome/common/osdd_handler.mojom (right):
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/common/osdd_hand...
> chrome/common/osdd_handler.mojom:12: OnPageHasOSDD(url.mojom.Url page_url,
> url.mojom.Url osdd_url);
> We usually don't include "On" in mojo method names except for observer
> interfaces.
> 
> I would also spell out OpenSearchDescriptionDocument instead of OSDD.
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/renderer/chrome_...
> File chrome/renderer/chrome_render_frame_observer.cc (right):
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/renderer/chrome_...
> chrome/renderer/chrome_render_frame_observer.cc:253:
> mojo::MakeRequest(&osdd_handler));
> GetInterface() has an overload that calls MakeRequest internally.

Thanks for the review; I've attempted to address your concerns.

[martis, 2017-03-15 05:00:56]

Thanks for the review; I've attempted to address your concerns.

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
SearchEngineTabHelper();
On 2017/03/15 00:38:48, Sam McNally wrote:
> Is this necessary?

I believe so - I've introduced a non-default constructor.

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/search_engine_tab_helper.h:44:
mojo::Binding<chrome::mojom::OSDDHandler> osdd_handler_binding_;
On 2017/03/15 00:38:48, Sam McNally wrote:
> A mojo::Binding only supports a single connection; mojo::BindingSet supports
> multiple connections, but in this case you want a
> content::WebContentsFrameBindingSet<chrome::mojom::OSDDHandler>. That supports
> multiple connections like a BindingSet, but also handles interface
registration.
> You'll want to add a check similar to the one in
>
https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind...
> to only accept messages from the main frame.
> 
> You'll need to use GetRemoteAssociatedInterfaces() on the renderer side since
> WebContentsFrameBindingSet uses an associated binding.

Done. I removed the WeakPtr (based on other uses of WebContentsFrameBindingSet);
will that work?

https://codereview.chromium.org/2746293005/diff/20001/chrome/common/osdd_hand...
File chrome/common/osdd_handler.mojom (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/common/osdd_hand...
chrome/common/osdd_handler.mojom:12: OnPageHasOSDD(url.mojom.Url page_url,
url.mojom.Url osdd_url);
On 2017/03/15 00:38:48, Sam McNally wrote:
> We usually don't include "On" in mojo method names except for observer
> interfaces.
> 
> I would also spell out OpenSearchDescriptionDocument instead of OSDD.

Done.

https://codereview.chromium.org/2746293005/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_frame_observer.cc (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_frame_observer.cc:253:
mojo::MakeRequest(&osdd_handler));
On 2017/03/15 00:38:48, Sam McNally wrote:
> GetInterface() has an overload that calls MakeRequest internally.

Done.

[Sam McNally, 2017-03-15 05:53:16]

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

[commit-bot: I haz the power, 2017-03-15 05:53:30]

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-03-15 05:55:15]

lgtm

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):

https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
SearchEngineTabHelper();
On 2017/03/15 05:00:56, martis wrote:
> On 2017/03/15 00:38:48, Sam McNally wrote:
> > Is this necessary?
> 
> I believe so - I've introduced a non-default constructor. 

The non-default constructor was there before. I don't think you need this.

[commit-bot: I haz the power, 2017-03-15 05:58:09]

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

[commit-bot: I haz the power, 2017-03-15 05:58:10]

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...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[martis, 2017-03-15 06:21:34]

On 2017/03/15 05:55:15, Sam McNally wrote:
> lgtm
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):
> 
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
> SearchEngineTabHelper();
> On 2017/03/15 05:00:56, martis wrote:
> > On 2017/03/15 00:38:48, Sam McNally wrote:
> > > Is this necessary?
> > 
> > I believe so - I've introduced a non-default constructor. 
> 
> The non-default constructor was there before. I don't think you need this.

I have tried to remove the declaration, but I get
chrome/browser/ui/search_engines/search_engine_tab_helper.cc:82:24: error:
out-of-line definition of 'SearchEngineTabHelper' does not match any declaration
in 'SearchEngineTabHelper'

[Sam McNally, 2017-03-15 06:26:12]

On 2017/03/15 06:21:34, martis wrote:
> On 2017/03/15 05:55:15, Sam McNally wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> > File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):
> > 
> >
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> > chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
> > SearchEngineTabHelper();
> > On 2017/03/15 05:00:56, martis wrote:
> > > On 2017/03/15 00:38:48, Sam McNally wrote:
> > > > Is this necessary?
> > > 
> > > I believe so - I've introduced a non-default constructor. 
> > 
> > The non-default constructor was there before. I don't think you need this.
> 
> I have tried to remove the declaration, but I get
> chrome/browser/ui/search_engines/search_engine_tab_helper.cc:82:24: error:
> out-of-line definition of 'SearchEngineTabHelper' does not match any
declaration
> in 'SearchEngineTabHelper'

Remove the definition there too.

[martis, 2017-03-15 06:47:34]

On 2017/03/15 06:26:12, Sam McNally wrote:
> On 2017/03/15 06:21:34, martis wrote:
> > On 2017/03/15 05:55:15, Sam McNally wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> > > File chrome/browser/ui/search_engines/search_engine_tab_helper.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2746293005/diff/20001/chrome/browser/ui/searc...
> > > chrome/browser/ui/search_engines/search_engine_tab_helper.h:24:
> > > SearchEngineTabHelper();
> > > On 2017/03/15 05:00:56, martis wrote:
> > > > On 2017/03/15 00:38:48, Sam McNally wrote:
> > > > > Is this necessary?
> > > > 
> > > > I believe so - I've introduced a non-default constructor. 
> > > 
> > > The non-default constructor was there before. I don't think you need this.
> > 
> > I have tried to remove the declaration, but I get
> > chrome/browser/ui/search_engines/search_engine_tab_helper.cc:82:24: error:
> > out-of-line definition of 'SearchEngineTabHelper' does not match any
> declaration
> > in 'SearchEngineTabHelper'
> 
> Remove the definition there too.

Chatted offline: I shouldn't have introduced a public constructor. Removed.

[Sam McNally, 2017-03-15 07:07:12]

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

[commit-bot: I haz the power, 2017-03-15 07:07:35]

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

[martis, 2017-03-15 07:11:43]

martis@chromium.org changed reviewers:
+ dcheng@chromium.org, jochen@chromium.org

[martis, 2017-03-15 07:11:43]

For OWNERS
+jochen
+dcheng

Thanks very much :)

[commit-bot: I haz the power, 2017-03-15 08:01:06]

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

[commit-bot: I haz the power, 2017-03-15 08:01:07]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-03-15 15:50:15]

lgtm

[dcheng, 2017-03-16 05:57:44]

mojo lgtm

https://codereview.chromium.org/2746293005/diff/80001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/search_engine_tab_helper.cc (right):

https://codereview.chromium.org/2746293005/diff/80001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/search_engine_tab_helper.cc:105:
web_contents()->GetMainFrame())
Huh. Did we not check this in the past? I guess this is a nice improvement then
=)

[martis, 2017-03-17 01:25:18]

martis@chromium.org changed reviewers:
+ mbarbella@chromium.org

[martis, 2017-03-17 01:26:03]

+mbarbella for IPC security OWNERS
Thank you

[dcheng, 2017-03-17 01:39:12]

On 2017/03/17 01:26:03, martis wrote:
> +mbarbella for IPC security OWNERS
> Thank you

FWIW, I'm pretty sure I own all the IPC related files in this patch.

[martis, 2017-03-17 01:56:56]

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

[commit-bot: I haz the power, 2017-03-17 01:57:19]

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

[martis, 2017-03-17 02:00:41]

martis@chromium.org changed reviewers:
- mbarbella@chromium.org

[martis, 2017-03-17 02:01:08]

On 2017/03/17 01:39:12, dcheng wrote:
> On 2017/03/17 01:26:03, martis wrote:
> > +mbarbella for IPC security OWNERS
> > Thank you
> 
> FWIW, I'm pretty sure I own all the IPC related files in this patch.

Oh great. If you're happy, I'll commit.

[dcheng, 2017-03-17 02:02:53]

On 2017/03/17 02:01:08, martis wrote:
> On 2017/03/17 01:39:12, dcheng wrote:
> > On 2017/03/17 01:26:03, martis wrote:
> > > +mbarbella for IPC security OWNERS
> > > Thank you
> > 
> > FWIW, I'm pretty sure I own all the IPC related files in this patch.
> 
> Oh great. If you're happy, I'll commit.

Yes, I already lgtmed above

[commit-bot: I haz the power, 2017-03-17 03:26:43]

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

[commit-bot: I haz the power, 2017-03-17 03:26:44]

Dry run: This issue passed the CQ dry run.

[martis, 2017-03-20 22:00:10]

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

[commit-bot: I haz the power, 2017-03-20 22:00: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, 2017-03-21 02:20:35]

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

[commit-bot: I haz the power, 2017-03-21 02:20:35]

Dry run: This issue passed the CQ dry run.

[martis, 2017-03-21 22:39:54]

The CQ bit was checked by martis@chromium.org

[martis, 2017-03-21 22:39:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
jochen@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2746293005/#ps100001
(title: "Merged with another Mojo CL.")

[commit-bot: I haz the power, 2017-03-21 22:40:17]

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

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

[commit-bot: I haz the power, 2017-03-21 23:06:43]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[martis, 2017-03-22 00:28:07]

The CQ bit was checked by martis@chromium.org

[commit-bot: I haz the power, 2017-03-22 00:28:50]

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-03-22 02:30:03]

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

[commit-bot: I haz the power, 2017-03-22 02:30:04]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[martis, 2017-03-22 06:08:30]

The CQ bit was checked by martis@chromium.org

[commit-bot: I haz the power, 2017-03-22 06:09:01]

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

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490162910541420,
"parent_rev": "d48b3d14741301605c1715aef0c323c710b16a79", "commit_rev":
"e3cf9093ddcaa466a5cf7153745e37d579679ab7"}

[commit-bot: I haz the power, 2017-03-22 06:48:37]

Description was changed from

==========
Migrated ChromeViewHostMsg_PageHasOSDD to Mojo.

Tested by manually loading two search engines (DuckDuckGo and Ask) and
logging the input to AssociateURLFetcherWithWebContents, which was found
to be the same before and after the change.

BUG=577685
==========

to

==========
Migrated ChromeViewHostMsg_PageHasOSDD to Mojo.

Tested by manually loading two search engines (DuckDuckGo and Ask) and
logging the input to AssociateURLFetcherWithWebContents, which was found
to be the same before and after the change.

BUG=577685

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

[commit-bot: I haz the power, 2017-03-22 06:48:38]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e3cf9093ddcaa466a5cf7153745e...