Plugin Power Saver: Always mark 'tiny' plugins as essential.

Plugin Power Saver: Always mark 'tiny' plugins as essential.

Tiny plugins often serve a vital purpose on a page, and cannot be effectively clicked by a user. Therefore are always marked as essential.

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

[tommycli, 2014-12-02 18:00:59]

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

[tommycli, 2014-12-02 18:01:24]

groby: let me know if this does the trick for you

[groby-ooo-7-16, 2014-12-02 18:10:45]

I don't think small plugins should trigger the cross-origin whitelisting. (For
the obvious reason that it otherwise allows easy sidestepping of the heuristic)

[tommycli, 2014-12-02 18:32:12]

groby: you are correct. See if this new version looks better.

[groby-ooo-7-16, 2014-12-03 00:57:27]

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_browsertest.cc (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_browsertest.cc:9: #include
"content/renderer/pepper/plugin_power_saver_helper_impl.h"
I know it's not directly relevant to this CL, but: Do you ever need the Impl
class here? Shouldn't this test against the base class interface?

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_impl.cc (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.cc:173: bool*
is_main_attraction) const {
You probably want to ensure a defined value for |is_main_attraction|, and not
rely on the caller to set that. 

It is always a result that should be determined by this function only, not an
accumulator that cares about previous state.

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_impl.h (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.h:44: // is cross-origin
and determined to be the "main attraction" of the page.
Is there a reason why we can't handle the whitelisting inside of PPSH? Does
anybody care about |is_main_attraction|, except to handle whitelisting?

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.h:48: bool*
is_main_attraction) const;
You should probably rename it in plugin_power_saver_helper.h as well.  In fact,
since that's the public API, you'd probably do well to just move all the
comments there, and remove comments here. (Otherwise you'll always maintain two
sets of comments)

[tommycli, 2014-12-03 19:34:13]

groby: Addressed your comments. Wasn't able to make all the changes you
requested. See below.

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_browsertest.cc (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_browsertest.cc:9: #include
"content/renderer/pepper/plugin_power_saver_helper_impl.h"
On 2014/12/03 00:57:27, groby wrote:
> I know it's not directly relevant to this CL, but: Do you ever need the Impl
> class here? Shouldn't this test against the base class interface?

I cannot, as the content/ owners requested that I pull ShouldThrottleContent and
WhitelistContentOrigin outside of the content-public interface.

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_impl.cc (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.cc:173: bool*
is_main_attraction) const {
On 2014/12/03 00:57:27, groby wrote:
> You probably want to ensure a defined value for |is_main_attraction|, and not
> rely on the caller to set that. 
> 
> It is always a result that should be determined by this function only, not an
> accumulator that cares about previous state.

Done.

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_impl.h (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.h:44: // is cross-origin
and determined to be the "main attraction" of the page.
On 2014/12/03 00:57:27, groby wrote:
> Is there a reason why we can't handle the whitelisting inside of PPSH? Does
> anybody care about |is_main_attraction|, except to handle whitelisting?

Originally it handled the whitelisting within this function. I pulled that out
because thestig recommended this method be 'const' and we have a separate
whitelisting function.

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.h:48: bool*
is_main_attraction) const;
On 2014/12/03 00:57:27, groby wrote:
> You should probably rename it in plugin_power_saver_helper.h as well.  In
fact,
> since that's the public API, you'd probably do well to just move all the
> comments there, and remove comments here. (Otherwise you'll always maintain
two
> sets of comments)

This doesn't exist in the public interface due to content/ owners requesting a
minimum number of exposed functions.

[tommycli, 2014-12-03 19:38:07]

tommycli@chromium.org changed reviewers:
+ dmichael@chromium.org, isherman@chromium.org

[tommycli, 2014-12-03 19:38:07]

dmichael: May I have a content/renderer/pepper review?
isherman: May I have a tools/ review? 

Thanks!

[Ilya Sherman, 2014-12-03 19:38:39]

histograms.xml lgtm

[dmichael (off chromium), 2014-12-03 20:33:46]

pepper lgtm when it lgtgroby

[tommycli, 2014-12-03 20:39:26]

dmichael, isherman: thanks guys. Yes I will wait on groby to take a look before
CQ.

[groby-ooo-7-16, 2014-12-04 02:42:58]

The CQ bit was checked by groby@chromium.org

[groby-ooo-7-16, 2014-12-04 02:42:59]

lgtm

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_browsertest.cc (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_browsertest.cc:9: #include
"content/renderer/pepper/plugin_power_saver_helper_impl.h"
On 2014/12/03 19:34:12, tommycli wrote:
> On 2014/12/03 00:57:27, groby wrote:
> > I know it's not directly relevant to this CL, but: Do you ever need the Impl
> > class here? Shouldn't this test against the base class interface?
> 
> I cannot, as the content/ owners requested that I pull ShouldThrottleContent
and
> WhitelistContentOrigin outside of the content-public interface.

Grumble mumble. Fine. At some point we'll need to find a single home for this,
either chrome/ or content/. The boundary skipping seems to overcomplicate
things. (But that's neither here nor there for the CL, so ignore)

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/plugin_power_saver_helper_impl.h (right):

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.h:44: // is cross-origin
and determined to be the "main attraction" of the page.
On 2014/12/03 19:34:12, tommycli wrote:
> On 2014/12/03 00:57:27, groby wrote:
> > Is there a reason why we can't handle the whitelisting inside of PPSH? Does
> > anybody care about |is_main_attraction|, except to handle whitelisting?
> 
> Originally it handled the whitelisting within this function. I pulled that out
> because thestig recommended this method be 'const' and we have a separate
> whitelisting function.

Let's take this out of this CL, but I don't like that we have split
responsibilities for whitelisting. (Plus, "tell don't ask" applies for state
queries)

https://codereview.chromium.org/764763005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/plugin_power_saver_helper_impl.h:48: bool*
is_main_attraction) const;
On 2014/12/03 19:34:12, tommycli wrote:
> On 2014/12/03 00:57:27, groby wrote:
> > You should probably rename it in plugin_power_saver_helper.h as well.  In
> fact,
> > since that's the public API, you'd probably do well to just move all the
> > comments there, and remove comments here. (Otherwise you'll always maintain
> two
> > sets of comments)
> 
> This doesn't exist in the public interface due to content/ owners requesting a
> minimum number of exposed functions.

Ah - I'm still operating on outdated codesearch results ;) So yes, no need to
rename.

[commit-bot: I haz the power, 2014-12-04 02:43:27]

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

[commit-bot: I haz the power, 2014-12-04 02:45:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-04 02:46:30]

Patchset 5 (id:??) landed as
https://crrev.com/94f90ec84580a0abebdb44184cbcd60f8e1d62ca
Cr-Commit-Position: refs/heads/master@{#306751}