Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8786005:
Move command line processing out of coordinator (Closed)
|
Created:
9 years ago by sehr (please use chromium) Modified:
9 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionThis CL moves all command line processing out of the coordinator. Key other features required by this change:
1) object and nexe are now passed by ppapi temporary files
2) native library and .o loading is done purely on demand
3) the command lines baked into llc and ld are used exclusively
One minor change that fit with the CL is that the coordinator now creates only one helper thread that runs both llc and ld.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2440
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114473
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 20
Patch Set 4 : '' #
Total comments: 51
Patch Set 5 : '' #
Total comments: 24
Patch Set 6 : '' #Messages
Total messages: 12 (0 generated)
There are still a couple of incremental improvements I can envision, but would like to get a first CL in to unblock the LD work for dynamic linking.
mostly a cry for more documentation/comments obviously the classes are not used as commonly as this well commented example: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/at_exit.h but then it is also a lot more complex so finding a middle ground would be best. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:969: pnacl_.Initialize(this); This is sort of pre-existing. why is this not done in the constructor? If there is a good reason, please add a comment Better yet, make the name more meaningful so that the comment is not necessary. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:38: // Represents a file used as a temporary between stages in translation. Maybe add a more concrete use example so that the reader can start assembling a mental picture how all these classes relate http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:79: class PnaclNonThreadSafeRefCount { the point of this class seems dubious as it does not do much http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:90: struct PnaclTranslationUnit { why a struct? http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:133: struct NaClMutex mu; these should be private if possible http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:145: // Usage: since we do not have a design the class comment would benefit from more verboseness http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:178: int32_t GetLoadedFileDesc(int32_t pp_error, comment this prototype http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:190: // Accessors for use by helper threads. 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 http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:200: protected: protected vs private is very subtle, please add comment. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:227: // Helper subprocesses loaded by the plugin (deleted by the plugin). please comment all variables and add any information that might be useful to understand the workings of the class.
Thanks for the review and excellent suggestions. Drastically revised CL uploaded. PTAL. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:969: pnacl_.Initialize(this); On 2011/12/12 16:23:29, robertm wrote: > This is sort of pre-existing. > why is this not done in the constructor? > If there is a good reason, please add a comment > Better yet, make the name more meaningful so that the comment > is not necessary. I have changed how the coordinator is instantiated, and made the initialization be hidden inside a factory. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:38: // Represents a file used as a temporary between stages in translation. On 2011/12/12 16:23:29, robertm wrote: > Maybe add a more concrete use example so that the reader can > start assembling a mental picture how all these classes > relate I hope what I have added is sufficient. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:79: class PnaclNonThreadSafeRefCount { On 2011/12/12 16:23:29, robertm wrote: > the point of this class seems dubious as it does not do much After fusing the two classes, it needed to be synchronized. Done. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:90: struct PnaclTranslationUnit { On 2011/12/12 16:23:29, robertm wrote: > why a struct? I eliminated this struct by merging it into PnaclCoordinator, as the two were not naturally separable. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:133: struct NaClMutex mu; On 2011/12/12 16:23:29, robertm wrote: > these should be private if possible Done. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:145: // Usage: On 2011/12/12 16:23:29, robertm wrote: > since we do not have a design the class comment would benefit from more > verboseness Done. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:178: int32_t GetLoadedFileDesc(int32_t pp_error, On 2011/12/12 16:23:29, robertm wrote: > comment this prototype Done. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:190: // Accessors for use by helper threads. On 2011/12/12 16:23:29, robertm wrote: > 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 Fusing PnaclTranslationUnit into PnaclCoordinator cleaned this up a bunch. http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:200: protected: On 2011/12/12 16:23:29, robertm wrote: > protected vs private is very > subtle, please add comment. There are no child classes, so I moved all this to private. I don't know where it came from to start with :-) http://codereview.chromium.org/8786005/diff/9002/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:227: // Helper subprocesses loaded by the plugin (deleted by the plugin). On 2011/12/12 16:23:29, robertm wrote: > please comment all variables and add any information that might be useful to > understand the workings of the class. Done.
partial review http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.h:491: PnaclCoordinator* pnacl_; Does this ever get deleted? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:134: int32_t posix_desc = _open_osfhandle(file_desc, _O_RDWR | _O_BINARY); nit: Should this know if it was GetFD for a readonly file? and use _O_RDONLY in that case? I guess in the non-Windows path you wouldn't have been able to specify RDONLY and we've been doing this in file_downloader.cc for a while, so it probably doesn't matter. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:137: CloseHandle(reinterpret_cast<HANDLE>(file_desc)); ReportPpapiError here as well, like the other error cases? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:174: return; I guess these "return;"s are counting on GetFD to report the error / run a callback to unstick the coordinator or the plugin that's waiting on the coordinator? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:413: NaClSubprocessId id = plugin_->LoadHelperNaClModule(wrapper, &error_info); The error_info here is set but dropped. Would it make sense to communicate the lower-level error messages up to the caller of StartSubprocess (and perhaps concat the "could not start compiler subprocess\n" / linker messages to get a full message)? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:437: // LLC returns values that are used to determine how linking is cone. cone -> done http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:71: void WriteFileDidOpen(int32_t pp_error); Can these callbacks be private? Also, looks like GetFD() is only used by the callback to set up the DescWrappers, so that may be private as well? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:279: struct NaClMutex mu_; might want to make the fields more descriptive in any case? E.g., "lookup_mu_" ?
another batch comments including commenting requests for pnacl_resources.* which is pre-exising. sorry, that you having to suffer for omissions by others, but those comments will help with this review. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:1623: // Will always call the callback on success or failure. it might be useful for debugging to issue ProgressEventProgress events just before the return statements if this is not already done inside the called functions http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:1624: pnacl_ = PnaclCoordinator::BitcodeToNative(this, is this overwritten every time? maybe use a scoped ptr and reset. Also, mention in a comment how it is used -- presumably it is needed for the lifetime of the callback? BTW: could it be passed into the callback and free there? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.h:26: #include "native_client/src/trusted/plugin/pnacl_coordinator.h" this may not longer be needed and could be replaced by a forward type declaration http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:35: // Fake filename for the object file generated by llvm. llvm -> llc http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:86: for (int32_t i = 0; i < kTempFileNameWords; ++i) { this looks like a good candidate for factoring out int to a static helper or private member Get32RandomHexDigits(). this way you can also drop the comment. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:189: PnaclCoordinator* coordinator = new PnaclCoordinator(plugin); it might be simpler to pass the args into the constructor http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:202: coordinator->resources_->RunWhenAllLoaded(resources_cb); could this also go into the contructor of PnaclResources() http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:14: #include "native_client/src/shared/platform/nacl_sync_checked.h" optional: can some of these eliminated in favor of forward type declarations http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:164: struct NaClDesc* LookupDesc(const nacl::string& url); This is a little bit unclear to me, resources are .o .a linkerscripts etc. right? Why is public? does the callsite of this know which of the two cases it is using? if so maybe this can be split in two functions http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:40: all_loaded_ = false; maybe having a counter of how many have been (successfully) downloaded which is than compared to the total number requests would make the code clearer and also be useful for debugging. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:56: delayed_callback_.reset( not sure whether the DelayedCallback adds all this much value in this case. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:95: delayed_callback_->RunIfTime(); do you need delayed_callback_ or could you make the decision if "it is time" based on all the data structures? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:25: class PnaclResources { pre-existing but since this is a central class: add class comment the protocol is actually not quite clear to me: is this class single use or multiple use? single use would be easier to understand. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:30: : plugin_(plugin), pre-existing the dependency on both the plugin and the coordiator create coupling which would be nice to avoid. maybe at least the plugin dependency can be avoided. by offering additional proxy functions in the coordinator, similar to ReportPpapiError() This makes the coordinator a little more ugly but I think it would be worth it, concretely: the sequence GetLoadedFileDesc() -> plugin_->wrapper_factory()->MakeFileDesc() should be wrapped in coordinator proxy function and plugin_->StreamAsFile() into another one http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:42: // Get the wrapper for the downloaded resource. when is it ok to call this function? http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:47: // Add an URL for download. mention: do not call this after StartDownloads() has been called http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:52: void RunWhenAllLoaded(pp::CompletionCallback& client_callback); This maybe irrelevant once the class protocol has been clarified: if this is required please add to contructor or to StartDownloads()
Thanks for the reviews. PTAL. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:1623: // Will always call the callback on success or failure. On 2011/12/13 17:06:39, robertm wrote: > it might be useful for debugging to issue > ProgressEventProgress events just before the return statements if this is not > already done inside the called functions There are tests that rely on how many progress events are delivered, so this would break them when we start running them on this path and today on the other return path... http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.cc:1624: pnacl_ = PnaclCoordinator::BitcodeToNative(this, On 2011/12/13 17:06:39, robertm wrote: > is this overwritten every time? > maybe use a scoped ptr and reset. > Also, mention in a comment how it is used -- presumably it is needed for the > lifetime of the callback? > BTW: could it be passed into the callback and free there? Used a scoped_ptr. The ugly thing is I think NaCl has been living with a dangling file pointer for the nexe for a while. I need to leave the temporary file owner around until the sel_ldr is done, I think, so yes, this is a bit of unavoidable debt. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/plugin.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.h:26: #include "native_client/src/trusted/plugin/pnacl_coordinator.h" On 2011/12/13 17:06:39, robertm wrote: > this may not longer be needed and could be replaced by a forward type > declaration My change to use scoped_ptr requires it be included. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/plugin.h:491: PnaclCoordinator* pnacl_; On 2011/12/13 03:14:33, jvoung wrote: > Does this ever get deleted? Thanks. Changed to scoped_ptr so that it goes away when plugin does. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:35: // Fake filename for the object file generated by llvm. On 2011/12/13 17:06:39, robertm wrote: > llvm -> llc Done. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:86: for (int32_t i = 0; i < kTempFileNameWords; ++i) { On 2011/12/13 17:06:39, robertm wrote: > this looks like a good candidate for factoring out int to a > static helper or private member Get32RandomHexDigits(). > this way you can also drop the comment. Done. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:134: int32_t posix_desc = _open_osfhandle(file_desc, _O_RDWR | _O_BINARY); On 2011/12/13 03:14:33, jvoung wrote: > nit: Should this know if it was GetFD for a readonly file? and use _O_RDONLY in > that case? I guess in the non-Windows path you wouldn't have been able to > specify RDONLY and we've been doing this in file_downloader.cc for a while, so > it probably doesn't matter. Great catch, though. Done here. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:137: CloseHandle(reinterpret_cast<HANDLE>(file_desc)); On 2011/12/13 03:14:33, jvoung wrote: > ReportPpapiError here as well, like the other error cases? I deferred error reporting to the clients as you suggest below. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:174: return; On 2011/12/13 03:14:33, jvoung wrote: > I guess these "return;"s are counting on GetFD to report the error / run a > callback to unstick the coordinator or the plugin that's waiting on the > coordinator? I moved the error handling to here. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:189: PnaclCoordinator* coordinator = new PnaclCoordinator(plugin); On 2011/12/13 17:06:39, robertm wrote: > it might be simpler to pass the args into the constructor Done. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:202: coordinator->resources_->RunWhenAllLoaded(resources_cb); On 2011/12/13 17:06:39, robertm wrote: > could this also go into the contructor of PnaclResources() Done. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:413: NaClSubprocessId id = plugin_->LoadHelperNaClModule(wrapper, &error_info); On 2011/12/13 03:14:33, jvoung wrote: > The error_info here is set but dropped. Would it make sense to communicate the > lower-level error messages up to the caller of StartSubprocess (and perhaps > concat the "could not start compiler subprocess\n" / linker messages to get a > full message)? I goofed bonding the error_info_ member variable through everywhere. I think I have fixed it now, albeit while losing the "compiler" or "linker" part. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:437: // LLC returns values that are used to determine how linking is cone. On 2011/12/13 03:14:33, jvoung wrote: > cone -> done Done. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:14: #include "native_client/src/shared/platform/nacl_sync_checked.h" On 2011/12/13 17:06:39, robertm wrote: > optional: can some of these eliminated in favor of forward type declarations Not this time :-(. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:71: void WriteFileDidOpen(int32_t pp_error); On 2011/12/13 03:14:33, jvoung wrote: > Can these callbacks be private? > > Also, looks like GetFD() is only used by the callback to set up the > DescWrappers, so that may be private as well? Yes to both. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:164: struct NaClDesc* LookupDesc(const nacl::string& url); On 2011/12/13 17:06:39, robertm wrote: > This is a little bit unclear to me, resources are > .o .a linkerscripts etc. right? > Why is public? > > > does the callsite of this know which of the two cases it is using? if so maybe > this can be split in two functions It was because I was using a non-member function to handle the SRPC method. I have made it a static member function. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:279: struct NaClMutex mu_; On 2011/12/13 03:14:33, jvoung wrote: > might want to make the fields more descriptive in any case? E.g., "lookup_mu_" ? Done. (lookup_service_mu_, lookup_service_cv_) http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:40: all_loaded_ = false; On 2011/12/13 17:06:39, robertm wrote: > maybe having a counter of how many have been (successfully) downloaded which is > than compared to the total number requests would make the code clearer and also > be useful for debugging. There is such a counter in DelayedCallback. I have removed all_loaded_ entirely. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:56: delayed_callback_.reset( On 2011/12/13 17:06:39, robertm wrote: > not sure whether the DelayedCallback adds all this much value in this case. It is needed to implement the barrier callback for multiple resources. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:95: delayed_callback_->RunIfTime(); On 2011/12/13 17:06:39, robertm wrote: > do you need delayed_callback_ or could you make the decision if "it is time" > based on all the data structures? I think using DelayedCallback and removing the others is better :-) http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:25: class PnaclResources { On 2011/12/13 17:06:39, robertm wrote: > pre-existing but since this is a central class: add class comment > > the protocol is actually not quite clear to me: > is this class single use or multiple use? > single use would be easier to understand. As this CL removes most of the need for the PnaclResources class, I have drastically simplified its features and added more documentation. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:30: : plugin_(plugin), On 2011/12/13 17:06:39, robertm wrote: > pre-existing > the dependency on both the plugin and the coordiator create coupling which would > be nice to avoid. > > maybe at least the plugin dependency can be avoided. > by offering additional proxy functions in the coordinator, > similar to ReportPpapiError() > > This makes the coordinator a little more ugly but I think it would be worth it, > concretely: > the sequence GetLoadedFileDesc() -> plugin_->wrapper_factory()->MakeFileDesc() > should be wrapped in coordinator proxy function > > and plugin_->StreamAsFile() into another one > > > > > > > > See above. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:42: // Get the wrapper for the downloaded resource. On 2011/12/13 17:06:39, robertm wrote: > when is it ok to call this function? Comment added. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:47: // Add an URL for download. On 2011/12/13 17:06:39, robertm wrote: > mention: do not call this after StartDownloads() has been called I removed this interface, adding a vector of URLs to the constructor, as you suggested. http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_resources.h:52: void RunWhenAllLoaded(pp::CompletionCallback& client_callback); On 2011/12/13 17:06:39, robertm wrote: > This maybe irrelevant once the class protocol has been clarified: > > if this is required please add to contructor or to StartDownloads() Done.
LGTM w.r.t. my comments http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): http://codereview.chromium.org/8786005/diff/6007/ppapi/native_client/src/trus... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:35: // Fake filename for the object file generated by llvm. On 2011/12/13 20:05:04, sehr wrote: > On 2011/12/13 17:06:39, robertm wrote: > > llvm -> llc > > Done. Actually, this comments sounds like it is for "__PNACL_GENERATED" instead of ResourceBaseUrl. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:214: // Signal that Pnacl translation failed. "from the translation thread" only?
mostly a bunch of request to be more noisy when there is an error. please also describe the "states" of the coordinator in the class comment. I think we pretty much have: LOAD_TRANSLATOR_BINARIES OPEN_LOCAL_FILE_SYSTEM OPEN_TMP_FOP_LLC_TO_LD_COMMUNICATION OPEN_TMP_FOR_LD_TO_SEL_LDR_COMMUNICATION PREPARE_PEXE_FOR_STREAMING START_LD_AND_LLC_SUBPROCESS_AND_INITIATE_TRANSLATION TRANSLATION_COMPLETE All this information is present in the individual callback comment but I think it is good to have an overview as well. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:244: file_system_.reset( add this to the ":" section and you wont need the reset (I think) http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:297: void PnaclCoordinator::PnaclFailed(const nacl::string& error_string) { rename to translateFailed or similar add comment: must be called from within the translatethread http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:388: callback_factory_.NewCallback(&PnaclCoordinator::PnaclDidFinish); PnaclDidFinish is not a good name, TranslateFinished would be better http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:394: const int32_t kArbitraryStackSize = 128 << 10; 128 * 1024 is less "clever". http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:403: NaClSubprocess* PnaclCoordinator::StartSubprocess(const nacl::string& url) { maybe rename url -> url_for_nexe http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:408: return NULL; error message? http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:415: Plugin* plugin = coordinator->plugin_; I the long it would probably best to move this thread into its own class. Besides a constructor, the class would have a simple interface like launch_thread() virtual lookup_file(...); virtual error(...) virtual complete(...) the class would not know about the coordinator the coordinator would subclass it and implement the virtual functions. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:439: NaClThreadExit(1); maybe printf http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:468: if (coordinator->SubprocessesShouldDie()) { add a PLUGIN_PRINTF http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:511: return; maybe also a add a PLUGIN_PRINTF here to make the error case stand out http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:70: coordinator_->ReportPpapiError(pp_error, maybe also add a PLUGIN_PRINTF() since the error code above might be overlooked
Thanks again for the reviews. PTAL. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:244: file_system_.reset( On 2011/12/13 22:12:51, robertm wrote: > add this to the ":" section and you wont need the reset (I think) Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:297: void PnaclCoordinator::PnaclFailed(const nacl::string& error_string) { On 2011/12/13 22:12:51, robertm wrote: > rename to translateFailed or similar > add comment: must be called from within the translatethread Name changed. Comment is in header. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:388: callback_factory_.NewCallback(&PnaclCoordinator::PnaclDidFinish); On 2011/12/13 22:12:51, robertm wrote: > PnaclDidFinish is not a good name, TranslateFinished would be better Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:394: const int32_t kArbitraryStackSize = 128 << 10; On 2011/12/13 22:12:51, robertm wrote: > 128 * 1024 is less "clever". Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:403: NaClSubprocess* PnaclCoordinator::StartSubprocess(const nacl::string& url) { On 2011/12/13 22:12:51, robertm wrote: > maybe rename url -> url_for_nexe Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:408: return NULL; On 2011/12/13 22:12:51, robertm wrote: > error message? Added a PLUGIN_PRINTF. Caller will invoke ReportPpapiError. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:415: Plugin* plugin = coordinator->plugin_; On 2011/12/13 22:12:51, robertm wrote: > > I the long it would probably best to move this thread into its own class. > > Besides a constructor, the class would have a simple interface like > launch_thread() > virtual lookup_file(...); > virtual error(...) > virtual complete(...) > > the class would not know about the coordinator > > the coordinator would subclass it and implement the virtual functions. As we discussed on the phone, I completely agree, but will take a TODO for this CL. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:439: NaClThreadExit(1); On 2011/12/13 22:12:51, robertm wrote: > maybe printf Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:468: if (coordinator->SubprocessesShouldDie()) { On 2011/12/13 22:12:51, robertm wrote: > add a PLUGIN_PRINTF Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:511: return; On 2011/12/13 22:12:51, robertm wrote: > maybe also a add a PLUGIN_PRINTF here to make the error case stand out Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:214: // Signal that Pnacl translation failed. On 2011/12/13 21:35:49, jvoung wrote: > "from the translation thread" only? Done. http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:70: coordinator_->ReportPpapiError(pp_error, On 2011/12/13 22:12:51, robertm wrote: > maybe also add a PLUGIN_PRINTF() since the error code above might be overlooked I added a print to the ReportPpapiError method so that all errors get reported this way.
LGTM Thanks, for hanging in there On 2011/12/14 16:34:19, sehr wrote: > Thanks again for the reviews. PTAL. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:244: > file_system_.reset( > On 2011/12/13 22:12:51, robertm wrote: > > add this to the ":" section and you wont need the reset (I think) > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:297: void > PnaclCoordinator::PnaclFailed(const nacl::string& error_string) { > On 2011/12/13 22:12:51, robertm wrote: > > rename to translateFailed or similar > > add comment: must be called from within the translatethread > > Name changed. Comment is in header. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:388: > callback_factory_.NewCallback(&PnaclCoordinator::PnaclDidFinish); > On 2011/12/13 22:12:51, robertm wrote: > > PnaclDidFinish is not a good name, TranslateFinished would be better > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:394: const int32_t > kArbitraryStackSize = 128 << 10; > On 2011/12/13 22:12:51, robertm wrote: > > 128 * 1024 is less "clever". > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:403: NaClSubprocess* > PnaclCoordinator::StartSubprocess(const nacl::string& url) { > On 2011/12/13 22:12:51, robertm wrote: > > maybe rename url -> url_for_nexe > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:408: return NULL; > On 2011/12/13 22:12:51, robertm wrote: > > error message? > > Added a PLUGIN_PRINTF. Caller will invoke ReportPpapiError. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:415: Plugin* plugin > = coordinator->plugin_; > On 2011/12/13 22:12:51, robertm wrote: > > > > I the long it would probably best to move this thread into its own class. > > > > Besides a constructor, the class would have a simple interface like > > launch_thread() > > virtual lookup_file(...); > > virtual error(...) > > virtual complete(...) > > > > the class would not know about the coordinator > > > > the coordinator would subclass it and implement the virtual functions. > > As we discussed on the phone, I completely agree, but will take a TODO for this > CL. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:439: > NaClThreadExit(1); > On 2011/12/13 22:12:51, robertm wrote: > > maybe printf > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:468: if > (coordinator->SubprocessesShouldDie()) { > On 2011/12/13 22:12:51, robertm wrote: > > add a PLUGIN_PRINTF > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:511: return; > On 2011/12/13 22:12:51, robertm wrote: > > maybe also a add a PLUGIN_PRINTF here to make the error case stand out > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:214: // Signal that > Pnacl translation failed. > On 2011/12/13 21:35:49, jvoung wrote: > > "from the translation thread" only? > > Done. > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:70: > coordinator_->ReportPpapiError(pp_error, > On 2011/12/13 22:12:51, robertm wrote: > > maybe also add a PLUGIN_PRINTF() since the error code above might be > overlooked > > I added a print to the ReportPpapiError method so that all errors get reported > this way.
On 2011/12/14 17:43:42, robertm wrote: > LGTM > > Thanks, for hanging in there > > On 2011/12/14 16:34:19, sehr wrote: > > Thanks again for the reviews. PTAL. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:244: > > file_system_.reset( > > On 2011/12/13 22:12:51, robertm wrote: > > > add this to the ":" section and you wont need the reset (I think) > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:297: void > > PnaclCoordinator::PnaclFailed(const nacl::string& error_string) { > > On 2011/12/13 22:12:51, robertm wrote: > > > rename to translateFailed or similar > > > add comment: must be called from within the translatethread > > > > Name changed. Comment is in header. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:388: > > callback_factory_.NewCallback(&PnaclCoordinator::PnaclDidFinish); > > On 2011/12/13 22:12:51, robertm wrote: > > > PnaclDidFinish is not a good name, TranslateFinished would be better > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:394: const int32_t > > kArbitraryStackSize = 128 << 10; > > On 2011/12/13 22:12:51, robertm wrote: > > > 128 * 1024 is less "clever". > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:403: > NaClSubprocess* > > PnaclCoordinator::StartSubprocess(const nacl::string& url) { > > On 2011/12/13 22:12:51, robertm wrote: > > > maybe rename url -> url_for_nexe > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:408: return NULL; > > On 2011/12/13 22:12:51, robertm wrote: > > > error message? > > > > Added a PLUGIN_PRINTF. Caller will invoke ReportPpapiError. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:415: Plugin* > plugin > > = coordinator->plugin_; > > On 2011/12/13 22:12:51, robertm wrote: > > > > > > I the long it would probably best to move this thread into its own class. > > > > > > Besides a constructor, the class would have a simple interface like > > > launch_thread() > > > virtual lookup_file(...); > > > virtual error(...) > > > virtual complete(...) > > > > > > the class would not know about the coordinator > > > > > > the coordinator would subclass it and implement the virtual functions. > > > > As we discussed on the phone, I completely agree, but will take a TODO for > this > > CL. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:439: > > NaClThreadExit(1); > > On 2011/12/13 22:12:51, robertm wrote: > > > maybe printf > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:468: if > > (coordinator->SubprocessesShouldDie()) { > > On 2011/12/13 22:12:51, robertm wrote: > > > add a PLUGIN_PRINTF > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:511: return; > > On 2011/12/13 22:12:51, robertm wrote: > > > maybe also a add a PLUGIN_PRINTF here to make the error case stand out > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:214: // Signal that > > Pnacl translation failed. > > On 2011/12/13 21:35:49, jvoung wrote: > > > "from the translation thread" only? > > > > Done. > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:70: > > coordinator_->ReportPpapiError(pp_error, > > On 2011/12/13 22:12:51, robertm wrote: > > > maybe also add a PLUGIN_PRINTF() since the error code above might be > > overlooked > > > > I added a print to the ReportPpapiError method so that all errors get reported > > this way. Thanks for the excellent review and for hanging in there with me also :-). Waiting for trybots.
Thanks again. Committed as r114473. On 2011/12/14 18:21:29, sehr wrote: > On 2011/12/14 17:43:42, robertm wrote: > > LGTM > > > > Thanks, for hanging in there > > > > On 2011/12/14 16:34:19, sehr wrote: > > > Thanks again for the reviews. PTAL. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right): > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:244: > > > file_system_.reset( > > > On 2011/12/13 22:12:51, robertm wrote: > > > > add this to the ":" section and you wont need the reset (I think) > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:297: void > > > PnaclCoordinator::PnaclFailed(const nacl::string& error_string) { > > > On 2011/12/13 22:12:51, robertm wrote: > > > > rename to translateFailed or similar > > > > add comment: must be called from within the translatethread > > > > > > Name changed. Comment is in header. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:388: > > > callback_factory_.NewCallback(&PnaclCoordinator::PnaclDidFinish); > > > On 2011/12/13 22:12:51, robertm wrote: > > > > PnaclDidFinish is not a good name, TranslateFinished would be better > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:394: const > int32_t > > > kArbitraryStackSize = 128 << 10; > > > On 2011/12/13 22:12:51, robertm wrote: > > > > 128 * 1024 is less "clever". > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:403: > > NaClSubprocess* > > > PnaclCoordinator::StartSubprocess(const nacl::string& url) { > > > On 2011/12/13 22:12:51, robertm wrote: > > > > maybe rename url -> url_for_nexe > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:408: return > NULL; > > > On 2011/12/13 22:12:51, robertm wrote: > > > > error message? > > > > > > Added a PLUGIN_PRINTF. Caller will invoke ReportPpapiError. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:415: Plugin* > > plugin > > > = coordinator->plugin_; > > > On 2011/12/13 22:12:51, robertm wrote: > > > > > > > > I the long it would probably best to move this thread into its own class. > > > > > > > > Besides a constructor, the class would have a simple interface like > > > > launch_thread() > > > > virtual lookup_file(...); > > > > virtual error(...) > > > > virtual complete(...) > > > > > > > > the class would not know about the coordinator > > > > > > > > the coordinator would subclass it and implement the virtual functions. > > > > > > As we discussed on the phone, I completely agree, but will take a TODO for > > this > > > CL. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:439: > > > NaClThreadExit(1); > > > On 2011/12/13 22:12:51, robertm wrote: > > > > maybe printf > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:468: if > > > (coordinator->SubprocessesShouldDie()) { > > > On 2011/12/13 22:12:51, robertm wrote: > > > > add a PLUGIN_PRINTF > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:511: return; > > > On 2011/12/13 22:12:51, robertm wrote: > > > > maybe also a add a PLUGIN_PRINTF here to make the error case stand out > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right): > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:214: // Signal > that > > > Pnacl translation failed. > > > On 2011/12/13 21:35:49, jvoung wrote: > > > > "from the translation thread" only? > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right): > > > > > > > > > http://codereview.chromium.org/8786005/diff/16001/ppapi/native_client/src/tru... > > > ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:70: > > > coordinator_->ReportPpapiError(pp_error, > > > On 2011/12/13 22:12:51, robertm wrote: > > > > maybe also add a PLUGIN_PRINTF() since the error code above might be > > > overlooked > > > > > > I added a print to the ReportPpapiError method so that all errors get > reported > > > this way. > > Thanks for the excellent review and for hanging in there with me also :-). > Waiting for trybots. |
