|
|
DescriptionPlugin Power Saver: Basic proof of concept experiment.
Basic proof of experiment of Power Saver feature. Behind a flag.
Builds on top of https://codereview.chromium.org/617073002/
Design doc: http://goo.gl/iDix3p
BUG=403800
Committed: https://crrev.com/5576f38d2570bdbcdc1a63b50821bf4d7689ec84
Cr-Commit-Position: refs/heads/master@{#298547}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : git cl formatted #
Total comments: 33
Patch Set 4 : address comments #Patch Set 5 : #
Total comments: 10
Patch Set 6 : address comments #
Messages
Total messages: 19 (5 generated)
tommycli@chromium.org changed reviewers: + raymes@chromium.org
raymes: PTAL
tommycli@chromium.org changed reviewers: + cpu@chromium.org
cpu: Hi Carlos, the Pepper guys wanted to make sure you were aware that code was starting to go in (under a flag). This implements the 'fake offscreen' method we discussed.
groby@chromium.org changed reviewers: + groby@chromium.org
More drive-by :) https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1112: if (plugin_throttled_) How does this interact with the delayed throttle task posted at creation? I.e. what happens if I click before that kicks in? https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1113: UnthrottlePlugin(); Do we need to swallow this event? It seems to me that currently clicks are immediately forwarded to the underlying plugin, which we probably don't want. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1685: ppapi::ViewData* view_data = NULL; Since the code below is derefing view_data every time, why not ppapi::ViewData& view_data = plugin_throttled_ ? empty_view_data_ : view_data_; (Ideally, const& - AFAICT there's no state mutation going on) https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:710: void ThrottlePlugin(); Can we take a bool or an enum, and have one instead of two methods? https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:736: // Indicates whether this plugin may be throttle to reduce power consumption. throttle*d* https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:737: bool enable_power_saver_; Wouldn't plugin_power_saver_is_enabled_ be better? enable_... sounds like setting it to true turns on power saving? https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_throttler.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.cc:24: FROM_HERE, Why not just post the throttle_closure? It saves you from having to create a weak_factory_ here, and the closure already needs to be weakly bound anyways. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.cc:34: DCHECK(!throttle_closure_.is_null()); You probably want to DCHECK in the ctor, if null closures aren't allowed at all.
https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:408: bool is_peripheral_content = true; so we are going to throttle all of them? It seems we want this part in the first version.
https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:408: bool is_peripheral_content = true; On 2014/10/06 18:07:52, cpu wrote: > so we are going to throttle all of them? > > It seems we want this part in the first version. The plan is to get this CL out without a heuristic, so people can get a feel for what it does at all. (That's why I suggested on the flag CL that it's not in chrome:flags, too - it's a very rough prototype)
https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:409: bool enable_power_saver = is_peripheral_content && Can we restrict this to flash plugins for the time being? You should be able to check the plugin name in "module()->name()" https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:410: CommandLine::ForCurrentProcess()->HasSwitch( nit: we never tend to indent in line with "=". Use a 4 space indent. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:418: enable_power_saver); Can we avoid adding the parameter to the constructor and just set the member variable based on the flag inside the constructor body? https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1113: UnthrottlePlugin(); Note that this will unthrottle on any input event (including mouse hover). Is that what we expect? https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1689: view_data = &view_data_; Note that |view_data_| is used in a lot of places. By giving the plugin a different impression of what the view is than what the renderer has may result in unexpected behavior. I don't really have any good suggestions here because I don't know what the problems might be but just wanted you to be aware that we may expect some unusual bugs to pop up when the plugin is throttled. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:743: ppapi::ViewData empty_view_data_; nit: could this be const? https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_throttler.h (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.h:14: // Throttles Pepper Plugin instances on Power Saver mode. Currently just a nit: on->in
raymes/cpu/groby: addressed your comments. Thanks! https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:408: bool is_peripheral_content = true; On 2014/10/06 18:07:52, cpu wrote: > so we are going to throttle all of them? > > It seems we want this part in the first version. cpu: Rachel is correct. We're still working on the heuristic. Throttling everything behind the flag seems okay to me. We will implement the layout-based heuristic asap. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:409: bool enable_power_saver = is_peripheral_content && On 2014/10/06 18:21:48, raymes wrote: > Can we restrict this to flash plugins for the time being? You should be able to > check the plugin name in "module()->name()" Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:410: CommandLine::ForCurrentProcess()->HasSwitch( On 2014/10/06 18:21:48, raymes wrote: > nit: we never tend to indent in line with "=". Use a 4 space indent. Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:418: enable_power_saver); On 2014/10/06 18:21:48, raymes wrote: > Can we avoid adding the parameter to the constructor and just set the member > variable based on the flag inside the constructor body? Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1112: if (plugin_throttled_) On 2014/10/06 17:39:44, groby wrote: > How does this interact with the delayed throttle task posted at creation? I.e. > what happens if I click before that kicks in? Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1113: UnthrottlePlugin(); On 2014/10/06 18:21:48, raymes wrote: > Note that this will unthrottle on any input event (including mouse hover). Is > that what we expect? I modified it to unthrottle on click. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1113: UnthrottlePlugin(); On 2014/10/06 17:39:44, groby wrote: > Do we need to swallow this event? It seems to me that currently clicks are > immediately forwarded to the underlying plugin, which we probably don't want. Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1685: ppapi::ViewData* view_data = NULL; On 2014/10/06 17:39:44, groby wrote: > Since the code below is derefing view_data every time, why not ppapi::ViewData& > view_data = plugin_throttled_ ? empty_view_data_ : view_data_; > > (Ideally, const& - AFAICT there's no state mutation going on) Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1689: view_data = &view_data_; On 2014/10/06 18:21:48, raymes wrote: > Note that |view_data_| is used in a lot of places. By giving the plugin a > different impression of what the view is than what the renderer has may result > in unexpected behavior. I don't really have any good suggestions here because I > don't know what the problems might be but just wanted you to be aware that we > may expect some unusual bugs to pop up when the plugin is throttled. Agreed - and this is noted in the design doc also. We proceeded with this hack since the alternative approach was to kill and restart the plugin, which is going to have nasty side effects on scripting too. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:710: void ThrottlePlugin(); On 2014/10/06 17:39:44, groby wrote: > Can we take a bool or an enum, and have one instead of two methods? Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:736: // Indicates whether this plugin may be throttle to reduce power consumption. On 2014/10/06 17:39:44, groby wrote: > throttle*d* Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:737: bool enable_power_saver_; On 2014/10/06 17:39:44, groby wrote: > Wouldn't plugin_power_saver_is_enabled_ be better? enable_... sounds like > setting it to true turns on power saving? Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.h:743: ppapi::ViewData empty_view_data_; On 2014/10/06 18:21:48, raymes wrote: > nit: could this be const? Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_throttler.cc (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.cc:24: FROM_HERE, On 2014/10/06 17:39:44, groby wrote: > Why not just post the throttle_closure? It saves you from having to create a > weak_factory_ here, and the closure already needs to be weakly bound anyways. Done. It was 'for the future'. But you're right, I'll add the complexity when I actually need it. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.cc:34: DCHECK(!throttle_closure_.is_null()); On 2014/10/06 17:39:44, groby wrote: > You probably want to DCHECK in the ctor, if null closures aren't allowed at all. Done. https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_throttler.h (right): https://codereview.chromium.org/624703002/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.h:14: // Throttles Pepper Plugin instances on Power Saver mode. Currently just a On 2014/10/06 18:21:48, raymes wrote: > nit: on->in Done.
tommycli@chromium.org changed reviewers: + thestig@chromium.org
+cc thestig
lgtm with nits https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1116: } nit: don't need {} https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1123: SetPluginThrottled(false /* throttled */); nit: I don't think you need the comment here (and elsewhere), I think it is clear without it. https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1124: } nit: don't need {} https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1129: } I think this block can be made a bit simpler: if (event.type == blink::WebInputEvent::MouseUp && power_saver_enabled_) power_saver_enabled_ = false; if (plugin_throttled_) { if (event.type == blink::WebInputEvent::MouseUp) SetPluginThrottled(false); return; }
lgtm https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_throttler.h (right): https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.h:10: #include "base/memory/weak_ptr.h" do you need macros.h or weak_ptr.h ?
raymes/cpu: thanks@ https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1116: } On 2014/10/06 23:26:02, raymes wrote: > nit: don't need {} Done. https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1123: SetPluginThrottled(false /* throttled */); On 2014/10/06 23:26:02, raymes wrote: > nit: I don't think you need the comment here (and elsewhere), I think it is > clear without it. Done. https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1124: } On 2014/10/06 23:26:02, raymes wrote: > nit: don't need {} Done. https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:1129: } On 2014/10/06 23:26:02, raymes wrote: > I think this block can be made a bit simpler: > > if (event.type == blink::WebInputEvent::MouseUp && power_saver_enabled_) > power_saver_enabled_ = false; > > if (plugin_throttled_) { > if (event.type == blink::WebInputEvent::MouseUp) > SetPluginThrottled(false); > > return; > } > Done. https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_throttler.h (right): https://codereview.chromium.org/624703002/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_throttler.h:10: #include "base/memory/weak_ptr.h" On 2014/10/06 23:46:01, cpu wrote: > do you need macros.h or weak_ptr.h ? need macros.h for DISALLOW_COPY_AND_ASSIGN didn't need weak_ptr
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/624703002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as e4db8f64f5dbbedb681ea757ff5c4fbb42e52d0c
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5576f38d2570bdbcdc1a63b50821bf4d7689ec84 Cr-Commit-Position: refs/heads/master@{#298547} |