Plugin Power Saver: Fix double-counting NEVER case in LoadablePluginPlaceholder.

Plugin Power Saver: Fix double-counting NEVER case in LoadablePluginPlaceholder.

Also:
 - Remove some redundant bool variables from LoadablePluginPlaceholder.
 - Remove LoadablePluginPlaceholder observing the throttler. (No longer necessary).

BUG=482205
Committed: https://crrev.com/fd2d6d70942f4a64845022066098622809c189e9
Cr-Commit-Position: refs/heads/master@{#327572}

[tommycli, 2015-04-28 23:15:51]

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

[tommycli, 2015-04-28 23:16:35]

bauerb: PTAL. This one has negative net SLOC. I think my logic is correct, and
I've manually tested these changes. All the placeholders lack automated tests
though, unfortunately.

Thanks!

Tommy

[tommycli, 2015-04-28 23:28:27]

One more thing:

All these #if defined(ENABLE_PLUGINS) statements make me feel like if
ENABLE_PLUGINS==0, we should not be using LoadablePluginPlaceholder.

In fact, when ENABLE_PLUGINS==0, we cannot load any plugins, so most of the code
in LoadablePluginPlaceholder is dead code.

Would you support making ChromePluginPlaceholder and LoadablePluginPlaceholder
compiled only for ENABLE_PLUGINS and making a NonloadablePluginPlaceholder that
derives directly from PluginPlaceholder?

Tommy

[Bernhard Bauer, 2015-04-29 10:46:00]

On 2015/04/28 23:28:27, tommycli wrote:
> One more thing:
> 
> All these #if defined(ENABLE_PLUGINS) statements make me feel like if
> ENABLE_PLUGINS==0, we should not be using LoadablePluginPlaceholder.
> 
> In fact, when ENABLE_PLUGINS==0, we cannot load any plugins, so most of the
code
> in LoadablePluginPlaceholder is dead code.
> 
> Would you support making ChromePluginPlaceholder and LoadablePluginPlaceholder
> compiled only for ENABLE_PLUGINS and making a NonloadablePluginPlaceholder
that
> derives directly from PluginPlaceholder?
> 
> Tommy

Hm, one thing to note is that we do use ChromePluginPlaceholder even if plugins
are not enabled (to show the "plugin not supported" placeholder), so we would
need to switch the base class depending on ENABLE_PLUGINS...? It's true that
there is probably a lot more cleanup that could be done if ENABLE_PLUGINS is not
defined though. Maybe we should do that first and then see if we can move all
the ENABLE_PLUGINS specific code to a separate class somehow?

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

https://codereview.chromium.org/1111893003/diff/60001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:216: if
(power_saver_enabled_) {
Early-return if |power_saver_enabled_| is false?

[tommycli, 2015-04-29 18:17:28]

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

https://codereview.chromium.org/1111893003/diff/60001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:216: if
(power_saver_enabled_) {
On 2015/04/29 10:46:00, Bernhard Bauer wrote:
> Early-return if |power_saver_enabled_| is false?

Err well we still want to call PluginPlaceholder::PluginDestroyed() at the very
end even if power_saver_enabled_ is false.

[tommycli, 2015-04-29 18:18:43]

Regarding the ENABLE_PLUGINS cleanup: Perhaps it would be best if we used the
Shadow DOM Placeholder business if ENABLE_PLUGINS is false.

The only job of the placeholder in that case would be to show a gray box, since
no plugin loading is possible.

Do you think that would have any memory / performance improvements over loading
a WebViewPlugin?

[Bernhard Bauer, 2015-04-29 18:53:26]

On 2015/04/29 18:18:43, tommycli wrote:
> Regarding the ENABLE_PLUGINS cleanup: Perhaps it would be best if we used the
> Shadow DOM Placeholder business if ENABLE_PLUGINS is false.
> 
> The only job of the placeholder in that case would be to show a gray box,
since
> no plugin loading is possible.
> 
> Do you think that would have any memory / performance improvements over
loading
> a WebViewPlugin?

+jbroman

Dunno, I'm not sure how much resources an additional WebView takes over shadow
DOM. I'm also not completely sure how far along the Shadown DOM plugin
placeholders are, as Jeremy has stopped working on them for now.

Anyway, LGTM.

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

https://codereview.chromium.org/1111893003/diff/60001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:216: if
(power_saver_enabled_) {
On 2015/04/29 18:17:28, tommycli wrote:
> On 2015/04/29 10:46:00, Bernhard Bauer wrote:
> > Early-return if |power_saver_enabled_| is false?
> 
> Err well we still want to call PluginPlaceholder::PluginDestroyed() at the
very
> end even if power_saver_enabled_ is false.

Oh, durr.

[jbroman, 2015-04-29 19:02:07]

On 2015/04/29 18:53:26, Bernhard Bauer wrote:
> On 2015/04/29 18:18:43, tommycli wrote:
> > Regarding the ENABLE_PLUGINS cleanup: Perhaps it would be best if we used
the
> > Shadow DOM Placeholder business if ENABLE_PLUGINS is false.
> > 
> > The only job of the placeholder in that case would be to show a gray box,
> since
> > no plugin loading is possible.
> > 
> > Do you think that would have any memory / performance improvements over
> loading
> > a WebViewPlugin?
> 
> +jbroman
> 
> Dunno, I'm not sure how much resources an additional WebView takes over shadow
> DOM. I'm also not completely sure how far along the Shadown DOM plugin
> placeholders are, as Jeremy has stopped working on them for now.
> 
> Anyway, LGTM.

It would probably save some memory, but not enough that I would bother with it
right now, especially given it's not being actively developed right now.

(I have no numbers to back this up off the top of my head, though.)

>
https://codereview.chromium.org/1111893003/diff/60001/components/plugins/rend...
> File components/plugins/renderer/loadable_plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/1111893003/diff/60001/components/plugins/rend...
> components/plugins/renderer/loadable_plugin_placeholder.cc:216: if
> (power_saver_enabled_) {
> On 2015/04/29 18:17:28, tommycli wrote:
> > On 2015/04/29 10:46:00, Bernhard Bauer wrote:
> > > Early-return if |power_saver_enabled_| is false?
> > 
> > Err well we still want to call PluginPlaceholder::PluginDestroyed() at the
> very
> > end even if power_saver_enabled_ is false.
> 
> Oh, durr.

[tommycli, 2015-04-29 20:03:20]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-04-29 20:04:33]

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

[commit-bot: I haz the power, 2015-04-29 21:41:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-29 21:42:36]

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