Blink Plugins: Remove Shadow DOM Plugin Placeholder

Blink Plugins: Remove Shadow DOM Plugin Placeholder

Corresponding Blink changes to https://codereview.chromium.org/1314643002/.

Reverts all the Blink patches in:
https://code.google.com/p/chromium/issues/detail?id=364716

BUG=524115
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201441

[tommycli, 2015-08-25 00:17:56]

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

[tommycli, 2015-08-25 00:18:11]

jbroman: this is the companion patch to
https://codereview.chromium.org/1314643002/ on the blink side.

IT'

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:168: bool shouldBypassMainWorldCSP = frame &&
frame->script().shouldBypassMainWorldCSP();
Needs extra scrutiny. Your patch added the shadow dom STYLE CSP exemption. Do we
still need it?

https://codereview.chromium.org/1313763002/diff/20001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/1313763002/diff/20001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:420: if (getNPObject() && pluginWidget()
&& pluginWidget()->isPluginView())
Extra scrutiny here:

This was added by this patch:
https://chromium.googlesource.com/chromium/blink/+/18dff13362438e5c0629d9e4f0...

If we mess it up Flash can be used as a global keylogger.

[tommycli, 2015-08-25 00:18:38]

It's mostly straight reverts of the patches on your bug, plus some fixups to
handle merge conflicts.

[jbroman, 2015-08-25 00:34:10]

lgtm

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:168: bool shouldBypassMainWorldCSP = frame &&
frame->script().shouldBypassMainWorldCSP();
On 2015/08/25 at 00:18:10, tommycli wrote:
> Needs extra scrutiny. Your patch added the shadow dom STYLE CSP exemption. Do
we still need it?

I'm not aware of any other users (but can't actually rule out any being added).
If nothing breaks, it's probably OK.

https://codereview.chromium.org/1313763002/diff/20001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/1313763002/diff/20001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:420: if (getNPObject() && pluginWidget()
&& pluginWidget()->isPluginView())
On 2015/08/25 at 00:18:10, tommycli wrote:
> Extra scrutiny here:
> 
> This was added by this patch:
https://chromium.googlesource.com/chromium/blink/+/18dff13362438e5c0629d9e4f0...
> 
> If we mess it up Flash can be used as a global keylogger.

Do we even still support NPAPI Flash? I'd suspect getNPObject() is always null
in Chrome today. Right? I'd add the author of that patch as a reviewer to be
sure there isn't anything more subtle here, though.

[jbroman, 2015-08-25 13:57:21]

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:168: bool shouldBypassMainWorldCSP = frame &&
frame->script().shouldBypassMainWorldCSP();
On 2015/08/25 at 00:34:10, jbroman wrote:
> On 2015/08/25 at 00:18:10, tommycli wrote:
> > Needs extra scrutiny. Your patch added the shadow dom STYLE CSP exemption.
Do we still need it?
> 
> I'm not aware of any other users (but can't actually rule out any being
added). If nothing breaks, it's probably OK.

FYI: I just got reminded of
https://code.google.com/p/chromium/issues/detail?id=424613 (which I filed, so I
ought to have remembered). It's possible that <marquee> may be relying, or may
start relying, on this, if it wants its CSS to work on CSP-restricted pages. (An
edge case, though, to be sure.)

This might be a reason to leave this logic intact.

[tommycli, 2015-08-25 16:48:17]

jbroman: thanks, i left the style csp exemption intact

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
File Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/1313763002/diff/20001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:168: bool shouldBypassMainWorldCSP = frame &&
frame->script().shouldBypassMainWorldCSP();
On 2015/08/25 13:57:21, jbroman wrote:
> On 2015/08/25 at 00:34:10, jbroman wrote:
> > On 2015/08/25 at 00:18:10, tommycli wrote:
> > > Needs extra scrutiny. Your patch added the shadow dom STYLE CSP exemption.
> Do we still need it?
> > 
> > I'm not aware of any other users (but can't actually rule out any being
> added). If nothing breaks, it's probably OK.
> 
> FYI: I just got reminded of
> https://code.google.com/p/chromium/issues/detail?id=424613 (which I filed, so
I
> ought to have remembered). It's possible that <marquee> may be relying, or may
> start relying, on this, if it wants its CSS to work on CSP-restricted pages.
(An
> edge case, though, to be sure.)
> 
> This might be a reason to leave this logic intact.

Done. I'll leave it intact.

[tommycli, 2015-08-25 17:38:16]

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

[tommycli, 2015-08-25 17:38:17]

pdr: PTAL

This patch looks big, but the vast majority are just deletes. jbroman is the
feature owner.

[pdr., 2015-08-25 18:06:29]

On 2015/08/25 at 17:38:17, tommycli wrote:
> pdr: PTAL
> 
> This patch looks big, but the vast majority are just deletes. jbroman is the
feature owner.

LGTM

[tommycli, 2015-08-26 00:15:15]

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

[commit-bot: I haz the power, 2015-08-26 00:15:49]

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

[commit-bot: I haz the power, 2015-08-26 02:52:44]

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

[commit-bot: I haz the power, 2015-08-26 02:52:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tommycli, 2015-08-27 17:50:33]

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

[commit-bot: I haz the power, 2015-08-27 17:50:52]

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

[commit-bot: I haz the power, 2015-08-27 17:52:28]

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

[commit-bot: I haz the power, 2015-08-27 17:52:29]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tommycli, 2015-08-28 21:50:08]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-08-28 21:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1313763002/#ps100001
(title: "Remove changes that were committed in a previous patch.")

[commit-bot: I haz the power, 2015-08-28 21:50:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313763002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313763002/100001

[commit-bot: I haz the power, 2015-08-28 21:51:58]

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

[commit-bot: I haz the power, 2015-08-28 21:51:59]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2015-08-28 21:55:42]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-08-28 21:55:43]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1313763002/#ps120001
(title: "merge origin/master")

[commit-bot: I haz the power, 2015-08-28 21:55:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313763002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313763002/120001

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

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201441