content: Refactor ChildThreadImpl::Options

content/child/child_thread_impl.h

Index: content/child/child_thread_impl.h
diff --git a/content/child/child_thread_impl.h b/content/child/child_thread_impl.h
index 4016861820ae15b4ca009ca52a801256fe642471..352bbeca8b3f936321886ee8f431aa527ee41b3c 100644
--- a/content/child/child_thread_impl.h
+++ b/content/child/child_thread_impl.h
@@ -63,10 +63,10 @@ class CONTENT_EXPORT ChildThreadImpl
  public:
   struct CONTENT_EXPORT Options {
     Options();

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

Could you make the constructor private and only callable by the Builder class?

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

Yep, done.

-    explicit Options(bool mojo);
-    Options(std::string name, bool mojo);
     ~Options();
 
+    class Builder;
+
     std::string channel_name;
     bool use_mojo_channel;
     bool in_browser_process;
@@ -305,6 +305,20 @@ class CONTENT_EXPORT ChildThreadImpl
   DISALLOW_COPY_AND_ASSIGN(ChildThreadImpl);
 };
 
+class ChildThreadImpl::Options::Builder {
+ public:
+  Builder();
+
+  Builder& InBrowserProcess(bool in_browser_process);
+  Builder& WithMojo(bool mojo);

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

nit - UseMojoChannel(bool use_mojo_channel)

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

Done.

+  Builder& WithChannelName(const std::string& channel_name);
+
+  Options Build();
+
+ private:
+  struct Options options_;

[rmcilroy, 2015/03/03 19:48:16]

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:15]

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]

No that's not what I meant - I prefer the builder pattern you have here too.
 
Yeah this is what I meant - this is much better, thanks!

+};
+
 }  // namespace content
 
 #endif  // CONTENT_CHILD_CHILD_THREAD_IMPL_H_