Convert network hints to Mojo

Convert network hints to Mojo

Previously landed as: refs/heads/master@{#406780}

Committed: https://crrev.com/1efd0ad3153587609220eeec1be6c1b2bbf9964b
Cr-Commit-Position: refs/heads/master@{#407030}

[tibell, 2016-07-12 04:50:13]

tibell@chromium.org changed reviewers:
+ dcheng@chromium.org, pmeenan@chromium.org, sammc@chromium.org

[tibell, 2016-07-12 04:50:13]

It would also be possible to split the two messages into two interfaces,
depending on how closely related (or not) we feel they are.

[tibell, 2016-07-12 04:52:12]

tibell@chromium.org changed reviewers:
+ juliatuttle@chromium.org
- pmeenan@chromium.org

[tibell, 2016-07-12 04:56:38]

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

[tibell, 2016-07-12 04:56:38]

halliwell@chromium.org: Please review changes in

[tibell, 2016-07-12 04:56:46]

halliwell@chromium.org: Please review changes in chromecast/

[Sam McNally, 2016-07-12 05:25:30]

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:971:
host->GetInterfaceRegistry()->AddInterface(
I think you want this interface to live on the IO thread.

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.h:50:
mojo::InterfaceRequest<network_hints::mojom::NetworkHints> request);
network_hints::mojom::NetworkHintsRequest

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
File components/network_hints/browser/network_hints_message_filter.cc (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
components/network_hints/browser/network_hints_message_filter.cc:10: #include
"components/network_hints/common/network_hints_param_traits.h"
Remove.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
components/network_hints/browser/network_hints_message_filter.cc:67:
mojo::InterfaceRequest<mojom::NetworkHints> request) {
network_hints::mojom::NetworkHintsRequest

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
File components/network_hints/browser/network_hints_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
components/network_hints/browser/network_hints_message_filter.h:22: class
NetworkHintsMessageFilter : public network_hints::mojom::NetworkHints {
Rename this now that it isn't a MessageFilter.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/co...
File components/network_hints/common/network_hints_param_traits.h (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/co...
components/network_hints/common/network_hints_param_traits.h:35: #define
IPC_MESSAGE_START NetworkHintsMsgStart
Remove.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/prescient_networking_dispatcher.cc
(right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/prescient_networking_dispatcher.cc:11:
PrescientNetworkingDispatcher::PrescientNetworkingDispatcher() {}
Did you mean to include this change?

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/renderer_dns_prefetch.cc (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/renderer_dns_prefetch.cc:17: #include
"components/network_hints/common/network_hints_param_traits.h"
Remove.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/renderer_dns_prefetch.h (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/renderer_dns_prefetch.h:84: void
EnsureConnected();
I would prefer a getter that returns a mojom::NetworkHints& that connects if
necessary.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/renderer_preconnect.cc:28:
network_hints_->Preconnect(url, allow_credentials, 1);
Is |network_hints_| connected here?

[tibell, 2016-07-12 07:14:24]

Thanks for reviewing! PTAL

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:971:
host->GetInterfaceRegistry()->AddInterface(
On 2016/07/12 05:25:29, Sam McNally wrote:
> I think you want this interface to live on the IO thread.

Done.

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.h:50:
mojo::InterfaceRequest<network_hints::mojom::NetworkHints> request);
On 2016/07/12 05:25:29, Sam McNally wrote:
> network_hints::mojom::NetworkHintsRequest

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
File components/network_hints/browser/network_hints_message_filter.cc (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
components/network_hints/browser/network_hints_message_filter.cc:10: #include
"components/network_hints/common/network_hints_param_traits.h"
On 2016/07/12 05:25:29, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
components/network_hints/browser/network_hints_message_filter.cc:67:
mojo::InterfaceRequest<mojom::NetworkHints> request) {
On 2016/07/12 05:25:29, Sam McNally wrote:
> network_hints::mojom::NetworkHintsRequest

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
File components/network_hints/browser/network_hints_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/br...
components/network_hints/browser/network_hints_message_filter.h:22: class
NetworkHintsMessageFilter : public network_hints::mojom::NetworkHints {
On 2016/07/12 05:25:29, Sam McNally wrote:
> Rename this now that it isn't a MessageFilter.

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/co...
File components/network_hints/common/network_hints_param_traits.h (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/co...
components/network_hints/common/network_hints_param_traits.h:35: #define
IPC_MESSAGE_START NetworkHintsMsgStart
On 2016/07/12 05:25:29, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/prescient_networking_dispatcher.cc
(right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/prescient_networking_dispatcher.cc:11:
PrescientNetworkingDispatcher::PrescientNetworkingDispatcher() {}
On 2016/07/12 05:25:29, Sam McNally wrote:
> Did you mean to include this change?

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/renderer_dns_prefetch.cc (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/renderer_dns_prefetch.cc:17: #include
"components/network_hints/common/network_hints_param_traits.h"
On 2016/07/12 05:25:30, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/renderer_dns_prefetch.h (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/renderer_dns_prefetch.h:84: void
EnsureConnected();
On 2016/07/12 05:25:30, Sam McNally wrote:
> I would prefer a getter that returns a mojom::NetworkHints& that connects if
> necessary.

Done.

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/1/components/network_hints/re...
components/network_hints/renderer/renderer_preconnect.cc:28:
network_hints_->Preconnect(url, allow_credentials, 1);
On 2016/07/12 05:25:30, Sam McNally wrote:
> Is |network_hints_| connected here?

Done.

[tibell, 2016-07-13 05:12:10]

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

[tibell, 2016-07-13 05:12:11]

+sky for

chrome/browser/BUILD.gn
chrome/browser/chrome_content_browser_client.cc
chrome/browser/renderer_host/chrome_render_message_filter.h
chrome/browser/renderer_host/chrome_render_message_filter.c
chrome/chrome_browser.gypi

[tibell, 2016-07-18 00:16:34]

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

[commit-bot: I haz the power, 2016-07-18 00:16: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, 2016-07-18 01:42:25]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Julia Tuttle, 2016-07-18 14:39:29]

components/network_hints lgtm. I left a few nits/questions that are probably
just due to my unfamiliarity with the new mojo world.

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/browser/network_hints_message_filter.cc (right):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/browser/network_hints_message_filter.cc:5: #include
"components/network_hints/browser/network_hints_message_filter.h"
Why is this not getting renamed to network_hints_impl.{cc,h}?

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/browser/network_hints_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/browser/network_hints_message_filter.h:32: void
Preconnect(const GURL& url,
Why is this empty?

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/common/network_hints_message_generator.h (left):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/common/network_hints_message_generator.h:4: 
(Why does this file exist?)

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/common/network_hints_param_traits.h (right):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/common/network_hints_param_traits.h:5: //
Multiply-included file, no traditional include guard.
Why? Unless I'm misreading, there are no declarations outside the
singly-included section below.

[sky, 2016-07-18 16:35:02]

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

https://codereview.chromium.org/2144533002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
ChromeRenderMessageFilter(id, profile);
Who owns ChromeRenderMessageFilter, and how do you the Unretained below is safe?

[tibell, 2016-07-19 01:54:13]

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

[commit-bot: I haz the power, 2016-07-19 01:55:05]

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

[tibell, 2016-07-19 01:55:37]

Thanks for reviewing. PTAL

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

https://codereview.chromium.org/2144533002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
ChromeRenderMessageFilter(id, profile);
On 2016/07/18 16:35:02, sky wrote:
> Who owns ChromeRenderMessageFilter, and how do you the Unretained below is
safe?

The RPH continues to own the message filter through the call to AddFilter. It
also (indirectly) owns the interface registry, which will be destroyed before
the message filter is. Want me to add a comment?

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/browser/network_hints_message_filter.cc (right):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/browser/network_hints_message_filter.cc:5: #include
"components/network_hints/browser/network_hints_message_filter.h"
On 2016/07/18 14:39:29, Julia Tuttle wrote:
> Why is this not getting renamed to network_hints_impl.{cc,h}?

Done.

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/browser/network_hints_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/browser/network_hints_message_filter.h:32: void
Preconnect(const GURL& url,
On 2016/07/18 14:39:29, Julia Tuttle wrote:
> Why is this empty?

In the Chrome IPC implementation this message was simply ignored (see the .cc
file). This is the Mojo version of ignoring a message.

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/common/network_hints_message_generator.h (left):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/common/network_hints_message_generator.h:4: 
On 2016/07/18 14:39:29, Julia Tuttle wrote:
> (Why does this file exist?)

Done.

No longer needed.

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
File components/network_hints/common/network_hints_param_traits.h (right):

https://codereview.chromium.org/2144533002/diff/120001/components/network_hin...
components/network_hints/common/network_hints_param_traits.h:5: //
Multiply-included file, no traditional include guard.
On 2016/07/18 14:39:29, Julia Tuttle wrote:
> Why? Unless I'm misreading, there are no declarations outside the
> singly-included section below.

Done.

[commit-bot: I haz the power, 2016-07-19 02:33:57]

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

[commit-bot: I haz the power, 2016-07-19 02:33:58]

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, 2016-07-19 03:05:12]

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

[commit-bot: I haz the power, 2016-07-19 03:05:43]

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

[tibell, 2016-07-19 03:11:49]

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

[tibell, 2016-07-19 03:11:50]

+mbarbella for ipc_fuzzer

[tibell, 2016-07-19 03:15:19]

tibell@chromium.org changed reviewers:
+ amistry@chromium.org, blundell@chromium.org

[tibell, 2016-07-19 03:15:20]

+blundell for components/typemaps.gni
+amistry for ipc/ipc_message_start.h

[tibell, 2016-07-19 03:16:42]

On 2016/07/19 03:15:20, tibell wrote:
> +blundell for components/typemaps.gni
> +amistry for ipc/ipc_message_start.h

-amistry for ipc/ipc_message_start.h
+dcheng for ipc/ipc_message_start.h (need security owner)

[tibell, 2016-07-19 04:09:58]

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

[commit-bot: I haz the power, 2016-07-19 04:10:27]

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

[mbarbella (wrong one), 2016-07-19 04:32:52]

mbarbella@google.com changed reviewers:
+ mbarbella@google.com

[mbarbella (wrong one), 2016-07-19 04:32:54]

ipc_fuzzer lgtm with a nit

https://codereview.chromium.org/2144533002/diff/180001/tools/ipc_fuzzer/messa...
File tools/ipc_fuzzer/message_lib/OWNERS (right):

https://codereview.chromium.org/2144533002/diff/180001/tools/ipc_fuzzer/messa...
tools/ipc_fuzzer/message_lib/OWNERS:1: per-file *_messages*.h=set noparent
We previously didn't include this since it's only used by the IPC fuzzer, and
shouldn't affect anything that ships. I'd rather not include this unless there's
a specific reason to do so.

[tibell, 2016-07-19 04:38:40]

https://codereview.chromium.org/2144533002/diff/180001/tools/ipc_fuzzer/messa...
File tools/ipc_fuzzer/message_lib/OWNERS (right):

https://codereview.chromium.org/2144533002/diff/180001/tools/ipc_fuzzer/messa...
tools/ipc_fuzzer/message_lib/OWNERS:1: per-file *_messages*.h=set noparent
On 2016/07/19 04:32:53, mbarbella (wrong one) wrote:
> We previously didn't include this since it's only used by the IPC fuzzer, and
> shouldn't affect anything that ships. I'd rather not include this unless
there's
> a specific reason to do so.

Done.

Presubmit complains if it's not here. But I can remove the file again.

[tibell, 2016-07-19 04:58:04]

I believe this is ready for "final" review. Reviewers please review the
following:

sky:
chrome/*

halliwell:
chromecast/*

blundell:
components/typemaps.gni

dcheng:
ipc/ipc_message_start.h

[commit-bot: I haz the power, 2016-07-19 05:13:55]

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

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

Dry run: This issue passed the CQ dry run.

[Martin Barbella, 2016-07-19 05:58:06]

lgtm from the right account

[blundell, 2016-07-19 06:28:35]

I'm not that familiar with Mojo in Chromium so maybe my question has a good
answer.

//components/typemap.gni LGTM. Even if my question is valid feel free to land
this (from my POV) and make changes in a followup.

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gni
File components/typemaps.gni (right):

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gn...
components/typemaps.gni:7:
"//components/network_hints/common/network_hints.typemap",
Why the divergence in directory structure for where the typemaps live?

i.e., ".../public/cpp" vs. ".../<nothing>" vs. ".../public/interfaces"

[tibell, 2016-07-19 07:06:10]

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gni
File components/typemaps.gni (right):

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gn...
components/typemaps.gni:7:
"//components/network_hints/common/network_hints.typemap",
On 2016/07/19 06:28:35, blundell wrote:
> Why the divergence in directory structure for where the typemaps live?
> 
> i.e., ".../public/cpp" vs. ".../<nothing>" vs. ".../public/interfaces"

We usually put them next to the .mojom, which begs the question "where do we put
the .mojom?". I chatted with some other mojo people and the answer seems to be
in public/interfaces, so in this case it should really be in
components/network_hints/public/interfaces/. I'll move it there.

[sky, 2016-07-19 17:38:44]

LGTM with a comment.

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

https://codereview.chromium.org/2144533002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
ChromeRenderMessageFilter(id, profile);
On 2016/07/19 01:55:36, tibell wrote:
> On 2016/07/18 16:35:02, sky wrote:
> > Who owns ChromeRenderMessageFilter, and how do you the Unretained below is
> safe?
> 
> The RPH continues to own the message filter through the call to AddFilter. It
> also (indirectly) owns the interface registry, which will be destroyed before
> the message filter is. Want me to add a comment?

Yes please.

[halliwell, 2016-07-19 19:08:37]

On 2016/07/19 17:38:44, sky wrote:
> LGTM with a comment.
> 
>
https://codereview.chromium.org/2144533002/diff/120001/chrome/browser/chrome_...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2144533002/diff/120001/chrome/browser/chrome_...
> chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
> ChromeRenderMessageFilter(id, profile);
> On 2016/07/19 01:55:36, tibell wrote:
> > On 2016/07/18 16:35:02, sky wrote:
> > > Who owns ChromeRenderMessageFilter, and how do you the Unretained below is
> > safe?
> > 
> > The RPH continues to own the message filter through the call to AddFilter.
It
> > also (indirectly) owns the interface registry, which will be destroyed
before
> > the message filter is. Want me to add a comment?
> 
> Yes please.

Apologies for delay, chromecast/ lgtm

[Sam McNally, 2016-07-20 00:03:02]

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

https://codereview.chromium.org/2144533002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
ChromeRenderMessageFilter(id, profile);
This could use a better name.

https://codereview.chromium.org/2144533002/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/2144533002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_message_filter.cc:28: #include
"components/network_hints/common/network_hints_param_traits.h"
Remove.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/common/OWNERS (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/common/OWNERS:3: per-file *_messages.cc=set noparent
Are these *_messages entries still needed?

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_dns_prefetch.cc (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_dns_prefetch.cc:44: if
(!network_hints_.is_bound()) {
if (!network_hints_) {

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_dns_prefetch.h (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_dns_prefetch.h:84:
mojom::NetworkHintsPtr& GetNetworkHints();
Why not return a mojom::NetworkHints&?

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:33: if
(!network_hints_.is_bound()) {
Here too.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.h (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.h:38:
mojom::NetworkHintsPtr& GetNetworkHints();
Here too.

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gni
File components/typemaps.gni (right):

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gn...
components/typemaps.gni:7:
"//components/network_hints/common/network_hints.typemap",
On 2016/07/19 07:06:10, tibell wrote:
> On 2016/07/19 06:28:35, blundell wrote:
> > Why the divergence in directory structure for where the typemaps live?
> > 
> > i.e., ".../public/cpp" vs. ".../<nothing>" vs. ".../public/interfaces"
> 
> We usually put them next to the .mojom, which begs the question "where do we
put
> the .mojom?". I chatted with some other mojo people and the answer seems to be
> in public/interfaces, so in this case it should really be in
> components/network_hints/public/interfaces/. I'll move it there.

I'd prefer typemaps to live with the traits and/or the C++ types. There can be
multiple typemaps for a given mojom, and it doesn't make much sense for blink
typemaps to live outside blink or chromium typemaps for blink mojoms to live in
blink.

[tibell, 2016-07-20 00:29:32]

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

[tibell, 2016-07-20 00:29:35]

PTAL

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

https://codereview.chromium.org/2144533002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
ChromeRenderMessageFilter(id, profile);
On 2016/07/19 17:38:44, sky wrote:
> On 2016/07/19 01:55:36, tibell wrote:
> > On 2016/07/18 16:35:02, sky wrote:
> > > Who owns ChromeRenderMessageFilter, and how do you the Unretained below is
> > safe?
> > 
> > The RPH continues to own the message filter through the call to AddFilter.
It
> > also (indirectly) owns the interface registry, which will be destroyed
before
> > the message filter is. Want me to add a comment?
> 
> Yes please.

Done.

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

https://codereview.chromium.org/2144533002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:970: auto chrome_render = new
ChromeRenderMessageFilter(id, profile);
On 2016/07/20 00:03:01, Sam McNally wrote:
> This could use a better name.

Done.

https://codereview.chromium.org/2144533002/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/2144533002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_message_filter.cc:28: #include
"components/network_hints/common/network_hints_param_traits.h"
On 2016/07/20 00:03:01, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/common/OWNERS (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/common/OWNERS:3: per-file *_messages.cc=set noparent
On 2016/07/20 00:03:01, Sam McNally wrote:
> Are these *_messages entries still needed?

Done.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_dns_prefetch.cc (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_dns_prefetch.cc:44: if
(!network_hints_.is_bound()) {
On 2016/07/20 00:03:01, Sam McNally wrote:
> if (!network_hints_) {

Done.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_dns_prefetch.h (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_dns_prefetch.h:84:
mojom::NetworkHintsPtr& GetNetworkHints();
On 2016/07/20 00:03:01, Sam McNally wrote:
> Why not return a mojom::NetworkHints&?

Done.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:33: if
(!network_hints_.is_bound()) {
On 2016/07/20 00:03:01, Sam McNally wrote:
> Here too.

Done.

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.h (right):

https://codereview.chromium.org/2144533002/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.h:38:
mojom::NetworkHintsPtr& GetNetworkHints();
On 2016/07/20 00:03:02, Sam McNally wrote:
> Here too.

Done.

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gni
File components/typemaps.gni (right):

https://codereview.chromium.org/2144533002/diff/180001/components/typemaps.gn...
components/typemaps.gni:7:
"//components/network_hints/common/network_hints.typemap",
On 2016/07/20 00:03:02, Sam McNally wrote:
> On 2016/07/19 07:06:10, tibell wrote:
> > On 2016/07/19 06:28:35, blundell wrote:
> > > Why the divergence in directory structure for where the typemaps live?
> > > 
> > > i.e., ".../public/cpp" vs. ".../<nothing>" vs. ".../public/interfaces"
> > 
> > We usually put them next to the .mojom, which begs the question "where do we
> put
> > the .mojom?". I chatted with some other mojo people and the answer seems to
be
> > in public/interfaces, so in this case it should really be in
> > components/network_hints/public/interfaces/. I'll move it there.
> 
> I'd prefer typemaps to live with the traits and/or the C++ types. There can be
> multiple typemaps for a given mojom, and it doesn't make much sense for blink
> typemaps to live outside blink or chromium typemaps for blink mojoms to live
in
> blink.

Done.

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

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

[commit-bot: I haz the power, 2016-07-20 00:35:10]

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

[Sam McNally, 2016-07-20 00:44:49]

LGTM

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
File components/network_hints/renderer/renderer_dns_prefetch.cc (right):

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
components/network_hints/renderer/renderer_dns_prefetch.cc:48: return
*network_hints_.get();
*network_hints_ should work.

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:37: return
*network_hints_.get();
Ditto.

[tibell, 2016-07-20 01:54:50]

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

[commit-bot: I haz the power, 2016-07-20 01:55:47]

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

[tibell, 2016-07-20 01:56:00]

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
File components/network_hints/renderer/renderer_dns_prefetch.cc (right):

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
components/network_hints/renderer/renderer_dns_prefetch.cc:48: return
*network_hints_.get();
On 2016/07/20 00:44:49, Sam McNally wrote:
> *network_hints_ should work.

Done.

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/200001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:37: return
*network_hints_.get();
On 2016/07/20 00:44:49, Sam McNally wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2016-07-20 03:14:05]

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

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

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-20 13:59:50]

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

https://codereview.chromium.org/2144533002/diff/240001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:972: auto chrome_render_filter =
new ChromeRenderMessageFilter(id, profile);
Nit: auto*

https://codereview.chromium.org/2144533002/diff/240001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_message_filter.h:40: public
network_hints::mojom::NetworkHints {
Does that mean this is going to end up implementing a lot of other mojo
interfaces?

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:234:
base::Unretained(network_hints_.get())),
This needs to describe why unretained is safe; better yet, we should figure out
a way not to need Unretained.

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:33: if
(!network_hints_) {
Some places like to cache this interface ptr while others just set up the
message pipe each time from scratch. What's the preferred practice?

Also, how much overhead is there from an interface ptr? Is it expected that each
class that wants to talk to this thing grabs its own interface ptr? When does it
make sense to coordinate through some sort of central location instead of having
this code duplicated?

[tibell, 2016-07-21 00:12:07]

PTAL

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

https://codereview.chromium.org/2144533002/diff/240001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:972: auto chrome_render_filter =
new ChromeRenderMessageFilter(id, profile);
On 2016/07/20 13:59:50, dcheng wrote:
> Nit: auto*

Done.

https://codereview.chromium.org/2144533002/diff/240001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

https://codereview.chromium.org/2144533002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_message_filter.h:40: public
network_hints::mojom::NetworkHints {
On 2016/07/20 13:59:50, dcheng wrote:
> Does that mean this is going to end up implementing a lot of other mojo
> interfaces?

Yes potentially. At some point it probably makes sense to see if parts of bigger
interface implementations, like this class, could be pulled apart into e.g.
services.

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:234:
base::Unretained(network_hints_.get())),
On 2016/07/20 13:59:50, dcheng wrote:
> This needs to describe why unretained is safe; better yet, we should figure
out
> a way not to need Unretained.

Added a comment.

I don't think we're going to get away from the Unretained unless the registry
owns the impl. From the discussion on chromium-mojo@ that seems to not be
preferred at the moment.

I do have a CL that has the host able to own impls (similar to how they own
message filters today): https://codereview.chromium.org/2072613003/ This doesn't
get rid of the Unretained, but puts it in one place. If that CL is accepted I
will convert this code to use it.

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:33: if
(!network_hints_) {
On 2016/07/20 13:59:50, dcheng wrote:
> Some places like to cache this interface ptr while others just set up the
> message pipe each time from scratch. What's the preferred practice?
> 
> Also, how much overhead is there from an interface ptr? Is it expected that
each
> class that wants to talk to this thing grabs its own interface ptr? When does
it
> make sense to coordinate through some sort of central location instead of
having
> this code duplicated?

Caching is preferred if you don't know for sure the connection will be used, as
the handshake messages sent back and forth to establish the connection aren't
free. Having one or a few connections per e.g. render thread to the same service
is OK. One per message would be a bit too inefficient.

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

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

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

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-21 01:54:55]

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

[commit-bot: I haz the power, 2016-07-21 01:54:57]

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...)

[dcheng, 2016-07-21 02:43:14]

The mojom lgtm but the duplicated code makes me quite sad. It feels like we're
missing a piece somewhere to connect all the pieces together: DnsPrefetcher and
RendererPreconnect are both owned by PrescientNetworkDispatcher: it feels quite
odd to me that two objects that are basically neighbors need to duplicate these
bits.

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:234:
base::Unretained(network_hints_.get())),
On 2016/07/21 00:12:07, tibell wrote:
> On 2016/07/20 13:59:50, dcheng wrote:
> > This needs to describe why unretained is safe; better yet, we should figure
> out
> > a way not to need Unretained.
> 
> Added a comment.
> 
> I don't think we're going to get away from the Unretained unless the registry
> owns the impl. From the discussion on chromium-mojo@ that seems to not be
> preferred at the moment.
> 
> I do have a CL that has the host able to own impls (similar to how they own
> message filters today): https://codereview.chromium.org/2072613003/ This
doesn't
> get rid of the Unretained, but puts it in one place. If that CL is accepted I
> will convert this code to use it.

Long-term, I think it's still a direction we should consider exploring.
Move-only callbacks are going to be coming to Chromium; it's just a matter of
when. At that point, we can guarantee that the registry has ownership of the
callbacks and we can control the deletion semantics fully.

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:33: if
(!network_hints_) {
On 2016/07/21 00:12:07, tibell wrote:
> On 2016/07/20 13:59:50, dcheng wrote:
> > Some places like to cache this interface ptr while others just set up the
> > message pipe each time from scratch. What's the preferred practice?
> > 
> > Also, how much overhead is there from an interface ptr? Is it expected that
> each
> > class that wants to talk to this thing grabs its own interface ptr? When
does
> it
> > make sense to coordinate through some sort of central location instead of
> having
> > this code duplicated?
> 
> Caching is preferred if you don't know for sure the connection will be used,
as
> the handshake messages sent back and forth to establish the connection aren't
> free. Having one or a few connections per e.g. render thread to the same
service
> is OK. One per message would be a bit too inefficient.

I guess it's inevitable, but I think it's a bit to figure out what the right
behavior is everywhere: for example, I had a similar question in
https://codereview.chromium.org/2063683002/diff/320001/chrome/renderer/page_l...
(since it sets up / tears down the connection every time it makes the call, but
apparently it's only called once per main frame load)

Just something to keep in mind... the code duplication is also pretty sad.

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

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:234:
base::Unretained(network_hints_.get())),
On 2016/07/21 02:43:13, dcheng wrote:
> On 2016/07/21 00:12:07, tibell wrote:
> > On 2016/07/20 13:59:50, dcheng wrote:
> > > This needs to describe why unretained is safe; better yet, we should
figure
> > out
> > > a way not to need Unretained.
> > 
> > Added a comment.
> > 
> > I don't think we're going to get away from the Unretained unless the
registry
> > owns the impl. From the discussion on chromium-mojo@ that seems to not be
> > preferred at the moment.
> > 
> > I do have a CL that has the host able to own impls (similar to how they own
> > message filters today): https://codereview.chromium.org/2072613003/ This
> doesn't
> > get rid of the Unretained, but puts it in one place. If that CL is accepted
I
> > will convert this code to use it.
> 
> Long-term, I think it's still a direction we should consider exploring.
> Move-only callbacks are going to be coming to Chromium; it's just a matter of
> when. At that point, we can guarantee that the registry has ownership of the
> callbacks and we can control the deletion semantics fully.

I think it might be the right direction as well. See if you can convince
rockot@. :)

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/2144533002/diff/240001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:33: if
(!network_hints_) {
On 2016/07/21 02:43:13, dcheng wrote:
> On 2016/07/21 00:12:07, tibell wrote:
> > On 2016/07/20 13:59:50, dcheng wrote:
> > > Some places like to cache this interface ptr while others just set up the
> > > message pipe each time from scratch. What's the preferred practice?
> > > 
> > > Also, how much overhead is there from an interface ptr? Is it expected
that
> > each
> > > class that wants to talk to this thing grabs its own interface ptr? When
> does
> > it
> > > make sense to coordinate through some sort of central location instead of
> > having
> > > this code duplicated?
> > 
> > Caching is preferred if you don't know for sure the connection will be used,
> as
> > the handshake messages sent back and forth to establish the connection
aren't
> > free. Having one or a few connections per e.g. render thread to the same
> service
> > is OK. One per message would be a bit too inefficient.
> 
> I guess it's inevitable, but I think it's a bit to figure out what the right
> behavior is everywhere: for example, I had a similar question in
>
https://codereview.chromium.org/2063683002/diff/320001/chrome/renderer/page_l...
> (since it sets up / tears down the connection every time it makes the call,
but
> apparently it's only called once per main frame load)
> 
> Just something to keep in mind... the code duplication is also pretty sad.

Agreed. sammc@ has a CL that makes InterfacePtr do this lazy init by itself but
there wasn't wide agreement on it being the right thing to do.

[tibell, 2016-07-21 05:26:17]

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

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

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

[tibell, 2016-07-21 05:42:53]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-07-21 05:42:55]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-07-21 05:42:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
juliatuttle@chromium.org, mbarbella@google.com, sky@chromium.org,
mbarbella@chromium.org, halliwell@chromium.org, blundell@chromium.org,
sammc@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2144533002/#ps280001
(title: "Merge")

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

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

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

Committed patchset #15 (id:280001)

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

Description was changed from

==========
Convert network hints to Mojo
==========

to

==========
Convert network hints to Mojo

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

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

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

[tibell, 2016-07-21 06:41:39]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2165023005/ by tibell@chromium.org.

The reason for reverting is: buildbot failure in Chromium GPU on GPU Win
Builder, due to untracked gyp dependency.

[tibell, 2016-07-21 06:46:46]

Description was changed from

==========
Convert network hints to Mojo

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

to

==========
Convert network hints to Mojo

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

[tibell, 2016-07-21 06:47:18]

Description was changed from

==========
Convert network hints to Mojo

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

to

==========
Convert network hints to Mojo

Previously landed as: refs/heads/master@{#406780}
==========

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

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

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

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

[tibell, 2016-07-21 07:21:49]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-07-21 08:13:10]

On 2016/07/21 06:41:39, tibell wrote:
> A revert of this CL (patchset #15 id:280001) has been created in
> https://codereview.chromium.org/2165023005/ by mailto:tibell@chromium.org.
> 
> The reason for reverting is: buildbot failure in Chromium GPU on GPU Win
> Builder, due to untracked gyp dependency.

The cause seems to be the following include chain:

downloads_api.cc -->
chrome_render_message_filter.h -->
network_hints.mojom.h

The "extensions" target, which downloads_api.cc is part of, doesn't depend on
the "browser" target, which includes chrome_render_message_filter.h, because it
can't without creating a cycle. The fix is to add the mojom target directly to
the "extensions" target. gn path shows that this seems to solve the issue.

Before fix:

$ gn path out/Default chrome/browser/extensions
components/network_hints/public/interfaces:network_hints_mojom
No non-data paths found between these two targets.

After fix:

$ gn path out/Default chrome/browser/extensions
components/network_hints/public/interfaces:network_hints_mojom
//chrome/browser/extensions:extensions --[private]-->
//components/network_hints/public/interfaces:network_hints_mojom

However, I cannot repro the compiler failure on my machine or a try bot. I will
try to commit this tomorrow when I'm around to do a rollback if needed.

[tibell, 2016-07-22 01:41:17]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-07-22 01:41:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@google.com, sky@chromium.org, sammc@chromium.org,
mbarbella@chromium.org, dcheng@chromium.org, juliatuttle@chromium.org,
halliwell@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2144533002/#ps300001
(title: "Add untracked dependency on mojom")

[commit-bot: I haz the power, 2016-07-22 01:42:30]

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

[commit-bot: I haz the power, 2016-07-22 03:36:06]

Description was changed from

==========
Convert network hints to Mojo

Previously landed as: refs/heads/master@{#406780}
==========

to

==========
Convert network hints to Mojo

Previously landed as: refs/heads/master@{#406780}
==========

[commit-bot: I haz the power, 2016-07-22 03:36:09]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-07-22 03:37:58]

Description was changed from

==========
Convert network hints to Mojo

Previously landed as: refs/heads/master@{#406780}
==========

to

==========
Convert network hints to Mojo

Previously landed as: refs/heads/master@{#406780}

Committed: https://crrev.com/1efd0ad3153587609220eeec1be6c1b2bbf9964b
Cr-Commit-Position: refs/heads/master@{#407030}
==========

[commit-bot: I haz the power, 2016-07-22 03:38:01]

Patchset 16 (id:??) landed as
https://crrev.com/1efd0ad3153587609220eeec1be6c1b2bbf9964b
Cr-Commit-Position: refs/heads/master@{#407030}

[tibell, 2016-07-24 23:51:59]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2179693002/ by tibell@chromium.org.

The reason for reverting is: Use-after-free issue:

https://bugs.chromium.org/p/chromium/issues/detail?id=630749.

[dcheng, 2016-07-25 02:50:47]

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

https://codereview.chromium.org/2144533002/diff/300001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:975:
base::Unretained(chrome_render_filter)),
Btw, I think we should just remove the base::Unretained() here:
ChromeRenderMessageFilter is refcounted.

[Ben Goodger (Google), 2016-07-25 18:25:14]

On 2016/07/25 02:50:47, dcheng wrote:
>
https://codereview.chromium.org/2144533002/diff/300001/chrome/browser/chrome_...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2144533002/diff/300001/chrome/browser/chrome_...
> chrome/browser/chrome_content_browser_client.cc:975:
> base::Unretained(chrome_render_filter)),
> Btw, I think we should just remove the base::Unretained() here:
> ChromeRenderMessageFilter is refcounted.

BTW this is being hooked up in the wrong place (RenderProcessWillLaunch).
Instead the interface should be exposed in ExposeInterfacesToRender. not-lgtm
until that's fixed.

[Charlie Harrison, 2016-08-25 03:47:19]

On 2016/07/25 18:25:14, Ben Goodger (Google) wrote:
> On 2016/07/25 02:50:47, dcheng wrote:
> >
>
https://codereview.chromium.org/2144533002/diff/300001/chrome/browser/chrome_...
> > File chrome/browser/chrome_content_browser_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2144533002/diff/300001/chrome/browser/chrome_...
> > chrome/browser/chrome_content_browser_client.cc:975:
> > base::Unretained(chrome_render_filter)),
> > Btw, I think we should just remove the base::Unretained() here:
> > ChromeRenderMessageFilter is refcounted.
> 
> BTW this is being hooked up in the wrong place (RenderProcessWillLaunch).
> Instead the interface should be exposed in ExposeInterfacesToRender. not-lgtm
> until that's fixed.

FYI I have a similar patch here that was blocked on similar u-a-f concerns
https://codereview.chromium.org/2043753002/
Let's sync up before landing either one.