Refactor Pnacl coordinator

Refactor Pnacl coordinator
Move compilation thread into its own class to make future multiple 
implementations easier. Also split LocalTempFile into its own file to break
circular dependency.

R= sehr@google.com,jvoung@google.com,robertm@chromium.org
BUG=none
TEST=nacl_integration


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137481

[Derek Schuff, 2012-05-15 23:01:40]

This code works in my tests thus far, but I'm open to alternative, especially
the big argument list in PnaclTranslateThread::RunTranslate.

[Derek Schuff, 2012-05-15 23:17:20]

a few other highlights

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/local_temp_file.h (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/local_temp_file.h:85: const static
nacl::string& GetPnaclTempDir() { return kPnaclTempDir; }
this replaces kPnaclTempDir in pnacl_coordinator.cc. I wasn't sure exactly what
to do about those static data that now need to be shared.

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:26: class PnaclUrls {
This replaces some static char*s and GetExtensionUrl in pnacl_coordinator.cc as
well as G and removes the TODO to find some other way to handle that.

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:50:
TranslateFailed("could not allocate thread struct.");
This (and line 58) is now TranslateFailed callback instead of
ReportNonPpapiError

[jvoung - send to chromium..., 2012-05-15 23:51:05]

looks pretty good -- just a few suggestions

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/local_temp_file.cc (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/local_temp_file.cc:65:
filename_(kPnaclTempDir + "/" + filename) {
Maybe we could just have the constructor take an argument that is the base temp
dir here and in the other constructor too, so that this is no longer tied to
pnacl.

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/local_temp_file.h (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/local_temp_file.h:45: class LocalTempFile
{
Maybe prefix the name with "Pnacl", but... up to you.  The only thing pnacl
specific is the temp dir really.

See later comment about possibly making it non-pnacl specific though.

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (left):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:297: // Signal that
Pnacl translation completed normally.
Leave the comment for TranslateFinished?

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:33: // Maybe
better to encapsulate these into a class/struct?
Somewhat related to this -- these shared data-structures plus having the
coordinator own them, was why we were having surfaway (click back button and
crash) issues...  coordinator dtors all of them while they are still used by the
thread =(


One idea was to encapsulate these in an ref-counted struct or something (so that
might be a reason to have these in a struct).


However, there's also some shared state from starting the helper process from
this thread ("plugin_->LoadHelperNaClModule"), so that also has some shared
state that could be pulled out from under the thread (don't remember what,
though), and the plugin can start dtor'ing things before it gets to the
coordinator's dtor.

[Derek Schuff, 2012-05-16 16:47:46]

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/local_temp_file.cc (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/local_temp_file.cc:65:
filename_(kPnaclTempDir + "/" + filename) {
On 2012/05/15 23:51:05, jvoung wrote:
> Maybe we could just have the constructor take an argument that is the base
temp
> dir here and in the other constructor too, so that this is no longer tied to
> pnacl.

Done.

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (left):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:297: // Signal that
Pnacl translation completed normally.
On 2012/05/15 23:51:05, jvoung wrote:
> Leave the comment for TranslateFinished?

Done.

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

https://chromiumcodereview.appspot.com/10377157/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:33: // Maybe
better to encapsulate these into a class/struct?
OK. so clearly we'll have to do something different here anyway in the future,
so for now i'll leave it as is and put a todo (obviously a fix for that belongs
in a different CL)
On 2012/05/15 23:51:05, jvoung wrote:
> Somewhat related to this -- these shared data-structures plus having the
> coordinator own them, was why we were having surfaway (click back button and
> crash) issues...  coordinator dtors all of them while they are still used by
the
> thread =(
> 
> 
> One idea was to encapsulate these in an ref-counted struct or something (so
that
> might be a reason to have these in a struct).
> 
> 
> However, there's also some shared state from starting the helper process from
> this thread ("plugin_->LoadHelperNaClModule"), so that also has some shared
> state that could be pulled out from under the thread (don't remember what,
> though), and the plugin can start dtor'ing things before it gets to the
> coordinator's dtor.
>

[jvoung - send to chromium..., 2012-05-16 17:01:05]

lgtm

[commit-bot: I haz the power, 2012-05-16 17:01:40]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sehr (please use chromium), 2012-05-16 17:19:56]

LGTM. Two very small items.  The request for using "svn cp" I would have made
could not be done with git svn.

http://codereview.chromium.org/10377157/diff/12001/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/10377157/diff/12001/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:25: const static
char kPnaclTempDir[] = "/.pnacl";
This is already in an anonymous namespace.  Static is not needed.

http://codereview.chromium.org/10377157/diff/12001/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

http://codereview.chromium.org/10377157/diff/12001/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:48: private:
DISALLOW_COPY_AND_ASSIGN(PnaclTranslateThread);

[Derek Schuff, 2012-05-16 17:48:09]

https://chromiumcodereview.appspot.com/10377157/diff/12001/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://chromiumcodereview.appspot.com/10377157/diff/12001/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:25: const static
char kPnaclTempDir[] = "/.pnacl";
On 2012/05/16 17:19:56, sehr wrote:
> This is already in an anonymous namespace.  Static is not needed.

Done.

https://chromiumcodereview.appspot.com/10377157/diff/12001/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

https://chromiumcodereview.appspot.com/10377157/diff/12001/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:48: private:
On 2012/05/16 17:19:56, sehr wrote:
> DISALLOW_COPY_AND_ASSIGN(PnaclTranslateThread);

Done.

[commit-bot: I haz the power, 2012-05-16 17:48:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/10377157/8003

[commit-bot: I haz the power, 2012-05-16 19:26:07]

Change committed as 137481