Move command line processing out of coordinator

ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h

Index: ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h
===================================================================
--- ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h	(revision 113812)
+++ ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h	(working copy)
@@ -14,56 +14,132 @@
 #include "native_client/src/shared/platform/nacl_sync_checked.h"
 #include "native_client/src/shared/platform/nacl_threads.h"
 #include "native_client/src/shared/srpc/nacl_srpc.h"
+#include "native_client/src/trusted/desc/nacl_desc_rng.h"
 #include "native_client/src/trusted/desc/nacl_desc_wrapper.h"
 #include "native_client/src/trusted/plugin/delayed_callback.h"
 #include "native_client/src/trusted/plugin/nacl_subprocess.h"
 #include "native_client/src/trusted/plugin/plugin_error.h"
 #include "native_client/src/trusted/plugin/pnacl_resources.h"
 
+#include "ppapi/c/pp_file_info.h"
 #include "ppapi/cpp/completion_callback.h"
+#include "ppapi/cpp/file_io.h"
+#include "ppapi/cpp/file_ref.h"
+#include "ppapi/cpp/file_system.h"
 
 struct NaClMutex;
+struct PPB_FileIOTrusted;
 
 namespace plugin {
 
 class Plugin;
 class PnaclCoordinator;
 
+// Represents a file used as a temporary between stages in translation.

[robertm, 2011/12/12 16:23:29]

Maybe add a more concrete use example so that the reader can
start assembling a mental picture how all these classes
relate

[sehr (please use chromium), 2011/12/13 00:12:45]

I hope what I have added is sufficient.

+class PnaclFileDescPair {
+ public:
+  PnaclFileDescPair(pp::FileSystem* file_system, PnaclCoordinator* coordinator);
+  ~PnaclFileDescPair();
+  // Opens a pair of file IO objects referring to a randomly named file in
+  // file_system_.  One IO is for writing the file and another for reading it.
+  void Open(const pp::CompletionCallback& cb);
+  // Gets the POSIX file descriptor for a resource.
+  int32_t GetFD(int32_t pp_error,
+                const pp::Resource& resource,
+                const nacl::string& component);
+  // Called when the writable file IO was opened.
+  void WriteFileDidOpen(int32_t pp_error);
+  // Called when the readable file IO was opened.
+  void ReadFileDidOpen(int32_t pp_error);
+
+  // Accessors.
+  nacl::DescWrapper* write_wrapper() { return write_wrapper_.get(); }
+  nacl::DescWrapper* read_wrapper() { return read_wrapper_.get(); }
+
+ private:
+  pp::FileSystem* file_system_;
+  PnaclCoordinator* coordinator_;
+  const PPB_FileIOTrusted* file_io_trusted_;
+  pp::CompletionCallbackFactory<PnaclFileDescPair> callback_factory_;
+  nacl::string filename_;
+  nacl::scoped_ptr<pp::FileRef> write_ref_;
+  nacl::scoped_ptr<pp::FileIO> write_io_;
+  nacl::scoped_ptr<nacl::DescWrapper> write_wrapper_;
+  nacl::scoped_ptr<pp::FileRef> read_ref_;
+  nacl::scoped_ptr<pp::FileIO> read_io_;
+  nacl::scoped_ptr<nacl::DescWrapper> read_wrapper_;
+  pp::CompletionCallback done_callback_;
+  // Random number generator used to create filenames.
+  struct NaClDescRng rng_desc_;
+};
+
+// A reference counting class that can be used for one background thread.
+// The default NonThreadSafeRefCount asserts that it is only used on the
+// main thread.  Needed for CompletionCallbackFactory in PnaclTranslationUnit.
+class PnaclNonThreadSafeRefCount {

[robertm, 2011/12/12 16:23:29]

the point of  this class seems dubious as it does not do much

[sehr (please use chromium), 2011/12/13 00:12:45]

After fusing the two classes, it needed to be synchronized.  Done.

+ public:
+  PnaclNonThreadSafeRefCount() : ref_(0) { }
+  int32_t AddRef() { return ++ref_; }
+  int32_t Release() { return --ref_; }
+
+ private:
+  int32_t ref_;
+};
+
+// Represents an individual bitcode file during a translation.
 struct PnaclTranslationUnit {

[robertm, 2011/12/12 16:23:29]

why a struct?

[sehr (please use chromium), 2011/12/13 00:12:45]

I eliminated this struct by merging it into PnaclCoordinator, as the two were
not naturally separable.

-  PnaclTranslationUnit(PnaclCoordinator* coord)
+ public:
+  PnaclTranslationUnit(PnaclCoordinator* coord, const nacl::string& url)
     : coordinator(coord),
-      obj_len(-1),
-      is_shared_library(false),
-      soname(""),
-      lib_dependencies("") {
+      pexe_url(url) {
+    NaClXMutexCtor(&mu);
+    NaClXCondVarCtor(&cv);
+    callback_factory.Initialize(this);
   }
-  // Punch hole in abstraction.
+
+  // Invoked on the main thread on behalf of the lookup service to start
+  // loading a particular URL.
+  void LoadOneFile(int32_t pp_error,
+                   const nacl::string& url,
+                   nacl::DescWrapper** wrapper,
+                   pp::CompletionCallback& done_cb);
+  // Invoked by the renderer when the file was loaded.
+  void DidLoadFile(int32_t pp_error,
+                   const nacl::string& full_url,
+                   nacl::DescWrapper** wrapper,
+                   pp::CompletionCallback& done_cb);
+  // Signals the waiting lookup service to resume.
+  void ResumeLookup(int32_t pp_error, PnaclTranslationUnit* trans);
+  // Performs the lookup of the descriptor for url.
+  nacl::DescWrapper* LookupDesc(const nacl::string& url);
+
+  // To refer back to shared resources.
   PnaclCoordinator* coordinator;
-
+  // The URL for the pexe file.
+  nacl::string pexe_url;
+  // Factory for callbacks used in file lookups.
+  pp::CompletionCallbackFactory<PnaclTranslationUnit,
+                                PnaclNonThreadSafeRefCount> callback_factory;
   // Borrowed reference which must outlive the thread.
   nacl::scoped_ptr<nacl::DescWrapper> pexe_wrapper;
-
-  // Object file produced by translator and consumed by the linker.
-  nacl::scoped_ptr<nacl::DescWrapper> obj_wrapper;
-  int32_t obj_len;
-
-  // Information extracted from the pexe that is needed by the linker.
-  bool is_shared_library;
-  nacl::string soname;
-  nacl::string lib_dependencies;
-
-  // The translated user nexe file.
-  nacl::scoped_ptr<nacl::DescWrapper> nexe_wrapper;
-
+  // Object file, produced by the translator and consumed by the linker.
+  nacl::scoped_ptr<PnaclFileDescPair> obj_file;
+  // Translated nexe file, produced by the linker and consumed by sel_ldr.
+  nacl::scoped_ptr<PnaclFileDescPair> nexe_file;
   // Callbacks to run when tasks or completed or an error has occurred.
   pp::CompletionCallback translate_done_cb;
-  pp::CompletionCallback link_done_cb;
 
+  // Support for file lookups.
+  struct NaClMutex mu;

[robertm, 2011/12/12 16:23:29]

these should be private if possible

[sehr (please use chromium), 2011/12/13 00:12:45]

Done.

+  struct NaClCondVar cv;
+  bool lookup_is_complete;
+
   ErrorInfo error_info;
 };
 
 
-typedef std::pair<nacl::string, pp::CompletionCallback> url_callback_pair;
+// typedef std::pair<nacl::string, pp::CompletionCallback> url_callback_pair;
+typedef std::vector<nacl::string> string_vector;
 
 // A class that handles PNaCl client-side translation.
 // Usage:

[robertm, 2011/12/12 16:23:29]

since we do not have a design the class comment would benefit from more
verboseness

[sehr (please use chromium), 2011/12/13 00:12:45]

Done.

@@ -80,7 +156,8 @@
       translate_notify_callback_(pp::BlockUntilComplete()),
       llc_subprocess_(NULL),
       ld_subprocess_(NULL),
-      subprocesses_should_die_(false) {
+      subprocesses_should_die_(false),
+      file_system_is_initialized_(false) {
         NaClXMutexCtor(&subprocess_mu_);
   }
 
@@ -102,41 +179,36 @@
                             const nacl::string& url,
                             const nacl::string& component);
 
-  // Run when faced with a PPAPI error condition. It brings control back to the
-  // plugin by invoking the |translate_notify_callback_|.
-  void PnaclPpapiError(int32_t pp_error);
   // Run |translate_notify_callback_| with an error condition that is not
   // PPAPI specific.
-  void PnaclNonPpapiError();
-  // Wrapper for Plugin ReportLoadAbort.
-  void ReportLoadAbort();
-  // Wrapper for Plugin ReportLoadError.
-  void ReportLoadError(const ErrorInfo& error);
+  void ReportNonPpapiError(const nacl::string& message);
+  // Run when faced with a PPAPI error condition. Bring control back to the
+  // plugin by invoking the |translate_notify_callback_|.
+  void ReportPpapiError(int32_t pp_error, const nacl::string& message);
+  void ReportPpapiError(int32_t pp_error, const ErrorInfo& error);
 
   // Accessors for use by helper threads.

[robertm, 2011/12/12 16:23:29]

can these be avoided? many seem to be used
during initialization of the helper threads.
In that case they could either be passed in as arguments
to the thread either individually or in special purpose
struct.  
This seems preferable over having a larger API

[sehr (please use chromium), 2011/12/13 00:12:45]

Fusing PnaclTranslationUnit into PnaclCoordinator cleaned this up a bunch.

   nacl::string resource_base_url() const { return resource_base_url_; }
+  PnaclResources* resources() const { return resources_.get(); }
   Plugin* plugin() const { return plugin_; }
-  nacl::string llc_url() const { return llc_url_; }
   NaClSubprocess* llc_subprocess() const { return llc_subprocess_; }
-  bool StartLlcSubProcess();
-  nacl::string ld_url() const { return ld_url_; }
   NaClSubprocess* ld_subprocess() const { return ld_subprocess_; }
-  bool StartLdSubProcess();
+  NaClSubprocess* StartSubProcess(const nacl::string& url);
   bool SubprocessesShouldDie();
   void SetSubprocessesShouldDie(bool subprocesses_should_die);
-  PnaclResources* resources() const { return resources_.get(); }
 
  protected:

[robertm, 2011/12/12 16:23:29]

protected vs private is very 
subtle, please add comment.

[sehr (please use chromium), 2011/12/13 00:12:45]

There are no child classes, so I moved all this to private.  I don't know where
it came from to start with :-)

+  // Callback for when various files, etc. have been downloaded.
+  void ResourcesDidLoad(int32_t pp_error);
 
-  // Callbacks for when various files, etc. have been downloaded.
-  void ResourcesDidLoad(int32_t pp_error,
-                        const nacl::string& url,
-                        PnaclTranslationUnit* translation_unit);
+  // Callbacks for temporary file related stages.
+  void FileSystemDidOpen(int32_t pp_error);
+  void ObjectPairDidOpen(int32_t pp_error, PnaclTranslationUnit* translation);
+  void NexePairDidOpen(int32_t pp_error, PnaclTranslationUnit* translation);
 
   // Callbacks for compute-based translation steps.
-  void RunTranslate(int32_t pp_error,
-                    const nacl::string& url,
-                    PnaclTranslationUnit* translation_unit);
+  void DoStartTranslation(int32_t pp_error, PnaclTranslationUnit* translation);
+  void RunTranslate(int32_t pp_error, PnaclTranslationUnit* translation_unit);
   void RunLink(int32_t pp_error, PnaclTranslationUnit* translation_unit);
 
   // Pnacl translation completed normally.
@@ -151,8 +223,6 @@
 
   // URLs used to lookup downloaded resources.
   nacl::string resource_base_url_;
-  nacl::string llc_url_;
-  nacl::string ld_url_;
 
   // Helper subprocesses loaded by the plugin (deleted by the plugin).

[robertm, 2011/12/12 16:23:29]

please comment all variables and add any information that might be useful to
understand the workings of the class.

[sehr (please use chromium), 2011/12/13 00:12:45]

Done.

   // We may want to do cleanup ourselves when we are in the
@@ -171,8 +241,9 @@
   // it easier to add steps later (the mechanism could look like PostMessage?).
   nacl::scoped_ptr<PnaclTranslationUnit> translation_unit_;
   nacl::scoped_ptr<NaClThread> translate_thread_;
-  nacl::scoped_ptr<NaClThread> link_thread_;
-
+  // Translation creates local temporary files.
+  nacl::scoped_ptr<pp::FileSystem> file_system_;
+  bool file_system_is_initialized_;
   // An auxiliary class that manages downloaded resources.
   nacl::scoped_ptr<PnaclResources> resources_;
 };