Plugin Power Saver: Implement srcset syntax for posters.

Plugin Power Saver: Implement srcset syntax for posters.

The objective of the patch is that the poster attribute for plugins can support high-dpi images via the img srcset syntax, i.e.

<param name="poster" value="poster1x.png 1x, poster2x.png 2x" />

BUG=457333
Committed: https://crrev.com/bacf2a1a5b79f59460b1258a502f9f889f18e59f
Cr-Commit-Position: refs/heads/master@{#319689}

[tommycli, 2015-03-04 19:49:47]

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

[tommycli, 2015-03-04 19:54:47]

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

[tommycli, 2015-03-04 19:55:17]

bauerb/tsepez: PTAL

Not sure of this approach. Has some security implications. Goal is in patch
description.

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:317: std::string
GetPluginInstancePoster(const blink::WebPluginParams& params) {
Instead of resolving to an absolute URL, we just get the plugin param verbatim
now.

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:179:
placeholder_url.is_valid() ? placeholder_url
Needs some security scrutiny:

In order to allow resolving of relative paths, we set the placeholder's URL to
the document URL instead of data:text/html,pluginplaceholderdata.

Is this safe? I could not think of another reasonable alternative to doing this.

Previously, we resolved the image to an absolute path before passing it into
here. Now, we are switching to the img srcset syntax, i.e. <img
srcset="poster1x.png 1x, poster2x.png 2x">.

The srcset parsing code is deep and nonpublic in Blink, so it would be a real
PITA to resolve them to absolute paths.

[Tom Sepez, 2015-03-04 20:55:02]

On 2015/03/04 19:55:17, tommycli wrote:
> bauerb/tsepez: PTAL
> 
> Not sure of this approach. Has some security implications. Goal is in patch
> description.
> 
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/chrome_c...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/chrome_c...
> chrome/renderer/chrome_content_renderer_client.cc:317: std::string
> GetPluginInstancePoster(const blink::WebPluginParams& params) {
> Instead of resolving to an absolute URL, we just get the plugin param verbatim
> now.
> 
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
> File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
> chrome/renderer/plugins/chrome_plugin_placeholder.cc:179:
> placeholder_url.is_valid() ? placeholder_url
> Needs some security scrutiny:
> 
> In order to allow resolving of relative paths, we set the placeholder's URL to
> the document URL instead of data:text/html,pluginplaceholderdata.
> 
> Is this safe? I could not think of another reasonable alternative to doing
this.
> 
Not a good idea.  Data URLs have unique origins.  We may need a new member
somewhere to hold this actual URL

[tommycli, 2015-03-04 21:40:37]

On 2015/03/04 20:55:02, Tom Sepez wrote:
> On 2015/03/04 19:55:17, tommycli wrote:
> > bauerb/tsepez: PTAL
> > 
> > Not sure of this approach. Has some security implications. Goal is in patch
> > description.
> > 
> >
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/chrome_c...
> > File chrome/renderer/chrome_content_renderer_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/chrome_c...
> > chrome/renderer/chrome_content_renderer_client.cc:317: std::string
> > GetPluginInstancePoster(const blink::WebPluginParams& params) {
> > Instead of resolving to an absolute URL, we just get the plugin param
verbatim
> > now.
> > 
> >
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
> > File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):
> > 
> >
>
https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
> > chrome/renderer/plugins/chrome_plugin_placeholder.cc:179:
> > placeholder_url.is_valid() ? placeholder_url
> > Needs some security scrutiny:
> > 
> > In order to allow resolving of relative paths, we set the placeholder's URL
to
> > the document URL instead of data:text/html,pluginplaceholderdata.
> > 
> > Is this safe? I could not think of another reasonable alternative to doing
> this.
> > 
> Not a good idea.  Data URLs have unique origins.  We may need a new member
> somewhere to hold this actual URL

Hey, is it alright if we control the contents of the inner WebPlugin? I
understand that assigning it the document's URL allows the placeholders' scripts
to mess with the origin's data, but what if the inner WebPlugin's contents are
fixed? 

https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/r...

I don't completely understand the situation.

[Bernhard Bauer, 2015-03-05 09:12:52]

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:179:
placeholder_url.is_valid() ? placeholder_url
On 2015/03/04 19:55:17, tommycli wrote:
> Needs some security scrutiny:
> 
> In order to allow resolving of relative paths, we set the placeholder's URL to
> the document URL instead of data:text/html,pluginplaceholderdata.
> 
> Is this safe? I could not think of another reasonable alternative to doing
this.
> 
> Previously, we resolved the image to an absolute path before passing it into
> here. Now, we are switching to the img srcset syntax, i.e. <img
> srcset="poster1x.png 1x, poster2x.png 2x">.
> 
> The srcset parsing code is deep and nonpublic in Blink, so it would be a real
> PITA to resolve them to absolute paths.

Could you just set a <base> tag in the header? That would allow resolving
relative URLs, without changing security-related properties (AFAIK).

[tommycli, 2015-03-05 19:49:12]

bauerb: Your suggestion solved the issue! PTAL

Thanks!

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/981623003/diff/40001/chrome/renderer/plugins/...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:179:
placeholder_url.is_valid() ? placeholder_url
On 2015/03/05 09:12:51, Bernhard (slow until Mar 9) wrote:
> On 2015/03/04 19:55:17, tommycli wrote:
> > Needs some security scrutiny:
> > 
> > In order to allow resolving of relative paths, we set the placeholder's URL
to
> > the document URL instead of data:text/html,pluginplaceholderdata.
> > 
> > Is this safe? I could not think of another reasonable alternative to doing
> this.
> > 
> > Previously, we resolved the image to an absolute path before passing it into
> > here. Now, we are switching to the img srcset syntax, i.e. <img
> > srcset="poster1x.png 1x, poster2x.png 2x">.
> > 
> > The srcset parsing code is deep and nonpublic in Blink, so it would be a
real
> > PITA to resolve them to absolute paths.
> 
> Could you just set a <base> tag in the header? That would allow resolving
> relative URLs, without changing security-related properties (AFAIK).

That totally worked! You guys are smarter than me!

[Tom Sepez, 2015-03-05 20:28:42]

LGTM from security perspective.

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:317: std::string
GetPluginInstancePoster(const blink::WebPluginParams& params) {
nit: maybe GetPluginInstancePosterAttribute(), since this gets an attribute
string, rather that the "Poster" itself, which implies in my tiny mind, some
sort of object representing a poster.

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
chrome/renderer/plugins/chrome_plugin_placeholder.h:20: // If |poster_attribute|
can contain relative paths, |base_url| must be
nit: s/can contain/contains/

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
chrome/renderer/plugins/plugin_preroller.h:56: std::string keyframe_data_url_;
Do we need this change under the new approach?

[Bernhard Bauer, 2015-03-05 20:29:43]

lgtm

[tommycli, 2015-03-06 00:17:31]

tsepez: Thanks for review!

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:317: std::string
GetPluginInstancePoster(const blink::WebPluginParams& params) {
On 2015/03/05 20:28:42, Tom Sepez wrote:
> nit: maybe GetPluginInstancePosterAttribute(), since this gets an attribute
> string, rather that the "Poster" itself, which implies in my tiny mind, some
> sort of object representing a poster.

Done.

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
chrome/renderer/plugins/chrome_plugin_placeholder.h:20: // If |poster_attribute|
can contain relative paths, |base_url| must be
On 2015/03/05 20:28:42, Tom Sepez wrote:
> nit: s/can contain/contains/

Done.

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
File chrome/renderer/plugins/plugin_preroller.h (right):

https://codereview.chromium.org/981623003/diff/60001/chrome/renderer/plugins/...
chrome/renderer/plugins/plugin_preroller.h:56: std::string keyframe_data_url_;
On 2015/03/05 20:28:42, Tom Sepez wrote:
> Do we need this change under the new approach? 

Done. I changed it back. I take it the GURL provides an extra layer of
validation against potentially malicious strings?

[tommycli, 2015-03-06 00:19:24]

bauerb: Could you take another look at the additions in Patchset 6?

To load the correct resource in the srcset, I had to propagate the device scale
factor into the inner webview.

However, I had to 'unapply' it during the paint stage, otherwise the scaling
transform would be applied twice and the inner webview would be quadruple size
when device scale = 2.

Seems a bit hacky but it works. Thoughts?

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:154: // Reapply the container's
device scale factor for resource loading.
Not sure if this is actually needed. It doesn't seem to have an effect in my
testing, but it seems like it would be good.

[Bernhard Bauer, 2015-03-06 10:13:00]

Seem reasonable to me if it works, but I'm absolutely no expert on Blink and
painting etc. If you really want to make sure that this makes sense, I would ask
someone who's more familiar with Blink.

Another thing that just _might_ work would be to only unapply the scaling on the
|canvas| before painting (because the inner WebView will apply it later),
without changing the device scale factor in the inner WebView. But then again, I
repeat my disclaimer: I have no idea what I'm doing :)

[tommycli, 2015-03-06 17:16:41]

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

[tommycli, 2015-03-06 17:16:42]

jbroman: Do you mind taking a look at the painting logic added between Patchsets
5 and 6?

It works on my machine, but I wanted a second opinion to see if I'm stepping
into any holes.

[jbroman, 2015-03-06 20:25:49]

https://codereview.chromium.org/981623003/diff/100001/chrome/renderer/resourc...
File chrome/renderer/resources/plugin_poster.html (right):

https://codereview.chromium.org/981623003/diff/100001/chrome/renderer/resourc...
chrome/renderer/resources/plugin_poster.html:60: <img id="poster"
i18n-values="srcset:poster">
I see you have security signoff, so you've probably already thought about this,
but...what content security policy applies in this context? Does this allow the
page to load images that the CSP forbids (and is that okay)?

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:149:
web_view_->setDeviceScaleFactor(1);
Changing the device scale factor of the page like this seems wrong (if nothing
else, it forces a style recalc).

Rather, I would think applying a scale by 1/dsf to the canvas (to counter the
one in blink::PageWidgetDelegate::paint) is reasonable.

(In my ideal world, it would be the caller of WebViewImpl::paint that's
responsible for setting up the initial scale, but that's not how things are at
present.)

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:154: // Reapply the container's
device scale factor for resource loading.
On 2015/03/06 00:19:23, tommycli wrote:
> Not sure if this is actually needed. It doesn't seem to have an effect in my
> testing, but it seems like it would be good.

See above.

[tommycli, 2015-03-06 21:30:55]

jbroman: Thanks!

https://codereview.chromium.org/981623003/diff/100001/chrome/renderer/resourc...
File chrome/renderer/resources/plugin_poster.html (right):

https://codereview.chromium.org/981623003/diff/100001/chrome/renderer/resourc...
chrome/renderer/resources/plugin_poster.html:60: <img id="poster"
i18n-values="srcset:poster">
On 2015/03/06 20:25:49, jbroman wrote:
> I see you have security signoff, so you've probably already thought about
this,
> but...what content security policy applies in this context? Does this allow
the
> page to load images that the CSP forbids (and is that okay)?

Within the placeholder, the origin is a data URL, so scripts have access to
nothing.

Also, IMO, it should be no more dangerous to load an image within the
placeholder than directly on the page.

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:149:
web_view_->setDeviceScaleFactor(1);
On 2015/03/06 20:25:49, jbroman wrote:
> Changing the device scale factor of the page like this seems wrong (if nothing
> else, it forces a style recalc).
> 
> Rather, I would think applying a scale by 1/dsf to the canvas (to counter the
> one in blink::PageWidgetDelegate::paint) is reasonable.
> 
> (In my ideal world, it would be the caller of WebViewImpl::paint that's
> responsible for setting up the initial scale, but that's not how things are at
> present.)

Done. Okay cool. That's the same thing as bauerb said, so I'm going to go with
that.

https://codereview.chromium.org/981623003/diff/100001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:154: // Reapply the container's
device scale factor for resource loading.
On 2015/03/06 20:25:49, jbroman wrote:
> On 2015/03/06 00:19:23, tommycli wrote:
> > Not sure if this is actually needed. It doesn't seem to have an effect in my
> > testing, but it seems like it would be good.
> 
> See above.

Done.

https://codereview.chromium.org/981623003/diff/120001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/981623003/diff/120001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:144: canvas->save();
I also changed the order so 'save' is called before both translate and scale. I
assume saving should be the first canvas op right?

[tommycli, 2015-03-06 21:34:11]

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

[tommycli, 2015-03-06 21:34:12]

thestig: May I have an owner review for:
    chrome/renderer/chrome_content_renderer_client.cc
    chrome/renderer/resources/plugin_poster.html

[Lei Zhang, 2015-03-06 21:40:11]

lgtm

[tommycli, 2015-03-06 22:48:06]

jbroman: I made changes based on your suggestions. Did you want to take another
look at the results, or should I send to CQ?

Thanks!

[jbroman, 2015-03-07 03:56:02]

(non-owner) LGTM

https://codereview.chromium.org/981623003/diff/100001/chrome/renderer/resourc...
File chrome/renderer/resources/plugin_poster.html (right):

https://codereview.chromium.org/981623003/diff/100001/chrome/renderer/resourc...
chrome/renderer/resources/plugin_poster.html:60: <img id="poster"
i18n-values="srcset:poster">
On 2015/03/06 21:30:55, tommycli wrote:
> On 2015/03/06 20:25:49, jbroman wrote:
> > I see you have security signoff, so you've probably already thought about
> this,
> > but...what content security policy applies in this context? Does this allow
> the
> > page to load images that the CSP forbids (and is that okay)?
> 
> Within the placeholder, the origin is a data URL, so scripts have access to
> nothing.
> 
> Also, IMO, it should be no more dangerous to load an image within the
> placeholder than directly on the page.

Agreed, though there are cases where sites may consider it dangerous to load an
image within the page, too.

e.g. if a page includes untrusted HTML but secures itself by preventing any
resource loads from other origins, then this HTML will not reveal the user (i.e.
it won't issue a request to trackingsite.com):

<img src="http://trackingsite.com/image.png">

But this HTML, I think, will:

<embed poster="http://trackingsite.com/image.png 1x"></embed>

Even if I'm right, though, I think it's a small issue.

https://codereview.chromium.org/981623003/diff/120001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/981623003/diff/120001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:144: canvas->save();
On 2015/03/06 21:30:55, tommycli wrote:
> I also changed the order so 'save' is called before both translate and scale.
I
> assume saving should be the first canvas op right?

Yeah, looks good.

[tommycli, 2015-03-09 17:28:08]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-03-09 17:28:09]

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

[commit-bot: I haz the power, 2015-03-09 17:29:21]

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

[commit-bot: I haz the power, 2015-03-09 18:20:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-09 18:21:58]

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