Plugin Power Saver: Add UI Overlay to throttled plugin using placeholder.

Plugin Power Saver: Add UI Overlay to throttled plugin using placeholder.

This patch enables the following flow for throttled plugins:
1. Place real plugin on page, preroll for a few seconds until a keyframe is extracted.
2. On throttle, replace the plugin with a PluginPlaceholder. Set the extracted keyframe as the poster. Meanwhile, the real plugin is hidden, throttled, but still alive.
3. On clicking the placeholder, replace the placeholder with the original throttled plugin.

BUG=443810, 403800
Committed: https://crrev.com/bde09712d6009e38b8c88316237d0736595814f7
Cr-Commit-Position: refs/heads/master@{#314724}

[tommycli, 2015-01-29 20:02:43]

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

[tommycli, 2015-01-29 20:02:43]

bauerb, piman: PTAL

This is a big patch. My apologies. It implements the
preroll->placeholder->plugin flow and allows a UI overlay for the captured
keyframe.

For all of this patch's faults (see below), it actually does work.

There's probably parts of the code that are confusing / undercommented too, let
me know about that.

Please give me some suggestions for improvement. Thanks!

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:59: void
PluginPreroller::OnThrottleStateChange() {
bauerb: Extra scrutiny here. I based this code on
LoadablePluginPlaceholder::ReplacePlugin. I think this method can be simpler
because we're replacing it with LoadablePluginPlaceholder->plugin() which is
more predictable, but i'm not sure.

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:90: void
PluginPreroller::OnThrottlerDestroyed() {
Relying on the fact that when the plugin gets destroyed, so does the throttler.

Not sure if more is needed to prevent leaks, or you can recommend a better
lifetime management strategy.

[groby-ooo-7-16, 2015-01-30 02:22:43]

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

[groby-ooo-7-16, 2015-01-30 02:22:44]

A few drive-by comments.

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:90: void
PluginPreroller::OnThrottlerDestroyed() {
On 2015/01/29 20:02:43, tommycli wrote:
> Relying on the fact that when the plugin gets destroyed, so does the
throttler.
> 
> Not sure if more is needed to prevent leaks, or you can recommend a better
> lifetime management strategy.

If the throttler's lifetime controls the prerollers lifetime, why not have one
own the other? (And if we _have_ to do this, don't bother zeroing out throttler_
- the object is *dead* after delete this, and zeroing out members will at best
mask use-after-free errors)

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:236: if
(!placeholder_was_replaced_ && premade_plugin_) {
Question: Who deletes the premade plugin if the placeholder wasn't replaced yet?

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:239:
premade_throttler_ = nullptr;
Also, shouldn't we stop observing the throttler here?

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.h:39: void ShowContextMenu(const
blink::WebMouseEvent&) override;
why do these need to be public now?

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

https://codereview.chromium.org/866173002/diff/140001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:126:
FOR_EACH_OBSERVER(Observer, observer_list_, OnKeyframeExtracted(bitmap));
Do we expect multiple observers?

https://codereview.chromium.org/866173002/diff/140001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:160:
DCHECK(!keyframe_extraction_timed_out_);
Why do we care about this as a precondition? Will anything go wrong if this is
called twice?

[Bernhard Bauer, 2015-01-30 14:12:57]

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:59: void
PluginPreroller::OnThrottleStateChange() {
On 2015/01/29 20:02:43, tommycli wrote:
> bauerb: Extra scrutiny here. I based this code on
> LoadablePluginPlaceholder::ReplacePlugin. I think this method can be simpler
> because we're replacing it with LoadablePluginPlaceholder->plugin() which is
> more predictable, but i'm not sure.

Yeah... I'll add some comments below.

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:90: void
PluginPreroller::OnThrottlerDestroyed() {
On 2015/01/30 02:22:44, groby wrote:
> On 2015/01/29 20:02:43, tommycli wrote:
> > Relying on the fact that when the plugin gets destroyed, so does the
> throttler.
> > 
> > Not sure if more is needed to prevent leaks, or you can recommend a better
> > lifetime management strategy.
> 
> If the throttler's lifetime controls the prerollers lifetime, why not have one
> own the other?

The Preroller is just an observer of the throttler, and the throttler doesn't
own its observers. It also can't know directly about the Preroller, because the
latter lives in chrome/ and the former in content/ :)

> (And if we _have_ to do this, don't bother zeroing out throttler_
> - the object is *dead* after delete this, and zeroing out members will at best
> mask use-after-free errors)

There is a check in the destructor to only remove ourselves as an observer if
the throttler is still alive; that might be the reason for zeroing it out here.

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

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:862: return
preroller->plugin();
So, we create the PluginPreroller with a plugin, and then we return that plugin.
We could get rid of the accessor by creating the plugin as a local variable,
passing it to the PluginPreroller, and then returning it. Then PluginPreroller
becomes a pure PluginInstanceThrottler::Observer that no one else has to care
about.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:50: std::string
png_as_string(reinterpret_cast<char*>(&png_data[0]),
If encoding the keyframe failed, the vector will be empty, so technically
accessing element 0 would already be out of bounds. Just return in that case?

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:55:
base::Base64Encode(png_as_string, &data_url_body);
base::Base64Encode() takes a base::StringPiece, so if you want to prematurely
optimize things, you could construct a base::StringPiece and save one copy :D

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:60: if (throttler_->IsThrottled()) {
Early-return if the throttler is not throttled? That decreases the indentation
level, and you could put |this| into a scoped_ptr and have it automatically be
destroyed when returning.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:70: // Set the new plug-in on the
container before initializing it.
I don't think the order of calls is important anymore, although keeping the
order as it is probably won't hurt either. Maybe let's just remove the comment,
to not create the impression that the order matters?

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:74: if
(!placeholder->plugin()->initialize(container)) {
WebViewPlugin::initialize() always returns true, so this branch isn't necessary.
(Just DCHECK the return value?)

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:82:
placeholder->plugin()->RestoreTitleText();
This is probably also not necessary, because the plugin didn't have time to
change the title text :)

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:58: std::string keyframe_data_url_;
Any reason you're not storing this directly as a GURL?

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:342: if
(premade_plugin_ && !placeholder_was_replaced_) {
Nit: braces are unnecessary for one-line bodies.

[groby-ooo-7-16, 2015-01-30 15:16:37]

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/100001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:90: void
PluginPreroller::OnThrottlerDestroyed() {
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> On 2015/01/30 02:22:44, groby wrote:
> > On 2015/01/29 20:02:43, tommycli wrote:
> > > Relying on the fact that when the plugin gets destroyed, so does the
> > throttler.
> > > 
> > > Not sure if more is needed to prevent leaks, or you can recommend a better
> > > lifetime management strategy.
> > 
> > If the throttler's lifetime controls the prerollers lifetime, why not have
one
> > own the other?
> 
> The Preroller is just an observer of the throttler, and the throttler doesn't
> own its observers. It also can't know directly about the Preroller, because
the
> latter lives in chrome/ and the former in content/ :)
> 
> > (And if we _have_ to do this, don't bother zeroing out throttler_
> > - the object is *dead* after delete this, and zeroing out members will at
best
> > mask use-after-free errors)
> 
> There is a check in the destructor to only remove ourselves as an observer if
> the throttler is still alive; that might be the reason for zeroing it out
here.

Indeed it is. I missed the call to "delete this" in OnThrottleStateChange :(

[tommycli, 2015-01-30 17:02:40]

groby/bauerb: Thanks for prompt review. This is starting to look a bit more
legit.

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

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:862: return
preroller->plugin();
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> So, we create the PluginPreroller with a plugin, and then we return that
plugin.
> We could get rid of the accessor by creating the plugin as a local variable,
> passing it to the PluginPreroller, and then returning it. Then PluginPreroller
> becomes a pure PluginInstanceThrottler::Observer that no one else has to care
> about.

Done.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:50: std::string
png_as_string(reinterpret_cast<char*>(&png_data[0]),
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> If encoding the keyframe failed, the vector will be empty, so technically
> accessing element 0 would already be out of bounds. Just return in that case?

Done.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:55:
base::Base64Encode(png_as_string, &data_url_body);
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> base::Base64Encode() takes a base::StringPiece, so if you want to prematurely
> optimize things, you could construct a base::StringPiece and save one copy :D

Done.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:60: if (throttler_->IsThrottled()) {
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> Early-return if the throttler is not throttled? That decreases the indentation
> level, and you could put |this| into a scoped_ptr and have it automatically be
> destroyed when returning.

Done. But I think I only want to destroy the preroller when it's throttled.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:70: // Set the new plug-in on the
container before initializing it.
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> I don't think the order of calls is important anymore, although keeping the
> order as it is probably won't hurt either. Maybe let's just remove the
comment,
> to not create the impression that the order matters?

Done.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:74: if
(!placeholder->plugin()->initialize(container)) {
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> WebViewPlugin::initialize() always returns true, so this branch isn't
necessary.
> (Just DCHECK the return value?)

Done.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:82:
placeholder->plugin()->RestoreTitleText();
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> This is probably also not necessary, because the plugin didn't have time to
> change the title text :)

Done.

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/866173002/diff/140001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:58: std::string keyframe_data_url_;
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> Any reason you're not storing this directly as a GURL?

Done.

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:236: if
(!placeholder_was_replaced_ && premade_plugin_) {
On 2015/01/30 02:22:44, groby wrote:
> Question: Who deletes the premade plugin if the placeholder wasn't replaced
yet?

This is the branch I'm counting on to destroy the plugin if the placeholder is
never replaced.

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:239:
premade_throttler_ = nullptr;
On 2015/01/30 02:22:44, groby wrote:
> Also, shouldn't we stop observing the throttler here?

Done. Oops thanks!

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:342: if
(premade_plugin_ && !placeholder_was_replaced_) {
On 2015/01/30 14:12:56, Bernhard Bauer wrote:
> Nit: braces are unnecessary for one-line bodies.

Done.

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/866173002/diff/140001/components/plugins/rend...
components/plugins/renderer/plugin_placeholder.h:39: void ShowContextMenu(const
blink::WebMouseEvent&) override;
On 2015/01/30 02:22:44, groby wrote:
> why do these need to be public now?

These are protected now because LoadablePluginPlaceholder::PluginDestroyed calls
PluginPlacheolder::PluginDestroyed. There's no good reason ShowContextMenu is
protected, I just wanted to keep that block together.

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

https://codereview.chromium.org/866173002/diff/140001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:126:
FOR_EACH_OBSERVER(Observer, observer_list_, OnKeyframeExtracted(bitmap));
On 2015/01/30 02:22:44, groby wrote:
> Do we expect multiple observers? 

In general, yes. For this particular call, there's currently only one observer.
The other observers have noop stub methods.

https://codereview.chromium.org/866173002/diff/140001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:160:
DCHECK(!keyframe_extraction_timed_out_);
On 2015/01/30 02:22:44, groby wrote:
> Why do we care about this as a precondition? Will anything go wrong if this is
> called twice?

Done.

[tommycli, 2015-01-30 17:03:28]

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

[tommycli, 2015-01-30 17:03:29]

thestig: ptal at three files from chrome/ plz. thanks!

[Bernhard Bauer, 2015-01-30 17:11:10]

LGTM!

https://codereview.chromium.org/866173002/diff/150001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/150001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:67: IDR_PLUGIN_POSTER_HTML,
message_, GURL(keyframe_data_url_), plugin_,
The GURL() now isn't necessary anymore.

[tommycli, 2015-01-30 17:45:17]

bernhard: thanks!

https://codereview.chromium.org/866173002/diff/150001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.cc (right):

https://codereview.chromium.org/866173002/diff/150001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.cc:67: IDR_PLUGIN_POSTER_HTML,
message_, GURL(keyframe_data_url_), plugin_,
On 2015/01/30 17:11:10, Bernhard Bauer wrote:
> The GURL() now isn't necessary anymore.

Done.

[Lei Zhang, 2015-01-30 21:50:07]

https://codereview.chromium.org/866173002/diff/170001/chrome/chrome_renderer....
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:41: 'renderer/plugins/chrome_plugin_placeholder.cc',
Probably should try to move some of these files into the
chrome_renderer_plugin_sources section later.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:182: if (premade_plugin &&
premade_throttler)
Can the CreateBlockedPlugin() caller do this instead, since it gets
|blocked_plugin| back anyway.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:15: class WebPlugin;
nit: alphabetical order

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:22: // Once a keyframe has been
extracted, this class will replace it with a
replace it -> replace itself?

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:27: // Does not take ownership of
either |plugin| or |throttler|.
What guarantees do you have that this outlives |plugin| ? What about
|render_frame| and |frame| ?

[tommycli, 2015-02-02 18:35:47]

thestig: addressed your comments.

Also, I was thinking we shouldn't add the 'play' graphic yet, and just reuse the
existing puzzle-piece graphic for this plugin. I don't want to bloat the git
repo of every developer with this graphic that will likely be replaced...

https://codereview.chromium.org/866173002/diff/170001/chrome/chrome_renderer....
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:41: 'renderer/plugins/chrome_plugin_placeholder.cc',
On 2015/01/30 21:50:07, Lei Zhang wrote:
> Probably should try to move some of these files into the
> chrome_renderer_plugin_sources section later.

Yes... kind of. chrome_plugin_placeholder is actually used on Android too... But
yes, some of these can probably be moved.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:182: if (premade_plugin &&
premade_throttler)
On 2015/01/30 21:50:07, Lei Zhang wrote:
> Can the CreateBlockedPlugin() caller do this instead, since it gets
> |blocked_plugin| back anyway.

Done.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:15: class WebPlugin;
On 2015/01/30 21:50:07, Lei Zhang wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:22: // Once a keyframe has been
extracted, this class will replace it with a
On 2015/01/30 21:50:07, Lei Zhang wrote:
> replace it -> replace itself?

Done. I changed the wording a bit.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:27: // Does not take ownership of
either |plugin| or |throttler|.
On 2015/01/30 21:50:07, Lei Zhang wrote:
> What guarantees do you have that this outlives |plugin| ? What about
> |render_frame| and |frame| ?

This isn't meant to outlive those. But |plugin|, |render_frame| and |frame|
should outlive this class.

That's ensured by the OnThrottlerDestroyed call. I hope.

[tommycli, 2015-02-02 20:21:32]

piman: ping for content/ review. Thanks!

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

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:27: // Does not take ownership of
either |plugin| or |throttler|.
On 2015/02/02 18:35:47, tommycli wrote:
> On 2015/01/30 21:50:07, Lei Zhang wrote:
> > What guarantees do you have that this outlives |plugin| ? What about
> > |render_frame| and |frame| ?
> 
> This isn't meant to outlive those. But |plugin|, |render_frame| and |frame|
> should outlive this class.
> 
> That's ensured by the OnThrottlerDestroyed call. I hope.

I can understand the fact that this class lives at most as long as |throttler_|,
and |plugin_| owns |throttler_|, so storing those two pointers is safe. I can't
work out the lifetime relationship between |plugin_| and |render_frame_|, but
|render_frame_| and |frame_| seems to be related.

You may want to be a RenderFrameObserver and observe |render_frame_| destruction
as well.

[tommycli, 2015-02-02 21:49:57]

thestig: Sounds good. That way it can't leak past the lifespan of a RenderFrame.

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/866173002/diff/170001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_preroller.h:27: // Does not take ownership of
either |plugin| or |throttler|.
On 2015/02/02 20:34:09, Lei Zhang wrote:
> On 2015/02/02 18:35:47, tommycli wrote:
> > On 2015/01/30 21:50:07, Lei Zhang wrote:
> > > What guarantees do you have that this outlives |plugin| ? What about
> > > |render_frame| and |frame| ?
> > 
> > This isn't meant to outlive those. But |plugin|, |render_frame| and |frame|
> > should outlive this class.
> > 
> > That's ensured by the OnThrottlerDestroyed call. I hope.
> 
> I can understand the fact that this class lives at most as long as
|throttler_|,
> and |plugin_| owns |throttler_|, so storing those two pointers is safe. I
can't
> work out the lifetime relationship between |plugin_| and |render_frame_|, but
> |render_frame_| and |frame_| seems to be related.
> 
> You may want to be a RenderFrameObserver and observe |render_frame_|
destruction
> as well.

Done.

[Lei Zhang, 2015-02-02 21:58:51]

lgtm

[tommycli, 2015-02-02 22:10:11]

thestig: thanks!

[tommycli, 2015-02-03 22:35:07]

piman: ping for content/ thanks!

[piman, 2015-02-04 01:14:38]

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

https://codereview.chromium.org/866173002/diff/240001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2050:
container_->setWebLayer(nullptr);
When is this called exactly? Why are we doing this?
Taking the layer out of the tree has consequences such as the layer not
committing any more, which has implications wrt graphics callbacks.

[tommycli, 2015-02-04 01:18:04]

piman: thanks, see below

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

https://codereview.chromium.org/866173002/diff/240001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2050:
container_->setWebLayer(nullptr);
On 2015/02/04 01:14:38, piman (Very slow to review) wrote:
> When is this called exactly? Why are we doing this?
> Taking the layer out of the tree has consequences such as the layer not
> committing any more, which has implications wrt graphics callbacks.

Hey,

This is called when we want to keep the plugin alive, but not draw it anymore.
The call path is:

LoadablePluginPlaceholder -> blink::WebPlugin::updateVisibility ->
PepperPluginInstanceImpl::UpdateVisibility.

We want to draw the placeholder instead. When I didn't do this, the 'real'
plugin kept drawing on top.

[piman, 2015-02-04 01:43:32]

On 2015/02/04 01:18:04, tommycli wrote:
> piman: thanks, see below
> 
>
https://codereview.chromium.org/866173002/diff/240001/content/renderer/pepper...
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
> 
>
https://codereview.chromium.org/866173002/diff/240001/content/renderer/pepper...
> content/renderer/pepper/pepper_plugin_instance_impl.cc:2050:
> container_->setWebLayer(nullptr);
> On 2015/02/04 01:14:38, piman (Very slow to review) wrote:
> > When is this called exactly? Why are we doing this?
> > Taking the layer out of the tree has consequences such as the layer not
> > committing any more, which has implications wrt graphics callbacks.
> 
> Hey,
> 
> This is called when we want to keep the plugin alive, but not draw it anymore.
> The call path is:
> 
> LoadablePluginPlaceholder -> blink::WebPlugin::updateVisibility ->
> PepperPluginInstanceImpl::UpdateVisibility.

But this is also called via blink paths in e.g. WebPluginContainerImpl::show
which I think depends on style visibility. I'm not sure of the exact
consequences, but it doesn't look limited to the placeholder.
> 
> We want to draw the placeholder instead. When I didn't do this, the 'real'
> plugin kept drawing on top.

Should this be limited to the placeholder use case then? I'm not sure
repurposing WebPlugin::updateVisibility is the right thing to do.

[tommycli, 2015-02-04 19:58:45]

piman: PTAL. I addressed your comment by removing the updateVisibility hack and
threading the SetHiddenForPlaceholder state through the throttler instead.

Thanks!

[piman, 2015-02-05 00:34:36]

one last thing...

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

https://codereview.chromium.org/866173002/diff/280001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2047: if (hidden) {
thinking about this a little more, you also need to keep this as a state, and
prevent UpdateLayer when it's set.
Otherwise what could happen is that the plugin is set as hidden because of the
placeholder, but then it decides for any reason to bind a new Graphics2D (or
3D), which will then call UpdateLayer and reattach to the tree.
I think you just need to keep a hidden_for_placeholder_ flag, and early exit in
UpdateLayer if it's set.

[tommycli, 2015-02-05 00:46:36]

piman: Sounds good! 

I was slightly concerned by that, but then I decided that UpdateLayer wouldn't
be called except for fullscreen transitions.

I added the change you recommended.

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

https://codereview.chromium.org/866173002/diff/280001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2047: if (hidden) {
On 2015/02/05 00:34:36, piman (Very slow to review) wrote:
> thinking about this a little more, you also need to keep this as a state, and
> prevent UpdateLayer when it's set.
> Otherwise what could happen is that the plugin is set as hidden because of the
> placeholder, but then it decides for any reason to bind a new Graphics2D (or
> 3D), which will then call UpdateLayer and reattach to the tree.
> I think you just need to keep a hidden_for_placeholder_ flag, and early exit
in
> UpdateLayer if it's set.

Done. I put this flag in the throttler, since dmichael told me to try to
minimize additions to PepperPluginInstanceImpl. Let me know if this is fine.

[piman, 2015-02-05 01:15:45]

LGTM. Yeah, that sounds better.

[tommycli, 2015-02-05 01:16:38]

piman: thanks! this was a big patch, thx for sticking it out.

[tommycli, 2015-02-05 01:16:44]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-02-05 01:17:37]

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

[commit-bot: I haz the power, 2015-02-05 02:39:28]

Committed patchset #17 (id:300001)

[commit-bot: I haz the power, 2015-02-05 02:40:32]

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