Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
467263006:
JavaScript Content Handler Version 0.0 (Closed)
|
Created:
6 years, 4 months ago by hansmuller Modified:
6 years, 3 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, ben+mojo_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionJavaScript Content Handler Version 0.0
BUG=403645
Committed: https://crrev.com/b2cf607c2035b942e36ceed49cc770cf15fb1db2
Cr-Commit-Position: refs/heads/master@{#294621}
Patch Set 1 : Use chromium application runner #
Total comments: 21
Patch Set 2 : Changes per review feedback #
Total comments: 2
Patch Set 3 : Working and relatively complete #
Total comments: 55
Patch Set 4 : Changes per review feedback. #
Total comments: 10
Patch Set 5 : Changes per review feedback round 2 #Patch Set 6 : Integrated isolate_holder changes, replaced main.js #Patch Set 7 : Use ScopedVector<JSApp> instead of std::vector<scoped_ptr<JSApp> > #Patch Set 8 : Added ICU build dependency #Patch Set 9 : #Patch Set 10 : #
Total comments: 1
Messages
Total messages: 37 (6 generated)
https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc File mojo/apps/js/main.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:91: class ContentHandlerApp : public ApplicationDelegate { Nit: the naming of this kind of confused me... I think it should be like JSApp or something. (actually: I think we should just rename the entire directory to 'bravo' or something, so that we have a distinct name for this style of js-baesd mojo application, but that can be separate). https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:134: // TODO(hansmuller): is this the right place to block and load the I agree with your comment that it would be better to not do a blocking read. Otherwise, launch of multiple JS apps could be serialized on the network. However, I think that's fine to improve in a separate change --- if changing module loading to be async would be more work. https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:139: content_handler_app_->LoadMainJSModule(module, path); Remember that OnConnect() can get called multiple times, each with a different js app that should be run. You should be constructing a new object to encapsulate each connection. See, e.g., PNGView in my CL. https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:19: bool BlockingCopyHelper(ScopedDataPipeConsumerHandle source, I think the callback-based refactor makes sense, but you should get Darin's opinion too. https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:69: if (result) Is there a reason to not pass |result|, or is this defensive programming? If it is defensive, don't be... just let it crash. Better to crash than carry on in a weird state. http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:85: void CompleteBlockingCopyToFile(const base::Callback<void(bool)>& callback, Yeah, looks like dead code.
darin@chromium.org changed reviewers: + darin@chromium.org
https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc File mojo/apps/js/main.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:115: base::MessageLoop::current()->PostTask(FROM_HERE, why do you need to use PostTask here? https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:51: size_t CopyToStringHelper( nit: Append{Bytes}ToString ? https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:58: size_t CopyToFileHelper(FILE* fp, const void* buffer, uint32_t num_bytes) { nit: Append{Bytes}ToFile ? https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.h (right): https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.h:31: bool MOJO_COMMON_EXPORT BlockingCopyToString( nit: provide a comment block here, explaining return type at least.
I've addressed most of the feedback. The next step is to graduate from "Hello World" to doing the Mojo. https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc File mojo/apps/js/main.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:91: class ContentHandlerApp : public ApplicationDelegate { On 2014/08/25 17:25:37, Aaron Boodman wrote: > Nit: the naming of this kind of confused me... I think it should be like JSApp > or something. > > (actually: I think we should just rename the entire directory to 'bravo' or > something, so that we have a distinct name for this style of js-baesd mojo > application, but that can be separate). I've renamed the class JSApp. It bugs me that the thing we're calling the application is explicitly an ApplicationDelegate however I agree that JSApp is better than ContentHandlerApp. https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:115: base::MessageLoop::current()->PostTask(FROM_HERE, On 2014/08/26 05:23:22, darin wrote: > why do you need to use PostTask here? It's not necessary to use PostTask. I've removed it. https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:134: // TODO(hansmuller): is this the right place to block and load the On 2014/08/25 17:25:37, Aaron Boodman wrote: > I agree with your comment that it would be better to not do a blocking read. > Otherwise, launch of multiple JS apps could be serialized on the network. > > However, I think that's fine to improve in a separate change --- if changing > module loading to be async would be more work. OK, I'll add async loading of JS modules in a subsequent CL. https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:139: content_handler_app_->LoadMainJSModule(module, path); On 2014/08/25 17:25:37, Aaron Boodman wrote: > Remember that OnConnect() can get called multiple times, each with a different > js app that should be run. You should be constructing a new object to > encapsulate each connection. See, e.g., PNGView in my CL. It looks like I should to wait for your CL (https://codereview.chromium.org/489493004) to land to do this? https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:51: size_t CopyToStringHelper( On 2014/08/26 05:23:22, darin wrote: > nit: Append{Bytes}ToString ? I thought there was some value in repeating most of the name of the function being helped; so CopyToStringHelper() goes with BlockingCopyToString(). If you think AppendBytesToString() is clearer, I'll be happy to change it. Likewise for CopyToFileHelper(). https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:58: size_t CopyToFileHelper(FILE* fp, const void* buffer, uint32_t num_bytes) { On 2014/08/26 05:23:23, darin wrote: > nit: Append{Bytes}ToFile ? [see previous comment] https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:69: if (result) On 2014/08/25 17:25:38, Aaron Boodman wrote: > Is there a reason to not pass |result|, or is this defensive programming? If it > is defensive, don't be... just let it crash. Better to crash than carry on in a > weird state. > > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- I was emulating base::ReadFileToString() which also uses a NULL return parameter to indicate that the input is to be read and discarded. I added a comment to the header file to explain the function's semantics a little better. https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:85: void CompleteBlockingCopyToFile(const base::Callback<void(bool)>& callback, On 2014/08/25 17:25:38, Aaron Boodman wrote: > Yeah, looks like dead code. OK, I deleted it. https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.h (right): https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.h:31: bool MOJO_COMMON_EXPORT BlockingCopyToString( On 2014/08/26 05:23:23, darin wrote: > nit: provide a comment block here, explaining return type at least. Done.
https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc File mojo/apps/js/main.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/apps/js/main.cc#new... mojo/apps/js/main.cc:139: content_handler_app_->LoadMainJSModule(module, path); On 2014/08/26 23:39:12, hansmuller wrote: > On 2014/08/25 17:25:37, Aaron Boodman wrote: > > Remember that OnConnect() can get called multiple times, each with a different > > js app that should be run. You should be constructing a new object to > > encapsulate each connection. See, e.g., PNGView in my CL. > > It looks like I should to wait for your CL > (https://codereview.chromium.org/489493004) to land to do this? I think that you could structure the code to have an object that owns each connection now. The only things my CL adds are: - changes the signature to OnConnect so that it works properly for large responses - hooks up viewmanager and window manager so that we can actually navigate to these things https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.cc (right): https://codereview.chromium.org/467263006/diff/40001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.cc:69: if (result) On 2014/08/26 23:39:12, hansmuller wrote: > On 2014/08/25 17:25:38, Aaron Boodman wrote: > > Is there a reason to not pass |result|, or is this defensive programming? If > it > > is defensive, don't be... just let it crash. Better to crash than carry on in > a > > weird state. > > > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- > > I was emulating base::ReadFileToString() which also uses a NULL return > parameter to indicate that the input is to be read and discarded. I added a > comment to the header file to explain the function's semantics a little better. OK, that functions gives the example of priming a disk cache as a reason you might want to read and discard. Would that example apply here too? I guess... Are there any better examples for why you might want to do this? It seems a little unfortunate to me to have this bear trap here for such a teeny potential use case. Crashing on NULL seems like what you want to happen most of the time. But... I defer to Darin.
On 2014/08/27 05:55:53, Aaron Boodman wrote: ... > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- > > > > I was emulating base::ReadFileToString() which also uses a NULL return > > parameter to indicate that the input is to be read and discarded. I added a > > comment to the header file to explain the function's semantics a little > better. > > OK, that functions gives the example of priming a disk cache as a reason you > might want to read and discard. Would that example apply here too? I guess... > Are there any better examples for why you might want to do this? > > It seems a little unfortunate to me to have this bear trap here for such a teeny > potential use case. Crashing on NULL seems like what you want to happen most of > the time. > > But... I defer to Darin. I didn't have a use case in mind, I was really just aping ReadFuileToString(). I'll add a CHECK() and tube the special case.
https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.h (right): https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.h:34: // If |contents| is NULL, the data is read and discarded. You should document the return value.
On 2014/08/27 21:30:10, darin wrote: > https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... > File mojo/common/data_pipe_utils.h (right): > > https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... > mojo/common/data_pipe_utils.h:34: // If |contents| is NULL, the data is read and > discarded. > You should document the return value. I thought "returns true on success and false on error" covered that (note also: I will remove the special case for NULL |contents|, https://codereview.chromium.org/467263006/#msg6).
https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... File mojo/common/data_pipe_utils.h (right): https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... mojo/common/data_pipe_utils.h:34: // If |contents| is NULL, the data is read and discarded. On 2014/08/27 21:30:10, darin wrote: > You should document the return value. Sorry, my eyes were failing me... what confused me was the "In case of I/O error" bit. Can this function return false and leave |contents| non-empty?
On 2014/08/27 21:51:04, darin wrote: > https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... > File mojo/common/data_pipe_utils.h (right): > > https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... > mojo/common/data_pipe_utils.h:34: // If |contents| is NULL, the data is read and > discarded. > On 2014/08/27 21:30:10, darin wrote: > > You should document the return value. > > Sorry, my eyes were failing me... what confused me was the "In case of I/O > error" bit. Can this function return false and leave |contents| non-empty? Yes, |contents| holds the data that was read before the I/O error occurred.
On 2014/08/27 22:01:45, hansmuller wrote: > On 2014/08/27 21:51:04, darin wrote: > > > https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... > > File mojo/common/data_pipe_utils.h (right): > > > > > https://codereview.chromium.org/467263006/diff/60001/mojo/common/data_pipe_ut... > > mojo/common/data_pipe_utils.h:34: // If |contents| is NULL, the data is read > and > > discarded. > > On 2014/08/27 21:30:10, darin wrote: > > > You should document the return value. > > > > Sorry, my eyes were failing me... what confused me was the "In case of I/O > > error" bit. Can this function return false and leave |contents| non-empty? > > Yes, |contents| holds the data that was read before the I/O error occurred. OK, I see.
I've uploaded a version of the JS content handler that manages multiple JS app threads and is integrated with JS via a global builtin "mojo" module, rather than a function created by the initial JS module. Because of the necessary threading and lifetime management issues, this CL is somewhat complicated. I'm going to upload another version with a more complete overall description as well as in-line comments this afternoon. This is just version zero. There are many improvements I'd like to make on both the C++ and JS sides. I'll be making that list this afternoon as well.
Exciting! Here is a first pass of comments... I tried to focus on structural things first. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc File mojo/apps/js/js_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:17: AddBuiltinModule(Mojo::kModuleName, Mojo::GetModule); So there is a lot of legwork in here to route these JS methods to right instance of JSApp. Gin already includes this kind of functionality, so you don't need to re-implement it. The following call: object_template_builder.SetMethod("connect", base::Bind(&JSApp::ConnectToService, js_app)) ... will route the JS method "connect" to the ConnectToService method of js_app. This will also allow you to get rid of kEmbedderMojo and the mojo_module.* files. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:21: const std::string& url, You should run `git cl format` on this entire patch. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:26: thread_("Mojo JS " + url), Is there a naming convention for threads in Mojo or Chrome? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:107: thread_.message_loop()->PostTask(FROM_HERE, This PostTask() doesn't seem necessary. We're posting to the same thread we're already on, no? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h File mojo/apps/js/js_app.h (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h#ne... mojo/apps/js/js_app.h:26: class JSApp : public base::RefCountedThreadSafe<JSApp> { RefCounting can make understanding code harder because it becomes unclear when, and on what thread, a class will actually be destroyed. I think you can get away without it if you have JSApp manage its own lifetime as mentioned elsewhere in these comments. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h#ne... mojo/apps/js/js_app.h:26: class JSApp : public base::RefCountedThreadSafe<JSApp> { In general it is preferred that every class have a class header comment that explains in a paragraph or so the 'big picture' of where the class fits in the design, what its responsibility is, etc. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h#ne... mojo/apps/js/js_app.h:57: friend class base::RefCountedThreadSafe<JSApp>; decl order is incorrect -- see reference to styleguide elsewhere in comments. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... File mojo/apps/js/js_content_handler_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:31: apps_.push_back(app); There does not seem to be any instance of JSContentHandlerApp using this pointer to JSApp... perhaps it should not be stored in the first place? JSApp could perhaps manage its own lifetime. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:42: void JSContentHandlerApp::ConnectToService(ScopedMessagePipeHandle pipe_handle1, Since there is only one pipe handle here, you don't need the 1. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:46: ServiceProvider *service_provider = applicationImpl_->ConnectToApplication( Instead of saving the ptr to ApplicationImpl*, save the ptr to ApplicationImpl::shell(). Darin recommended in a recent CL that we eventually remove ApplicationImpl::ConnectToApplication(), and it makes sense to me. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:62: content_handler_app_->StartJSApp(url.To<std::string>(), content.Pass()); I'm not sure this is the right lifetime for these JSApp instances. Right now they will never die unless they themselves call quit(). In the PNGViewer, they go away when the view hosting them goes away. Probably neither of those is correct. One way to think about it is that the client called Shell::ConnectToApplication and passed a service provider, presumably so it could get or provide some services. So maybe the JSApp should go away when that service provider connection goes away... Or maybe it should go away when all services vended by that service provider have gone away... Darin? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... File mojo/apps/js/js_content_handler_app.h (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:5: #ifndef JS_CONTENT_HANDLER_H_ See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_G... https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:8: #include "base/bind.h" Is this used? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:11: #include "mojo/public/cpp/application/application_impl.h" I think you can forward declare ApplicationImpl to avoid this include. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...). https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:21: class JSContentHandlerImpl : public InterfaceImpl<ContentHandler> { Is there any reason for the "JS" prefix? Why not just ContentHandlerImpl? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:35: class JSContentHandlerApp : I think it is confusing to call this ContentHandlerApp in the presence of the ContentHandler mojo interface, and ContentHandlerImpl. I know there are a lot of "apps" in this stack of abstractions ... Maybe fall back on the interface names for help? ApplicationDelegateImpl, ContentHandlerImpl, etc? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:41: virtual void Initialize(ApplicationImpl* app) MOJO_OVERRIDE; Add a comment like: // ApplicationDelegate methods Also, consider making these interface implementations private if they are not actually being called except for via the interface. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:52: typedef std::vector<scoped_refptr<JSApp> > Apps; We would typically call this type AppVector, but see comments elsewhere about not having this. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:54: ApplicationImpl* applicationImpl_; application_impl_, but see comments about saving Shell* instead. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:59: virtual ~JSContentHandlerApp(); The ordering is incorrect. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module.cc File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:22: std::string CopyToString(const gin::Arguments& args, Handle source) { You don't seem to be using |args|. Remove? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:28: ? result : "Fail"; You could also throw a JS exception. Throwing an exception is a reasonably JavaScripty behavior here. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:45: gin::WrapperInfo g_wrapper_info = { gin::kEmbedderNativeGin }; Should this be gin::kEmbedderMojo? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:59: .SetMethod("copyToString", CopyToString) Isn't readData() sufficient? https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin... https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:60: .SetMethod("quit", Quit) It seems like https://code.google.com/p/chromium/codesearch#chromium/src/mojo/apps/js/bindi... could work here when combined with my other suggestion that a JSApp could manage its own lifetime. The threading module would tell the current message loop to quit, and then the code after the message loop returns would delete JSApp.
https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module.cc File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:45: gin::WrapperInfo g_wrapper_info = { gin::kEmbedderNativeGin }; On 2014/09/10 02:22:10, Aaron Boodman wrote: > Should this be gin::kEmbedderMojo? Whoops I meant to delete this comment... it of course is irrelevant with the changes I suggested to how you are using ObjectTemplateBuilder.
I've made nearly all the changes you suggested. There no more ref counted classes and I'm no longer using v8 to keep track of the mapping from isolates to JSApps. The vector of scoped_refptr<JSApp> still exists and still defines the lifetime of JSApps, but without the ref counting stuff I think it's clearer. I've submitted to git cl format however I still haven't included helpful overviews atop each class declaration. I've also renamed all of the classes in the way you suggested. I've got blisters on my fingers. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc File mojo/apps/js/js_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:17: AddBuiltinModule(Mojo::kModuleName, Mojo::GetModule); On 2014/09/10 02:22:09, Aaron Boodman wrote: > So there is a lot of legwork in here to route these JS methods to right instance > of JSApp. Gin already includes this kind of functionality, so you don't need to > re-implement it. > > The following call: > > object_template_builder.SetMethod("connect", > base::Bind(&JSApp::ConnectToService, js_app)) > > ... will route the JS method "connect" to the ConnectToService method of js_app. > > This will also allow you to get rid of kEmbedderMojo and the mojo_module.* > files. I've updated gin::ModuleRunnerDelegate() as we discussed: the elements of the list of not-yet-loaded modules are now Callbacks, instead of ModuelGetter functions. This made it possible to defined the "mojo" builtin module with a Callback that captures the corresponding JSApp instance. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:26: thread_("Mojo JS " + url), On 2014/09/10 02:22:09, Aaron Boodman wrote: > Is there a naming convention for threads in Mojo or Chrome? I haven't observed one, but I'll ask around. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:107: thread_.message_loop()->PostTask(FROM_HERE, On 2014/09/10 02:22:08, Aaron Boodman wrote: > This PostTask() doesn't seem necessary. We're posting to the same thread we're > already on, no? Sorry, I should have included a comment to explain what's going on. I'm posting a task instead of just doing the work directly so that the shell_runner isn't destroyed while we're still inside the JavaScript mojo.quit() function. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h File mojo/apps/js/js_app.h (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h#ne... mojo/apps/js/js_app.h:26: class JSApp : public base::RefCountedThreadSafe<JSApp> { On 2014/09/10 02:22:09, Aaron Boodman wrote: > In general it is preferred that every class have a class header comment that > explains in a paragraph or so the 'big picture' of where the class fits in the > design, what its responsibility is, etc. OK https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.h#ne... mojo/apps/js/js_app.h:57: friend class base::RefCountedThreadSafe<JSApp>; On 2014/09/10 02:22:09, Aaron Boodman wrote: > decl order is incorrect -- see reference to styleguide elsewhere in comments. I've removed the RefCountedThreadSafe boilerplate here. I think that's what failed style. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... File mojo/apps/js/js_content_handler_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:31: apps_.push_back(app); On 2014/09/10 02:22:09, Aaron Boodman wrote: > There does not seem to be any instance of JSContentHandlerApp using this pointer > to JSApp... perhaps it should not be stored in the first place? JSApp could > perhaps manage its own lifetime. At the moment the apps_ list entries defined the lifetimes of their JSApp values. I've removed the ref counting stuff from JSApp, so this is it. The QuitJSApp() method destroys a JSApp when it "erases" its list element. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:42: void JSContentHandlerApp::ConnectToService(ScopedMessagePipeHandle pipe_handle1, On 2014/09/10 02:22:09, Aaron Boodman wrote: > Since there is only one pipe handle here, you don't need the 1. That was just a reminder that the caller was holding on to pipe.handle0. But maybe the suffix number is confusing. I'll remove it. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:46: ServiceProvider *service_provider = applicationImpl_->ConnectToApplication( On 2014/09/10 02:22:09, Aaron Boodman wrote: > Instead of saving the ptr to ApplicationImpl*, save the ptr to > ApplicationImpl::shell(). Darin recommended in a recent CL that we eventually > remove ApplicationImpl::ConnectToApplication(), and it makes sense to me. ApplicationImpl::ConnectToApplication() updates an internal service registry after calling shell()->ConnectToApplication(). Do I need to do that somehow? https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... File mojo/apps/js/js_content_handler_app.h (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:5: #ifndef JS_CONTENT_HANDLER_H_ On 2014/09/10 02:22:09, Aaron Boodman wrote: > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_G... Done. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:8: #include "base/bind.h" On 2014/09/10 02:22:09, Aaron Boodman wrote: > Is this used? No, I've removed it. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:11: #include "mojo/public/cpp/application/application_impl.h" On 2014/09/10 02:22:10, Aaron Boodman wrote: > I think you can forward declare ApplicationImpl to avoid this include. > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...). Good point, I've forwarded it. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:21: class JSContentHandlerImpl : public InterfaceImpl<ContentHandler> { On 2014/09/10 02:22:09, Aaron Boodman wrote: > Is there any reason for the "JS" prefix? Why not just ContentHandlerImpl? I thought the prefix was warranted since this was a content handler for JavaScript. I don't mind removing the prefix however if we have a bunch of classes all called ContentHandlerImpl (and etc) it may become confusing. The fact there are two files called core.cc already confuses emacs gdb-mode :-). https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:35: class JSContentHandlerApp : On 2014/09/10 02:22:09, Aaron Boodman wrote: > I think it is confusing to call this ContentHandlerApp in the presence of the > ContentHandler mojo interface, and ContentHandlerImpl. > > I know there are a lot of "apps" in this stack of abstractions ... Maybe fall > back on the interface names for help? ApplicationDelegateImpl, > ContentHandlerImpl, etc? OK, I'll remove the JS prefixes I agree that the profusion of things called application has pretty much drained any meaning from the term. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:41: virtual void Initialize(ApplicationImpl* app) MOJO_OVERRIDE; On 2014/09/10 02:22:09, Aaron Boodman wrote: > Add a comment like: > > // ApplicationDelegate methods > > Also, consider making these interface implementations private if they are not > actually being called except for via the interface. Done. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:52: typedef std::vector<scoped_refptr<JSApp> > Apps; On 2014/09/10 02:22:09, Aaron Boodman wrote: > We would typically call this type AppVector, but see comments elsewhere about > not having this. I've changed the type to AppVector since we haven't decided on another way to manage the lifetime of JSApps yet. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.h:59: virtual ~JSContentHandlerApp(); On 2014/09/10 02:22:09, Aaron Boodman wrote: > The ordering is incorrect. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module.cc File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:22: std::string CopyToString(const gin::Arguments& args, Handle source) { On 2014/09/10 02:22:10, Aaron Boodman wrote: > You don't seem to be using |args|. Remove? Done. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:45: gin::WrapperInfo g_wrapper_info = { gin::kEmbedderNativeGin }; On 2014/09/10 17:39:17, Aaron Boodman wrote: > On 2014/09/10 02:22:10, Aaron Boodman wrote: > > Should this be gin::kEmbedderMojo? > > Whoops I meant to delete this comment... it of course is irrelevant with the > changes I suggested to how you are using ObjectTemplateBuilder. Correct. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:59: .SetMethod("copyToString", CopyToString) On 2014/09/10 02:22:10, Aaron Boodman wrote: > Isn't readData() sufficient? > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin... > The core readData() function just hang in my case. Maybe it should work like BlockingCopyHelper() in data_pipe_utils.h
https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc File mojo/apps/js/js_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:84: // The lifetime of the this JSApp is managed by the content_handler_app_. I understand you're trying to explain the Unretained(this), but I think having this comment at every PostTask() is not worth the noise. Recommend removing it here and elsewhere. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:94: // content_handler_app_ to drop its reference to this app, which implicitly Since this class is no longer ref-counted "drop its reference" is incorrect. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:107: thread_.message_loop()->PostTask(FROM_HERE, On 2014/09/11 00:26:16, hansmuller wrote: > On 2014/09/10 02:22:08, Aaron Boodman wrote: > > This PostTask() doesn't seem necessary. We're posting to the same thread we're > > already on, no? > Sorry, I should have included a comment to explain what's going on. I'm posting > a task instead of just doing the work directly so that the shell_runner isn't > destroyed while we're still inside the JavaScript mojo.quit() function. I see. I was thinking that since the only reason that ApplicationDelegateImpl maintains a pointer to JSApp is so that JSApp can tell it "hey, delete me now please", then JSApp might as well just delete itself. So basically, the AppVector would go away and Quit would turn into thread_.message_loop()->DeleteSoon(FROM_HERE, this). But I can see the argument that ApplicationDelegateImpl should really own these things, too. So your way is fine. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... File mojo/apps/js/js_content_handler_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:46: ServiceProvider *service_provider = applicationImpl_->ConnectToApplication( On 2014/09/11 00:26:17, hansmuller wrote: > On 2014/09/10 02:22:09, Aaron Boodman wrote: > > Instead of saving the ptr to ApplicationImpl*, save the ptr to > > ApplicationImpl::shell(). Darin recommended in a recent CL that we eventually > > remove ApplicationImpl::ConnectToApplication(), and it makes sense to me. > > ApplicationImpl::ConnectToApplication() updates an internal service registry > after calling shell()->ConnectToApplication(). Do I need to do that somehow? Huh. I'm not really sure. I guess this can be part of the "figure out lifetime" that I mentioned in the other comment. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_content_... mojo/apps/js/js_content_handler_app.cc:62: content_handler_app_->StartJSApp(url.To<std::string>(), content.Pass()); On 2014/09/10 02:22:09, Aaron Boodman wrote: > I'm not sure this is the right lifetime for these JSApp instances. Right now > they will never die unless they themselves call quit(). In the PNGViewer, they > go away when the view hosting them goes away. Probably neither of those is > correct. > > One way to think about it is that the client called Shell::ConnectToApplication > and passed a service provider, presumably so it could get or provide some > services. So maybe the JSApp should go away when that service provider > connection goes away... Or maybe it should go away when all services vended by > that service provider have gone away... > > Darin? We still need to figure this out. I suppose in can be a subsequent CL though. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module.cc File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:59: .SetMethod("copyToString", CopyToString) On 2014/09/11 00:26:18, hansmuller wrote: > On 2014/09/10 02:22:10, Aaron Boodman wrote: > > Isn't readData() sufficient? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin... > > > > The core readData() function just hang in my case. Maybe it should work like > BlockingCopyHelper() in data_pipe_utils.h I'm sorry... I don't follow. It seems like we should have sufficient primitives in core.js to read from a pipe into a string. What is missing? https://codereview.chromium.org/467263006/diff/100001/gin/modules/module_runn... File gin/modules/module_runner_delegate.cc (right): https://codereview.chromium.org/467263006/diff/100001/gin/modules/module_runn... gin/modules/module_runner_delegate.cc:34: builtin_modules_[id] = base::Bind(CallModuleGetter, base::Unretained(getter)); It's not possible to just do: base::Bind(ModuleGetter) ? https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... File mojo/apps/js/content_handler.cc (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... mojo/apps/js/content_handler.cc:57: : content_handler_(app) { Nit: since ContentHandlerImpl is declared first in the .h file, it would be nice to define it first in the .cc file. https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... File mojo/apps/js/content_handler.h (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... mojo/apps/js/content_handler.h:5: #ifndef MOJO_APPS_JS_CONTENT_HANDLER_H_ I think this file should be called application_delegate_impl.h, since ApplicationDelegateImpl is the lowest part of the stack that is contained here. You could see ContentHandlerImpl as an implementation detail of ApplicationDelegateImpl, but the reverse is not really true. Also, more of the code in the cc file is actually implementing ApplicationDelegateImpl than ContentHandlerImpl. Alternately, you could just split ContentHandlerImpl out into its own file, which will probably make sense as this all grows bigger anyway. https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/js_app.h File mojo/apps/js/js_app.h (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/js_app.h#n... mojo/apps/js/js_app.h:25: class JSAppRunnerDelegate : public MojoRunnerDelegate { Why subclass MojoRunnerDelegate? It looks like all you are using is AddBuiltinModule() and that is public, so JSApp could just create an instance of MojoRunnerDelegate and call that method. https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/mojo_modul... File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/mojo_modul... mojo/apps/js/mojo_module.cc:39: const char Mojo::kModuleName[] = "mojo"; This is not used.
I've made all of the suggested changes. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc File mojo/apps/js/js_app.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:84: // The lifetime of the this JSApp is managed by the content_handler_app_. On 2014/09/11 05:24:52, Aaron Boodman wrote: > I understand you're trying to explain the Unretained(this), but I think having > this comment at every PostTask() is not worth the noise. Recommend removing it > here and elsewhere. Done. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/js_app.cc#n... mojo/apps/js/js_app.cc:94: // content_handler_app_ to drop its reference to this app, which implicitly On 2014/09/11 05:24:53, Aaron Boodman wrote: > Since this class is no longer ref-counted "drop its reference" is incorrect. I'll reword this. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module.cc File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:28: ? result : "Fail"; On 2014/09/10 02:22:10, Aaron Boodman wrote: > You could also throw a JS exception. Throwing an exception is a reasonably > JavaScripty behavior here. Done. https://codereview.chromium.org/467263006/diff/80001/mojo/apps/js/mojo_module... mojo/apps/js/mojo_module.cc:59: .SetMethod("copyToString", CopyToString) On 2014/09/11 05:24:53, Aaron Boodman wrote: > On 2014/09/11 00:26:18, hansmuller wrote: > > On 2014/09/10 02:22:10, Aaron Boodman wrote: > > > Isn't readData() sufficient? > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin... > > > > > > > The core readData() function just hang in my case. Maybe it should work like > > BlockingCopyHelper() in data_pipe_utils.h > > I'm sorry... I don't follow. It seems like we should have sufficient primitives > in core.js to read from a pipe into a string. What is missing? I was mistaken about the hang. The core readData() function is good enough to demonstrate that things are working. I've removed the mojo copyToString() method (and I agree that it didn't seem to belong anyway). As I understand it, core.readData() reads as many bytes as are currently available in the pipe. If it's OK with you, I'd like to add another core method, that reads all available data into a string (and throws an exception if the read fails) in a separate CL. https://codereview.chromium.org/467263006/diff/100001/gin/modules/module_runn... File gin/modules/module_runner_delegate.cc (right): https://codereview.chromium.org/467263006/diff/100001/gin/modules/module_runn... gin/modules/module_runner_delegate.cc:34: builtin_modules_[id] = base::Bind(CallModuleGetter, base::Unretained(getter)); On 2014/09/11 05:24:53, Aaron Boodman wrote: > It's not possible to just do: > > base::Bind(ModuleGetter) ? Sorry about that, I didn't need to insert the trampoline function. I've removed it. https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... File mojo/apps/js/content_handler.cc (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... mojo/apps/js/content_handler.cc:57: : content_handler_(app) { On 2014/09/11 05:24:53, Aaron Boodman wrote: > Nit: since ContentHandlerImpl is declared first in the .h file, it would be nice > to define it first in the .cc file. Done. https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... File mojo/apps/js/content_handler.h (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/content_ha... mojo/apps/js/content_handler.h:5: #ifndef MOJO_APPS_JS_CONTENT_HANDLER_H_ On 2014/09/11 05:24:53, Aaron Boodman wrote: > I think this file should be called application_delegate_impl.h, since > ApplicationDelegateImpl is the lowest part of the stack that is contained here. > > You could see ContentHandlerImpl as an implementation detail of > ApplicationDelegateImpl, but the reverse is not really true. > > Also, more of the code in the cc file is actually implementing > ApplicationDelegateImpl than ContentHandlerImpl. > > Alternately, you could just split ContentHandlerImpl out into its own file, > which will probably make sense as this all grows bigger anyway. I've renamed the file and will split out ContentHandlerImpl as soon as it gets bigger. https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/js_app.h File mojo/apps/js/js_app.h (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/js_app.h#n... mojo/apps/js/js_app.h:25: class JSAppRunnerDelegate : public MojoRunnerDelegate { On 2014/09/11 05:24:53, Aaron Boodman wrote: > Why subclass MojoRunnerDelegate? It looks like all you are using is > AddBuiltinModule() and that is public, so JSApp could just create an instance of > MojoRunnerDelegate and call that method. Good point, I've eliminated the JSAppRunnerDelegate class.l https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/mojo_modul... File mojo/apps/js/mojo_module.cc (right): https://codereview.chromium.org/467263006/diff/100001/mojo/apps/js/mojo_modul... mojo/apps/js/mojo_module.cc:39: const char Mojo::kModuleName[] = "mojo"; On 2014/09/11 05:24:53, Aaron Boodman wrote: > This is not used. It's used by the mojo AddBuiltinModule() call, which is now in the JSApp constructor body.
Last thing... can you remove main.js and check in your test files in its place?
Sync'd with Jochen's isolate_holder change (https://codereview.chromium.org/553903003) and replaced apps/js/main.js with a working content handler demo.
On 2014/09/11 22:37:43, hansmuller wrote: > Sync'd with Jochen's isolate_holder change > (https://codereview.chromium.org/553903003) and replaced apps/js/main.js with a > working content handler demo. Lgtm
The CQ bit was checked by hansmuller@chromium.org
I defer to aa@ On Sep 11, 2014 4:34 PM, <aa@chromium.org> wrote: > On 2014/09/11 22:37:43, hansmuller wrote: > >> Sync'd with Jochen's isolate_holder change >> (https://codereview.chromium.org/553903003) and replaced apps/js/main.js >> with >> > a > >> working content handler demo. >> > > Lgtm > > https://codereview.chromium.org/467263006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/467263006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hansmuller@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/467263006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hansmuller@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/467263006/220001
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as 07755bb656c00e0e849fdbe8ebcc07f48b6da817
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b2cf607c2035b942e36ceed49cc770cf15fb1db2 Cr-Commit-Position: refs/heads/master@{#294621}
Message was sent while issue was closed.
Patchset #11 (id:240001) has been deleted
Message was sent while issue was closed.
https://codereview.chromium.org/467263006/diff/220001/mojo/apps/js/js_app.cc File mojo/apps/js/js_app.cc (right): https://codereview.chromium.org/467263006/diff/220001/mojo/apps/js/js_app.cc#... mojo/apps/js/js_app.cc:69: return pipe.handle0.release(); Little cleanup I realized on the train this morning: Looks a bit weird to release the scoped handled here. Why not add a converter for ScopedHandle, so that you can return pipe.handle0.Pass() instead?
Message was sent while issue was closed.
On 2014/09/15 16:57:44, Aaron Boodman wrote: > https://codereview.chromium.org/467263006/diff/220001/mojo/apps/js/js_app.cc > File mojo/apps/js/js_app.cc (right): > > https://codereview.chromium.org/467263006/diff/220001/mojo/apps/js/js_app.cc#... > mojo/apps/js/js_app.cc:69: return pipe.handle0.release(); > Little cleanup I realized on the train this morning: > > Looks a bit weird to release the scoped handled here. Why not add a converter > for ScopedHandle, so that you can return pipe.handle0.Pass() instead? So the the converter for ScopedHandle => Handle would do the release(), that's right? I'm just fixing another problem I didn't catch with this CL - https://code.google.com/p/chromium/issues/detail?id=414338. I can include this change as well.
Message was sent while issue was closed.
On 2014/09/15 17:30:24, hansmuller wrote: > On 2014/09/15 16:57:44, Aaron Boodman wrote: > > https://codereview.chromium.org/467263006/diff/220001/mojo/apps/js/js_app.cc > > File mojo/apps/js/js_app.cc (right): > > > > > https://codereview.chromium.org/467263006/diff/220001/mojo/apps/js/js_app.cc#... > > mojo/apps/js/js_app.cc:69: return pipe.handle0.release(); > > Little cleanup I realized on the train this morning: > > > > Looks a bit weird to release the scoped handled here. Why not add a converter > > for ScopedHandle, so that you can return pipe.handle0.Pass() instead? > > So the the converter for ScopedHandle => Handle would do the release(), that's > right? I meant a gin::Converter, like this one: https://code.google.com/p/chromium/codesearch#chromium/src/mojo/bindings/js/h... ... for converting C++ values to v8 values. Yes, it would just release. > I'm just fixing another problem I didn't catch with this CL - > https://code.google.com/p/chromium/issues/detail?id=414338. I can include this > change as well. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
