Move plugin placeholder style to CSS, and allow it to bypass main world CSP.

Move plugin placeholder style to CSS, and allow it to bypass main world CSP.

<style> elements inside UA shadow roots are permitted to load inline style,
even if the host document CSP disallows this, since the style is supplied by
the user agent.

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

[jbroman, 2014-08-29 15:46:28]

Patchset #2 (id:20001) has been deleted

[PhistucK, 2014-08-31 06:32:23]

Is there any way to use <link> (or some other way) than to punch more holes in
CSP? Perhaps the inline style simply makes sense it this case. Perhaps adding
this to the normal user agent stylesheet somehow?

[jbroman, 2014-08-31 16:17:20]

On 2014/08/31 06:32:23, PhistucK wrote:
> Is there any way to use <link> (or some other way) than to punch more holes in
> CSP? Perhaps the inline style simply makes sense it this case. Perhaps adding
> this to the normal user agent stylesheet somehow?

(This patch has not yet been sent for review, but nonetheless...)

I'm definitely open to suggestions; this only seemed like the most
straightforward approach.

I don't believe <link> gets the same scoped-style properties that <style> does,
and an @import in a <style> would, I believe, still be subject to the
inline-style restriction.

I'm not sure what form adding this to the UA stylesheet would take. Our
traditional approach is to use pseudo-IDs, like video::-webkit-media-controls,
but not only are these exposed to the author in cases we might not wish it (we
could implement a notion of internal pseudo-IDs, of course), but I don't think
they facilitate cases like "I want all of the links in this shadow root to be
pink" (though I haven't dug that deeply).

Ultimately, we *are* loading a style resource that the document has not allowed
(in the worst case, it might say "style-src 'none'"), so we're bypassing the
content security policy in one way or another.

[jbroman, 2014-09-01 00:19:42]

On 2014/08/31 16:17:20, jbroman wrote:
> On 2014/08/31 06:32:23, PhistucK wrote:
> > Is there any way to use <link> (or some other way) than to punch more holes
in
> > CSP? Perhaps the inline style simply makes sense it this case. Perhaps
adding
> > this to the normal user agent stylesheet somehow?
> 
> (This patch has not yet been sent for review, but nonetheless...)
> 
> I'm definitely open to suggestions; this only seemed like the most
> straightforward approach.
> 
> I don't believe <link> gets the same scoped-style properties that <style>
does,
> and an @import in a <style> would, I believe, still be subject to the
> inline-style restriction.
> 
> I'm not sure what form adding this to the UA stylesheet would take. Our
> traditional approach is to use pseudo-IDs, like video::-webkit-media-controls,
> but not only are these exposed to the author in cases we might not wish it (we
> could implement a notion of internal pseudo-IDs, of course), but I don't think
> they facilitate cases like "I want all of the links in this shadow root to be
> pink" (though I haven't dug that deeply).
> 
> Ultimately, we *are* loading a style resource that the document has not
allowed
> (in the worst case, it might say "style-src 'none'"), so we're bypassing the
> content security policy in one way or another.

I suppose one other alternative would be for us to modify
ShadowTreeStyleSheetCollection or one of the related classes to give us another
way to supply a style sheet to the style engine that wasn't associated with a
<style> element. That seems like it's moving further away from the platform we
give authors, though.

As far as I can tell, the only way to get a stylesheet to be scoped to a tree
scope other than the document is currently a <style> element within such a
scope. If that is so, then I think it's most in the spirit of the extensible web
platform to show such an element in our own UA shadow DOM when we want that
facility.

[haraken, 2014-09-01 00:59:42]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-09-01 00:59:43]

https://codereview.chromium.org/516273002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/516273002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptController.h:155: class BypassMainWorldCSP :
public TemporaryChange<bool> {

The idea of introducing BypassMainWorldCSP looks good, but the implementation
looks a bit too complicated (e.g., we don't want to add a friend class). In
addition, it's a bit dangerous to provide a general way to bypass the main world
CSP. We should limit the usage only to private scripts.

How about implementing a helper method like this?

// static method of PrivateScriptRunner
void PrivateScriptRunner::injectCSS(Element* parentElement, String cssSource) {
  ...;
  scriptController->setBypassMainWorldCSP(true);
  ...; // Do the actual work to inject the CSS to parentElement.
  scriptController->setBypassMainWorldCSP(false);

}

https://codereview.chromium.org/516273002/diff/60001/Source/core/css/pluginPl...
File Source/core/css/pluginPlaceholder.css (right):

https://codereview.chromium.org/516273002/diff/60001/Source/core/css/pluginPl...
Source/core/css/pluginPlaceholder.css:2: * The style sheet loaded for plugin
placeholders.

This comment should be written after the copyright. The copyright should be put
the first without any modification.

[jbroman, 2014-09-02 15:44:04]

I've rebased on top of the Blink-in-JS version of:
https://codereview.chromium.org/522783002/

I've also changed the approach from globally disabling CSP temporarily to
empowering an individual HTMLStyleElement to ignore the inline style
restriction.

The last two patchsets give two variations on this: one that sets a boolean, and
one that explicitly validates that the whitelisted source has not changed (this
should be fast, as the same StringImpl pointer ends up used).

It should not be possible for anything else to get a hold of this style element
to modify it, so I'm not sure whether this extra check is useful.

What do you think of this approach?

https://codereview.chromium.org/516273002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/516273002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptController.h:155: class BypassMainWorldCSP :
public TemporaryChange<bool> {
On 2014/09/01 00:59:43, haraken wrote:
> 
> The idea of introducing BypassMainWorldCSP looks good, but the implementation
> looks a bit too complicated (e.g., we don't want to add a friend class).

The friend class is there to keep m_bypassMainWorldCSP private, and it seemed
clear to me that it uses no other privilege. The reason I did this instead of a
pair of methods is to prevent a caller from turning the bypass bit on and
forgetting to turn it off. This is how StyleEngine::IgnoringPendingStylesheet
works, for instance. But I don't feel strongly.

This is moot in light of the below, though.

> In
> addition, it's a bit dangerous to provide a general way to bypass the main
world
> CSP. We should limit the usage only to private scripts.
> 
> How about implementing a helper method like this?
> 
> // static method of PrivateScriptRunner
> void PrivateScriptRunner::injectCSS(Element* parentElement, String cssSource)
{
>   ...;
>   scriptController->setBypassMainWorldCSP(true);
>   ...; // Do the actual work to inject the CSS to parentElement.
>   scriptController->setBypassMainWorldCSP(false);
> 
> }

I realized after some more investigation that there are some issues
with my original approach, namely that the times the CSP is checked
are a little complicated (it's checked only when it's in a document,
either on attachment or when children change).

The current patch set takes another approach. I'm happy to move it
into a helper if you think that's helpful.

https://codereview.chromium.org/516273002/diff/60001/Source/core/css/pluginPl...
File Source/core/css/pluginPlaceholder.css (right):

https://codereview.chromium.org/516273002/diff/60001/Source/core/css/pluginPl...
Source/core/css/pluginPlaceholder.css:2: * The style sheet loaded for plugin
placeholders.
On 2014/09/01 00:59:43, haraken wrote:
> 
> This comment should be written after the copyright. The copyright should be
put
> the first without any modification.

This comment is removed in the latest patchset.

(I'd been following some of the older .css files we have, which have a
description ahead of the copyright header.)

https://codereview.chromium.org/516273002/diff/140001/public/blink_resources.grd
File public/blink_resources.grd (right):

https://codereview.chromium.org/516273002/diff/140001/public/blink_resources....
public/blink_resources.grd:53: <include
name="IDR_PLUGIN_PLACEHOLDER_ELEMENT_CSS"
file="../Source/core/html/shadow/PluginPlaceholderElement.css" type="BINDATA"/>
According to http://crbug.com/312586, this is the "not ready yet" approach (and
the various Python scripts to generated .h files are the deprecated way) to get
resources in Blink.

Unfortunately this approach requires a 3-sided change (as BlinkPlatformImpl
needs to know about new resources), and the build system does not seem to
correctly understand dependencies (I've needed to clobber it to see changes in
the .css file).

If that doesn't get better before this is ready to land, I'll probably move it
back to make-file-arrays.py instead.

[abarth-chromium, 2014-09-02 20:11:09]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2014-09-02 20:11:10]

not lgtm

Can we whitelist at the shadow root instead?

[jbroman, 2014-09-02 20:13:57]

On 2014/09/02 20:11:10, abarth wrote:
> not lgtm
> 
> Can we whitelist at the shadow root instead?

That's an interesting idea. Do you mean "whitelist all <style> nodes within the
shadow root"? If so, do you mean "any UA shadow root", or "specially marked UA
shadow roots"?
I'm also not sure how that would interact with distributed elements (though
there aren't any in this case).

[abarth-chromium, 2014-09-02 20:25:42]

On 2014/09/02 at 20:13:57, jbroman wrote:
> That's an interesting idea. Do you mean "whitelist all <style> nodes within
the shadow root"? If so, do you mean "any UA shadow root", or "specially marked
UA shadow roots"?

"any UA shadow root"

> I'm also not sure how that would interact with distributed elements (though
there aren't any in this case).

You should look at the DOM, not the render tree, to find the shadow root.  Then
you'll be looking at the tree before distribution.

[jbroman, 2014-09-02 22:12:07]

Patchset #7 (id:160001) has been deleted

[jbroman, 2014-09-02 22:12:26]

Patchset #7 (id:180001) has been deleted

[jbroman, 2014-09-02 22:18:38]

On 2014/09/02 20:25:42, abarth wrote:
> On 2014/09/02 at 20:13:57, jbroman wrote:
> > That's an interesting idea. Do you mean "whitelist all <style> nodes within
> the shadow root"? If so, do you mean "any UA shadow root", or "specially
marked
> UA shadow roots"?
> 
> "any UA shadow root"
> 
> > I'm also not sure how that would interact with distributed elements (though
> there aren't any in this case).
> 
> You should look at the DOM, not the render tree, to find the shadow root. 
Then
> you'll be looking at the tree before distribution.

Ah, duh. My mental model of shadow DOM was out of whack.

Rewritten to whitelist any HTMLStyleElement contained in a user agent shadow
root.

For now, I'm loading the stylesheet simply by providing its content (via
make-file-arrays.py) as a really simple bindings method.
Later on, we might want something more sophisticated, but this works for now.

[abarth-chromium, 2014-09-02 22:34:34]

Yay, much better!

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

https://codereview.chromium.org/516273002/diff/220001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:145: inline static bool
shouldBypassMainWorldCSP(Element* e)
inline and static are redundant here.  Just static is fine.

https://codereview.chromium.org/516273002/diff/220001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:148: LocalFrame* frame = e->document().frame();
s/e/element/

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:38: return
pluginPlaceholderElementCss;
Please use GRD to load this resource.  See other callers of
Platform::current()->loadResource(...)

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.idl (right):

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.idl:12:
[OnlyExposedToPrivateScript] readonly attribute DOMString styleSource;
Please don't add this function.  Instead, private scripts should have a generic
ability to call Platform::current()->loadResource(...)

[jbroman, 2014-09-02 23:53:32]

Will act on the comments shortly, but had a question about what sort of generic
approach would be best.

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

https://codereview.chromium.org/516273002/diff/220001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:145: inline static bool
shouldBypassMainWorldCSP(Element* e)
On 2014/09/02 22:34:33, abarth wrote:
> inline and static are redundant here.  Just static is fine.

Will do.

https://codereview.chromium.org/516273002/diff/220001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:148: LocalFrame* frame = e->document().frame();
On 2014/09/02 22:34:33, abarth wrote:
> s/e/element/

Will do. This name was just here because I moved code from
StyleElement::createSheet.

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:38: return
pluginPlaceholderElementCss;
On 2014/09/02 22:34:34, abarth wrote:
> Please use GRD to load this resource.  See other callers of
> Platform::current()->loadResource(...)

I'd done that previously, though I had trouble. In particular, I wasn't able to
get it to actually rebuild when I changed a resource, unless I clobbered. I'll
try again, but I'm not sure if there's a trick here.

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.idl (right):

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.idl:12:
[OnlyExposedToPrivateScript] readonly attribute DOMString styleSource;
On 2014/09/02 22:34:34, abarth wrote:
> Please don't add this function.  Instead, private scripts should have a
generic
> ability to call Platform::current()->loadResource(...)

There are about three ways I can think of here. What seems best?

1. Providing some binding exposed to private script. This seems simplest.
Something like this:

    partial interface Window {
        [OnlyExposedToPrivateScript] DOMString loadPrivateResource(DOMString
name);
    }

2. Having chrome provide a special scheme for this, like it has
chrome-extension:// and such already. Then we could just have an @import of
blink-resource://PluginPlaceholderElement.css or similar. If I understand the
architecture of this, I think it would go to the browser process as part of
this, but that may not be a big deal. This might be generally useful (for
loading images etc. as well), but it's more complicated. We'd also have to
decide whether it's okay for these to just always be loadable (e.g. I think
web-accessible extension resources always are, but I'm not sure), or whether we
need to confine their loading to private script only.

3. Same as 2, but implemented in Blink before crossing the embedder API
boundary. Seems pointless as we'd just cross the embedder API boundary anyhow.

[abarth-chromium, 2014-09-03 00:38:59]

On 2014/09/02 at 23:53:32, jbroman wrote:
> 2. Having chrome provide a special scheme for this, like it has
chrome-extension:// and such already. Then we could just have an @import of
blink-resource://PluginPlaceholderElement.css or similar. If I understand the
architecture of this, I think it would go to the browser process as part of
this, but that may not be a big deal. This might be generally useful (for
loading images etc. as well), but it's more complicated. We'd also have to
decide whether it's okay for these to just always be loadable (e.g. I think
web-accessible extension resources always are, but I'm not sure), or whether we
need to confine their loading to private script only.

^^^ That sounds ideal.

[haraken, 2014-09-03 04:08:33]

On 2014/09/03 00:38:59, abarth wrote:
> On 2014/09/02 at 23:53:32, jbroman wrote:
> > 2. Having chrome provide a special scheme for this, like it has
> chrome-extension:// and such already. Then we could just have an @import of
> blink-resource://PluginPlaceholderElement.css or similar. If I understand the
> architecture of this, I think it would go to the browser process as part of
> this, but that may not be a big deal. This might be generally useful (for
> loading images etc. as well), but it's more complicated. We'd also have to
> decide whether it's okay for these to just always be loadable (e.g. I think
> web-accessible extension resources always are, but I'm not sure), or whether
we
> need to confine their loading to private script only.
> 
> ^^^ That sounds ideal.

Another +1 to the idea. I don't have a concern in exposing the blink-resource://
to the web since the resource is anyway exposed.

[haraken, 2014-09-03 04:08:55]

haraken@chromium.org changed reviewers:
+ tasak@google.com, vivekg@chromium.org

[haraken, 2014-09-03 04:08:56]

+vivekg, +tasak

[tasak, 2014-09-03 07:03:01]

On 2014/09/03 04:08:56, haraken wrote:
> +vivekg, +tasak

I'm afraid that I bring up the same discussion again.
However I would like to ask one thing.

As far as I understand,  when we implement a new element to blink, we will add
style for the element to html.css.
So is there any reason why PluginPlaceHolderElement does not use html.css?

[abarth-chromium, 2014-09-03 17:19:29]

On 2014/09/03 at 07:03:01, tasak wrote:
> As far as I understand,  when we implement a new element to blink, we will add
style for the element to html.css.
> So is there any reason why PluginPlaceHolderElement does not use html.css?

If we add the CSS to html.css, we'll need to process it for every web page on
the Internet.  If we supply the CSS in the shadow root for the placeholder
element, then the CSS will be processed only when there's actually a plugin
placeholder on the page and then only inside the shadow root for the plugin
placeholder.

Esoteric features like the placeholder plugin shouldn't take up global
resources, like space in html.css.

[jbroman, 2014-09-16 21:39:26]

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

https://codereview.chromium.org/516273002/diff/220001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:145: inline static bool
shouldBypassMainWorldCSP(Element* e)
On 2014/09/02 23:53:32, jbroman wrote:
> On 2014/09/02 22:34:33, abarth wrote:
> > inline and static are redundant here.  Just static is fine.
> 
> Will do.

Done.

https://codereview.chromium.org/516273002/diff/220001/Source/core/dom/StyleEl...
Source/core/dom/StyleElement.cpp:148: LocalFrame* frame = e->document().frame();
On 2014/09/02 23:53:32, jbroman wrote:
> On 2014/09/02 22:34:33, abarth wrote:
> > s/e/element/
> 
> Will do. This name was just here because I moved code from
> StyleElement::createSheet.

Done.

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.cpp (right):

https://codereview.chromium.org/516273002/diff/220001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.cpp:38: return
pluginPlaceholderElementCss;
On 2014/09/02 23:53:32, jbroman wrote:
> On 2014/09/02 22:34:34, abarth wrote:
> > Please use GRD to load this resource.  See other callers of
> > Platform::current()->loadResource(...)
> 
> I'd done that previously, though I had trouble. In particular, I wasn't able
to
> get it to actually rebuild when I changed a resource, unless I clobbered. I'll
> try again, but I'm not sure if there's a trick here.

Done.

[jbroman, 2014-09-16 21:42:50]

Previous change has now landed, so I'm bringing this up again.

This will need to land as a three-sided change at present; if this looks good
I'll split off two trivial CLs to create the PluginPlaceholderElement.css
resource
and land this afterward.

On 2014/09/03 00:38:59, abarth wrote:
> On 2014/09/02 at 23:53:32, jbroman wrote:
> > 2. Having chrome provide a special scheme for this, like it has
> chrome-extension:// and such already. Then we could just have an @import of
> blink-resource://PluginPlaceholderElement.css or similar. If I understand the
> architecture of this, I think it would go to the browser process as part of
> this, but that may not be a big deal. This might be generally useful (for
> loading images etc. as well), but it's more complicated. We'd also have to
> decide whether it's okay for these to just always be loadable (e.g. I think
> web-accessible extension resources always are, but I'm not sure), or whether
we
> need to confine their loading to private script only.
> 
> ^^^ That sounds ideal.

Investigation reveals that it seems fairly involved. I've filed
http://crbug.com/414849.

In the spirit of at least being general now, I have exposed
"loadPlatformResource" to private script and left a FIXME referring to the bug.

WDYT? Is that good enough, or is this CL blocked on a more complete solution for
private script loading resources?

[jbroman, 2014-09-30 13:22:26]

On 2014/09/16 21:42:50, jbroman wrote:
> Previous change has now landed, so I'm bringing this up again.
> 
> This will need to land as a three-sided change at present; if this looks good
> I'll split off two trivial CLs to create the PluginPlaceholderElement.css
> resource
> and land this afterward.
> 
> On 2014/09/03 00:38:59, abarth wrote:
> > On 2014/09/02 at 23:53:32, jbroman wrote:
> > > 2. Having chrome provide a special scheme for this, like it has
> > chrome-extension:// and such already. Then we could just have an @import of
> > blink-resource://PluginPlaceholderElement.css or similar. If I understand
the
> > architecture of this, I think it would go to the browser process as part of
> > this, but that may not be a big deal. This might be generally useful (for
> > loading images etc. as well), but it's more complicated. We'd also have to
> > decide whether it's okay for these to just always be loadable (e.g. I think
> > web-accessible extension resources always are, but I'm not sure), or whether
> we
> > need to confine their loading to private script only.
> > 
> > ^^^ That sounds ideal.
> 
> Investigation reveals that it seems fairly involved. I've filed
> http://crbug.com/414849.
> 
> In the spirit of at least being general now, I have exposed
> "loadPlatformResource" to private script and left a FIXME referring to the
bug.
> 
> WDYT? Is that good enough, or is this CL blocked on a more complete solution
for
> private script loading resources?

bump (2 weeks since last activity)

[abarth-chromium, 2014-10-03 18:11:20]

Sorry for the delay.  I'm happy to defer to other reviewers.  Maybe haraken
would be willing to complete the review?

[haraken, 2014-10-04 00:56:20]

The CL looks good, if abarth@ is fine with the private API added to
WindowPrivateScript.idl.

https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
File Source/core/frame/WindowPrivateScript.idl (right):

https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
Source/core/frame/WindowPrivateScript.idl:11: [OnlyExposedToPrivateScript]
DOMString loadPlatformResource(DOMString name);

Will we be able to use this API to load images, javascript etc, not limited to
css?

https://codereview.chromium.org/516273002/diff/240001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/516273002/diff/240001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:12: styleElement.textContent
= loadPlatformResource('PluginPlaceholderElement.css');

Just to confirm: loadPlatformResource takes a relative path, right?

[jbroman, 2014-10-04 03:36:07]

https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
File Source/core/frame/WindowPrivateScript.idl (right):

https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
Source/core/frame/WindowPrivateScript.idl:11: [OnlyExposedToPrivateScript]
DOMString loadPlatformResource(DOMString name);
On 2014/10/04 00:56:20, haraken wrote:
> 
> Will we be able to use this API to load images, javascript etc, not limited to
> css?

Since it returns a string, I don't think it would be suitable for images. I
don't think this is ultimately the best approach for this reason, and the fact
that it doesn't facilitate resolution of dependent resources (e.g. images in
stylesheets); see crbug.com/414849. Perhaps a parallel one could produce a data
URL, but I'm hoping this will be short-lived.

https://codereview.chromium.org/516273002/diff/240001/Source/core/html/shadow...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/516273002/diff/240001/Source/core/html/shadow...
Source/core/html/shadow/PluginPlaceholderElement.js:12: styleElement.textContent
= loadPlatformResource('PluginPlaceholderElement.css');
On 2014/10/04 00:56:20, haraken wrote:
> 
> Just to confirm: loadPlatformResource takes a relative path, right?

At the moment, it passes the path to Platform::loadResource, which is mapped to
data via a map in content::BlinkPlatformImpl (from a name to a resource ID), and
in blink_resources.grd (from a resource ID to a source filename).

The existing convention for resource names is filenames without any leading
path, which is why this does not need qualification.

(Unless vivekg's effort to refactor this mapping lands, this patch will be
broken into three to include the Chromium-side patch. But two of those will be
trivial. The Chromium side will look roughly like
https://codereview.chromium.org/556793006/, but with CSS instead of JS.)

[haraken, 2014-10-05 23:54:39]

https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
File Source/core/frame/WindowPrivateScript.idl (right):

https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
Source/core/frame/WindowPrivateScript.idl:11: [OnlyExposedToPrivateScript]
DOMString loadPlatformResource(DOMString name);
On 2014/10/04 03:36:06, jbroman wrote:
> On 2014/10/04 00:56:20, haraken wrote:
> > 
> > Will we be able to use this API to load images, javascript etc, not limited
to
> > css?
> 
> Since it returns a string, I don't think it would be suitable for images. I
> don't think this is ultimately the best approach for this reason, and the fact
> that it doesn't facilitate resolution of dependent resources (e.g. images in
> stylesheets); see crbug.com/414849. Perhaps a parallel one could produce a
data
> URL, but I'm hoping this will be short-lived.

Yeah, if the loadPlatformResource API doesn't facilitate resolution of dependent
resources, we need rethink about the API.

Whats's the "parallel one"?

I don't quite follow the discussion in crbug.com/414849, but did you give up the
"chrome-resource://" approach? (I guess the "chrome-resource://" approach will
resolve dependent resources as well.)

[jbroman, 2014-10-06 14:58:13]

On 2014/10/05 23:54:39, haraken wrote:
>
https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
> Source/core/frame/WindowPrivateScript.idl:11: [OnlyExposedToPrivateScript]
> DOMString loadPlatformResource(DOMString name);
> On 2014/10/04 03:36:06, jbroman wrote:
> > On 2014/10/04 00:56:20, haraken wrote:
> > > 
> > > Will we be able to use this API to load images, javascript etc, not
limited
> to
> > > css?
> > 
> > Since it returns a string, I don't think it would be suitable for images. I
> > don't think this is ultimately the best approach for this reason, and the
fact
> > that it doesn't facilitate resolution of dependent resources (e.g. images in
> > stylesheets); see crbug.com/414849. Perhaps a parallel one could produce a
> data
> > URL, but I'm hoping this will be short-lived.
> 
> Yeah, if the loadPlatformResource API doesn't facilitate resolution of
dependent
> resources, we need rethink about the API.
> 
> Whats's the "parallel one"?
> 
> I don't quite follow the discussion in crbug.com/414849, but did you give up
the
> "chrome-resource://" approach? (I guess the "chrome-resource://" approach will
> resolve dependent resources as well.)

I'm sorry, I didn't express that very clearly. Please allow me to (attempt to)
clarify.

I agree that we want to allow resolution of dependent resources, and that this
API does not do that.

I still believe that a chrome-resource:// scheme is the right approach, but
someone does need to take the time to implement that in Chrome. I don't think
it's a ton of work, but it's not super-easy either (especially since I think it
requires a security approval). My hope was to implement this as a "stop-gap"
measure only until a better solution, such as chrome-resource://, is available
(i.e. bug 414849 is fixed).

My offhand comment to a "parallel one" was not very clear. What I meant was: the
API in this CL does not facilitate images because it does not support sending
binary data to JavaScript. It could be extended with another method, say
"loadPlatformResourceAsDataURI", that returned binary data encoded as a data
URI. That is not necessary for this CL, and it is not flexible enough for
dependent resource loading, so I don't think it is worthwhile. But such an API
would be possible if we required it soon.

I do think we should implement chrome-resource:// or something similar. I'm just
not sure whether that solution should block this CL, or whether this workaround
is acceptable in the meantime.

[haraken, 2014-10-06 15:20:34]

On 2014/10/06 14:58:13, jbroman wrote:
> On 2014/10/05 23:54:39, haraken wrote:
> >
>
https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
> > Source/core/frame/WindowPrivateScript.idl:11: [OnlyExposedToPrivateScript]
> > DOMString loadPlatformResource(DOMString name);
> > On 2014/10/04 03:36:06, jbroman wrote:
> > > On 2014/10/04 00:56:20, haraken wrote:
> > > > 
> > > > Will we be able to use this API to load images, javascript etc, not
> limited
> > to
> > > > css?
> > > 
> > > Since it returns a string, I don't think it would be suitable for images.
I
> > > don't think this is ultimately the best approach for this reason, and the
> fact
> > > that it doesn't facilitate resolution of dependent resources (e.g. images
in
> > > stylesheets); see crbug.com/414849. Perhaps a parallel one could produce a
> > data
> > > URL, but I'm hoping this will be short-lived.
> > 
> > Yeah, if the loadPlatformResource API doesn't facilitate resolution of
> dependent
> > resources, we need rethink about the API.
> > 
> > Whats's the "parallel one"?
> > 
> > I don't quite follow the discussion in crbug.com/414849, but did you give up
> the
> > "chrome-resource://" approach? (I guess the "chrome-resource://" approach
will
> > resolve dependent resources as well.)
> 
> I'm sorry, I didn't express that very clearly. Please allow me to (attempt to)
> clarify.
> 
> I agree that we want to allow resolution of dependent resources, and that this
> API does not do that.
> 
> I still believe that a chrome-resource:// scheme is the right approach, but
> someone does need to take the time to implement that in Chrome. I don't think
> it's a ton of work, but it's not super-easy either (especially since I think
it
> requires a security approval). My hope was to implement this as a "stop-gap"
> measure only until a better solution, such as chrome-resource://, is available
> (i.e. bug 414849 is fixed).
> 
> My offhand comment to a "parallel one" was not very clear. What I meant was:
the
> API in this CL does not facilitate images because it does not support sending
> binary data to JavaScript. It could be extended with another method, say
> "loadPlatformResourceAsDataURI", that returned binary data encoded as a data
> URI. That is not necessary for this CL, and it is not flexible enough for
> dependent resource loading, so I don't think it is worthwhile. But such an API
> would be possible if we required it soon.
> 
> I do think we should implement chrome-resource:// or something similar. I'm
just
> not sure whether that solution should block this CL, or whether this
workaround
> is acceptable in the meantime.

Thanks for a lot of contexts; understood.

The discussion seems to be about how strict we should be to approve private
APIs. Various folks have various thoughts on this. For example, when tasak@
tried to implement XSLT-in-JS, he noticed that we cannot implement XSLT-in-JS
without either adding a private API or deprecating external DTDs. Then we
decided to defer the XSLT-in-JS work until we collect enough use counters and
deprecate the external DTDs.

I think you should ask an opinion of abarth@ about the private API.

[jbroman, 2014-10-07 20:14:45]

On 2014/10/06 15:20:34, haraken wrote:
> On 2014/10/06 14:58:13, jbroman wrote:
> > On 2014/10/05 23:54:39, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
> > > Source/core/frame/WindowPrivateScript.idl:11: [OnlyExposedToPrivateScript]
> > > DOMString loadPlatformResource(DOMString name);
> > > On 2014/10/04 03:36:06, jbroman wrote:
> > > > On 2014/10/04 00:56:20, haraken wrote:
> > > > > 
> > > > > Will we be able to use this API to load images, javascript etc, not
> > limited
> > > to
> > > > > css?
> > > > 
> > > > Since it returns a string, I don't think it would be suitable for
images.
> I
> > > > don't think this is ultimately the best approach for this reason, and
the
> > fact
> > > > that it doesn't facilitate resolution of dependent resources (e.g.
images
> in
> > > > stylesheets); see crbug.com/414849. Perhaps a parallel one could produce
a
> > > data
> > > > URL, but I'm hoping this will be short-lived.
> > > 
> > > Yeah, if the loadPlatformResource API doesn't facilitate resolution of
> > dependent
> > > resources, we need rethink about the API.
> > > 
> > > Whats's the "parallel one"?
> > > 
> > > I don't quite follow the discussion in crbug.com/414849, but did you give
up
> > the
> > > "chrome-resource://" approach? (I guess the "chrome-resource://" approach
> will
> > > resolve dependent resources as well.)
> > 
> > I'm sorry, I didn't express that very clearly. Please allow me to (attempt
to)
> > clarify.
> > 
> > I agree that we want to allow resolution of dependent resources, and that
this
> > API does not do that.
> > 
> > I still believe that a chrome-resource:// scheme is the right approach, but
> > someone does need to take the time to implement that in Chrome. I don't
think
> > it's a ton of work, but it's not super-easy either (especially since I think
> it
> > requires a security approval). My hope was to implement this as a "stop-gap"
> > measure only until a better solution, such as chrome-resource://, is
available
> > (i.e. bug 414849 is fixed).
> > 
> > My offhand comment to a "parallel one" was not very clear. What I meant was:
> the
> > API in this CL does not facilitate images because it does not support
sending
> > binary data to JavaScript. It could be extended with another method, say
> > "loadPlatformResourceAsDataURI", that returned binary data encoded as a data
> > URI. That is not necessary for this CL, and it is not flexible enough for
> > dependent resource loading, so I don't think it is worthwhile. But such an
API
> > would be possible if we required it soon.
> > 
> > I do think we should implement chrome-resource:// or something similar. I'm
> just
> > not sure whether that solution should block this CL, or whether this
> workaround
> > is acceptable in the meantime.
> 
> Thanks for a lot of contexts; understood.
> 
> The discussion seems to be about how strict we should be to approve private
> APIs. Various folks have various thoughts on this. For example, when tasak@
> tried to implement XSLT-in-JS, he noticed that we cannot implement XSLT-in-JS
> without either adding a private API or deprecating external DTDs. Then we
> decided to defer the XSLT-in-JS work until we collect enough use counters and
> deprecate the external DTDs.
> 
> I think you should ask an opinion of abarth@ about the private API.

My reading of https://codereview.chromium.org/516273002/#msg26 is that abarth@
doesn't have the time to deal with this CL right now, and is asking you to do
so.
Are you asking me to IM him to ask him to weigh in on this particular issue?

[haraken, 2014-10-08 00:27:19]

On 2014/10/07 20:14:45, jbroman wrote:
> On 2014/10/06 15:20:34, haraken wrote:
> > On 2014/10/06 14:58:13, jbroman wrote:
> > > On 2014/10/05 23:54:39, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/516273002/diff/240001/Source/core/frame/Windo...
> > > > Source/core/frame/WindowPrivateScript.idl:11:
[OnlyExposedToPrivateScript]
> > > > DOMString loadPlatformResource(DOMString name);
> > > > On 2014/10/04 03:36:06, jbroman wrote:
> > > > > On 2014/10/04 00:56:20, haraken wrote:
> > > > > > 
> > > > > > Will we be able to use this API to load images, javascript etc, not
> > > limited
> > > > to
> > > > > > css?
> > > > > 
> > > > > Since it returns a string, I don't think it would be suitable for
> images.
> > I
> > > > > don't think this is ultimately the best approach for this reason, and
> the
> > > fact
> > > > > that it doesn't facilitate resolution of dependent resources (e.g.
> images
> > in
> > > > > stylesheets); see crbug.com/414849. Perhaps a parallel one could
produce
> a
> > > > data
> > > > > URL, but I'm hoping this will be short-lived.
> > > > 
> > > > Yeah, if the loadPlatformResource API doesn't facilitate resolution of
> > > dependent
> > > > resources, we need rethink about the API.
> > > > 
> > > > Whats's the "parallel one"?
> > > > 
> > > > I don't quite follow the discussion in crbug.com/414849, but did you
give
> up
> > > the
> > > > "chrome-resource://" approach? (I guess the "chrome-resource://"
approach
> > will
> > > > resolve dependent resources as well.)
> > > 
> > > I'm sorry, I didn't express that very clearly. Please allow me to (attempt
> to)
> > > clarify.
> > > 
> > > I agree that we want to allow resolution of dependent resources, and that
> this
> > > API does not do that.
> > > 
> > > I still believe that a chrome-resource:// scheme is the right approach,
but
> > > someone does need to take the time to implement that in Chrome. I don't
> think
> > > it's a ton of work, but it's not super-easy either (especially since I
think
> > it
> > > requires a security approval). My hope was to implement this as a
"stop-gap"
> > > measure only until a better solution, such as chrome-resource://, is
> available
> > > (i.e. bug 414849 is fixed).
> > > 
> > > My offhand comment to a "parallel one" was not very clear. What I meant
was:
> > the
> > > API in this CL does not facilitate images because it does not support
> sending
> > > binary data to JavaScript. It could be extended with another method, say
> > > "loadPlatformResourceAsDataURI", that returned binary data encoded as a
data
> > > URI. That is not necessary for this CL, and it is not flexible enough for
> > > dependent resource loading, so I don't think it is worthwhile. But such an
> API
> > > would be possible if we required it soon.
> > > 
> > > I do think we should implement chrome-resource:// or something similar.
I'm
> > just
> > > not sure whether that solution should block this CL, or whether this
> > workaround
> > > is acceptable in the meantime.
> > 
> > Thanks for a lot of contexts; understood.
> > 
> > The discussion seems to be about how strict we should be to approve private
> > APIs. Various folks have various thoughts on this. For example, when tasak@
> > tried to implement XSLT-in-JS, he noticed that we cannot implement
XSLT-in-JS
> > without either adding a private API or deprecating external DTDs. Then we
> > decided to defer the XSLT-in-JS work until we collect enough use counters
and
> > deprecate the external DTDs.
> > 
> > I think you should ask an opinion of abarth@ about the private API.
> 
> My reading of https://codereview.chromium.org/516273002/#msg26 is that abarth@
> doesn't have the time to deal with this CL right now, and is asking you to do
> so.
> Are you asking me to IM him to ask him to weigh in on this particular issue?

I'll start a separate thread.

We need to improve the decision-making process for private APIs.

[PhistucK, 2014-10-17 07:36:49]

On 2014/08/31 16:17:20, jbroman wrote:
> On 2014/08/31 06:32:23, PhistucK wrote:
> > Is there any way to use <link> (or some other way) than to punch more holes
in
> > CSP? Perhaps the inline style simply makes sense it this case. Perhaps
adding
> > this to the normal user agent stylesheet somehow?
> 
> (This patch has not yet been sent for review, but nonetheless...)
> 
> I'm definitely open to suggestions; this only seemed like the most
> straightforward approach.
> 
> I don't believe <link> gets the same scoped-style properties that <style>
does,
> and an @import in a <style> would, I believe, still be subject to the
> inline-style restriction.
> 
> I'm not sure what form adding this to the UA stylesheet would take. Our
> traditional approach is to use pseudo-IDs, like video::-webkit-media-controls,
> but not only are these exposed to the author in cases we might not wish it (we
> could implement a notion of internal pseudo-IDs, of course), but I don't think
> they facilitate cases like "I want all of the links in this shadow root to be
> pink" (though I haven't dug that deeply).
> 
> Ultimately, we *are* loading a style resource that the document has not
allowed
> (in the worst case, it might say "style-src 'none'"), so we're bypassing the
> content security policy in one way or another.

(Sorry for the rather late response, I was not notified)
I believe there is already an internal pseudo IDs notion, actually. Search for
-internal-foo stuff, if I am not mistaken.
Oh, perhaps these are internal CSS properties or values...
I am pretty sure there were pseudo elements with an -internal- prefix...

[jbroman, 2014-10-17 12:50:04]

On 2014/10/17 07:36:49, PhistucK wrote:
> On 2014/08/31 16:17:20, jbroman wrote:
> > On 2014/08/31 06:32:23, PhistucK wrote:
> > > Is there any way to use <link> (or some other way) than to punch more
holes
> in
> > > CSP? Perhaps the inline style simply makes sense it this case. Perhaps
> adding
> > > this to the normal user agent stylesheet somehow?
> > 
> > (This patch has not yet been sent for review, but nonetheless...)
> > 
> > I'm definitely open to suggestions; this only seemed like the most
> > straightforward approach.
> > 
> > I don't believe <link> gets the same scoped-style properties that <style>
> does,
> > and an @import in a <style> would, I believe, still be subject to the
> > inline-style restriction.
> > 
> > I'm not sure what form adding this to the UA stylesheet would take. Our
> > traditional approach is to use pseudo-IDs, like
video::-webkit-media-controls,
> > but not only are these exposed to the author in cases we might not wish it
(we
> > could implement a notion of internal pseudo-IDs, of course), but I don't
think
> > they facilitate cases like "I want all of the links in this shadow root to
be
> > pink" (though I haven't dug that deeply).
> > 
> > Ultimately, we *are* loading a style resource that the document has not
> allowed
> > (in the worst case, it might say "style-src 'none'"), so we're bypassing the
> > content security policy in one way or another.
> 
> (Sorry for the rather late response, I was not notified)
> I believe there is already an internal pseudo IDs notion, actually. Search for
> -internal-foo stuff, if I am not mistaken.
> Oh, perhaps these are internal CSS properties or values...
> I am pretty sure there were pseudo elements with an -internal- prefix...

I couldn't find a pseudo-ID using "-internal" (though yes, I did find such
properties). Even if we had those, though, we'd still have to put it in html.css
or equivalent (which loads with every page) and loading dependent images, etc.
would still have to be done through -webkit-appearance or another similar back
door (which requires quite a bit of boilerplate).

[jbroman, 2014-10-17 18:25:08]

Okay, I've adjusted this to simply have the CSS source inline in the script file
for now. This will eventually hit limitations, but it does at least ensure that
a stylesheet can be loaded in this context (even with CSP on, thanks to the UA
shadow root exemption suggested by abarth above).

I've left a FIXME to load this from a separate file later on (hopefully in a way
that can load image resources, etc.).

I think this is the non-controversial part of this CL, so might as well at least
land that part now if it looks good.

WDYT, haraken-san?

[haraken, 2014-10-18 05:25:48]

On 2014/10/17 18:25:08, jbroman wrote:
> Okay, I've adjusted this to simply have the CSS source inline in the script
file
> for now. This will eventually hit limitations, but it does at least ensure
that
> a stylesheet can be loaded in this context (even with CSP on, thanks to the UA
> shadow root exemption suggested by abarth above).
> 
> I've left a FIXME to load this from a separate file later on (hopefully in a
way
> that can load image resources, etc.).
> 
> I think this is the non-controversial part of this CL, so might as well at
least
> land that part now if it looks good.
> 
> WDYT, haraken-san?

LGTM. Let's introduce a better way to handle this once the blink-dev discussion
about private script APIs or the security discussion about chrome-resource:// is
settled.

[jbroman, 2014-10-18 14:20:38]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-10-18 14:21:22]

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

[commit-bot: I haz the power, 2014-10-18 14:21:25]

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

[commit-bot: I haz the power, 2014-10-18 14:21:26]

A disapproval has been posted by following reviewers: abarth@chromium.org.
Please make sure to get positive review before another CQ attempt.

[abarth-chromium, 2014-10-20 20:40:04]

lgtm to make my name not red (didn't read the cl)

[jbroman, 2014-10-20 20:40:22]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-10-20 20:41:21]

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

[commit-bot: I haz the power, 2014-10-20 20:45:57]

Committed patchset #12 (id:300001) as 184015