content: Refactor ChildThreadImpl::Options

content/child/child_thread_impl.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_CHILD_CHILD_THREAD_IMPL_H_
 #define CONTENT_CHILD_CHILD_THREAD_IMPL_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 44 matching lines @@
 class ThreadSafeSender;
 class WebSocketDispatcher;
 struct RequestInfo;
 
 // The main thread of a child process derives from this class.
 class CONTENT_EXPORT ChildThreadImpl
     : public IPC::Listener,
       virtual public ChildThread {
  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;
     std::vector<IPC::MessageFilter*> startup_filters;
   };
 
   // Creates the thread.
   ChildThreadImpl();
   // Allow to be used for single-process mode and for in process gpu mode via
   // options.
@@ skipping 218 matching lines @@
 
   scoped_refptr<NavigatorConnectDispatcher> navigator_connect_dispatcher_;
 
   bool in_browser_process_;
 
   base::WeakPtrFactory<ChildThreadImpl> channel_connected_factory_;
 
   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_