[HBD] Merge two identical ShouldUseJavaScriptSettingForPlugin impls

[HBD] Merge two identical ShouldUseJavaScriptSettingForPlugin impls

We are probably going to change this soon to only use the Plugin
content setting for Flash. Before that, we should merge these two
identical implementations so we don't have to change it in two places.

BUG=615738
Committed: https://crrev.com/d9adeff125559612a223819de5fce36c5e337b4f
Cr-Commit-Position: refs/heads/master@{#420666}

[tommycli, 2016-09-22 19:38:05]

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

[tommycli, 2016-09-22 19:38:06]

thestig: PTAL,

Also happy to put the merged fn within chrome/common/pepper_flash.cc if you
think that's appropriate.

(Eventually only Flash will use the Plugins content setting)

[tommycli, 2016-09-22 21:09:52]

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

[commit-bot: I haz the power, 2016-09-22 21:10:15]

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

[tommycli, 2016-09-22 21:12:10]

Patchset #2 (id:20001) has been deleted

[tommycli, 2016-09-22 21:53:27]

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

[commit-bot: I haz the power, 2016-09-22 21:54:00]

Dry run: 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-09-22 22:58:40]

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

[commit-bot: I haz the power, 2016-09-22 22:58:40]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-23 05:00:51]

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

[Lei Zhang, 2016-09-23 05:00:52]

+xhwang to take a look since it involves CDMs.

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
chrome/common/plugin_utils.cc:15: // For certain sandboxed Pepper plugins, use
the JavaScript Content Settings.
Put this in the header?

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.h (right):

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
chrome/common/plugin_utils.h:12: // Only Flash should use the Plugin content
setting.
This mentions Flash, but the impl doesn't check for Flash?

[xhwang, 2016-09-23 16:42:17]

The widevine part looking good to me.

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.h (right):

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
chrome/common/plugin_utils.h:12: // Only Flash should use the Plugin content
setting.
On 2016/09/23 05:00:52, Lei Zhang wrote:
> This mentions Flash, but the impl doesn't check for Flash?

+1

And JavaScriptSettingForPlugin isn't always self explanatory for a lot of
people.

[tommycli, 2016-09-23 16:46:46]

thestig / xhwang: Thanks! I updated the comments!

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.cc (right):

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
chrome/common/plugin_utils.cc:15: // For certain sandboxed Pepper plugins, use
the JavaScript Content Settings.
On 2016/09/23 05:00:51, Lei Zhang wrote:
> Put this in the header?

Done.

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.h (right):

https://codereview.chromium.org/2367553002/diff/40001/chrome/common/plugin_ut...
chrome/common/plugin_utils.h:12: // Only Flash should use the Plugin content
setting.
On 2016/09/23 16:42:17, xhwang (slow) wrote:
> On 2016/09/23 05:00:52, Lei Zhang wrote:
> > This mentions Flash, but the impl doesn't check for Flash?
> 
> +1
> 
> And JavaScriptSettingForPlugin isn't always self explanatory for a lot of
> people.

Done.

Updated the comment. Sorry, this leaked into this patch from the followup patch

[Lei Zhang, 2016-09-23 16:49:44]

lgtm

https://codereview.chromium.org/2367553002/diff/80001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.h (right):

https://codereview.chromium.org/2367553002/diff/80001/chrome/common/plugin_ut...
chrome/common/plugin_utils.h:12: // For certain sandboxed Pepper plugins, use
the JavaScript Content Settings.
// Returns whether a given |plugin| should use the JavaScript Content Settings
or not.

[tommycli, 2016-09-23 16:53:31]

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

[tommycli, 2016-09-23 16:53:34]

thestig: thank you!

https://codereview.chromium.org/2367553002/diff/80001/chrome/common/plugin_ut...
File chrome/common/plugin_utils.h (right):

https://codereview.chromium.org/2367553002/diff/80001/chrome/common/plugin_ut...
chrome/common/plugin_utils.h:12: // For certain sandboxed Pepper plugins, use
the JavaScript Content Settings.
On 2016/09/23 16:49:44, Lei Zhang wrote:
> // Returns whether a given |plugin| should use the JavaScript Content Settings
> or not.

Done. Thanks!

[commit-bot: I haz the power, 2016-09-23 16:53:54]

Dry run: 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-09-23 18:07:08]

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

[commit-bot: I haz the power, 2016-09-23 18:07:09]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-09-23 18:12:06]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-09-23 18:12:07]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2367553002/#ps100001
(title: "update comment")

[commit-bot: I haz the power, 2016-09-23 18:13:25]

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-09-23 18:20:25]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-23 18:22:24]

Description was changed from

==========
[HBD] Merge two identical ShouldUseJavaScriptSettingForPlugin impls

We are probably going to change this soon to only use the Plugin
content setting for Flash. Before that, we should merge these two
identical implementations so we don't have to change it in two places.

BUG=615738
==========

to

==========
[HBD] Merge two identical ShouldUseJavaScriptSettingForPlugin impls

We are probably going to change this soon to only use the Plugin
content setting for Flash. Before that, we should merge these two
identical implementations so we don't have to change it in two places.

BUG=615738

Committed: https://crrev.com/d9adeff125559612a223819de5fce36c5e337b4f
Cr-Commit-Position: refs/heads/master@{#420666}
==========

[commit-bot: I haz the power, 2016-09-23 18:22:25]

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

[tommycli, 2016-09-23 18:28:35]

Thank you gentlemen!