content: Refactor ChildThreadImpl::Options

content: Refactor ChildThreadImpl::Options

Refactor the way options are passed to ChildThreadImpl to make the
call sites easier to read and to make it easier to add a task runner
parameter later.

BUG=444764
Committed: https://crrev.com/f28ccd0b2709832d05156bd107d29d48a4a440cc
Cr-Commit-Position: refs/heads/master@{#319117}

[Sami, 2015-03-03 15:50:19]

skyostil@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Sami, 2015-03-03 15:50:37]

Ross, wanna do the first pass on this?

[rmcilroy, 2015-03-03 19:48:17]

lgtm with a couple of minor nits.

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:65: Options();
Could you make the constructor private and only callable by the Builder class?

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:313: Builder& WithMojo(bool mojo);
nit - UseMojoChannel(bool use_mojo_channel)

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:319: struct Options options_;
nit - I think this might be clearer inlined in the Options class, or at least
nearby the Option's declaration.

[Sami, 2015-03-04 11:37:14]

skyostil@chromium.org changed reviewers:
+ sievers@chromium.org

[Sami, 2015-03-04 11:37:15]

Thanks Ross. Daniel, PTAL.

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:65: Options();
On 2015/03/03 19:48:17, rmcilroy wrote:
> Could you make the constructor private and only callable by the Builder class?

Yep, done.

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:313: Builder& WithMojo(bool mojo);
On 2015/03/03 19:48:17, rmcilroy wrote:
> nit - UseMojoChannel(bool use_mojo_channel)

Done.

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:319: struct Options options_;
On 2015/03/03 19:48:16, rmcilroy wrote:
> nit - I think this might be clearer inlined in the Options class, or at least
> nearby the Option's declaration.

Did you mean the Options object itself would have the WithChannelName() etc.
methods? I dunno, I kind of prefer having the builder as a separate thing since
that's the pattern we tend to follow.

If you instead meant just moving this physically closer to where Options is
defined, then I did try that but you can't really inline the entire Builder
because it needs to hold an instance of Options, which isn't defined at that
point.

I also tried defining Builder right after Options but C++ got in the way: you
can't define the inner Builder class inside the declaration of ChildThreadImpl
("non-friend class member 'Builder' cannot have a qualified name").

I also considered just making Options be the Builder, but that doesn't work well
with inheritance.

I've now moved Options down here together with the Builder which at least makes
them fit on the same screen.

[rmcilroy, 2015-03-04 12:22:01]

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/974933002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.h:319: struct Options options_;
On 2015/03/04 11:37:15, Sami wrote:
> On 2015/03/03 19:48:16, rmcilroy wrote:
> > nit - I think this might be clearer inlined in the Options class, or at
least
> > nearby the Option's declaration.
> 
> Did you mean the Options object itself would have the WithChannelName() etc.
> methods? I dunno, I kind of prefer having the builder as a separate thing
since
> that's the pattern we tend to follow.

No that's not what I meant - I prefer the builder pattern you have here too.
 
> If you instead meant just moving this physically closer to where Options is
> defined, then I did try that but you can't really inline the entire Builder
> because it needs to hold an instance of Options, which isn't defined at that
> point.
> 
> I also tried defining Builder right after Options but C++ got in the way: you
> can't define the inner Builder class inside the declaration of ChildThreadImpl
> ("non-friend class member 'Builder' cannot have a qualified name").
> 
> I also considered just making Options be the Builder, but that doesn't work
well
> with inheritance.
> 
> I've now moved Options down here together with the Builder which at least
makes
> them fit on the same screen.

Yeah this is what I meant - this is much better, thanks!

[no sievers, 2015-03-04 19:10:49]

LGTM but we should figure out correct usage of |in_browser_process| for the
render thread, see comment.

https://codereview.chromium.org/974933002/diff/60001/content/child/child_thre...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/974933002/diff/60001/content/child/child_thre...
content/child/child_thread_impl.h:324: struct Options options_;
DISALLOW_COPY_AND_ASSIGN()? Unless it's a useful pattern, but seems like it'd
more likely be odd.

https://codereview.chromium.org/974933002/diff/60001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/974933002/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:455: .InBrowserProcess(true)
Has this been wrong even before your patch?
This is the constructor used when running in the renderer process, but it looks
like it's been using an Options constructor that sets |in_browser_process| to
|true|.

[Sami, 2015-03-04 19:21:16]

Thanks Daniel.

https://codereview.chromium.org/974933002/diff/60001/content/child/child_thre...
File content/child/child_thread_impl.h (right):

https://codereview.chromium.org/974933002/diff/60001/content/child/child_thre...
content/child/child_thread_impl.h:324: struct Options options_;
On 2015/03/04 19:10:49, sievers wrote:
> DISALLOW_COPY_AND_ASSIGN()? Unless it's a useful pattern, but seems like it'd
> more likely be odd.

Done.

https://codereview.chromium.org/974933002/diff/60001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/974933002/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:455: .InBrowserProcess(true)
On 2015/03/04 19:10:49, sievers wrote:
> Has this been wrong even before your patch?
> This is the constructor used when running in the renderer process, but it
looks
> like it's been using an Options constructor that sets |in_browser_process| to
> |true|.

Yes, that's one of the reasons why I wanted to do this refactoring :) I'll add a
TODO here and keep the existing behavior for this patch.

[no sievers, 2015-03-04 19:26:05]

On 2015/03/04 19:21:16, Sami wrote:
> Thanks Daniel.
> 
>
https://codereview.chromium.org/974933002/diff/60001/content/child/child_thre...
> File content/child/child_thread_impl.h (right):
> 
>
https://codereview.chromium.org/974933002/diff/60001/content/child/child_thre...
> content/child/child_thread_impl.h:324: struct Options options_;
> On 2015/03/04 19:10:49, sievers wrote:
> > DISALLOW_COPY_AND_ASSIGN()? Unless it's a useful pattern, but seems like
it'd
> > more likely be odd.
> 
> Done.
> 
>
https://codereview.chromium.org/974933002/diff/60001/content/renderer/render_...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/974933002/diff/60001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:455: .InBrowserProcess(true)
> On 2015/03/04 19:10:49, sievers wrote:
> > Has this been wrong even before your patch?
> > This is the constructor used when running in the renderer process, but it
> looks
> > like it's been using an Options constructor that sets |in_browser_process|
to
> > |true|.
> 
> Yes, that's one of the reasons why I wanted to do this refactoring :) I'll add
a
> TODO here and keep the existing behavior for this patch.

Cool, sounds good. You could consider defaulting |in_browser_process| to false
then in a follow-up when that issue gets addressed. And potentially even
changing Builder::InBrowserProcess(bool) to EnableInBrowserProcess() to make it
very explicit (and optional).

[Sami, 2015-03-04 19:28:28]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-03-04 19:28:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/974933002/#ps80001 (title:
"Review comments.")

[commit-bot: I haz the power, 2015-03-04 19:29:23]

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

[commit-bot: I haz the power, 2015-03-04 20:21:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-04 20:21:31]

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