ChannelMojo: Add ChannelMojo::CanBeUsed() to enable it incrementally

ChannelMojo: Add ChannelMojo::CanBeUsed() to enable it incrementally

ChannelMojo::CanBeUsed() let the code enable the feature for specific
platform regardless of the command line switch.

TEST=none
R=viettrungluu@chromium.org
BUG=377980
Committed: https://crrev.com/49ca46d9a1b8e03b28b1925ce7e9bdb96149f46f
Cr-Commit-Position: refs/heads/master@{#300394}

[viettrungluu, 2014-10-20 20:19:59]

I find the interface of ChannelMojo::ShouldBeUsed() rather odd: it checks a
command-line switch whose name has to be passed in (and yet it has to use the
global singleton CommandLine).

I think it'd be cleaner (even if it results in a bit more duplicated code) for
the renderer (host) to continue checking the command-line switch itself, and
have a (static) method ChannelMojo::CanBeUsed() (feel free to come up with a
better name) that returns a bool.

[Hajime Morrita, 2014-10-20 21:04:50]

morrita@chromium.org changed reviewers:
+ nasko@chromium.org

[Hajime Morrita, 2014-10-20 21:05:59]

Thanks for the review Trung! PTAL?

Nasko: Could you rubberstamp content/ change?

On 2014/10/20 20:19:59, viettrungluu wrote:
> I find the interface of ChannelMojo::ShouldBeUsed() rather odd: it checks a
> command-line switch whose name has to be passed in (and yet it has to use the
> global singleton CommandLine).
> 
> I think it'd be cleaner (even if it results in a bit more duplicated code) for
> the renderer (host) to continue checking the command-line switch itself, and
> have a (static) method ChannelMojo::CanBeUsed() (feel free to come up with a
> better name) that returns a bool.
Makes sense. Done.

[viettrungluu, 2014-10-20 21:50:12]

https://codereview.chromium.org/665993002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/665993002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:651: return
command_line.HasSwitch(switches::kEnableRendererMojoChannel) ||
Should this be "&&" instead of "||"?

My interpretation of the flag would be to "enable if possible" (which means
"&&").

Otherwise, this is more of a "force enable" flag ("enable even if it's not
entirely supported").

Another possibility would be to replace it with kDisableRendererMojoChannel, and
do |ChannelMojoHost::CanBeUsed() &&
!command_line.HasSwitch(switches::kDisableRendererMojoChannel)|.

https://codereview.chromium.org/665993002/diff/20001/ipc/mojo/ipc_channel_mojo.h
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/665993002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo.h:70: // True if ChannelMojo is ready to use for th e
underlying platform.
"ready to use" -> "can be used"
"th e" -> "the"

[Hajime Morrita, 2014-10-20 23:06:50]

PTAL?

https://codereview.chromium.org/665993002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/665993002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:651: return
command_line.HasSwitch(switches::kEnableRendererMojoChannel) ||
On 2014/10/20 21:50:11, viettrungluu wrote:
> Should this be "&&" instead of "||"?
> 
> My interpretation of the flag would be to "enable if possible" (which means
> "&&").
> 
> Otherwise, this is more of a "force enable" flag ("enable even if it's not
> entirely supported").
> 
> Another possibility would be to replace it with kDisableRendererMojoChannel,
and
> do |ChannelMojoHost::CanBeUsed() &&
> !command_line.HasSwitch(switches::kDisableRendererMojoChannel)|.
My intention here is that let people try this flag locally + turn
the feature on some platform. So I want to force it. 
Renamed ShouldBeUsed().

https://codereview.chromium.org/665993002/diff/20001/ipc/mojo/ipc_channel_mojo.h
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/665993002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo.h:70: // True if ChannelMojo is ready to use for th e
underlying platform.
On 2014/10/20 21:50:11, viettrungluu wrote:
> "ready to use" -> "can be used"
> "th e" -> "the"

Done.

[nasko, 2014-10-20 23:56:44]

content/ LGTM

[viettrungluu, 2014-10-21 00:08:58]

lgtm

[Hajime Morrita, 2014-10-21 00:32:41]

Thank you folks!

[Hajime Morrita, 2014-10-21 00:32:46]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:34:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665993002/40001

[commit-bot: I haz the power, 2014-10-21 01:16:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-21 01:18:12]

Patchset 3 (id:??) landed as
https://crrev.com/49ca46d9a1b8e03b28b1925ce7e9bdb96149f46f
Cr-Commit-Position: refs/heads/master@{#300394}