Plugin Power Saver: Fix implicitly sized and below the fold plugins.

Plugin Power Saver: Fix implicitly sized and below the fold plugins.

This patch changes a few behaviors.

1. Make implicitly sized (e.g. width="100%") plugins work with Plugin Power Saver. Previously sizes were read from plugin parameters, which doesn't work for implicitly sized plugins. Now, we look at the actual blink::WebRect view bounds.

2. Timeout keyframe extraction after 150 frames rather than a fixed 5 seconds. The previous behavior was broken for plugins that are loaded off-screen. (The countdown begins even though no frames are being generated until the plugin goes on-screen).

3. When "Detect and run..." is on, always respect the 'poster' parameter. This is a tweak to the existing behavior, which is to use the 'poster' param only for peripheral plugins. This should have no user impact, since no one actually uses the 'poster' param right now.

4. Clean up metric collection in LoadablePluginPlaceholder. Previous behavior was overcounting in some circumstances.

5. Reduces RenderFrame public interface and total SLOC.

BUG=443431, 456217, 403800
Committed: https://crrev.com/d7798e13906ebb1b531043738446eb378da369b1
Cr-Commit-Position: refs/heads/master@{#315393}

[tommycli, 2015-02-06 19:15:01]

tommycli@chromium.org changed reviewers:
+ bauerb@chromium.org, piman@chromium.org, thestig@chromium.org

[tommycli, 2015-02-06 19:15:01]

tommycli@chromium.org changed reviewers:
+ bauerb@chromium.org, piman@chromium.org, thestig@chromium.org

[tommycli, 2015-02-06 19:15:44]

piman/thestig/bauerb: PTAL

This is mostly a bugfix and refactor CL. Behavior changes are documented in the
description, but nothing too major.

[piman, 2015-02-06 22:13:12]

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

https://codereview.chromium.org/904913003/diff/80001/content/public/renderer/...
content/public/renderer/plugin_instance_throttler.h:69: const GURL&
page_base_url);
Why is this even here? It doesn't need anything from content/ and could be
implemented directly in ChromeContentRendererClient?

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

https://codereview.chromium.org/904913003/diff/80001/content/renderer/pepper/...
content/renderer/pepper/plugin_instance_throttler_impl.cc:29: const char
kPosterParamName[] = "poster";
We're exposing this to the web, I think this should go through the blink-dev
intents process... I don't think that we've done that? This CL is a refactoring
so I won't block it, but we should follow through.

[tommycli, 2015-02-06 22:57:10]

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

https://codereview.chromium.org/904913003/diff/80001/content/public/renderer/...
content/public/renderer/plugin_instance_throttler.h:69: const GURL&
page_base_url);
On 2015/02/06 22:13:12, piman (Very slow to review) wrote:
> Why is this even here? It doesn't need anything from content/ and could be
> implemented directly in ChromeContentRendererClient?

Done. It was to support a unit test. But I think the method is trivial enough to
not require a unit test, so I moved it.

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

https://codereview.chromium.org/904913003/diff/80001/content/renderer/pepper/...
content/renderer/pepper/plugin_instance_throttler_impl.cc:29: const char
kPosterParamName[] = "poster";
On 2015/02/06 22:13:12, piman (Very slow to review) wrote:
> We're exposing this to the web, I think this should go through the blink-dev
> intents process... I don't think that we've done that? This CL is a
refactoring
> so I won't block it, but we should follow through.

Okay I will follow through on this. I will email blink-dev right now.

[tommycli, 2015-02-06 22:58:20]

piman:

Actually are you sure we need to contact blink-dev? It's not a new attribute. It
works like:

<object type="application/x-shockwave-flash">
    <param name="movie" value="watch.swf">
    <param name="allowfullscreen" value="true">
    <param name="flashvars" value="video.mp4">
    <param name="poster" value="videoposter.jpg">
    <video controls src="video.mp4" poster="videoposter.jpg">
      <a href="video.mp4">View video</a>.
    </video>
</object>


It leverages the existing param structure, which can contain anything.

[piman, 2015-02-07 00:02:13]

lgtm

https://codereview.chromium.org/904913003/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/plugin_instance_throttler_impl_unittest.cc (right):

https://codereview.chromium.org/904913003/diff/80001/content/renderer/pepper/...
content/renderer/pepper/plugin_instance_throttler_impl_unittest.cc:83:
TEST_F(PluginInstanceThrottlerImplTest, PosterImage) {
This could still go to
chrome/renderer/chrome_content_renderer_client_unittest.cc

[piman, 2015-02-07 00:05:49]

On Fri, Feb 6, 2015 at 2:58 PM, <tommycli@chromium.org> wrote:

> piman:
>
> Actually are you sure we need to contact blink-dev? It's not a new
> attribute. It
> works like:
>
> <object type="application/x-shockwave-flash">
>     <param name="movie" value="watch.swf">
>     <param name="allowfullscreen" value="true">
>     <param name="flashvars" value="video.mp4">
>     <param name="poster" value="videoposter.jpg">
>     <video controls src="video.mp4" poster="videoposter.jpg">
>       <a href="video.mp4">View video</a>.
>     </video>
> </object>
>
>
> It leverages the existing param structure, which can contain anything.
>

It's actually an attribute when on an <embed> tag instead of an <object>
tag.
But regardless of the form, the point is that we're adding a new API to the
web, that we need to support forever, and there's compatibility concerns to
think about, both from the pov of cross-browser standardization and from
the pov of existing content, where "poster" might already be used, since
params are passed to the plugin content. blink-dev is the proper place to
discuss that.


>
> https://codereview.chromium.org/904913003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2015-02-09 11:52:28]

lgtm

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

https://codereview.chromium.org/904913003/diff/60002/content/renderer/pepper/...
content/renderer/pepper/plugin_instance_throttler_impl.h:29: static const int
kMaximumFramesToExamine;
Make this private, seeing that it's only used outside of this class in the test,
which is already a friend?

[tommycli, 2015-02-09 19:21:21]

New patchsets have been uploaded after l-g-t-m from
piman@chromium.org,bauerb@chromium.org

[tommycli, 2015-02-09 19:22:11]

bauerb: Moved constant to the private. Had to add a new field to the test class
to make it work.

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

https://codereview.chromium.org/904913003/diff/60002/content/renderer/pepper/...
content/renderer/pepper/plugin_instance_throttler_impl.h:29: static const int
kMaximumFramesToExamine;
On 2015/02/09 11:52:27, Bernhard Bauer wrote:
> Make this private, seeing that it's only used outside of this class in the
test,
> which is already a friend?

Done.

[tommycli, 2015-02-09 19:22:31]

thestig: ptal, thanks!

[Lei Zhang, 2015-02-09 20:02:05]

https://codereview.chromium.org/904913003/diff/90001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/904913003/diff/90001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:277: if
(params.attributeNames[i] == "poster") {
Should this be params.attributeNames[i].utf8() ? Not obvious how the WebString
vs const char [] comparison actually happens.

https://codereview.chromium.org/904913003/diff/90001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:279: if
(!poster_value.empty())
You can call isEmpty() on the WebString, and only convert it to a std::string if
it's not empty.

[tommycli, 2015-02-09 20:12:17]

thestig: addressed your comments. Thanks!

https://codereview.chromium.org/904913003/diff/90001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/904913003/diff/90001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:277: if
(params.attributeNames[i] == "poster") {
On 2015/02/09 20:02:05, Lei Zhang wrote:
> Should this be params.attributeNames[i].utf8() ? Not obvious how the WebString
> vs const char [] comparison actually happens.

Done.

https://codereview.chromium.org/904913003/diff/90001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:279: if
(!poster_value.empty())
On 2015/02/09 20:02:05, Lei Zhang wrote:
> You can call isEmpty() on the WebString, and only convert it to a std::string
if
> it's not empty.

Done.

[Lei Zhang, 2015-02-09 20:18:23]

lgtm

[tommycli, 2015-02-09 20:19:06]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-02-09 20:20:36]

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

[commit-bot: I haz the power, 2015-02-09 21:09:12]

Committed patchset #8 (id:110001)

[commit-bot: I haz the power, 2015-02-09 21:10:19]

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