Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
61063003:
Keep NaCl plugins used in app background pages alive when active. (Closed)
|
Created:
7 years, 1 month ago by scheib Modified:
7 years ago CC:
chromium-reviews, native-client-reviews_googlegroups.com, sadrul, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, dmichael (off chromium) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionKeep NaCl plugins used in app background pages alive when active.
Activity in Native Client plugins results in IPC messages
sent to the BrowserPpapiHostImpl and routed to call
extensions::ProcessManager::KeepaliveImpulse.
Implementation patch, to be followed by tests. See:
https://codereview.chromium.org/111563006/ Tests.
https://codereview.chromium.org/105873003/ Cumulative patch.
Design doc:
https://docs.google.com/a/chromium.org/document/d/1mI0lS1rfAf-BAGLmWAEcWy37Xq9dOvgfMx8OqeUMXts/edit#
BUG=298339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241702
Patch Set 1 : working prototype #
Total comments: 36
Patch Set 2 : fix ref counted compile warning #
Total comments: 24
Patch Set 3 : diii #
Total comments: 18
Patch Set 4 : Move callback impl to nacl_browser_delegate_impl.cc. #
Total comments: 42
Patch Set 5 : Address comments in patch 3 & 4. #
Total comments: 31
Patch Set 6 : Update after yzshen comments on impl patch 5 #Patch Set 7 : rebase #
Total comments: 13
Patch Set 8 : comment for 'active' instance. #Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/61063003/diff/100001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/61063003/diff/100001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_process_host.cc:778: virtual void OnIdleState(bool idle, OnIdleStateChange? OnIdleStateTransition? https://codereview.chromium.org/61063003/diff/100001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_process_host.cc:823: }; This should probably go up in the unnamed namespace. https://codereview.chromium.org/61063003/diff/100001/content/browser/ppapi_pl... File content/browser/ppapi_plugin_process_host.cc (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/ppapi_pl... content/browser/ppapi_plugin_process_host.cc:240: host_impl_ = new BrowserPpapiHostImpl(NULL, this, permissions, This looks kind of messy, since that argument is only relevant for NaCl (and even in that case, we don't call this constructor directly). What about not having that argument? We could instead have either SetDelegate or SetIdleCallback (I think I prefer the latter). https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:63: message_filter_ = new HostMessageFilter(this, ppapi_host_.get()); Maybe you should give the HostMessageFilter a callback bound to OnIdleState instead, and it won't need the pointer to BrowserPpapiHostImpl? I think you'd be able to use a WeakPtrFactory and bind the callback to a WeakPtr instead of making BrowserPpapiHostImpl RefCountedThreadSafe. Since the callback would only ever be invoked on the IO thread, that should be safe. https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:154: for (InstanceMap::iterator i = instance_map_.begin(); Hmm... it might be nice if this code was more instance-aware. As is, it should be fine for your intended use case of NaCl (since there's 1 instance per process), but it's a little bit weird if this code ran for trusted plugins. Let's say 10 extensions embed some Pepper plugin, e.g. Flash (I'm assuming they can do that?). There's 1 Flash process, but I presume 10 renderers (one per extension). If just 1 of those Flash instances is active, does this mean that we're treating all of the extensions as active, even if the rest are idle? It seems like the "right" thing to do in that case would be to let the plugin process live on (because 1 instance is active), but kill the 9 idle renderers. I just looked at where you're calling MarkPluginAsIdle, and you *could* add a PP_Instance argument there and plumb it through to here. The instance would unfortunately not be available in all cases, however (PPB_Var, PPB_Core, PPB_MessageLoop at least). You could just treat that as activity for all plugins in the process, I guess. Anyway... this is all a bit hypothetical, since this is only being checked in NaCl at present, where there's 1 PP_Instance per process anyway. So I'm leaning towards recommending you do it basically how you have it, omitting PP_Instance. But there should be a big fat comment here, probably... https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:93: explicit HostMessageFilter(BrowserPpapiHostImpl* host, nit: explicit not necessary now w/2 params https://codereview.chromium.org/61063003/diff/100001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/100001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:40: class Delegate { How about instead having something like this on BrowserPpapiHost: virtual void SetIdleStateChangeCallback( const base::Callback<void (bool, /* idle */ base::FilePath, /* profile_data_dir */ const RenderViewHost*, GURL /*document_url*/)>& callback) = 0; https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_globa... File ppapi/proxy/plugin_globals.h (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_globa... ppapi/proxy/plugin_globals.h:169: // TODO(scheib) Mark that this is OK to hit from any thread Virtually all the ppapi/proxy stuff is guarded by ppapi::ProxyLock (ppapi/shared_impl/proxy_lock.h). So this goes without saying. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:270: const int checkIntervalInSeconds = 1; nit: kCheckIntervalInSeconds Some rationale about why 1 second is appropriate might be good (it's not obvious to me) https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:271: bool last_was_active = false; Could this be a parameter to CheckActivity instead of a global? Also... kCheckIntervalInSeconds and g_last_was_active (if left global) should probably move to the unnamed namespace. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:273: void CheckActivity(base::MessageLoop* loop, PpapiDispatcher* dispatcher) { You're not using dispatcher, and it will become invalid at process shutdown. I think you want to just remove the argument. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:274: bool was_active = PluginGlobals::Get()->plugin_has_been_active(); At shutdown, PluginGlobals()::Get() will be NULL. Maybe you should just break early in that case. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:279: new PpapiHostMsg_IdleState(!was_active)); So we're only sending a message if the state changes? If the plugin says it's active (not idle), and then doesn't send a message for a long time, does the browser then assume that it's active? Is that what we want? https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:283: loop->PostDelayedTask(FROM_HERE, base::Bind(&CheckActivity, loop, dispatcher), Why not use MessageLoop::current()? I'm not used to seeing a bare MessageLoop pointer passed around; if a parameter was necessary, scoped_refptr<MessageLoopProxy> would seem better. But it looks like MessageLoop::current() will do the trick here... https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:311: base::Bind(&CheckActivity, CheckActivity needs to have the global proxy lock, since it uses shared stuff like PluginGlobals. You can use "RunWhileLocked" from ppapi/shared_impl/proxy_lock.h here to make that happen cleanly. (https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...) Example usage: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_va... https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:314: base::TimeDelta::FromSeconds(checkIntervalInSeconds)); (nit: all the arguments should just be indented by 4 so this one can fit) https://codereview.chromium.org/61063003/diff/100001/ppapi/thunk/enter.cc File ppapi/thunk/enter.cc (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/thunk/enter.cc#new... ppapi/thunk/enter.cc:66: PpapiGlobals::Get()->MarkPluginIsActive(); In these latter 4 cases, you either have the instance as an arg, or you can get it from resource_... So you could pass it. In some other cases, however (PPB_Var, PPB_Core, PPB_MessageLoop) we might not have the PP_Instance. Hmm. I think this is probably good as-is, since we're only doing this for NaCl. But I think a comment could be good in BrowserPpapiHostImpl::OnIdleState. I'll comment there as well...
https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:190: base::Bind(&BrowserPpapiHostImpl::OnIdleState, Important: BrowserPpapiHostImpl should be accessed on the IO thread only. https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:27: { Please move { back to the previous line. https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:30: // NULL, and is used only for OnIdleState for NaCl extensions. Please void references to "NaCl" in content/ https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:30: struct PepperRendererInstanceData; It is not needed here. https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:38: public base::RefCountedThreadSafe<BrowserPpapiHost> { If possible, please don't make it a ref-counted object. This should only be accessed on the IO thread. https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:54: // NULL, and is used only for OnIdleState for NaCl extensions. A general rule is not to mention "NaCl" in content/, because it is chrome specific, please consider "external plugin". https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_globa... File ppapi/proxy/plugin_globals.h (right): https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_globa... ppapi/proxy/plugin_globals.h:169: // TODO(scheib) Mark that this is OK to hit from any thread The access to this object is protected by the pepper proxy lock. So it is safe to access it from any thread. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:270: const int checkIntervalInSeconds = 1; Please use kCheck.... Shall we put these two variable in the anonymous namespace? https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:273: void CheckActivity(base::MessageLoop* loop, PpapiDispatcher* dispatcher) { |dispatcher| is not needed, right? https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:274: bool was_active = PluginGlobals::Get()->plugin_has_been_active(); You need to lock the pepper proxy lock before you can access PluginGlobals. Please see ProxyAutoLock. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:283: loop->PostDelayedTask(FROM_HERE, base::Bind(&CheckActivity, loop, dispatcher), nit: you could use MessageLoop::current() instead of passing |loop|. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:314: base::TimeDelta::FromSeconds(checkIntervalInSeconds)); wrong indent.
Thanks both for feedback -- looks like overall architecture is mostly on the right track with some threading changes and dropping the smart pointer use. I'll move to callbacks instead of the delegate pattern. https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:154: for (InstanceMap::iterator i = instance_map_.begin(); On 2013/11/06 22:39:21, dmichael wrote: > Hmm... it might be nice if this code was more instance-aware. As is, it should > be fine for your intended use case of NaCl (since there's 1 instance per > process), but it's a little bit weird if this code ran for trusted plugins. > > Let's say 10 extensions embed some Pepper plugin, e.g. Flash (I'm assuming they > can do that?). There's 1 Flash process, but I presume 10 renderers (one per > extension). If just 1 of those Flash instances is active, does this mean that > we're treating all of the extensions as active, even if the rest are idle? > > It seems like the "right" thing to do in that case would be to let the plugin > process live on (because 1 instance is active), but kill the 9 idle renderers. > > I just looked at where you're calling MarkPluginAsIdle, and you *could* add a > PP_Instance argument there and plumb it through to here. The instance would > unfortunately not be available in all cases, however (PPB_Var, PPB_Core, > PPB_MessageLoop at least). You could just treat that as activity for all plugins > in the process, I guess. > > Anyway... this is all a bit hypothetical, since this is only being checked in > NaCl at present, where there's 1 PP_Instance per process anyway. So I'm leaning > towards recommending you do it basically how you have it, omitting PP_Instance. > But there should be a big fat comment here, probably... Comment certainly warranted. I'm not a fan of tracking and plumbing all the instances through if there's really no need. But, I would also like to move this loop to after the work that happens in chrome/browser. The blocker there is mainly just building a set of all the info to push upwards without exposing PepperRendererInstanceData outside of content. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:279: new PpapiHostMsg_IdleState(!was_active)); On 2013/11/06 22:39:21, dmichael wrote: > So we're only sending a message if the state changes? If the plugin says it's > active (not idle), and then doesn't send a message for a long time, does the > browser then assume that it's active? Is that what we want? I think it is what we want. Browser side we are tracking activity by incrementing / decrementing a counter. There's no value to sending more IPC traffic I think. If the plugin goes dark while we think it's active that's an orthogonal problem I believe is already solved by a watchdog that will notice a frozen plugin process.
Yes, I think the overall approach is good. I think it makes sense to not plumb PP_Instance through, though it should be well documented. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:279: new PpapiHostMsg_IdleState(!was_active)); On 2013/11/07 01:06:42, scheib wrote: > On 2013/11/06 22:39:21, dmichael wrote: > > So we're only sending a message if the state changes? If the plugin says it's > > active (not idle), and then doesn't send a message for a long time, does the > > browser then assume that it's active? Is that what we want? > > I think it is what we want. Browser side we are tracking activity by > incrementing / decrementing a counter. There's no value to sending more IPC > traffic I think. If the plugin goes dark while we think it's active that's an > orthogonal problem I believe is already solved by a watchdog that will notice a > frozen plugin process. > Sounds good. I wrote this comment before I'd read the rest of the CL.
FYI I think this patch is in fairly good shape BUT WITHOUT TESTS. dmichael, I'm just starting to research what I need to do there, if you have any general comments, recommendations, or warnings about gotchas, I'd love to hear. My general plan is to do a browser_test functional test. 2 NaCl modules (one that does nothing, one that posts messages to itself) and then mimic whatever browser_test pattern I find for the extension level idle process tests. I'm not certain unit_tests alone would suffice, but if a pattern of using them sounds like it could displace the need for a browser test I'd like your opinion on that. https://codereview.chromium.org/61063003/diff/100001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/61063003/diff/100001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_process_host.cc:778: virtual void OnIdleState(bool idle, On 2013/11/06 22:39:21, dmichael wrote: > OnIdleStateChange? > OnIdleStateTransition? Done: IdleStateChange https://codereview.chromium.org/61063003/diff/100001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_process_host.cc:823: }; On 2013/11/06 22:39:21, dmichael wrote: > This should probably go up in the unnamed namespace. Done. https://codereview.chromium.org/61063003/diff/100001/content/browser/ppapi_pl... File content/browser/ppapi_plugin_process_host.cc (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/ppapi_pl... content/browser/ppapi_plugin_process_host.cc:240: host_impl_ = new BrowserPpapiHostImpl(NULL, this, permissions, On 2013/11/06 22:39:21, dmichael wrote: > This looks kind of messy, since that argument is only relevant for NaCl (and > even in that case, we don't call this constructor directly). What about not > having that argument? We could instead have either SetDelegate or > SetIdleCallback (I think I prefer the latter). Done. https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:63: message_filter_ = new HostMessageFilter(this, ppapi_host_.get()); On 2013/11/06 22:39:21, dmichael wrote: > Maybe you should give the HostMessageFilter a callback bound to OnIdleState > instead, and it won't need the pointer to BrowserPpapiHostImpl? > > I think you'd be able to use a WeakPtrFactory and bind the callback to a WeakPtr > instead of making BrowserPpapiHostImpl RefCountedThreadSafe. Since the callback > would only ever be invoked on the IO thread, that should be safe. Done. https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:154: for (InstanceMap::iterator i = instance_map_.begin(); On 2013/11/06 22:39:21, dmichael wrote: > Hmm... it might be nice if this code was more instance-aware. As is, it should > be fine for your intended use case of NaCl (since there's 1 instance per > process), but it's a little bit weird if this code ran for trusted plugins. > > Let's say 10 extensions embed some Pepper plugin, e.g. Flash (I'm assuming they > can do that?). There's 1 Flash process, but I presume 10 renderers (one per > extension). If just 1 of those Flash instances is active, does this mean that > we're treating all of the extensions as active, even if the rest are idle? > > It seems like the "right" thing to do in that case would be to let the plugin > process live on (because 1 instance is active), but kill the 9 idle renderers. > > I just looked at where you're calling MarkPluginAsIdle, and you *could* add a > PP_Instance argument there and plumb it through to here. The instance would > unfortunately not be available in all cases, however (PPB_Var, PPB_Core, > PPB_MessageLoop at least). You could just treat that as activity for all plugins > in the process, I guess. > > Anyway... this is all a bit hypothetical, since this is only being checked in > NaCl at present, where there's 1 PP_Instance per process anyway. So I'm leaning > towards recommending you do it basically how you have it, omitting PP_Instance. > But there should be a big fat comment here, probably... I have moved the for loop to the handler in chrome/*, and added a comment here acknowledging that idle is not tracked per instance. https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/100001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:93: explicit HostMessageFilter(BrowserPpapiHostImpl* host, On 2013/11/06 22:39:21, dmichael wrote: > nit: explicit not necessary now w/2 params Done. https://codereview.chromium.org/61063003/diff/100001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/100001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:40: class Delegate { On 2013/11/06 22:39:21, dmichael wrote: > How about instead having something like this on BrowserPpapiHost: > virtual void SetIdleStateChangeCallback( > const base::Callback<void (bool, /* idle */ > base::FilePath, /* profile_data_dir */ > const RenderViewHost*, > GURL /*document_url*/)>& callback) = 0; Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_globa... File ppapi/proxy/plugin_globals.h (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_globa... ppapi/proxy/plugin_globals.h:169: // TODO(scheib) Mark that this is OK to hit from any thread On 2013/11/06 22:39:21, dmichael wrote: > Virtually all the ppapi/proxy stuff is guarded by ppapi::ProxyLock > (ppapi/shared_impl/proxy_lock.h). So this goes without saying. Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:270: const int checkIntervalInSeconds = 1; On 2013/11/06 22:39:21, dmichael wrote: > nit: kCheckIntervalInSeconds > > Some rationale about why 1 second is appropriate might be good (it's not obvious > to me) Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:271: bool last_was_active = false; On 2013/11/06 22:39:21, dmichael wrote: > Could this be a parameter to CheckActivity instead of a global? > > Also... kCheckIntervalInSeconds and g_last_was_active (if left global) should > probably move to the unnamed namespace. Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:273: void CheckActivity(base::MessageLoop* loop, PpapiDispatcher* dispatcher) { On 2013/11/06 22:39:21, dmichael wrote: > You're not using dispatcher, and it will become invalid at process shutdown. I > think you want to just remove the argument. Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:274: bool was_active = PluginGlobals::Get()->plugin_has_been_active(); On 2013/11/06 22:39:21, dmichael wrote: > At shutdown, PluginGlobals()::Get() will be NULL. Maybe you should just break > early in that case. Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:283: loop->PostDelayedTask(FROM_HERE, base::Bind(&CheckActivity, loop, dispatcher), On 2013/11/06 22:39:21, dmichael wrote: > Why not use MessageLoop::current()? I'm not used to seeing a bare MessageLoop > pointer passed around; if a parameter was necessary, > scoped_refptr<MessageLoopProxy> would seem better. But it looks like > MessageLoop::current() will do the trick here... Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:311: base::Bind(&CheckActivity, On 2013/11/06 22:39:21, dmichael wrote: > CheckActivity needs to have the global proxy lock, since it uses shared stuff > like PluginGlobals. You can use "RunWhileLocked" from > ppapi/shared_impl/proxy_lock.h here to make that happen cleanly. > (https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...) > > Example usage: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_va... Done via: ppapi::ProxyAutoLock lock; https://codereview.chromium.org/61063003/diff/100001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:314: base::TimeDelta::FromSeconds(checkIntervalInSeconds)); On 2013/11/06 22:39:21, dmichael wrote: > (nit: all the arguments should just be indented by 4 so this one can fit) Done. https://codereview.chromium.org/61063003/diff/100001/ppapi/thunk/enter.cc File ppapi/thunk/enter.cc (right): https://codereview.chromium.org/61063003/diff/100001/ppapi/thunk/enter.cc#new... ppapi/thunk/enter.cc:66: PpapiGlobals::Get()->MarkPluginIsActive(); On 2013/11/06 22:39:21, dmichael wrote: > In these latter 4 cases, you either have the instance as an arg, or you can get > it from resource_... > > So you could pass it. In some other cases, however (PPB_Var, PPB_Core, > PPB_MessageLoop) we might not have the PP_Instance. Hmm. > > I think this is probably good as-is, since we're only doing this for NaCl. But I > think a comment could be good in BrowserPpapiHostImpl::OnIdleState. I'll comment > there as well... Done. https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:190: base::Bind(&BrowserPpapiHostImpl::OnIdleState, On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > Important: BrowserPpapiHostImpl should be accessed on the IO thread only. Done. https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:27: { On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > Please move { back to the previous line. Done. https://codereview.chromium.org/61063003/diff/140001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:30: // NULL, and is used only for OnIdleState for NaCl extensions. On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > Please void references to "NaCl" in content/ Done. https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:30: struct PepperRendererInstanceData; On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > It is not needed here. Done. https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:38: public base::RefCountedThreadSafe<BrowserPpapiHost> { On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > If possible, please don't make it a ref-counted object. > > This should only be accessed on the IO thread. Done. https://codereview.chromium.org/61063003/diff/140001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:54: // NULL, and is used only for OnIdleState for NaCl extensions. On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > A general rule is not to mention "NaCl" in content/, because it is chrome > specific, please consider "external plugin". Done. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_globa... File ppapi/proxy/plugin_globals.h (right): https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_globa... ppapi/proxy/plugin_globals.h:169: // TODO(scheib) Mark that this is OK to hit from any thread On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > The access to this object is protected by the pepper proxy lock. So it is safe > to access it from any thread. Done. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:270: const int checkIntervalInSeconds = 1; On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > Please use kCheck.... > > Shall we put these two variable in the anonymous namespace? Done. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:273: void CheckActivity(base::MessageLoop* loop, PpapiDispatcher* dispatcher) { On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > |dispatcher| is not needed, right? Done. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:274: bool was_active = PluginGlobals::Get()->plugin_has_been_active(); On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > You need to lock the pepper proxy lock before you can access PluginGlobals. > > Please see ProxyAutoLock. Done. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:283: loop->PostDelayedTask(FROM_HERE, base::Bind(&CheckActivity, loop, dispatcher), On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > nit: you could use MessageLoop::current() instead of passing |loop|. Done. https://codereview.chromium.org/61063003/diff/140001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:314: base::TimeDelta::FromSeconds(checkIntervalInSeconds)); On 2013/11/06 22:43:15, yzshen (OOO Nov 11-15) wrote: > wrong indent. Done.
https://codereview.chromium.org/61063003/diff/610001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/61063003/diff/610001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_process_host.cc:28: #include "chrome/browser/extensions/extension_service.h" I don't know if you want a review for this change yet, but we are not supposed to have any dependencies on Chrome extensions in this file any more. (The file is planned to be moved to components/nacl/ soon.) So any references to chrome/'s extensions subsystem here should be done via NaClBrowser::GetDelegate().
This is looking really good to me, aside from Mark's comment about factoring and some comment nits. Re: testing. It's almost possible to do a unit test, except that this only works in NaCl. We have, for example, ppapi/proxy/websocket_resource_unittest.cc. It does proxy-side stuff, and rather than messages going to the host, they go to the Message Sink, which the test can read to make sure it got the right messages. But... we've never done the work to make these function in NaCl, and it's not fair to ask you to figure that out, so I think integration tests are probably appropriate. It might be worthwhile to think of a way to adjust the idle timeouts, because we don't want a test that's guaranteed to actually take 10-15 seconds to run. Hopefully the extension/app tests are informative. https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:191: // An instance has become idle or active. The on_idle_chnge_callback_ will be chnge->change https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:197: // state for all instances together. "all instances" -> "all instances for this module" https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:57: const BrowserPpapiHost::OnIdleChangeCallback callback) OVERRIDE; could be const& https://codereview.chromium.org/61063003/diff/610001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/610001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:97: // Set callback used when the plugin transitions in or out of an idle state. nit: This comment could be a little clearer. "Sets a callback that the BrowserPpapiHost will invoke when the plugin transitions in or out of an idle state." https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... File ppapi/shared_impl/ppapi_globals.h (right): https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:131: // Record that plugin is active. Avoids being shutdown in containers that nits: "the" plugin Avoids->Prevents https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:132: // detect idle content. such as background apps? https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:134: // renderer process will have an effect. an->no ?
https://codereview.chromium.org/61063003/diff/610001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/610001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:268: void CheckActivity(bool last_was_active) { It looks like this will make every NaCl process wake up every 5 seconds. This is generally not a good idea. It will waste power, which is a problem on battery-powered systems, and it will increase memory pressure. If NaCl processes are idle, it should be possible for the system to swap out all the pages they're using, and not page in the nexe (and the NaCl trusted runtime) until the user starts interacting with the nexe again. If the process wakes up every 5 seconds, the system probably won't be able to page it out. If you really need to do this activity check for nexes in background pages in Chrome extensions, the check should only enable the wakeup in that case so that it doesn't affect other cases. (I don't understand the motivation for this activity check fully yet, but I'll read the design doc.)
https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // process BrowserPpapiHost upon transitioning in our out of idleness. The our -> or https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:51: content::BrowserPpapiHost::OnIdleChangeInstanceData intance_data, This whole vector is copied quite a few times: when passed into OnIdleStateChange(); when bind() in OnIdleStateChange(); when OnIdleStateChangeOnUIThread is called. Can we avoid this? https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:52: const base::FilePath profile_data_directory, const &, please. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:97: fprintf(stderr, "%s:%s:%d DecrementLazyKeepaliveCount\n", Is this intended to be in the production code? https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:99: pm->DecrementLazyKeepaliveCount(extension); If the plugin is destroyed while the last call is pm->IncrementLazyKeepaliveCount(), is it true that the extension will be considered active forever? https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:101: fprintf(stderr, "%s:%s:%d IncrementLazyKeepaliveCount\n", Is this debugging code that should be removed? https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_browser_delegate_impl.h (right): https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.h:32: virtual const content::BrowserPpapiHost::OnIdleChangeCallback why do we need 'const' here? https://codereview.chromium.org/61063003/diff/800001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/61063003/diff/800001/components/nacl.gyp#newc... components/nacl.gyp:43: 'nacl/browser/test_nacl_browser_delegate.cc', alphabetical order, please. https://codereview.chromium.org/61063003/diff/800001/components/nacl/browser/... File components/nacl/browser/nacl_browser_delegate.h (right): https://codereview.chromium.org/61063003/diff/800001/components/nacl/browser/... components/nacl/browser/nacl_browser_delegate.h:76: virtual const content::BrowserPpapiHost::OnIdleChangeCallback Why do we need 'const' here? https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:158: { put it on the previous line, please https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:183: { previous line, please https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:186: void BrowserPpapiHostImpl::HostMessageFilter::OnIdleStateChange(bool idle) { nit: it is better to reorder the method definitions so that HostMessageFilter impl doesn't appear in the middle. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:191: // An instance has become idle or active. The on_idle_chnge_callback_ will be chnge -> change https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:57: const BrowserPpapiHost::OnIdleChangeCallback callback) OVERRIDE; const &, please. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:92: HostMessageFilter(ppapi::host::PpapiHost* ppapi_host, If we directly pass in BrowserPpapiHostImpl*: - we don't have to have OnIdleChangeCallback, instead we directly call BrowserPpapiHostImpl::OnIdleChange - we can remove the weakptr factory, OnHostDestroy() notifies us when BrowserPpapiHostImpl dies. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:93: const OnIdleChangeCallback on_idle_change_callback); const &, please https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:44: void (OnIdleChangeInstanceData intance_data, You could use const & here. https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:45: base::FilePath profile_data_directory, const &, please. https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:98: virtual void SetOnIdleChangeCallback(const OnIdleChangeCallback callback) = 0; const &, please https://codereview.chromium.org/61063003/diff/800001/ppapi/shared_impl/ppapi_... File ppapi/shared_impl/ppapi_globals.h (right): https://codereview.chromium.org/61063003/diff/800001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:134: // renderer process will have an effect. an -> no?
Implementation patch, to be followed by tests. See: https://codereview.chromium.org/111563006/ Tests. https://codereview.chromium.org/105873003/ Cumulative patch. https://codereview.chromium.org/61063003/diff/610001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/61063003/diff/610001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_process_host.cc:28: #include "chrome/browser/extensions/extension_service.h" On 2013/11/15 01:32:56, Mark Seaborn wrote: > I don't know if you want a review for this change yet, but we are not supposed > to have any dependencies on Chrome extensions in this file any more. (The file > is planned to be moved to components/nacl/ soon.) > > So any references to chrome/'s extensions subsystem here should be done via > NaClBrowser::GetDelegate(). Done. https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:191: // An instance has become idle or active. The on_idle_chnge_callback_ will be On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > chnge->change Done. https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:197: // state for all instances together. On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > "all instances" -> "all instances for this module" Done. https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/610001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:57: const BrowserPpapiHost::OnIdleChangeCallback callback) OVERRIDE; On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > could be const& Done. https://codereview.chromium.org/61063003/diff/610001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/610001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:97: // Set callback used when the plugin transitions in or out of an idle state. On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > nit: This comment could be a little clearer. > > "Sets a callback that the BrowserPpapiHost will invoke when the plugin > transitions in or out of an idle state." Done. https://codereview.chromium.org/61063003/diff/610001/ppapi/proxy/plugin_main_... File ppapi/proxy/plugin_main_nacl.cc (right): https://codereview.chromium.org/61063003/diff/610001/ppapi/proxy/plugin_main_... ppapi/proxy/plugin_main_nacl.cc:268: void CheckActivity(bool last_was_active) { On 2013/11/15 23:51:24, Mark Seaborn wrote: > It looks like this will make every NaCl process wake up every 5 seconds. This > is generally not a good idea. It will waste power, which is a problem on > battery-powered systems, and it will increase memory pressure. If NaCl > processes are idle, it should be possible for the system to swap out all the > pages they're using, and not page in the nexe (and the NaCl trusted runtime) > until the user starts interacting with the nexe again. If the process wakes up > every 5 seconds, the system probably won't be able to page it out. > > If you really need to do this activity check for nexes in background pages in > Chrome extensions, the check should only enable the wakeup in that case so that > it doesn't affect other cases. > > (I don't understand the motivation for this activity check fully yet, but I'll > read the design doc.) Done by having the plugin only report when it is active (at a throttled rate) and having the browser implement the conversion of activity impulses to a increment / decrement for keep alive. https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... File ppapi/shared_impl/ppapi_globals.h (right): https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:131: // Record that plugin is active. Avoids being shutdown in containers that On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > nits: > "the" plugin > Avoids->Prevents Done. https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:132: // detect idle content. On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > such as background apps? Done. https://codereview.chromium.org/61063003/diff/610001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:134: // renderer process will have an effect. On 2013/11/15 18:26:22, dmichael (OOO 11.21-11.29) wrote: > an->no ? Done. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // process BrowserPpapiHost upon transitioning in our out of idleness. The On 2013/11/21 18:01:50, yzshen1 wrote: > our -> or Done. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:51: content::BrowserPpapiHost::OnIdleChangeInstanceData intance_data, On 2013/11/21 18:01:50, yzshen1 wrote: > This whole vector is copied quite a few times: > > when passed into OnIdleStateChange(); > when bind() in OnIdleStateChange(); > when OnIdleStateChangeOnUIThread is called. > > Can we avoid this? > Done. Parameters changed to all const references. Also, I learned today that base::Bind makes copies of const references for you. I'll update it's doc to be clearer: https://codereview.chromium.org/81713003/ https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:52: const base::FilePath profile_data_directory, On 2013/11/21 18:01:50, yzshen1 wrote: > const &, please. Done. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:52: const base::FilePath profile_data_directory, On 2013/11/21 18:01:50, yzshen1 wrote: > const &, please. Done. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:97: fprintf(stderr, "%s:%s:%d DecrementLazyKeepaliveCount\n", On 2013/11/21 18:01:50, yzshen1 wrote: > Is this intended to be in the production code? No, this patch was still Work In Progress soliciting thoughts on testing. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:99: pm->DecrementLazyKeepaliveCount(extension); On 2013/11/21 18:01:50, yzshen1 wrote: > If the plugin is destroyed while the last call is > pm->IncrementLazyKeepaliveCount(), is it true that the extension will be > considered active forever? No longer an issue as the design has changed. Now if KeepaliveImpulse isn't continuously called the extension will time out to being considered idle. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:101: fprintf(stderr, "%s:%s:%d IncrementLazyKeepaliveCount\n", On 2013/11/21 18:01:50, yzshen1 wrote: > Is this debugging code that should be removed? Done. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_browser_delegate_impl.h (right): https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.h:32: virtual const content::BrowserPpapiHost::OnIdleChangeCallback On 2013/11/21 18:01:50, yzshen1 wrote: > why do we need 'const' here? Done. We don't. https://codereview.chromium.org/61063003/diff/800001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/61063003/diff/800001/components/nacl.gyp#newc... components/nacl.gyp:43: 'nacl/browser/test_nacl_browser_delegate.cc', On 2013/11/21 18:01:50, yzshen1 wrote: > alphabetical order, please. No longer part of patch. https://codereview.chromium.org/61063003/diff/800001/components/nacl/browser/... File components/nacl/browser/nacl_browser_delegate.h (right): https://codereview.chromium.org/61063003/diff/800001/components/nacl/browser/... components/nacl/browser/nacl_browser_delegate.h:76: virtual const content::BrowserPpapiHost::OnIdleChangeCallback On 2013/11/21 18:01:50, yzshen1 wrote: > Why do we need 'const' here? Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:158: { On 2013/11/21 18:01:50, yzshen1 wrote: > put it on the previous line, please Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:183: { On 2013/11/21 18:01:50, yzshen1 wrote: > previous line, please Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:186: void BrowserPpapiHostImpl::HostMessageFilter::OnIdleStateChange(bool idle) { On 2013/11/21 18:01:50, yzshen1 wrote: > nit: it is better to reorder the method definitions so that HostMessageFilter > impl doesn't appear in the middle. Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:191: // An instance has become idle or active. The on_idle_chnge_callback_ will be On 2013/11/21 18:01:50, yzshen1 wrote: > chnge -> change Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:57: const BrowserPpapiHost::OnIdleChangeCallback callback) OVERRIDE; On 2013/11/21 18:01:50, yzshen1 wrote: > const &, please. Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:92: HostMessageFilter(ppapi::host::PpapiHost* ppapi_host, On 2013/11/21 18:01:50, yzshen1 wrote: > If we directly pass in BrowserPpapiHostImpl*: > - we don't have to have OnIdleChangeCallback, instead we directly call > BrowserPpapiHostImpl::OnIdleChange > - we can remove the weakptr factory, OnHostDestroy() notifies us when > BrowserPpapiHostImpl dies. Done. https://codereview.chromium.org/61063003/diff/800001/content/browser/renderer... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:93: const OnIdleChangeCallback on_idle_change_callback); On 2013/11/21 18:01:50, yzshen1 wrote: > const &, please Done. https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... File content/public/browser/browser_ppapi_host.h (right): https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:44: void (OnIdleChangeInstanceData intance_data, On 2013/11/21 18:01:50, yzshen1 wrote: > You could use const & here. Done. https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:45: base::FilePath profile_data_directory, On 2013/11/21 18:01:50, yzshen1 wrote: > const &, please. Done. https://codereview.chromium.org/61063003/diff/800001/content/public/browser/b... content/public/browser/browser_ppapi_host.h:98: virtual void SetOnIdleChangeCallback(const OnIdleChangeCallback callback) = 0; On 2013/11/21 18:01:50, yzshen1 wrote: > const &, please Done. https://codereview.chromium.org/61063003/diff/800001/ppapi/shared_impl/ppapi_... File ppapi/shared_impl/ppapi_globals.h (right): https://codereview.chromium.org/61063003/diff/800001/ppapi/shared_impl/ppapi_... ppapi/shared_impl/ppapi_globals.h:134: // renderer process will have an effect. On 2013/11/21 18:01:50, yzshen1 wrote: > an -> no? Done.
only nits. Mostly looks good. https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/800001/chrome/browser/nacl_host... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:51: content::BrowserPpapiHost::OnIdleChangeInstanceData intance_data, > Done. Parameters changed to all const references. Also, I learned today that > base::Bind makes copies of const references for you. I'll update it's doc to be > clearer: https://codereview.chromium.org/81713003/ Right. Thanks for the nice document! https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... File components/nacl/browser/nacl_browser_delegate.h (right): https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... components/nacl/browser/nacl_browser_delegate.h:11: // Included because nested OnKeepaliveCallback class can't be forward declared. nit: might be okay to remove this comment because this happens quite often. (optional) https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:803: nacl::NaClBrowser::GetDelegate()->GetOnKeepaliveCallback()); nit: this code is in nacl:: namespace right? you could remove the namespace prefix. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:150: void BrowserPpapiHostImpl::OnKeepalive() { I notice you used Keep*a*live in quite some places. I thought they are two words so we should do Keep*A*live, but maybe you have good reasons? https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:152: // used to permit the content embedder to handle this, e.g. by shutting down It seems we need to update the comment, now that it is called OnKeepalive(), "shutting down" seems confusing. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:182: {} { should be on the previous line. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:206: {} ditto. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:83: void OnKeepalive(); it doesn't need to be a public method. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... File ppapi/proxy/plugin_globals.cc (right): https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:66: { On the previous line, please. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:82: plugin_recently_active_(false), Please also init keep_alive_throttle_interval_milliseconds_ https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... File ppapi/proxy/plugin_globals.h (right): https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:135: int keep_aliave_throttle_interval_milliseconds(); alive, please. And it seems you use keepalive without '_' in the middle elsewhere, I think it is better to be consistent. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:135: int keep_aliave_throttle_interval_milliseconds(); const for the method, please. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:145: void OnReleaseKeepaliveThrottle(); It seems better to comment that "Locks the proxy lock and releases ..." https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:179: int keep_aliave_throttle_interval_milliseconds_; alive, please. (You are using auto-completion for sure! :)) https://codereview.chromium.org/61063003/diff/1180001/ppapi/shared_impl/ppapi... File ppapi/shared_impl/ppapi_globals.h (right): https://codereview.chromium.org/61063003/diff/1180001/ppapi/shared_impl/ppapi... ppapi/shared_impl/ppapi_globals.h:131: // Record that the plugin is active. The plugin reports that it is active to Record -> Records
https://codereview.chromium.org/61063003/diff/1180001/chrome/browser/nacl_hos... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/1180001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // process BrowserPpapiHost upon transitioning in or out of idleness. The This comment seems outdated.
Thanks. https://codereview.chromium.org/61063003/diff/1180001/chrome/browser/nacl_hos... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/1180001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // process BrowserPpapiHost upon transitioning in or out of idleness. The On 2013/12/13 21:30:07, yzshen1 wrote: > This comment seems outdated. Done. https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... File components/nacl/browser/nacl_browser_delegate.h (right): https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... components/nacl/browser/nacl_browser_delegate.h:11: // Included because nested OnKeepaliveCallback class can't be forward declared. On 2013/12/13 21:23:15, yzshen1 wrote: > nit: might be okay to remove this comment because this happens quite often. > (optional) Done. https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/61063003/diff/1180001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:803: nacl::NaClBrowser::GetDelegate()->GetOnKeepaliveCallback()); On 2013/12/13 21:23:15, yzshen1 wrote: > nit: this code is in nacl:: namespace right? you could remove the namespace > prefix. Done. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:150: void BrowserPpapiHostImpl::OnKeepalive() { On 2013/12/13 21:23:15, yzshen1 wrote: > I notice you used Keep*a*live in quite some places. I thought they are two words > so we should do Keep*A*live, but maybe you have good reasons? I've moved everything to "keepalive". Both are used commonly. E.g. a google search for "keep alive" returns a top result of "Keepalive" and a second result frequently using "Keep-Alive". The Chromium code base is also split, though I see more "keepalive". Because this plumbs eventually to extensions::ProcessManager::IncrementLazyKeepaliveCount I decided to be consistent with that and use keepalive as one word. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:152: // used to permit the content embedder to handle this, e.g. by shutting down On 2013/12/13 21:23:15, yzshen1 wrote: > It seems we need to update the comment, now that it is called OnKeepalive(), > "shutting down" seems confusing. Done. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:182: {} On 2013/12/13 21:23:15, yzshen1 wrote: > { should be on the previous line. Done. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:206: {} On 2013/12/13 21:23:15, yzshen1 wrote: > ditto. Done. https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right): https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:83: void OnKeepalive(); On 2013/12/13 21:23:15, yzshen1 wrote: > it doesn't need to be a public method. Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... File ppapi/proxy/plugin_globals.cc (right): https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:66: { On 2013/12/13 21:23:15, yzshen1 wrote: > On the previous line, please. Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:82: plugin_recently_active_(false), On 2013/12/13 21:23:15, yzshen1 wrote: > Please also init keep_alive_throttle_interval_milliseconds_ Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... File ppapi/proxy/plugin_globals.h (right): https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:135: int keep_aliave_throttle_interval_milliseconds(); On 2013/12/13 21:23:15, yzshen1 wrote: > alive, please. And it seems you use keepalive without '_' in the middle > elsewhere, I think it is better to be consistent. Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:135: int keep_aliave_throttle_interval_milliseconds(); On 2013/12/13 21:23:15, yzshen1 wrote: > const for the method, please. Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:145: void OnReleaseKeepaliveThrottle(); On 2013/12/13 21:23:15, yzshen1 wrote: > It seems better to comment that "Locks the proxy lock and releases ..." Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.h:179: int keep_aliave_throttle_interval_milliseconds_; On 2013/12/13 21:23:15, yzshen1 wrote: > alive, please. > (You are using auto-completion for sure! :)) Done. https://codereview.chromium.org/61063003/diff/1180001/ppapi/shared_impl/ppapi... File ppapi/shared_impl/ppapi_globals.h (right): https://codereview.chromium.org/61063003/diff/1180001/ppapi/shared_impl/ppapi... ppapi/shared_impl/ppapi_globals.h:131: // Record that the plugin is active. The plugin reports that it is active to On 2013/12/13 21:23:15, yzshen1 wrote: > Record -> Records Done.
LGTM https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/61063003/diff/1180001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:150: void BrowserPpapiHostImpl::OnKeepalive() { Sounds good. Thanks! On 2013/12/14 00:07:41, scheib wrote: > On 2013/12/13 21:23:15, yzshen1 wrote: > > I notice you used Keep*a*live in quite some places. I thought they are two > words > > so we should do Keep*A*live, but maybe you have good reasons? > > I've moved everything to "keepalive". > > Both are used commonly. E.g. a google search for "keep alive" returns a top > result of "Keepalive" and a second result frequently using "Keep-Alive". The > Chromium code base is also split, though I see more "keepalive". > > Because this plumbs eventually to > extensions::ProcessManager::IncrementLazyKeepaliveCount I decided to be > consistent with that and use keepalive as one word.
Owners: mseaborn: chrome/browser/nacl_host/nacl_browser_delegate_impl.cc chrome/browser/nacl_host/nacl_browser_delegate_impl.h components/nacl/browser/nacl_browser_delegate.h components/nacl/browser/nacl_process_host.cc components/nacl/browser/test_nacl_browser_delegate.cc components/nacl/browser/test_nacl_browser_delegate.h joi: content/public/browser/browser_ppapi_host.h tsepez: ppapi/proxy/ppapi_messages.h
//content/public LGTM.
ppapi_messages.h LGTM
https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:44: // There is 1:many relationship for extension:nacl-embeds, but only a nit: "is +a+ 1:many" https://codereview.chromium.org/61063003/diff/1220001/ppapi/proxy/plugin_glob... File ppapi/proxy/plugin_globals.cc (right): https://codereview.chromium.org/61063003/diff/1220001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:190: base::MessageLoop::current()->PostDelayedTask(FROM_HERE, MarkPluginIsActive can be called by any thread, including one that the plugin created, which may not have a message loop. You should use the loop for the main thread (GetMainThreadMessageLoop()). https://codereview.chromium.org/61063003/diff/1220001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:191: base::Bind(&PluginGlobals::OnReleaseKeepaliveThrottle, Are you able to use "RunWhileLocked" (from ppapi/proxy/proxy_lock.h) instead of locking explicitly in OnReleaseKeepaliveThrottle?
LGTM https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // BrowserPpapiHost at a trottled rate. The content::BrowserPpapiHost passes Typo: "throttled". Could you emphasise here that the keepalive IPCs only occur for NaCl processes that are part of extensions, not on normal web pages? https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:50: const content::BrowserPpapiHost::OnKeepaliveInstanceData& intance_data, "instance_data" https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:94: const content::BrowserPpapiHost::OnKeepaliveInstanceData& intance_data, "instance_data"
Thanks, mseaborn: Take a look at the IPC frequency note and see if you agree. https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // BrowserPpapiHost at a trottled rate. The content::BrowserPpapiHost passes On 2013/12/16 22:40:46, Mark Seaborn wrote: > Typo: "throttled". Could you emphasise here that the keepalive IPCs only occur > for NaCl processes that are part of extensions, not on normal web pages? Done. "throttled". Bugged: https://code.google.com/p/chromium/issues/detail?id=329037 for not sending IPCs from normal web pages. IMHO an IPC every 5 seconds is pretty minor and can be addressed in follow up work. https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:44: // There is 1:many relationship for extension:nacl-embeds, but only a On 2013/12/16 22:17:54, dmichael wrote: > nit: "is +a+ 1:many" Done. https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:50: const content::BrowserPpapiHost::OnKeepaliveInstanceData& intance_data, On 2013/12/16 22:40:46, Mark Seaborn wrote: > "instance_data" Done. https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:94: const content::BrowserPpapiHost::OnKeepaliveInstanceData& intance_data, On 2013/12/16 22:40:46, Mark Seaborn wrote: > "instance_data" Done. https://codereview.chromium.org/61063003/diff/1220001/ppapi/proxy/plugin_glob... File ppapi/proxy/plugin_globals.cc (right): https://codereview.chromium.org/61063003/diff/1220001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:190: base::MessageLoop::current()->PostDelayedTask(FROM_HERE, On 2013/12/16 22:17:54, dmichael wrote: > MarkPluginIsActive can be called by any thread, including one that the plugin > created, which may not have a message loop. You should use the loop for the main > thread (GetMainThreadMessageLoop()). Done. https://codereview.chromium.org/61063003/diff/1220001/ppapi/proxy/plugin_glob... ppapi/proxy/plugin_globals.cc:191: base::Bind(&PluginGlobals::OnReleaseKeepaliveThrottle, On 2013/12/16 22:17:54, dmichael wrote: > Are you able to use "RunWhileLocked" (from ppapi/proxy/proxy_lock.h) instead of > locking explicitly in OnReleaseKeepaliveThrottle? Done.
lgtm
https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right): https://codereview.chromium.org/61063003/diff/1220001/chrome/browser/nacl_hos... chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:40: // BrowserPpapiHost at a trottled rate. The content::BrowserPpapiHost passes On 2013/12/16 23:13:44, scheib wrote: > On 2013/12/16 22:40:46, Mark Seaborn wrote: > > Typo: "throttled". Could you emphasise here that the keepalive > > IPCs only occur for NaCl processes that are part of extensions, > > not on normal web pages? > > Done. "throttled". Bugged: > https://code.google.com/p/chromium/issues/detail?id=329037 for not > sending IPCs from normal web pages. IMHO an IPC every 5 seconds is > pretty minor and can be addressed in follow up work. Oh, I think I misread "active" in the comment as applying to any instantiated NaCl process. Maybe define it by saying "when active (actively making PPAPI calls or receiving PPAPI callbacks)"? Given that, I don't think you need to fix 329037. LGTM still.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/61063003/1260001
Message was sent while issue was closed.
Change committed as 241702 |
