Plugin Power Saver: Unthrottle dynamically sized plugins correctly.

Plugin Power Saver: Unthrottle dynamically sized plugins correctly.

This is to unthrottle plugins that start small but then are resized by Javascript to be large.

Notably it fixes the weatherspark.com use case. (Resize your browser window to be 1270x760 ish to test)

BUG=489376
Committed: https://crrev.com/4f7f69cb743e3b1e7572c5b04d6d7af4c498d254
Cr-Commit-Position: refs/heads/master@{#333790}

[tommycli, 2015-06-05 21:57:44]

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

[tommycli, 2015-06-05 21:57:54]

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

[tommycli, 2015-06-05 21:58:02]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 21:58:26]

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

[commit-bot: I haz the power, 2015-06-05 22:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:15:21]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[tommycli, 2015-06-05 22:19:20]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 22:21:33]

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

[tommycli, 2015-06-05 22:23:05]

groby: This hacky hack does fix the weatherspark case. 

It has to work around the Blink bug. After a resize notification, it does a size
recheck after 100ms to discard spurious size updates (like with the YT
masthead).

Let me know if you have any thoughts. O/w I will send this off to be reviewed
soon.

I think we should probably sunset this hacky hack next year.

[commit-bot: I haz the power, 2015-06-05 22:41:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:41:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[groby-ooo-7-16, 2015-06-05 23:03:45]

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

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:51: //
TODO(tommycli): After an unthrottling size update, re-check the size after
#if defined(ENABLE_PLUGINS)

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:53: // compositing
state is dirty. Chosen because it seems to work.
Please add a bug to undo this. I don't trust TODO's :)

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:288: if
(in_size_recheck_)
Why do we have this test?

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:296: // compositing
state is dirty. https://crbug.com/343769
Once that bug is gone, does this whole machinery vanish?

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:437:
DCHECK(!in_size_recheck_);
Do we need to protect against reentrancy?

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
File components/plugins/renderer/loadable_plugin_placeholder.h (right):

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.h:144:
base::WeakPtrFactory<LoadablePluginPlaceholder> size_update_weak_factory_;
You can skip the WeakPtrFactory thing, and instead just store your callback in a
CancelableCallback.

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

https://codereview.chromium.org/1124173008/diff/80001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:25: // peripheral
content. These are similar to the numbers used by WebKit.
Last sentence can probably go

[tommycli, 2015-06-05 23:05:03]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 23:07:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124173008/100001

[tommycli, 2015-06-05 23:37:12]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 23:39:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124173008/140001

[tommycli, 2015-06-06 01:23:22]

groby: Made some changes based on your suggestions below.

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

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:51: //
TODO(tommycli): After an unthrottling size update, re-check the size after
On 2015/06/05 23:03:45, groby wrote:
> #if defined(ENABLE_PLUGINS)

Done.

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:53: // compositing
state is dirty. Chosen because it seems to work.
On 2015/06/05 23:03:45, groby wrote:
> Please add a bug to undo this. I don't trust TODO's :)

Done.

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:288: if
(in_size_recheck_)
On 2015/06/05 23:03:45, groby wrote:
> Why do we have this test?

Done. Added comment above.

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:296: // compositing
state is dirty. https://crbug.com/343769
On 2015/06/05 23:03:45, groby wrote:
> Once that bug is gone, does this whole machinery vanish?

Yes, if there was no bug, and the reported plugin size was always accurate,
there would be no reason to do a delayed size recheck. We would just
MarkPluginEssential directly here.

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:437:
DCHECK(!in_size_recheck_);
On 2015/06/05 23:03:45, groby wrote:
> Do we need to protect against reentrancy?

Uh.... well I hope this never happens. If re-entrancy does happen, I'm not sure
what the behavior is.

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
File components/plugins/renderer/loadable_plugin_placeholder.h (right):

https://codereview.chromium.org/1124173008/diff/80001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.h:144:
base::WeakPtrFactory<LoadablePluginPlaceholder> size_update_weak_factory_;
On 2015/06/05 23:03:45, groby wrote:
> You can skip the WeakPtrFactory thing, and instead just store your callback in
a
> CancelableCallback.

Delaying this change per offline discussion.

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

https://codereview.chromium.org/1124173008/diff/80001/content/renderer/pepper...
content/renderer/pepper/plugin_instance_throttler_impl.cc:25: // peripheral
content. These are similar to the numbers used by WebKit.
On 2015/06/05 23:03:45, groby wrote:
> Last sentence can probably go

Done.

[commit-bot: I haz the power, 2015-06-06 01:42:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-06 01:42:59]

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-06-09 00:37:15]

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

[tommycli, 2015-06-09 23:49:39]

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

[tommycli, 2015-06-09 23:49:40]

bauerb, piman, isherman: PTAL, thanks!

[Ilya Sherman, 2015-06-09 23:51:27]

histograms.xml lgtm

[piman, 2015-06-10 00:50:55]

LGTM for content/, I didn't review the rest

[Bernhard Bauer, 2015-06-10 11:13:24]

LGTM w/ nits:

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

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
components/plugins/renderer/loadable_plugin_placeholder.cc:49: namespace {
Nit: Anonymous namespace isn't necessary for constants, as they automatically
get internal linkage. If there is nothing else that warrants an anonymous
namespace, I would remove it.

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
components/plugins/renderer/loadable_plugin_placeholder.cc:299: if
(!size_update_weak_factory_.HasWeakPtrs()) {
You could use a base::Timer for this instead of the second WeakPtrFactory.

[tommycli, 2015-06-10 18:55:52]

isherman, piman, bauerb: thanks all for quick review!

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

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
components/plugins/renderer/loadable_plugin_placeholder.cc:49: namespace {
On 2015/06/10 11:13:24, Bernhard Bauer wrote:
> Nit: Anonymous namespace isn't necessary for constants, as they automatically
> get internal linkage. If there is nothing else that warrants an anonymous
> namespace, I would remove it.

Done.

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
components/plugins/renderer/loadable_plugin_placeholder.cc:299: if
(!size_update_weak_factory_.HasWeakPtrs()) {
On 2015/06/10 11:13:24, Bernhard Bauer wrote:
> You could use a base::Timer for this instead of the second WeakPtrFactory.

Done.

[tommycli, 2015-06-10 18:56:14]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2015-06-10 18:56:15]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
isherman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1124173008/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-06-10 18:57:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124173008/180001

[commit-bot: I haz the power, 2015-06-10 20:20:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 20:20:45]

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-06-10 20:30:51]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-06-10 20:31:13]

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

[commit-bot: I haz the power, 2015-06-10 20:36:26]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-10 20:37:20]

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

[groby-ooo-7-16, 2015-06-11 23:32:56]

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
File components/plugins/renderer/loadable_plugin_placeholder.h (right):

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
components/plugins/renderer/loadable_plugin_placeholder.h:88: #if
defined(ENABLE_PLUGINS)
Question - why do we need to ENABLE_PLUGINS for *parts* of the plugin
placeholder? Shouldn't the PPH not exist if we don't have plugins?

[tommycli, 2015-06-11 23:39:21]

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
File components/plugins/renderer/loadable_plugin_placeholder.h (right):

https://codereview.chromium.org/1124173008/diff/140001/components/plugins/ren...
components/plugins/renderer/loadable_plugin_placeholder.h:88: #if
defined(ENABLE_PLUGINS)
On 2015/06/11 23:32:56, groby wrote:
> Question - why do we need to ENABLE_PLUGINS for *parts* of the plugin
> placeholder? Shouldn't the PPH not exist if we don't have plugins?

Because previously a ChromePluginPlaceholder was used on Android also even
though ENABLE_PLUGINS is false.

This was lame and finally addressed by this refactor:
https://codereview.chromium.org/1161923004/

Now LoadablePluginPlaceholder and ChromePluginPlaceholder are only compiled if
ENABLE_PLUGINS is on, and these ifdefs have been removed