Plugin Power Saver: Improve Poster behavior for essential plugins.

Plugin Power Saver: Improve Poster behavior for essential plugins.

This patch does two things:

1) Early exit plugin placeholder if the poster-specified plugin is
   same-origin or whitelisted-origin. The placeholder is never created.

2) Fix bug that made the poster geometry recheck flaky. This fixes one
   bug, but there may be more. The geometry recheck logic is super janky.

BUG=560590
Committed: https://crrev.com/fa5a32e29048ba0be1cbed8221fb74b7731dc55e
Cr-Commit-Position: refs/heads/master@{#364781}

[tommycli, 2015-12-03 00:33:29]

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

[tommycli, 2015-12-03 00:33:29]

groby: PTAL. This patch (purportedly) fixes the essential plugin poster issues
uncovered by the jwplayer staff.

[tommycli, 2015-12-04 20:00:52]

groby: ping, thanks!

[groby-ooo-7-16, 2015-12-04 22:06:56]

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:827: power_saver_enabled &&
render_frame->IsHidden();
Don't we need to fix up power_saver_enabled before we check
blocked_for_background_tab?

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:833: url::Origin(params.url),
-1, -1, nullptr);
Ugh. Three mystery params.

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:843: }
In general, that's a whole chunk of code just dealing with PPS, and this
function is already atrociously long - factor out?

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:186: }
??

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:275: // loading,
request another one. Needed for correct large poster unthrottling.
Is it, even if large posters don't get throttled?

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:149: // |width| and |height| are zoom and
device scale independent logical pixels.
Can we make this gfx::Size?

And we probably want to ignore the sizing if it's empty (i.e. 0x0), unless
there's a specific reason to have 0x0 logical pixels?

https://codereview.chromium.org/1497623002/diff/1/content/renderer/pepper/plu...
File content/renderer/pepper/plugin_power_saver_helper.cc (right):

https://codereview.chromium.org/1497623002/diff/1/content/renderer/pepper/plu...
content/renderer/pepper/plugin_power_saver_helper.cc:105: if
(plugin_size_unknown) {
Can you leave a comment why we record these here?

https://codereview.chromium.org/1497623002/diff/1/content/renderer/peripheral...
File content/renderer/peripheral_content_heuristic.cc (right):

https://codereview.chromium.org/1497623002/diff/1/content/renderer/peripheral...
content/renderer/peripheral_content_heuristic.cc:43: if
(origin_whitelist.count(content_origin))
Since order matters, probably worth having either a comment here, or a test that
checks we have the proper ordering.

[tommycli, 2015-12-07 22:06:18]

groby: Hey - I refactored some stuff based on your feedback. Thanks!

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:827: power_saver_enabled &&
render_frame->IsHidden();
On 2015/12/04 22:06:56, groby wrote:
> Don't we need to fix up power_saver_enabled before we check
> blocked_for_background_tab?

Hmm... actually I think the ordering here is correct, since we should still
defer background-tab same-origin content.

I added a comment to that effect.

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:833: url::Origin(params.url),
-1, -1, nullptr);
On 2015/12/04 22:06:56, groby wrote:
> Ugh. Three mystery params. 

Done.

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:843: }
On 2015/12/04 22:06:56, groby wrote:
> In general, that's a whole chunk of code just dealing with PPS, and this
> function is already atrociously long - factor out?

Done.

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:186: }
On 2015/12/04 22:06:56, groby wrote:
> ??

Done.

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:275: // loading,
request another one. Needed for correct large poster unthrottling.
On 2015/12/04 22:06:56, groby wrote:
> Is it, even if large posters don't get throttled?

Yes, as large posters are throttled, and then become unthrottled.

This is because we can't tell a plugin's actual size until after a placeholder
plugin is placed onto the page and we get a geometry update.

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:149: // |width| and |height| are zoom and
device scale independent logical pixels.
On 2015/12/04 22:06:56, groby wrote:
> Can we make this gfx::Size?
> 
> And we probably want to ignore the sizing if it's empty (i.e. 0x0), unless
> there's a specific reason to have 0x0 logical pixels?

Done. Yeah that's a good idea. That way I can use an empty gfx::Size (0x0) as
the sentinel value indicating unknown size.

This also exposes another issue that I have filed a separate bug for:
https://code.google.com/p/chromium/issues/detail?id=566194&can=3&colspec=ID%2...

https://codereview.chromium.org/1497623002/diff/1/content/renderer/pepper/plu...
File content/renderer/pepper/plugin_power_saver_helper.cc (right):

https://codereview.chromium.org/1497623002/diff/1/content/renderer/pepper/plu...
content/renderer/pepper/plugin_power_saver_helper.cc:105: if
(plugin_size_unknown) {
On 2015/12/04 22:06:56, groby wrote:
> Can you leave a comment why we record these here?

Done. It's very non-obvious.

https://codereview.chromium.org/1497623002/diff/1/content/renderer/peripheral...
File content/renderer/peripheral_content_heuristic.cc (right):

https://codereview.chromium.org/1497623002/diff/1/content/renderer/peripheral...
content/renderer/peripheral_content_heuristic.cc:43: if
(origin_whitelist.count(content_origin))
On 2015/12/04 22:06:56, groby wrote:
> Since order matters, probably worth having either a comment here, or a test
that
> checks we have the proper ordering.

Sounds good. I added a test.

[groby-ooo-7-16, 2015-12-07 22:16:04]

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:827: power_saver_enabled &&
render_frame->IsHidden();
On 2015/12/07 22:06:18, tommycli wrote:
> On 2015/12/04 22:06:56, groby wrote:
> > Don't we need to fix up power_saver_enabled before we check
> > blocked_for_background_tab?
> 
> Hmm... actually I think the ordering here is correct, since we should still
> defer background-tab same-origin content.
> 
> I added a comment to that effect.

Wait - we defer even same-origin on the background tab?

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:275: // loading,
request another one. Needed for correct large poster unthrottling.
On 2015/12/07 22:06:18, tommycli wrote:
> On 2015/12/04 22:06:56, groby wrote:
> > Is it, even if large posters don't get throttled?
> 
> Yes, as large posters are throttled, and then become unthrottled.
> 
> This is because we can't tell a plugin's actual size until after a placeholder
> plugin is placed onto the page and we get a geometry update.

That's... limited happiness. Will that mean poster images will always flash,
even if the thing is clearly large size? Doesn't that mean that poster= is then
a worse experience than just loading the swf and letting us determine the image?

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:149: // |width| and |height| are zoom and
device scale independent logical pixels.
On 2015/12/07 22:06:18, tommycli wrote:
> On 2015/12/04 22:06:56, groby wrote:
> > Can we make this gfx::Size?
> > 
> > And we probably want to ignore the sizing if it's empty (i.e. 0x0), unless
> > there's a specific reason to have 0x0 logical pixels?
> 
> Done. Yeah that's a good idea. That way I can use an empty gfx::Size (0x0) as
> the sentinel value indicating unknown size.
> 
> This also exposes another issue that I have filed a separate bug for:
>
https://code.google.com/p/chromium/issues/detail?id=566194&can=3&colspec=ID%2...

Careful - it _might_ be we need to treat 0x0 plugins differently. I don't think
we do, but it's worth considering that.

[tommycli, 2015-12-07 22:22:13]

groby: see below. thanks!

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:827: power_saver_enabled &&
render_frame->IsHidden();
On 2015/12/07 22:16:04, groby wrote:
> On 2015/12/07 22:06:18, tommycli wrote:
> > On 2015/12/04 22:06:56, groby wrote:
> > > Don't we need to fix up power_saver_enabled before we check
> > > blocked_for_background_tab?
> > 
> > Hmm... actually I think the ordering here is correct, since we should still
> > defer background-tab same-origin content.
> > 
> > I added a comment to that effect.
> 
> Wait - we defer even same-origin on the background tab?

Yep. Same behavior on VIDEO tag now too. The plugin is not loaded until you
foreground the tab for the first time.

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:275: // loading,
request another one. Needed for correct large poster unthrottling.
On 2015/12/07 22:16:04, groby wrote:
> On 2015/12/07 22:06:18, tommycli wrote:
> > On 2015/12/04 22:06:56, groby wrote:
> > > Is it, even if large posters don't get throttled?
> > 
> > Yes, as large posters are throttled, and then become unthrottled.
> > 
> > This is because we can't tell a plugin's actual size until after a
placeholder
> > plugin is placed onto the page and we get a geometry update.
> 
> That's... limited happiness. Will that mean poster images will always flash,
> even if the thing is clearly large size? Doesn't that mean that poster= is
then
> a worse experience than just loading the swf and letting us determine the
image?

This patch makes the "flash" imperceptible on my machine. I will send it to you
after this one finishes. https://codereview.chromium.org/1491893004/

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:149: // |width| and |height| are zoom and
device scale independent logical pixels.
On 2015/12/07 22:16:04, groby wrote:
> On 2015/12/07 22:06:18, tommycli wrote:
> > On 2015/12/04 22:06:56, groby wrote:
> > > Can we make this gfx::Size?
> > > 
> > > And we probably want to ignore the sizing if it's empty (i.e. 0x0), unless
> > > there's a specific reason to have 0x0 logical pixels?
> > 
> > Done. Yeah that's a good idea. That way I can use an empty gfx::Size (0x0)
as
> > the sentinel value indicating unknown size.
> > 
> > This also exposes another issue that I have filed a separate bug for:
> >
>
https://code.google.com/p/chromium/issues/detail?id=566194&can=3&colspec=ID%2...
> 
> Careful - it _might_ be we need to treat 0x0 plugins differently. I don't
think
> we do, but it's worth considering that.

Yeah this comment has gone away. Currently, 0x0 plugins are exempted via
CONTENT_STATUS_ESSENTIAL_UNKNOWN_SIZE.

Since we switched to gfx::Size, (which is nice actually), there is no such thing
as negative sizing. Any negative dimension gets zeroed.

[tommycli, 2015-12-08 23:31:17]

groby: ping thanks

On 2015/12/07 22:22:13, tommycli wrote:
> groby: see below. thanks!
> 
>
https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/1497623002/diff/1/chrome/renderer/chrome_cont...
> chrome/renderer/chrome_content_renderer_client.cc:827: power_saver_enabled &&
> render_frame->IsHidden();
> On 2015/12/07 22:16:04, groby wrote:
> > On 2015/12/07 22:06:18, tommycli wrote:
> > > On 2015/12/04 22:06:56, groby wrote:
> > > > Don't we need to fix up power_saver_enabled before we check
> > > > blocked_for_background_tab?
> > > 
> > > Hmm... actually I think the ordering here is correct, since we should
still
> > > defer background-tab same-origin content.
> > > 
> > > I added a comment to that effect.
> > 
> > Wait - we defer even same-origin on the background tab?
> 
> Yep. Same behavior on VIDEO tag now too. The plugin is not loaded until you
> foreground the tab for the first time.
> 
>
https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
> File components/plugins/renderer/loadable_plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/1497623002/diff/1/components/plugins/renderer...
> components/plugins/renderer/loadable_plugin_placeholder.cc:275: // loading,
> request another one. Needed for correct large poster unthrottling.
> On 2015/12/07 22:16:04, groby wrote:
> > On 2015/12/07 22:06:18, tommycli wrote:
> > > On 2015/12/04 22:06:56, groby wrote:
> > > > Is it, even if large posters don't get throttled?
> > > 
> > > Yes, as large posters are throttled, and then become unthrottled.
> > > 
> > > This is because we can't tell a plugin's actual size until after a
> placeholder
> > > plugin is placed onto the page and we get a geometry update.
> > 
> > That's... limited happiness. Will that mean poster images will always flash,
> > even if the thing is clearly large size? Doesn't that mean that poster= is
> then
> > a worse experience than just loading the swf and letting us determine the
> image?
> 
> This patch makes the "flash" imperceptible on my machine. I will send it to
you
> after this one finishes. https://codereview.chromium.org/1491893004/
> 
>
https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
> File content/public/renderer/render_frame.h (right):
> 
>
https://codereview.chromium.org/1497623002/diff/1/content/public/renderer/ren...
> content/public/renderer/render_frame.h:149: // |width| and |height| are zoom
and
> device scale independent logical pixels.
> On 2015/12/07 22:16:04, groby wrote:
> > On 2015/12/07 22:06:18, tommycli wrote:
> > > On 2015/12/04 22:06:56, groby wrote:
> > > > Can we make this gfx::Size?
> > > > 
> > > > And we probably want to ignore the sizing if it's empty (i.e. 0x0),
unless
> > > > there's a specific reason to have 0x0 logical pixels?
> > > 
> > > Done. Yeah that's a good idea. That way I can use an empty gfx::Size (0x0)
> as
> > > the sentinel value indicating unknown size.
> > > 
> > > This also exposes another issue that I have filed a separate bug for:
> > >
> >
>
https://code.google.com/p/chromium/issues/detail?id=566194&can=3&colspec=ID%2...
> > 
> > Careful - it _might_ be we need to treat 0x0 plugins differently. I don't
> think
> > we do, but it's worth considering that.
> 
> Yeah this comment has gone away. Currently, 0x0 plugins are exempted via
> CONTENT_STATUS_ESSENTIAL_UNKNOWN_SIZE.
> 
> Since we switched to gfx::Size, (which is nice actually), there is no such
thing
> as negative sizing. Any negative dimension gets zeroed.

[groby-ooo-7-16, 2015-12-09 02:17:15]

Outside of various yaks, this looks good to me - but you might want to loop in
an OWNER so it counts :)

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

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:315: PowerSaverInfo
GetPowerSaverInfo(
Dare I suggest that this is now pretty unit-testable?

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:318:
ChromeViewHostMsg_GetPluginInfo_Status plugin_info_status,
I wonder if this should be instead "bool power_saver_setting_on", and you pass
in the result of 
    plugin_info_status ==
ChromeViewHostMsg_GetPluginInfo_Status::kPlayImportantContent

Reason being that at that point the function is pretty much Chrome-independent
and can move to content/

(Welcome to this week's yak shave :)

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:327: // This feature has only
been tested throughly with Flash thus far.
thoroughly

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:347:
render_frame->GetWebFrame()->top()->securityOrigin(),
Weird question: Is that function ever called with a different security origin?
If not, would it make sense for GetPeripheralContentStatus to just get the
securityOrigin locally, since it also lives on render_frame?

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:863: if
(power_saver_info.blocked_for_background_tab || is_prerendering ||
Can we combine the two power_saver_info members into
power_saver_info.should_block?

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

https://codereview.chromium.org/1497623002/diff/60001/content/renderer/pepper...
content/renderer/pepper/plugin_power_saver_helper_browsertest.cc:44:
EXPECT_EQ(RenderFrame::CONTENT_STATUS_PERIPHERAL,
It bugs me these have to be browser tests. Any chance we can move to unit tests?

[tommycli, 2015-12-09 23:25:45]

groby: addressed your comments below, thanks!

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

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:315: PowerSaverInfo
GetPowerSaverInfo(
On 2015/12/09 02:17:15, groby wrote:
> Dare I suggest that this is now pretty unit-testable?

Done. I moved this block of code to its own file.

To unit test it, the render_frame calls are the main obstacle. Since
render_frame is a little too big to easily mock.

We could make a PowerSaverInfo::GetInternal method that takes the render_frame
calls as parameters, and takes GetPeripheralContentStatus as a closure parameter
and instead test that. It's a bit complicated though. Thoughts?

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:318:
ChromeViewHostMsg_GetPluginInfo_Status plugin_info_status,
On 2015/12/09 02:17:15, groby wrote:
> I wonder if this should be instead "bool power_saver_setting_on", and you pass
> in the result of 
>     plugin_info_status ==
> ChromeViewHostMsg_GetPluginInfo_Status::kPlayImportantContent
> 
> Reason being that at that point the function is pretty much Chrome-independent
> and can move to content/
> 
> (Welcome to this week's yak shave :)

Done. Though I didn't move this to content/, since all the callers are in
chrome/, and it would require making a new content/public interface.

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:327: // This feature has only
been tested throughly with Flash thus far.
On 2015/12/09 02:17:15, groby wrote:
> thoroughly

Done.

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:347:
render_frame->GetWebFrame()->top()->securityOrigin(),
On 2015/12/09 02:17:15, groby wrote:
> Weird question: Is that function ever called with a different security origin?
> If not, would it make sense for GetPeripheralContentStatus to just get the
> securityOrigin locally, since it also lives on render_frame?

I think ... we specifically want to get the top-level frame's securityOrigin,
rather than the current render_frame's securityOrigin.

https://codereview.chromium.org/1497623002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:863: if
(power_saver_info.blocked_for_background_tab || is_prerendering ||
On 2015/12/09 02:17:15, groby wrote:
> Can we combine the two power_saver_info members into
> power_saver_info.should_block?

Don't think so. We have to set a separate 'block' for prerendering on line 872
placeholder->set_blocked_for_prerendering(is_prerendering)

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

https://codereview.chromium.org/1497623002/diff/60001/content/renderer/pepper...
content/renderer/pepper/plugin_power_saver_helper_browsertest.cc:44:
EXPECT_EQ(RenderFrame::CONTENT_STATUS_PERIPHERAL,
On 2015/12/09 02:17:15, groby wrote:
> It bugs me these have to be browser tests. Any chance we can move to unit
tests?

For this particular test, I think it needs to be a RenderViewTest (almost a
browser test) to verify that: 1. IPCs are sent, and 2. Navigations clear the
whitelist.

The heuristic tests that don't require a RenderView have already been moved to
this unit test:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[tommycli, 2015-12-09 23:27:15]

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

[tommycli, 2015-12-09 23:27:15]

piman: May I have a content/ review?
thestig: May I have a chrome/ review?

Thanks!

[Lei Zhang, 2015-12-10 01:01:33]

Just to confirm, the chrome/ sade is all refactoring, with no behavior changes,
right? Did I miss anything?

https://codereview.chromium.org/1497623002/diff/80001/chrome/chrome_renderer....
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/1497623002/diff/80001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:179: 'renderer/plugins/power_saver_info.cc',
alphabetical order

https://codereview.chromium.org/1497623002/diff/80001/chrome/renderer/plugins...
File chrome/renderer/plugins/power_saver_info.cc (right):

https://codereview.chromium.org/1497623002/diff/80001/chrome/renderer/plugins...
chrome/renderer/plugins/power_saver_info.cc:44: if (power_saver_enabled)
RecordPosterParamPresence(power_saver_enabled ? FOO_ENABLED : FOO_DISABLED); ?

Same with line 53.

https://codereview.chromium.org/1497623002/diff/80001/chrome/renderer/plugins...
chrome/renderer/plugins/power_saver_info.cc:109:
CONTENT_STATUS_ESSENTIAL_CROSS_ORIGIN_WHITELISTED) {
If you add "using content::RenderFrame", can you get this to fit on the prev
line?

[tommycli, 2015-12-10 23:11:16]

thestig: thanks!

The behavior change is on line 104 here:
https://codereview.chromium.org/1497623002/diff/100001/chrome/renderer/plugin...

If the content is SAME or WHITELISTED origin, disable PPS immediately.

All other things are refactoring.

https://codereview.chromium.org/1497623002/diff/80001/chrome/chrome_renderer....
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/1497623002/diff/80001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:179: 'renderer/plugins/power_saver_info.cc',
On 2015/12/10 01:01:32, Lei Zhang wrote:
> alphabetical order

Done. Whoops, thanks!

https://codereview.chromium.org/1497623002/diff/80001/chrome/renderer/plugins...
File chrome/renderer/plugins/power_saver_info.cc (right):

https://codereview.chromium.org/1497623002/diff/80001/chrome/renderer/plugins...
chrome/renderer/plugins/power_saver_info.cc:44: if (power_saver_enabled)
On 2015/12/10 01:01:33, Lei Zhang wrote:
> RecordPosterParamPresence(power_saver_enabled ? FOO_ENABLED : FOO_DISABLED); ?
> 
> Same with line 53.

Done.

https://codereview.chromium.org/1497623002/diff/80001/chrome/renderer/plugins...
chrome/renderer/plugins/power_saver_info.cc:109:
CONTENT_STATUS_ESSENTIAL_CROSS_ORIGIN_WHITELISTED) {
On 2015/12/10 01:01:33, Lei Zhang wrote:
> If you add "using content::RenderFrame", can you get this to fit on the prev
> line?

Unfortunately it still doesn't fit.

[Lei Zhang, 2015-12-10 23:22:56]

https://codereview.chromium.org/1497623002/diff/100001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/100001/content/public/rendere...
content/public/renderer/render_frame.h:62: CONTENT_STATUS_PERIPHERAL = 0,
Add some comments to describe what these are. Pretend Dave is reading this for
the first time.

https://codereview.chromium.org/1497623002/diff/100001/content/public/rendere...
content/public/renderer/render_frame.h:68: CONTENT_STATUS_NUM_ITEMS
Add a comment saying "must be last"

[tommycli, 2015-12-10 23:30:32]

thestig: thanks!

https://codereview.chromium.org/1497623002/diff/100001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/100001/content/public/rendere...
content/public/renderer/render_frame.h:62: CONTENT_STATUS_PERIPHERAL = 0,
On 2015/12/10 23:22:56, Lei Zhang wrote:
> Add some comments to describe what these are. Pretend Dave is reading this for
> the first time.

Done.

https://codereview.chromium.org/1497623002/diff/100001/content/public/rendere...
content/public/renderer/render_frame.h:68: CONTENT_STATUS_NUM_ITEMS
On 2015/12/10 23:22:56, Lei Zhang wrote:
> Add a comment saying "must be last"

Done.

[Lei Zhang, 2015-12-11 00:07:34]

https://codereview.chromium.org/1497623002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/power_saver_info.cc (right):

https://codereview.chromium.org/1497623002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/power_saver_info.cc:104: if (status ==
content::RenderFrame::CONTENT_STATUS_ESSENTIAL_SAME_ORIGIN ||
So why only these 2 essential types? Why not: if (status !=
CONTENT_STATUS_PERIPHERAL) { // disable } else { ... }

[Lei Zhang, 2015-12-11 00:11:42]

Channeling mark@

https://codereview.chromium.org/1497623002/diff/120001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:72: // Content is essential because we
can't determine the size.
"... because the size can't be determined."

Who is this we that you speak of?

[tommycli, 2015-12-11 00:22:11]

thestig: See if these new comments float your boat a bit better. Thanks.

https://codereview.chromium.org/1497623002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/power_saver_info.cc (right):

https://codereview.chromium.org/1497623002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/power_saver_info.cc:104: if (status ==
content::RenderFrame::CONTENT_STATUS_ESSENTIAL_SAME_ORIGIN ||
On 2015/12/11 00:07:34, Lei Zhang wrote:
> So why only these 2 essential types? Why not: if (status !=
> CONTENT_STATUS_PERIPHERAL) { // disable } else { ... }

I added a comment to this effect.

https://codereview.chromium.org/1497623002/diff/120001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1497623002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:72: // Content is essential because we
can't determine the size.
On 2015/12/11 00:11:42, Lei Zhang wrote:
> "... because the size can't be determined."
> 
> Who is this we that you speak of?

Done.

[Lei Zhang, 2015-12-11 01:38:38]

lgtm

[piman, 2015-12-11 06:57:58]

lgtm

[tommycli, 2015-12-11 18:26:03]

thank you all!

[tommycli, 2015-12-11 18:32:24]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-12-11 18:33:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497623002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497623002/140001

[commit-bot: I haz the power, 2015-12-11 20:06:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-11 20:08:14]

Description was changed from

==========
Plugin Power Saver: Improve Poster behavior for essential plugins.

This patch does two things:

1) Early exit plugin placeholder if the poster-specified plugin is
   same-origin or whitelisted-origin. The placeholder is never created.

2) Fix bug that made the poster geometry recheck flaky. This fixes one
   bug, but there may be more. The geometry recheck logic is super janky.

BUG=560590
==========

to

==========
Plugin Power Saver: Improve Poster behavior for essential plugins.

This patch does two things:

1) Early exit plugin placeholder if the poster-specified plugin is
   same-origin or whitelisted-origin. The placeholder is never created.

2) Fix bug that made the poster geometry recheck flaky. This fixes one
   bug, but there may be more. The geometry recheck logic is super janky.

BUG=560590

Committed: https://crrev.com/fa5a32e29048ba0be1cbed8221fb74b7731dc55e
Cr-Commit-Position: refs/heads/master@{#364781}
==========

[commit-bot: I haz the power, 2015-12-11 20:08:15]

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

[jam, 2015-12-14 19:17:14]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1528653002/ by jam@chromium.org.

The reason for reverting is: Causing lots of flakiness in
PrerenderBrowserTest.PrerenderContentSettingDetect

BUG=569198.