Plugin Power Saver: Enable JS access to throttled plugins.

Plugin Power Saver: Enable JS access to throttled plugins.

Currently, throttled plugins cannot be accessed via JS. Once the original plugin detaches from the WebPluginContainer and is replaced by the WebViewPlugin, the V8 scriptable object can no longer be accessed.

This patch passes-through access to the underlying throttled plugin, if it exists.

BUG=485327
Committed: https://crrev.com/0ec19711ba3ed3223abd0c3e8062bb35002658ae
Cr-Commit-Position: refs/heads/master@{#328958}

[tommycli, 2015-05-06 22:11:00]

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

[commit-bot: I haz the power, 2015-05-06 22:12:40]

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

[tommycli, 2015-05-06 22:13:52]

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

[tommycli, 2015-05-06 22:14:47]

bauerb: ptal, thanks!

[commit-bot: I haz the power, 2015-05-07 00:09:11]

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

[commit-bot: I haz the power, 2015-05-07 00:09:11]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2015-05-07 10:07:13]

LGTM

https://codereview.chromium.org/1124343002/diff/1/components/plugins/renderer...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/1124343002/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.h:52: // Called to enable JavaScript
pass-through to an underlying plugin object.
Can you add a comment to state that this only works for throttled plugins (which
have been loaded), not for blocked ones (which have not been loaded yet)?

[tommycli, 2015-05-07 17:22:19]

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

[tommycli, 2015-05-07 17:22:20]

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

[tommycli, 2015-05-07 17:22:59]

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

[tommycli, 2015-05-07 17:23:00]

thestig: ptal chrome/browser/plugins/plugin_power_saver_browsertest.cc

bauerb: thanks!

https://codereview.chromium.org/1124343002/diff/1/components/plugins/renderer...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/1124343002/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.h:52: // Called to enable JavaScript
pass-through to an underlying plugin object.
On 2015/05/07 10:07:13, Bernhard Bauer wrote:
> Can you add a comment to state that this only works for throttled plugins
(which
> have been loaded), not for blocked ones (which have not been loaded yet)?

Done.

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:140:
IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, SmallCrossOrigin) {
I believe this patch incidentally fixes this flaky test (but I am not sure).

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

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

[Bernhard Bauer, 2015-05-07 17:46:28]

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:140:
IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, SmallCrossOrigin) {
On 2015/05/07 17:23:00, tommycli wrote:
> I believe this patch incidentally fixes this flaky test (but I am not sure).

Does it? I thought there was the race condition where we dispatch the click
before the inner HTML has loaded?

[tommycli, 2015-05-07 17:55:31]

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:140:
IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, SmallCrossOrigin) {
On 2015/05/07 17:46:28, Bernhard Bauer wrote:
> On 2015/05/07 17:23:00, tommycli wrote:
> > I believe this patch incidentally fixes this flaky test (but I am not sure).
> 
> Does it? I thought there was the race condition where we dispatch the click
> before the inner HTML has loaded?

Since we changed it to VerifyPluginIsThrottled, it should wait until
event.data.isHiddenForPlaceholder is true also. (See line 58)

Since throttler->SetHiddenForPlaceholder is only called in
DidFinishLoadingCallback, my theory is that in this particular case, the race
condition is resolved.

The remaining issue is for when there is no throttled plugin and we have a
poster image only. Then we have no notification for DidFinishLoadingCallback.

[Bernhard Bauer, 2015-05-07 18:23:27]

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:140:
IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, SmallCrossOrigin) {
On 2015/05/07 17:55:31, tommycli wrote:
> On 2015/05/07 17:46:28, Bernhard Bauer wrote:
> > On 2015/05/07 17:23:00, tommycli wrote:
> > > I believe this patch incidentally fixes this flaky test (but I am not
sure).
> > 
> > Does it? I thought there was the race condition where we dispatch the click
> > before the inner HTML has loaded?
> 
> Since we changed it to VerifyPluginIsThrottled, it should wait until
> event.data.isHiddenForPlaceholder is true also. (See line 58)
> 
> Since throttler->SetHiddenForPlaceholder is only called in
> DidFinishLoadingCallback, my theory is that in this particular case, the race
> condition is resolved.
> 
> The remaining issue is for when there is no throttled plugin and we have a
> poster image only. Then we have no notification for DidFinishLoadingCallback.

Acknowledged.

[commit-bot: I haz the power, 2015-05-07 18:28:22]

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

[commit-bot: I haz the power, 2015-05-07 18:28:24]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-05-07 23:15:59]

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (left):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:57: "     
event.data.source === 'getPowerSaverStatusResponse') {"
Do you still care about the event's data source?

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:64: "if
(plugin.postMessage !== undefined) {"
So previously, if plugin.postMessage is undefined, it would send() and
RunTestScript() would return. Now, the code does nothing. Is it relying on
handleEvent() to eventually call send(), or will this end up hanging and timing
out?

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:140:
IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, SmallCrossOrigin) {
On 2015/05/07 17:55:31, tommycli wrote:
> On 2015/05/07 17:46:28, Bernhard Bauer wrote:
> > On 2015/05/07 17:23:00, tommycli wrote:
> > > I believe this patch incidentally fixes this flaky test (but I am not
sure).
> > 
> > Does it? I thought there was the race condition where we dispatch the click
> > before the inner HTML has loaded?
> 
> Since we changed it to VerifyPluginIsThrottled, it should wait until
> event.data.isHiddenForPlaceholder is true also. (See line 58)
> 
> Since throttler->SetHiddenForPlaceholder is only called in
> DidFinishLoadingCallback, my theory is that in this particular case, the race
> condition is resolved.
> 
> The remaining issue is for when there is no throttled plugin and we have a
> poster image only. Then we have no notification for DidFinishLoadingCallback.

Can you add a comment to the top of VerifyPluginIsThrottled() to mention it
waits for the plugin to finish loading?

[tommycli, 2015-05-08 00:48:43]

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

[tommycli, 2015-05-08 00:48:43]

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

[tommycli, 2015-05-08 00:49:00]

thestig: thanks!

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (left):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:57: "     
event.data.source === 'getPowerSaverStatusResponse') {"
On 2015/05/07 23:15:59, Lei Zhang wrote:
> Do you still care about the event's data source?

We don't care about the source anymore. We just care that all the three
conditions are fulfilled.

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_power_saver_browsertest.cc (right):

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:64: "if
(plugin.postMessage !== undefined) {"
On 2015/05/07 23:15:59, Lei Zhang wrote:
> So previously, if plugin.postMessage is undefined, it would send() and
> RunTestScript() would return. Now, the code does nothing. Is it relying on
> handleEvent() to eventually call send(), or will this end up hanging and
timing
> out?

Exactly. Now it expects that if there is no postMessage yet, the plugin will
send the "initial" status messages and we will catch those.

https://codereview.chromium.org/1124343002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_power_saver_browsertest.cc:140:
IN_PROC_BROWSER_TEST_F(PluginPowerSaverBrowserTest, SmallCrossOrigin) {
On 2015/05/07 23:15:59, Lei Zhang wrote:
> On 2015/05/07 17:55:31, tommycli wrote:
> > On 2015/05/07 17:46:28, Bernhard Bauer wrote:
> > > On 2015/05/07 17:23:00, tommycli wrote:
> > > > I believe this patch incidentally fixes this flaky test (but I am not
> sure).
> > > 
> > > Does it? I thought there was the race condition where we dispatch the
click
> > > before the inner HTML has loaded?
> > 
> > Since we changed it to VerifyPluginIsThrottled, it should wait until
> > event.data.isHiddenForPlaceholder is true also. (See line 58)
> > 
> > Since throttler->SetHiddenForPlaceholder is only called in
> > DidFinishLoadingCallback, my theory is that in this particular case, the
race
> > condition is resolved.
> > 
> > The remaining issue is for when there is no throttled plugin and we have a
> > poster image only. Then we have no notification for
DidFinishLoadingCallback.
> 
> Can you add a comment to the top of VerifyPluginIsThrottled() to mention it
> waits for the plugin to finish loading?

Done.

[commit-bot: I haz the power, 2015-05-08 00:51:31]

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

[Lei Zhang, 2015-05-08 02:33:23]

lgtm

[commit-bot: I haz the power, 2015-05-08 03:11:09]

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

[commit-bot: I haz the power, 2015-05-08 03:11:11]

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-05-08 15:21:52]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-05-08 15:23:19]

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

[commit-bot: I haz the power, 2015-05-08 15:28:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-08 15:29:29]

Patchset 3 (id:??) landed as
https://crrev.com/0ec19711ba3ed3223abd0c3e8062bb35002658ae
Cr-Commit-Position: refs/heads/master@{#328958}