Plugin Power Saver: Make PepperPluginInstanceThrottler interface public.

Plugin Power Saver: Make PepperPluginInstanceThrottler interface public.

This patch does a few things.

a) Make PepperPluginInstanceThrottler under content/public. We want to add an overlay for throttled Flash instances, and the plugin placeholder seems to be a logical place to do this. We make this interface public so the placeholder knows when it is throttled and how to unthrottle the plugin.

b) Rename PepperPluginInstanceThrottler -> PluginInstanceThrottler. Soon there is no more NPAPI, so shorten the name.

c) Refactor some metrics collection code out of PluginInstanceThrottler and PepperPluginInstanceImpl and into a separate pepper_plugin_instance_metrics file.

BUG=403800
Committed: https://crrev.com/e87227095477cea2725cc9d4cbaa49811455736c
Cr-Commit-Position: refs/heads/master@{#311873}

[tommycli, 2015-01-13 21:25:00]

tommycli@chromium.org changed reviewers:
+ piman@chromium.org

[tommycli, 2015-01-13 21:25:23]

tommycli@chromium.org changed reviewers:
+ bauerb@chromium.org

[tommycli, 2015-01-13 21:25:24]

piman/bauerb: PTAL let me know what you think of this approach

[tommycli, 2015-01-13 21:27:47]

By the way, this patch shouldn't change any functionality or behavior. It's a
refactoring to prepare for the plugin overlay.

[Bernhard Bauer, 2015-01-13 23:29:37]

Looks pretty good, mostly nits:

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
content/public/renderer/render_frame.h:89: // plugin was found. |throttler| may
be NULL.
Nit: s/NULL/nullptr/? :)

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
content/public/renderer/render_frame.h:94: PluginInstanceThrottler* throttler) =
0;
This takes ownership of |throttler|, right? In which case, pass a scoped_ptr?

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:812: if (throttler) {
If this is called with a nullptr and |throttler_| is already set, do we keep the
old one? If that can't happen, can we add a DCHECK?

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:725: // Set to true first
time plugin is clicked. Used to collect metrics.
Nit: "Set to true _the_ first time _the_ plugin is clicked."

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_metrics.cc (right):

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_metrics.cc:16:
UMA_HISTOGRAM_SPARSE_SLOWLY(                          \
I think this should be indented four spaces per the usual rule?

[piman, 2015-01-14 00:09:33]

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:89: void OnImageFlush(const
SkBitmap* bitmap);
Are you actually going to call this and other concrete methods outside of
content/ ?

We generally try to limit the public content interface. This doesn't feel like
it should belong here.

Can you create a virtual interface in public, and have a concrete class as an
implementation detail? See e.g. RenderThread, RenderView, RenderFrame etc.

[tommycli, 2015-01-14 01:41:16]

Patchset #15 (id:280001) has been deleted

[tommycli, 2015-01-14 01:45:00]

bauerb/piman: Addressed your concerns below. Thanks for review!

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:89: void OnImageFlush(const
SkBitmap* bitmap);
On 2015/01/14 00:09:32, piman (Very slow to review) wrote:
> Are you actually going to call this and other concrete methods outside of
> content/ ?
> 
> We generally try to limit the public content interface. This doesn't feel like
> it should belong here.
> 
> Can you create a virtual interface in public, and have a concrete class as an
> implementation detail? See e.g. RenderThread, RenderView, RenderFrame etc.

Done.

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
content/public/renderer/render_frame.h:89: // plugin was found. |throttler| may
be NULL.
On 2015/01/13 23:29:36, Bernhard Bauer wrote:
> Nit: s/NULL/nullptr/? :)

Done.

https://codereview.chromium.org/849723002/diff/220001/content/public/renderer...
content/public/renderer/render_frame.h:94: PluginInstanceThrottler* throttler) =
0;
On 2015/01/13 23:29:36, Bernhard Bauer wrote:
> This takes ownership of |throttler|, right? In which case, pass a scoped_ptr?

Done.

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:812: if (throttler) {
On 2015/01/13 23:29:36, Bernhard Bauer wrote:
> If this is called with a nullptr and |throttler_| is already set, do we keep
the
> old one? If that can't happen, can we add a DCHECK?

Done.

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:725: // Set to true first
time plugin is clicked. Used to collect metrics.
On 2015/01/13 23:29:36, Bernhard Bauer wrote:
> Nit: "Set to true _the_ first time _the_ plugin is clicked."

Done.

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_metrics.cc (right):

https://codereview.chromium.org/849723002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_metrics.cc:16:
UMA_HISTOGRAM_SPARSE_SLOWLY(                          \
On 2015/01/13 23:29:36, Bernhard Bauer wrote:
> I think this should be indented four spaces per the usual rule?

Done. I just used git cl format. Not sure why it's breaking style?

[piman, 2015-01-14 02:08:14]

Mostly nits, otherwise LGTM.

https://codereview.chromium.org/849723002/diff/260001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/260001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:62: static
PluginInstanceThrottler* Create(RenderFrame* frame,
nit: return a scoped_ptr<PluginInstanceThrottler> to clarify ownership.

https://codereview.chromium.org/849723002/diff/260001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:71: virtual bool
is_throttled() const = 0;
style nit: style_with_underscore is reserved for small inline methods. Virtual
methods use CamelCase

https://codereview.chromium.org/849723002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/849723002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:817:
static_cast<PluginInstanceThrottlerImpl*>(throttler.release()));
nit: can you move this cast to RenderFrameImpl and pass a
scoped_ptr<PluginInstanceThrottlerImpl> here, so that it's at a level as high as
possible? To avoid other potential PepperPluginInstanceImpl embedders (e.g.
tests) passing a mocked version or something.

[tommycli, 2015-01-14 18:43:26]

piman/bauerb: Addressed your comments below. Thanks!

https://codereview.chromium.org/849723002/diff/260001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/260001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:62: static
PluginInstanceThrottler* Create(RenderFrame* frame,
On 2015/01/14 02:08:14, piman (Very slow to review) wrote:
> nit: return a scoped_ptr<PluginInstanceThrottler> to clarify ownership.

Done.

https://codereview.chromium.org/849723002/diff/260001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:71: virtual bool
is_throttled() const = 0;
On 2015/01/14 02:08:14, piman (Very slow to review) wrote:
> style nit: style_with_underscore is reserved for small inline methods. Virtual
> methods use CamelCase

Done.

https://codereview.chromium.org/849723002/diff/260001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/849723002/diff/260001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:817:
static_cast<PluginInstanceThrottlerImpl*>(throttler.release()));
On 2015/01/14 02:08:14, piman (Very slow to review) wrote:
> nit: can you move this cast to RenderFrameImpl and pass a
> scoped_ptr<PluginInstanceThrottlerImpl> here, so that it's at a level as high
as
> possible? To avoid other potential PepperPluginInstanceImpl embedders (e.g.
> tests) passing a mocked version or something.

Done.

[tommycli, 2015-01-14 18:44:04]

tommycli@chromium.org changed reviewers:
+ thestig@chromium.org

[tommycli, 2015-01-14 18:44:04]

thestig: May i have a review for
chrome/renderer/chrome_content_renderer_client.cc? Thanks!

[piman, 2015-01-14 19:15:43]

lgtm

https://codereview.chromium.org/849723002/diff/340001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/849723002/diff/340001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:851: render_frame, url,
power_saver_mode).release());
nit: throttler = content::PluginInstanceThrottler::Create(...); should work.

https://codereview.chromium.org/849723002/diff/340001/components/plugins/rend...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/849723002/diff/340001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.cc:258: render_frame(),
plugin_params_.url, power_saver_mode_).release());
ditto

https://codereview.chromium.org/849723002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/849723002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:816:
throttler_.reset(throttler.release());
nit: or throttler_ = throttler.Pass()

[tommycli, 2015-01-14 19:22:06]

piman: excellent, thank you!

https://codereview.chromium.org/849723002/diff/340001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/849723002/diff/340001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:851: render_frame, url,
power_saver_mode).release());
On 2015/01/14 19:15:43, piman (Very slow to review) wrote:
> nit: throttler = content::PluginInstanceThrottler::Create(...); should work.

Done.

https://codereview.chromium.org/849723002/diff/340001/components/plugins/rend...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/849723002/diff/340001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.cc:258: render_frame(),
plugin_params_.url, power_saver_mode_).release());
On 2015/01/14 19:15:43, piman (Very slow to review) wrote:
> ditto

Done.

https://codereview.chromium.org/849723002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/849723002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:816:
throttler_.reset(throttler.release());
On 2015/01/14 19:15:43, piman (Very slow to review) wrote:
> nit: or throttler_ = throttler.Pass()

Done.

[Lei Zhang, 2015-01-14 23:49:33]

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:813: if (status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::
nit: can you pick one style of line breaks and stick with it? (vs line 847)

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:815: plugin.name ==
base::ASCIIToUTF16(content::kFlashPluginName)) {
Is this a behavior change that you didn't mention in the CL description?

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:850: throttler =
content::PluginInstanceThrottler::Create(
You are never creating a throttler for POWER_SAVER_MODE_ESSENTIAL in all the
Create() calls, but plugin_instance_throttler_impl.cc has a "power_saver_mode !=
POWER_SAVER_MODE_ESSENTIAL" comparison that will be always true.

Previously, you always created a throttler.

[tommycli, 2015-01-15 00:26:05]

thestig: Thank you for the fast review!

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:813: if (status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::
On 2015/01/14 23:49:33, Lei Zhang wrote:
> nit: can you pick one style of line breaks and stick with it? (vs line 847)

I'm just doing 'git cl format'. Does it look better after reordering the
conditional?

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:815: plugin.name ==
base::ASCIIToUTF16(content::kFlashPluginName)) {
On 2015/01/14 23:49:33, Lei Zhang wrote:
> Is this a behavior change that you didn't mention in the CL description?

Previous the Flash-only conditional was in the Throttler. Now it's moved out
into here.

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:850: throttler =
content::PluginInstanceThrottler::Create(
On 2015/01/14 23:49:33, Lei Zhang wrote:
> You are never creating a throttler for POWER_SAVER_MODE_ESSENTIAL in all the
> Create() calls, but plugin_instance_throttler_impl.cc has a "power_saver_mode
!=
> POWER_SAVER_MODE_ESSENTIAL" comparison that will be always true.
> 
> Previously, you always created a throttler.

Nice catch! You are right. That was an oversight and very confusing.

Yes, the intent is now that throttlers are no longer created for ESSENTIAL
plugins.

The 'is_peripheral_content_' variable now has a very misleading name. I've
renamed it 'has_been_unthrottled_' and flipped all the conditionals.

[Lei Zhang, 2015-01-15 01:18:38]

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:813: if (status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::
On 2015/01/15 00:26:05, tommycli wrote:
> On 2015/01/14 23:49:33, Lei Zhang wrote:
> > nit: can you pick one style of line breaks and stick with it? (vs line 847)
> 
> I'm just doing 'git cl format'. Does it look better after reordering the
> conditional?

No, but whatever.

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:815: plugin.name ==
base::ASCIIToUTF16(content::kFlashPluginName)) {
On 2015/01/15 00:26:05, tommycli wrote:
> On 2015/01/14 23:49:33, Lei Zhang wrote:
> > Is this a behavior change that you didn't mention in the CL description?
> 
> Previous the Flash-only conditional was in the Throttler. Now it's moved out
> into here.

The comment in content/public/renderer/plugin_instance_throttler.h that mentions
Flash should be updated then?

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:850: throttler =
content::PluginInstanceThrottler::Create(
On 2015/01/15 00:26:05, tommycli wrote:
> On 2015/01/14 23:49:33, Lei Zhang wrote:
> > You are never creating a throttler for POWER_SAVER_MODE_ESSENTIAL in all the
> > Create() calls, but plugin_instance_throttler_impl.cc has a
"power_saver_mode
> !=
> > POWER_SAVER_MODE_ESSENTIAL" comparison that will be always true.
> > 
> > Previously, you always created a throttler.
> 
> Nice catch! You are right. That was an oversight and very confusing.
> 
> Yes, the intent is now that throttlers are no longer created for ESSENTIAL
> plugins.
> 
> The 'is_peripheral_content_' variable now has a very misleading name. I've
> renamed it 'has_been_unthrottled_' and flipped all the conditionals.

It's still a bit unintuitive. We have:

ESSENTIAL: no throttler at all
UNTHROTTLED: throttler created, internally |power_saver_enabled_| is disabled...
why create the throttler in this case?
THROTTLED: throttler created, internally |power_saver_enabled_| is enabled

Also, you can still potentially create a throttler with ESSENTIAL as the power
mode, but it gets treated as UNTHROTTLED. PluginInstanceThrottler::Create()
should at least document this with a DCHECK in
Create/PluginInstanceThrottler{Impl}, or the enum should be a bool, or don't
create it for the unthrottled case and drop the parameter.

https://codereview.chromium.org/849723002/diff/380001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/849723002/diff/380001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:64: //
|power_saver_enabled_| implies |is_peripheral_content_|.
You still reference |is_peripheral_content_| here.

https://codereview.chromium.org/849723002/diff/380001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:69: bool
has_been_unthrottled_;
Now this variable name and |plugin_throttled_| below are more similar in name.
You may want to describe how the 3 bools here are related. There are 8 potential
combinations but I think only 3 or 4 are actually reachable.

[tommycli, 2015-01-15 23:19:33]

thestig: I refactored the throttler's internal state variables based on your
feedback. Thanks!

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:813: if (status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::
On 2015/01/15 01:18:38, Lei Zhang wrote:
> On 2015/01/15 00:26:05, tommycli wrote:
> > On 2015/01/14 23:49:33, Lei Zhang wrote:
> > > nit: can you pick one style of line breaks and stick with it? (vs line
847)
> > 
> > I'm just doing 'git cl format'. Does it look better after reordering the
> > conditional?
> 
> No, but whatever.

Done.

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:815: plugin.name ==
base::ASCIIToUTF16(content::kFlashPluginName)) {
On 2015/01/15 01:18:38, Lei Zhang wrote:
> On 2015/01/15 00:26:05, tommycli wrote:
> > On 2015/01/14 23:49:33, Lei Zhang wrote:
> > > Is this a behavior change that you didn't mention in the CL description?
> > 
> > Previous the Flash-only conditional was in the Throttler. Now it's moved out
> > into here.
> 
> The comment in content/public/renderer/plugin_instance_throttler.h that
mentions
> Flash should be updated then?

Done.

https://codereview.chromium.org/849723002/diff/360001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:850: throttler =
content::PluginInstanceThrottler::Create(
On 2015/01/15 01:18:38, Lei Zhang wrote:
> On 2015/01/15 00:26:05, tommycli wrote:
> > On 2015/01/14 23:49:33, Lei Zhang wrote:
> > > You are never creating a throttler for POWER_SAVER_MODE_ESSENTIAL in all
the
> > > Create() calls, but plugin_instance_throttler_impl.cc has a
> "power_saver_mode
> > !=
> > > POWER_SAVER_MODE_ESSENTIAL" comparison that will be always true.
> > > 
> > > Previously, you always created a throttler.
> > 
> > Nice catch! You are right. That was an oversight and very confusing.
> > 
> > Yes, the intent is now that throttlers are no longer created for ESSENTIAL
> > plugins.
> > 
> > The 'is_peripheral_content_' variable now has a very misleading name. I've
> > renamed it 'has_been_unthrottled_' and flipped all the conditionals.
> 
> It's still a bit unintuitive. We have:
> 
> ESSENTIAL: no throttler at all
> UNTHROTTLED: throttler created, internally |power_saver_enabled_| is
disabled...
> why create the throttler in this case?
> THROTTLED: throttler created, internally |power_saver_enabled_| is enabled
> 
> Also, you can still potentially create a throttler with ESSENTIAL as the power
> mode, but it gets treated as UNTHROTTLED. PluginInstanceThrottler::Create()
> should at least document this with a DCHECK in
> Create/PluginInstanceThrottler{Impl}, or the enum should be a bool, or don't
> create it for the unthrottled case and drop the parameter.

Done.

Okay this comment helped clarify my thinking.

The throttler previously had 4 bools. But there's only 4 accessible states,
which I've now explicitly enumerated.

And yes, since the throttler is now no longer needed for the ESSENTIAL case,
I've replaced the argument with a bool.

I still need to pass a tri-state 'mode' to the placeholder though, so I've
preserved that enum. Check out the inter-patchset deltas.

https://codereview.chromium.org/849723002/diff/380001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/849723002/diff/380001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:64: //
|power_saver_enabled_| implies |is_peripheral_content_|.
On 2015/01/15 01:18:38, Lei Zhang wrote:
> You still reference |is_peripheral_content_| here.

Done.

https://codereview.chromium.org/849723002/diff/380001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:69: bool
has_been_unthrottled_;
On 2015/01/15 01:18:38, Lei Zhang wrote:
> Now this variable name and |plugin_throttled_| below are more similar in name.
> You may want to describe how the 3 bools here are related. There are 8
potential
> combinations but I think only 3 or 4 are actually reachable.

Done.

[Lei Zhang, 2015-01-16 00:19:32]

https://codereview.chromium.org/849723002/diff/440001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/440001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:29: // the plugin instance
will never actually be throttled, but we will still
nit: s/we will still collect/still collects/

https://codereview.chromium.org/849723002/diff/440001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:64: // Return a nullptr if
no throttler needed based on |power_saver_mode|.
nit: Returns

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:95:
DCHECK(needs_representative_keyframe());
Can this DCHECK fail? What if the MarkPluginEssential() happens before the
plugin flushes?

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:24: class
CONTENT_EXPORT PluginInstanceThrottlerImpl
You may want to define "essential" and "peripheral", and mention this class only
deals with peripheral plugins.

[tommycli, 2015-01-16 00:51:56]

thestig: Thanks!

https://codereview.chromium.org/849723002/diff/440001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/440001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:29: // the plugin instance
will never actually be throttled, but we will still
On 2015/01/16 00:19:32, Lei Zhang wrote:
> nit: s/we will still collect/still collects/

Done.

https://codereview.chromium.org/849723002/diff/440001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:64: // Return a nullptr if
no throttler needed based on |power_saver_mode|.
On 2015/01/16 00:19:32, Lei Zhang wrote:
> nit: Returns

Done.

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.cc (right):

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:95:
DCHECK(needs_representative_keyframe());
On 2015/01/16 00:19:32, Lei Zhang wrote:
> Can this DCHECK fail? What if the MarkPluginEssential() happens before the
> plugin flushes?

It can't fail right now. The only caller is pepper_graphics_2d_host.cc and that
checks that needs_representative_keyframe() is true in its conditional.

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
File content/renderer/pepper/plugin_instance_throttler_impl.h (right):

https://codereview.chromium.org/849723002/diff/440001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.h:24: class
CONTENT_EXPORT PluginInstanceThrottlerImpl
On 2015/01/16 00:19:32, Lei Zhang wrote:
> You may want to define "essential" and "peripheral", and mention this class
only
> deals with peripheral plugins.

Done. I added to the comments in the plugin_instance_throttler.h file.

[Lei Zhang, 2015-01-16 01:09:58]

https://codereview.chromium.org/849723002/diff/480001/components/plugins/rend...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/849723002/diff/480001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.cc:251: #if
defined(ENABLE_PLUGINS)
Can you see if this works for platforms without ENABLE_PLUGINS? Same for
chrome_content_renderer_client.cc.

scoped_ptr<content::PluginInstanceThrottler> throttler;
#if defined(ENABLE_PLUGINS)
throttler = content::PluginInstanceThrottler::Get(...);
#endif
CreatePlugin(..., throttler.Pass());

https://codereview.chromium.org/849723002/diff/480001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.cc:260: // TODO(mmenke):  In the
case of prerendering, feed into
Is this referring to the CreatePlugin() calls, which you have moved above the
comment?

https://codereview.chromium.org/849723002/diff/480001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/480001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:19: // not "essential" to
the web page content. See RenderFrame comments for
Which "RenderFrame comments" are we referring to?

[tommycli, 2015-01-16 01:26:10]

thestig: thanks!

https://codereview.chromium.org/849723002/diff/480001/components/plugins/rend...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/849723002/diff/480001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.cc:251: #if
defined(ENABLE_PLUGINS)
On 2015/01/16 01:09:58, Lei Zhang wrote:
> Can you see if this works for platforms without ENABLE_PLUGINS? Same for
> chrome_content_renderer_client.cc.
> 
> scoped_ptr<content::PluginInstanceThrottler> throttler;
> #if defined(ENABLE_PLUGINS)
> throttler = content::PluginInstanceThrottler::Get(...);
> #endif
> CreatePlugin(..., throttler.Pass());

Done.

https://codereview.chromium.org/849723002/diff/480001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.cc:260: // TODO(mmenke):  In the
case of prerendering, feed into
On 2015/01/16 01:09:58, Lei Zhang wrote:
> Is this referring to the CreatePlugin() calls, which you have moved above the
> comment?

Done.

https://codereview.chromium.org/849723002/diff/480001/content/public/renderer...
File content/public/renderer/plugin_instance_throttler.h (right):

https://codereview.chromium.org/849723002/diff/480001/content/public/renderer...
content/public/renderer/plugin_instance_throttler.h:19: // not "essential" to
the web page content. See RenderFrame comments for
On 2015/01/16 01:09:58, Lei Zhang wrote:
> Which "RenderFrame comments" are we referring to?

Done.

[Lei Zhang, 2015-01-16 01:33:46]

lgtm

[tommycli, 2015-01-16 01:35:42]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-01-16 01:36:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849723002/500001

[commit-bot: I haz the power, 2015-01-16 01:40:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 01:40:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bernhard Bauer, 2015-01-16 09:24:22]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-01-16 09:24:23]

lgtm

[commit-bot: I haz the power, 2015-01-16 09:24:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849723002/500001

[commit-bot: I haz the power, 2015-01-16 11:41:09]

Committed patchset #25 (id:500001)

[commit-bot: I haz the power, 2015-01-16 11:42:49]

Patchset 25 (id:??) landed as
https://crrev.com/e87227095477cea2725cc9d4cbaa49811455736c
Cr-Commit-Position: refs/heads/master@{#311873}