Hookup the plugin placeholder to the Flash permission prompt

Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
TBR=calamity

Committed: https://crrev.com/8926e8575261ecd801073a3a950d384cbb787e7d
Cr-Commit-Position: refs/heads/master@{#423070}

[raymes, 2016-10-02 11:56:42]

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

[raymes, 2016-10-02 11:56:42]

Hey Tommy,

PTAL.

I can't land this yet because the tests are flaky now. There seems to be a bug
where sometimes, if there is no fallback content, the plugin won't display the
plugin placeholder and will instead get fully loaded. This might be a bug in the
plugin placeholder, I haven't dug into it.

Some other bugs I uncovered with testing:
1) When the user clicks the plugin placeholder, the prompt gets triggered but
the flash swf also runs successfully. This feels weird because the user hasn't
yet clicked on the permission prompt. We should probably just wait for the
permission prompt.
2) When PPS is active (e.g. the swf is below a certain size), a permission
prompt won't be shown at all, the old behavior seems to apply.

We have a holiday here tomorrow, but I'll file bugs on these when I'm back but I
thought I'd let you know in case you have time to look into these.

[tommycli, 2016-10-03 17:28:50]

lgtm except:

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
File chrome/test/data/permissions/flash.html (right):

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:14: function
triggerPromptViaPluginPlaceholder() {
I think this function is not used.

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:29:
window.domAutomationController.send(false);
I wonder if this is the source of the flakiness.

In the other browsertests, we don't auto-fail when postMessage is undefined.
That might simply mean the plugin is still loading.

Once the plugin loads, it will automatically send the power saver status. The
postMessage is only for the case when the plugin has already loaded and we have
missed the initial power saver status message.

https://cs.chromium.org/chromium/src/chrome/test/data/prerender/prerender_plu...

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:37: function
flashIsEnabledForPluginWithoutFallack() {
This is getting confusing. Perhaps split into multiple html files?

[tommycli, 2016-10-03 22:23:48]

On 2016/10/02 11:56:42, raymes wrote:
> Hey Tommy,
> 
> PTAL.
> 
> I can't land this yet because the tests are flaky now. There seems to be a bug
> where sometimes, if there is no fallback content, the plugin won't display the
> plugin placeholder and will instead get fully loaded. This might be a bug in
the
> plugin placeholder, I haven't dug into it.
> 
> Some other bugs I uncovered with testing:
> 1) When the user clicks the plugin placeholder, the prompt gets triggered but
> the flash swf also runs successfully. This feels weird because the user hasn't
> yet clicked on the permission prompt. We should probably just wait for the
> permission prompt.
> 2) When PPS is active (e.g. the swf is below a certain size), a permission
> prompt won't be shown at all, the old behavior seems to apply.
> 
> We have a holiday here tomorrow, but I'll file bugs on these when I'm back but
I
> thought I'd let you know in case you have time to look into these.

Hi, I wasn't able to repro those two bugs.

I did notice that after the user clicked Allow, the placeholder is immediately
loaded. That may or may not be desired. I'm not sure yet.

I'm throwing something on your calendar for 11AM your time on Tuesday, but if
you can't make it, it's no biggie. I realize it's short notice.

Tommy

[raymes, 2016-10-04 05:07:21]

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
File chrome/test/data/permissions/flash.html (right):

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:14: function
triggerPromptViaPluginPlaceholder() {
On 2016/10/03 17:28:50, tommycli wrote:
> I think this function is not used.

Done.

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:29:
window.domAutomationController.send(false);
postMessage should appear synchronously on the pepper plugin object when it is
instantiated so I don't think this is the cause

https://codereview.chromium.org/2385033002/diff/20001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:37: function
flashIsEnabledForPluginWithoutFallack() {
On 2016/10/03 17:28:50, tommycli wrote:
> This is getting confusing. Perhaps split into multiple html files?

Splitting it into another test will add its own complexity to the test driver
(needing to navigate to different pages). So I think keeping it here is roughly
as good.

[raymes, 2016-10-04 05:07:30]

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

[commit-bot: I haz the power, 2016-10-04 05:07:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[raymes, 2016-10-04 05:08:44]

raymes@chromium.org changed reviewers:
+ bauerb@chromium.org, calamity@chromium.org, thestig@chromium.org

[raymes, 2016-10-04 05:08:44]

+thestig for chrome/browser/chrome_content_browser_client.cc
+bauerb for chrome/browser/plugins/plugin_observer.cc
+calamity for 	chrome/browser/engagement/site_engagement_score.h

[Bernhard Bauer, 2016-10-04 09:21:00]

LGTM with nits:

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_download_interception.h (right):

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/flash_download_interception.h:37:
DISALLOW_COPY_AND_ASSIGN(FlashDownloadInterception);
This should probably be DISALLOW_IMPLICIT_CONSTRUCTORS to avoid any
instantiation of the class?

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_browsertest.cc (right):

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_browsertest.cc:52:
RunScriptReturnBool("flashIsEnabledForPluginWithoutFallack();");
Why not combine these expressions in Javascript?

https://codereview.chromium.org/2385033002/diff/60001/chrome/test/data/permis...
File chrome/test/data/permissions/flash.html (right):

https://codereview.chromium.org/2385033002/diff/60001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:31: function
flashIsEnabledForPluginWithoutFallack() {
fallack?

[Lei Zhang, 2016-10-04 18:36:40]

lgtm

[raymes, 2016-10-04 23:12:19]

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_download_interception.h (right):

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/flash_download_interception.h:37:
DISALLOW_COPY_AND_ASSIGN(FlashDownloadInterception);
On 2016/10/04 09:21:00, Bernhard Bauer wrote:
> This should probably be DISALLOW_IMPLICIT_CONSTRUCTORS to avoid any
> instantiation of the class?

Done.

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_browsertest.cc (right):

https://codereview.chromium.org/2385033002/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_browsertest.cc:52:
RunScriptReturnBool("flashIsEnabledForPluginWithoutFallack();");
On 2016/10/04 09:21:00, Bernhard Bauer wrote:
> Why not combine these expressions in Javascript?

It's just a bit simpler to do it here, otherwise callbacks need to be chained in
JS and gets a bit ugly. But I'm happy to change it if you feel strongly.

https://codereview.chromium.org/2385033002/diff/60001/chrome/test/data/permis...
File chrome/test/data/permissions/flash.html (right):

https://codereview.chromium.org/2385033002/diff/60001/chrome/test/data/permis...
chrome/test/data/permissions/flash.html:31: function
flashIsEnabledForPluginWithoutFallack() {
On 2016/10/04 09:21:00, Bernhard Bauer wrote:
> fallack?

Done.

[raymes, 2016-10-04 23:12:43]

TBR calamity for trivial change in site_engagement_score.h

[raymes, 2016-10-04 23:12:58]

Description was changed from

==========
Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
==========

to

==========
Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
TBR=calamity
==========

[raymes, 2016-10-04 23:13:02]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-10-04 23:13:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, thestig@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2385033002/#ps100001
(title: "Hookup the plugin placeholder to the Flash permission prompt")

[commit-bot: I haz the power, 2016-10-04 23:13:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 00:08:07]

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

[commit-bot: I haz the power, 2016-10-05 00:08:09]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2016-10-05 02:57:59]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-10-05 02:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, tommycli@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2385033002/#ps160001
(title: "Hookup the plugin placeholder to the Flash permission prompt")

[commit-bot: I haz the power, 2016-10-05 02:58:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 03:55:17]

Description was changed from

==========
Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
TBR=calamity
==========

to

==========
Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
TBR=calamity
==========

[commit-bot: I haz the power, 2016-10-05 03:55:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-05 03:58:45]

Description was changed from

==========
Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
TBR=calamity
==========

to

==========
Hookup the plugin placeholder to the Flash permission prompt

This hooks up the plugin placeholder to the Flash permission prompt
such that the prompt is displayed when the placeholder is clicked.

BUG=641615
TBR=calamity

Committed: https://crrev.com/8926e8575261ecd801073a3a950d384cbb787e7d
Cr-Commit-Position: refs/heads/master@{#423070}
==========

[commit-bot: I haz the power, 2016-10-05 03:58:46]

Patchset 9 (id:??) landed as
https://crrev.com/8926e8575261ecd801073a3a950d384cbb787e7d
Cr-Commit-Position: refs/heads/master@{#423070}