Issue 1114623002: Plugin Power Saver: Make PPS work well with prerendered pages.

Issue 1114623002: Plugin Power Saver: Make PPS work well with prerendered pages. (Closed)

Created:
5 years, 7 months ago by tommycli

Modified:
5 years, 7 months ago

Reviewers:
Lei Zhang, Bernhard Bauer, raymes, Tom Sepez, piman

CC:
chromium-reviews, davidben+watch_chromium.org, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, tzik, tburkard+watch_chromium.org, binji+watch_chromium.org, jam, raymes+watch_chromium.org, gavinp+prer_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, bradnelson+warch_chromium.org, mkwst+moarreviews-renderer_chromium.org, piman+watch_chromium.org, ihf+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Plugin Power Saver: Make PPS work well with prerendered pages. This patch does two things: 1. Fixes PPS interaction with prerender. This is done by having ChromePluginPlaceholder create a PluginPreroller along with the throttler. 2. Adds a browsertest for the above process. a. This required adding a IsThrottled method to PPB_Testing_Private. b. This required modifying the PPS Test Plugin to paint some dummy 'interesting' frames. BUG=471427 Committed: https://crrev.com/ee0a49b6fa5c285ec3069f858ee9ba839570eddb Cr-Commit-Position: refs/heads/master@{#328381}

Patch Set 1 #

Patch Set 2 : merge #

Total comments: 12

Patch Set 3 : #

Patch Set 4 : #

Total comments: 6

Patch Set 5 : #

Total comments: 4

Patch Set 6 : #

Total comments: 4

Patch Set 7 : #

Patch Set 8 : update comments #

Total comments: 4

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Total comments: 2

Patch Set 13 : remove extraneous newline #

Patch Set 14 : #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+303 lines, -169 lines)			Patch
M	chrome/browser/plugins/plugin_power_saver_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	3 chunks	+10 lines, -10 lines	0 comments	Download
M	chrome/browser/prerender/prerender_browsertest.cc	View	1 2 3 4 5 6	5 chunks	+17 lines, -7 lines	0 comments	Download
M	chrome/renderer/chrome_content_renderer_client.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -5 lines	0 comments	Download
M	chrome/renderer/plugins/chrome_plugin_placeholder.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/renderer/plugins/chrome_plugin_placeholder.cc	View	1 2 3 4 5 6	2 chunks	+19 lines, -0 lines	0 comments	Download
D	chrome/test/data/prerender/plugin_delay_load.html	View		1 chunk	+0 lines, -32 lines	0 comments	Download
D	chrome/test/data/prerender/plugin_never_load.html	View		1 chunk	+0 lines, -33 lines	0 comments	Download
M	chrome/test/data/prerender/prerender_iframe_plugin_delay_load.html	View		1 chunk	+1 line, -1 line	0 comments	Download
A +	chrome/test/data/prerender/prerender_plugin_delay_load.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	chrome/test/data/prerender/prerender_plugin_never_load.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A	chrome/test/data/prerender/prerender_plugin_power_saver.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+54 lines, -0 lines	0 comments	Download
M	components/plugins/renderer/loadable_plugin_placeholder.h	View	1 2 3 4 5 6	3 chunks	+6 lines, -0 lines	0 comments	Download
M	components/plugins/renderer/loadable_plugin_placeholder.cc	View	1 2 3 4 5 6	2 chunks	+5 lines, -14 lines	0 comments	Download
M	content/public/renderer/plugin_instance_throttler.h	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/renderer/pepper/plugin_instance_throttler_impl.h	View	1 2 3 4 5 6	3 chunks	+2 lines, -4 lines	0 comments	Download
M	content/renderer/pepper/plugin_instance_throttler_impl.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	5 chunks	+6 lines, -13 lines	0 comments	Download
M	content/renderer/pepper/plugin_module.cc	View	1 2 3 4 5 6 7 8 9 10 11	4 chunks	+71 lines, -4 lines	0 comments	Download
M	ppapi/api/private/ppb_testing_private.idl	View	1 2 3 4 5 6 7 8	1 chunk	+12 lines, -3 lines	0 comments	Download
M	ppapi/c/private/ppb_testing_private.h	View	1 2 3 4 5 6 7 8	2 chunks	+11 lines, -3 lines	0 comments	Download
M	ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c	View	1 2 3 4 5 6 7 8	2 chunks	+9 lines, -3 lines	0 comments	Download
M	ppapi/proxy/ppapi_messages.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+5 lines, -3 lines	0 comments	Download
M	ppapi/proxy/ppb_testing_proxy.h	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -1 line	0 comments	Download
M	ppapi/proxy/ppb_testing_proxy.cc	View	1 2 3 4 5 6 7 8	4 chunks	+29 lines, -11 lines	0 comments	Download
M	ppapi/tests/power_saver_test_plugin.cc	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+40 lines, -24 lines	0 comments	Download

Messages

Total messages: 49 (14 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

tommycli

thestig/bauerb/raymes: Another CL for you guys to review. This fixes the PPS interaction with prerender ...

5 years, 7 months ago (2015-04-28 18:43:16 UTC) #2

raymes

https://codereview.chromium.org/1114623002/diff/20001/content/renderer/pepper/plugin_instance_throttler_impl.cc File content/renderer/pepper/plugin_instance_throttler_impl.cc (right): https://codereview.chromium.org/1114623002/diff/20001/content/renderer/pepper/plugin_instance_throttler_impl.cc#newcode244 content/renderer/pepper/plugin_instance_throttler_impl.cc:244: } It feels a little weird to have this ...

5 years, 7 months ago (2015-04-29 00:12:35 UTC) #3

Bernhard Bauer

Looks good for the most part, just some nits on the test case (which were ...

5 years, 7 months ago (2015-04-29 09:42:37 UTC) #4

tommycli

raymes, bauerb: Addressed your concerns below. I rebased this patch on top of https://codereview.chromium.org/1111893003/, so ...

5 years, 7 months ago (2015-04-29 20:25:47 UTC) #5

commit-bot: I haz the power

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

5 years, 7 months ago (2015-04-29 21:48:18 UTC) #7

Lei Zhang

https://codereview.chromium.org/1114623002/diff/60001/chrome/test/data/prerender/prerender_plugin_power_saver.html File chrome/test/data/prerender/prerender_plugin_power_saver.html (right): https://codereview.chromium.org/1114623002/diff/60001/chrome/test/data/prerender/prerender_plugin_power_saver.html#newcode3 chrome/test/data/prerender/prerender_plugin_power_saver.html:3: This test checks the interaction between prerender and the ...

5 years, 7 months ago (2015-04-29 22:14:16 UTC) #8

commit-bot: I haz the power

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_chromium_gn_compile_rel/builds/83181)

5 years, 7 months ago (2015-04-29 22:15:53 UTC) #10

tommycli

thestig: thanks! https://codereview.chromium.org/1114623002/diff/60001/chrome/test/data/prerender/prerender_plugin_power_saver.html File chrome/test/data/prerender/prerender_plugin_power_saver.html (right): https://codereview.chromium.org/1114623002/diff/60001/chrome/test/data/prerender/prerender_plugin_power_saver.html#newcode3 chrome/test/data/prerender/prerender_plugin_power_saver.html:3: This test checks the interaction between prerender ...

5 years, 7 months ago (2015-04-29 22:25:16 UTC) #12

commit-bot: I haz the power

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

5 years, 7 months ago (2015-04-29 22:27:08 UTC) #13

Lei Zhang

https://codereview.chromium.org/1114623002/diff/50025/chrome/test/data/prerender/prerender_plugin_power_saver.html File chrome/test/data/prerender/prerender_plugin_power_saver.html (right): https://codereview.chromium.org/1114623002/diff/50025/chrome/test/data/prerender/prerender_plugin_power_saver.html#newcode30 chrome/test/data/prerender/prerender_plugin_power_saver.html:30: if (event.data.isThrottled) { Do you need to handle the ...

5 years, 7 months ago (2015-04-29 22:40:43 UTC) #14

tommycli

thestig: thanks https://codereview.chromium.org/1114623002/diff/50025/chrome/test/data/prerender/prerender_plugin_power_saver.html File chrome/test/data/prerender/prerender_plugin_power_saver.html (right): https://codereview.chromium.org/1114623002/diff/50025/chrome/test/data/prerender/prerender_plugin_power_saver.html#newcode30 chrome/test/data/prerender/prerender_plugin_power_saver.html:30: if (event.data.isThrottled) { On 2015/04/29 22:40:43, Lei ...

5 years, 7 months ago (2015-04-29 22:44:20 UTC) #15

raymes

https://codereview.chromium.org/1114623002/diff/90001/content/renderer/pepper/plugin_instance_throttler_impl.cc File content/renderer/pepper/plugin_instance_throttler_impl.cc (right): https://codereview.chromium.org/1114623002/diff/90001/content/renderer/pepper/plugin_instance_throttler_impl.cc#newcode191 content/renderer/pepper/plugin_instance_throttler_impl.cc:191: } It still feels a bit nasty to have ...

5 years, 7 months ago (2015-04-30 00:42:07 UTC) #17

tommycli

5 years, 7 months ago (2015-04-30 01:27:10 UTC) #18

Bernhard Bauer

lgtm https://codereview.chromium.org/1114623002/diff/90001/chrome/test/data/prerender/prerender_plugin_power_saver.html File chrome/test/data/prerender/prerender_plugin_power_saver.html (right): https://codereview.chromium.org/1114623002/diff/90001/chrome/test/data/prerender/prerender_plugin_power_saver.html#newcode37 chrome/test/data/prerender/prerender_plugin_power_saver.html:37: if (plugin.postMessage == undefined) { ===

5 years, 7 months ago (2015-04-30 08:09:36 UTC) #19

tommycli

raymes: I took your suggestion, and also made more changes to the Pepper testing interface. ...

5 years, 7 months ago (2015-04-30 21:54:31 UTC) #20

raymes

Thanks Tommy! A few more suggestions. Sorry for the long review. https://codereview.chromium.org/1114623002/diff/130001/content/renderer/pepper/plugin_module.cc File content/renderer/pepper/plugin_module.cc (right): ...

5 years, 7 months ago (2015-05-01 01:02:05 UTC) #21

commit-bot: I haz the power

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

5 years, 7 months ago (2015-05-01 22:56:32 UTC) #24

tommycli

raymes: No problem, thanks for sticking with it despite this being the browsertest from hell. ...

5 years, 7 months ago (2015-05-01 23:12:43 UTC) #25

raymes: No problem, thanks for sticking with it despite this being the
browsertest from hell.

I changed it so that it directly posts the status to Javascript instead of
bouncing to the Pepper plugin. Hopefully that's less nasty, since Pepper is no
longer posting a message to the plugin itself.

I was not able to cut it down to one API call because:

1) If the JS test runs before plugin.postMessage is available (if it's still a
placeholder), it will register an event handler, and wait for the Power Saver
messages that are sent on plugin startup or status change.

2) If the JS test runs after the Plugin is done loading / unthrottling, all the
notification messages have already been discarded. It then registers the event
handler and "manually" requests a status update.

The above logic kind of confuses me too... let me know if you find any holes in
it.

https://codereview.chromium.org/1114623002/diff/130001/content/renderer/peppe...
File content/renderer/pepper/plugin_module.cc (right):

https://codereview.chromium.org/1114623002/diff/130001/content/renderer/peppe...
content/renderer/pepper/plugin_module.cc:217:
ppapi::StringVar::StringToPPVar(message)));
On 2015/05/01 01:02:05, raymes wrote:
> This is still a bit un-idiomatic from a pepper perspective. Really it would be
> better to have the plugin call GetPowerSaverStatusNotification(PP_Callback
> callback, uint32_t notification_type) and then when the callback runs it can
> call GetPowerSaverStatusNotification again.
> 
> Because this is just for testing, I'm tempted to say it doesn't matter too
much.
> But if we decide to do that, we should put a note in explaining exactly what
> happens in the API definition and suggesting that it's not an ideal approach
but
> it's convenient for testing.
> 

Done.

https://codereview.chromium.org/1114623002/diff/130001/content/renderer/peppe...
content/renderer/pepper/plugin_module.cc:305:
PP_MakeBool(PP_FromBool(throttler->IsThrottled())));
On 2015/05/01 01:02:05, raymes wrote:
> Also, if we're going to postMessage the data, why not just send it all in the
> message? You could put all this data into the message that gets sent above and
> avoid having 2 API functions. wdyt?

Done.

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 7 months ago (2015-05-02 00:22:04 UTC) #27

raymes

Cool this looks way better!! I just have one last question for potential simplification of ...

5 years, 7 months ago (2015-05-04 01:56:48 UTC) #28

raymes

Cool this looks way better!! I just have one last question for potential simplification of ...

5 years, 7 months ago (2015-05-04 01:56:54 UTC) #29

tommycli

raymes: Answered below. Thanks! https://codereview.chromium.org/1114623002/diff/210001/ppapi/tests/power_saver_test_plugin.cc File ppapi/tests/power_saver_test_plugin.cc (right): https://codereview.chromium.org/1114623002/diff/210001/ppapi/tests/power_saver_test_plugin.cc#newcode33 ppapi/tests/power_saver_test_plugin.cc:33: GetTestingInterface()->PostPowerSaverStatus(pp_instance()); On 2015/05/04 01:56:48, raymes ...

5 years, 7 months ago (2015-05-04 16:25:54 UTC) #30

commit-bot: I haz the power

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

5 years, 7 months ago (2015-05-04 23:17:21 UTC) #33

commit-bot: I haz the power

Dry run: Commit queue failed due to new patchset.

5 years, 7 months ago (2015-05-05 00:22:53 UTC) #36

commit-bot: I haz the power

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

5 years, 7 months ago (2015-05-05 00:23:04 UTC) #37

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

5 years, 7 months ago (2015-05-05 01:25:43 UTC) #40

tommycli

tsepez: PTAL ppapi/proxy/ppapi_messages.h piman: PTAL content/public/renderer/plugin_instance_throttler.h raymes: thanks!

5 years, 7 months ago (2015-05-05 16:28:03 UTC) #42

commit-bot: I haz the power

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

5 years, 7 months ago (2015-05-05 18:53:34 UTC) #47

commit-bot: I haz the power

5 years, 7 months ago (2015-05-05 19:41:53 UTC) #49

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages