Convert some NetError[Tab]Helper messages to mojom

Convert some NetError[Tab]Helper messages to mojom

Converts ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

The browser-side interface is exposed via a
WebContentsFrameBindingSet<T>.

The renderer-side (client) interface is exposed via
RenderFrameImpl's AssociatedInterfaceRegistry.

This CL serves as a simple example of converting routed IPC
messages which are handled by WebContentsObservers or other
WebContents-bound things.

BUG=612500
Committed: https://crrev.com/e1807f0bc6e60da26c609da2e87aaaae4b03c7b2
Cr-Commit-Position: refs/heads/master@{#419259}

[Ken Rockot(use gerrit already), 2016-09-02 19:59:12]

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

[commit-bot: I haz the power, 2016-09-02 19:59:52]

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

[Ken Rockot(use gerrit already), 2016-09-02 20:02:00]

Description was changed from

==========
Convert some NetError[Tab]Helper messages to mojom

Uses our shiny new WebContents associated interface support to
convert ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

BUG=612500
==========

to

==========
Convert some NetError[Tab]Helper messages to mojom

Uses our shiny new WebContents associated interface support to
convert ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2301123004
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. This CL

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-09-02 20:16:40]

rockot@chromium.org changed reviewers:
+ jam@chromium.org, sammc@chromium.org, tibell@chromium.org

[Ken Rockot(use gerrit already), 2016-09-02 20:16:41]

Here's what I came up with for helping WebContentsObservers do stuff.

I started out by modifying WebContentsObserver itself, but eventually it became
clear that this was not only unnecessary but also limiting.

What do you guys think?

[commit-bot: I haz the power, 2016-09-02 20:54:31]

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

[commit-bot: I haz the power, 2016-09-02 20:54:33]

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-05 00:47:33]

https://codereview.chromium.org/2310583002/diff/1/chrome/browser/net/net_erro...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/2310583002/diff/1/chrome/browser/net/net_erro...
chrome/browser/net/net_error_tab_helper.cc:189: case 0:  // MSVC complains if we
have a switch with only a default caluse.
Typo: caluse

https://codereview.chromium.org/2310583002/diff/1/chrome/browser/net/net_erro...
chrome/browser/net/net_error_tab_helper.cc:200:
interfaces_(content::WebContentsInterfaceRegistry::Create(contents)),
Shouldn't there just be one WebContentsInterfaceRegistry, belonging to the
WebContents?

[Ken Rockot(use gerrit already), 2016-09-07 00:41:45]

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

[commit-bot: I haz the power, 2016-09-07 00:42:10]

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

[Ken Rockot(use gerrit already), 2016-09-07 00:43:23]

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

[commit-bot: I haz the power, 2016-09-07 00:43:26]

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

[Ken Rockot(use gerrit already), 2016-09-07 00:47:12]

Updated CL #3 with some changes and rebased here

https://codereview.chromium.org/2310583002/diff/1/chrome/browser/net/net_erro...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/2310583002/diff/1/chrome/browser/net/net_erro...
chrome/browser/net/net_error_tab_helper.cc:189: case 0:  // MSVC complains if we
have a switch with only a default caluse.
On 2016/09/05 at 00:47:33, tibell wrote:
> Typo: caluse

Done

https://codereview.chromium.org/2310583002/diff/1/chrome/browser/net/net_erro...
chrome/browser/net/net_error_tab_helper.cc:200:
interfaces_(content::WebContentsInterfaceRegistry::Create(contents)),
On 2016/09/05 at 00:47:33, tibell wrote:
> Shouldn't there just be one WebContentsInterfaceRegistry, belonging to the
WebContents?

You're right, because (I'm pretty sure) we don't want to end up doubling the
number of WebContentsObservers in existence. I've moved
WebContentsInterfaceRegistry so an impl is composed directly into
WebContentsImpl.

However, I've also now added WebContentsFrameInterfaceBinding<T> which uses its
own scope to manage the lifetime of a registered interface on the WebContents's
registry. WebContents lifetime issues are subtle especially with respect to
observers: Many but not all WebContentsObservers are UserData and thus destroyed
during WebContentsImpl destruction, but after WebContentsImpl's destructor has
run.

Rather than expect everyone do get this right, WCFIB does the dirty work for you
without needing to be a WebContentsObserver itself. WDYT?

[Ken Rockot(use gerrit already), 2016-09-07 00:51:26]

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

[commit-bot: I haz the power, 2016-09-07 00:51:44]

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-07 01:28:03]

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

[commit-bot: I haz the power, 2016-09-07 01:28:03]

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

[Sam McNally, 2016-09-07 01:48:47]

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:103:
remote_network_diagnostics_client_.ForFrame(render_frame_host)
Do we need to keep connections alive beyond this method?

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:189: case 0:  // MSVC complains if we
have a switch with only a default clause.
Can we move the entire message map into the #if?

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:299: !=
web_contents()->GetMainFrame())
{}

https://codereview.chromium.org/2310583002/diff/40001/chrome/common/network_d...
File chrome/common/network_diagnostics.mojom (right):

https://codereview.chromium.org/2310583002/diff/40001/chrome/common/network_d...
chrome/common/network_diagnostics.mojom:16:
SetCanShowNetworkDiagnosticsDialog(bool can_show);
I suspect we can delete this IPC now and let the renderer figure it out.

[Ken Rockot(use gerrit already), 2016-09-07 22:44:29]

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

[commit-bot: I haz the power, 2016-09-07 22:44:57]

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

[Ken Rockot(use gerrit already), 2016-09-07 22:46:23]

Description was changed from

==========
Convert some NetError[Tab]Helper messages to mojom

Uses our shiny new WebContents associated interface support to
convert ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2301123004
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. This CL

BUG=612500
==========

to

==========
Convert some NetError[Tab]Helper messages to mojom

Uses our shiny new WebContents associated interface support to
convert ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. https://codereview.chromium.org/2310563002
  4. This CL

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-09-07 22:48:18]

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:103:
remote_network_diagnostics_client_.ForFrame(render_frame_host)
On 2016/09/07 at 01:48:47, Sam McNally wrote:
> Do we need to keep connections alive beyond this method?

Not in this case. I've removed RemoteFrameInterface for now.

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:189: case 0:  // MSVC complains if we
have a switch with only a default clause.
On 2016/09/07 at 01:48:47, Sam McNally wrote:
> Can we move the entire message map into the #if?

Done

https://codereview.chromium.org/2310583002/diff/40001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:299: !=
web_contents()->GetMainFrame())
On 2016/09/07 at 01:48:47, Sam McNally wrote:
> {}

Done

https://codereview.chromium.org/2310583002/diff/40001/chrome/common/network_d...
File chrome/common/network_diagnostics.mojom (right):

https://codereview.chromium.org/2310583002/diff/40001/chrome/common/network_d...
chrome/common/network_diagnostics.mojom:16:
SetCanShowNetworkDiagnosticsDialog(bool can_show);
On 2016/09/07 at 01:48:47, Sam McNally wrote:
> I suspect we can delete this IPC now and let the renderer figure it out.

How? Do you mean add a message to query it from the NetworkDiagnostics
interface?

[commit-bot: I haz the power, 2016-09-07 23:49:17]

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

[commit-bot: I haz the power, 2016-09-07 23:49:18]

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, 2016-09-09 04:06:43]

lgtm

[Sam McNally, 2016-09-09 06:08:12]

lgtm

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.h (right):

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.h:24: class
WebContentsInterfaceRegistry;
Remove.

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper_unittest.cc:48: return
static_cast<mojom::NetworkDiagnostics*>(this);
I don't think the explicit cast is necessary.

[Ken Rockot(use gerrit already), 2016-09-09 16:43:21]

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

[commit-bot: I haz the power, 2016-09-09 16:44:06]

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-09 16:47:28]

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

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

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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_...)

[Ken Rockot(use gerrit already), 2016-09-09 17:22:16]

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

[Ken Rockot(use gerrit already), 2016-09-09 17:22:27]

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.h (right):

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.h:24: class
WebContentsInterfaceRegistry;
On 2016/09/09 at 06:08:12, Sam McNally wrote:
> Remove.

Done

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2310583002/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper_unittest.cc:48: return
static_cast<mojom::NetworkDiagnostics*>(this);
On 2016/09/09 at 06:08:12, Sam McNally wrote:
> I don't think the explicit cast is necessary.

Done

[commit-bot: I haz the power, 2016-09-09 17:22:49]

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-09 18:18:52]

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

[commit-bot: I haz the power, 2016-09-09 18:18:53]

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-09-14 19:39:41]

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

[commit-bot: I haz the power, 2016-09-14 19:40:15]

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-14 20:46:34]

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

[commit-bot: I haz the power, 2016-09-14 20:46:35]

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

[Ken Rockot(use gerrit already), 2016-09-15 17:20:18]

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

[commit-bot: I haz the power, 2016-09-15 17:20:41]

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-15 18:30:31]

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

[commit-bot: I haz the power, 2016-09-15 18:30:33]

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

[Ken Rockot(use gerrit already), 2016-09-15 21:15:18]

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

[commit-bot: I haz the power, 2016-09-15 21:16:15]

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

[Ken Rockot(use gerrit already), 2016-09-15 21:16:35]

Patchset #10 (id:180001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-15 21:16:42]

Patchset #9 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-15 21:17:00]

Patchset #8 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-09-15 22:24:06]

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

[commit-bot: I haz the power, 2016-09-15 22:24:07]

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

[Ken Rockot(use gerrit already), 2016-09-15 22:24:24]

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

[commit-bot: I haz the power, 2016-09-15 22:24:52]

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

[Ken Rockot(use gerrit already), 2016-09-15 22:27:57]

Description was changed from

==========
Convert some NetError[Tab]Helper messages to mojom

Uses our shiny new WebContents associated interface support to
convert ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. https://codereview.chromium.org/2310563002
  4. This CL

BUG=612500
==========

to

==========
Convert some NetError[Tab]Helper messages to mojom

Converts ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

The browser-side interface is exposed via a
WebContentsFrameBindingSet<T>.

The renderer-side (client) interface is exposed via
RenderFrameImpl's AssociatedInterfaceRegistry.

This CL serves as a simple example of converting routed IPC
messages which are handled by WebContentsObservers or other
WebContents-bound things.

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-09-15 22:28:10]

Patchset #8 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-09-15 23:30:32]

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

[commit-bot: I haz the power, 2016-09-15 23:30:33]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-09-15 23:53:43]

rockot@chromium.org changed reviewers:
+ tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-09-15 23:53:44]

+tsepez@ for mojom

[jam, 2016-09-16 00:18:57]

lgtm

[Ken Rockot(use gerrit already), 2016-09-16 00:19:28]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-16 00:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
tibell@chromium.org
Link to the patchset: https://codereview.chromium.org/2310583002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-16 00:19:51]

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-16 00:29:44]

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

[commit-bot: I haz the power, 2016-09-16 00:29:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tom Sepez, 2016-09-16 19:50:17]

mojom lgtm

[Ken Rockot(use gerrit already), 2016-09-16 19:52:01]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-09-16 19:52:35]

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-16 19:58:24]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-09-16 19:59:54]

Description was changed from

==========
Convert some NetError[Tab]Helper messages to mojom

Converts ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

The browser-side interface is exposed via a
WebContentsFrameBindingSet<T>.

The renderer-side (client) interface is exposed via
RenderFrameImpl's AssociatedInterfaceRegistry.

This CL serves as a simple example of converting routed IPC
messages which are handled by WebContentsObservers or other
WebContents-bound things.

BUG=612500
==========

to

==========
Convert some NetError[Tab]Helper messages to mojom

Converts ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

The browser-side interface is exposed via a
WebContentsFrameBindingSet<T>.

The renderer-side (client) interface is exposed via
RenderFrameImpl's AssociatedInterfaceRegistry.

This CL serves as a simple example of converting routed IPC
messages which are handled by WebContentsObservers or other
WebContents-bound things.

BUG=612500

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

[commit-bot: I haz the power, 2016-09-16 19:59:56]

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