Iframe placeholders, security and resizing

Iframe placeholders, security and resizing

This change adds javascript to fill in the YouTube placeholders
created by the distiller. the content security policy now allows
for iframes from all sources (distiller closely controls which
iframes are allowed). Some css has also been added to appropriately
resize these iframes when the page is resized.

This change will not work correctly by itself, it depends on the
following CL to work correctly:

https://codereview.chromium.org/1015463004

BUG=468862
Committed: https://crrev.com/d5d75e70536ec3dd67149d9281ca210b3ffbb95a
Cr-Commit-Position: refs/heads/master@{#323934}

[mdjones, 2015-01-30 17:18:00]

mdjones@chromium.org changed reviewers:
+ cjhopman@chromium.org

[mdjones, 2015-03-04 23:30:27]

PTAL

[cjhopman, 2015-03-12 02:39:22]

https://codereview.chromium.org/880983007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right):

https://codereview.chromium.org/880983007/diff/40001/components/dom_distiller...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:5: function
addToPage(html) {
Is this only called for pages 2+? Maybe we should change that...

https://codereview.chromium.org/880983007/diff/40001/components/dom_distiller...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:118:
setTimeout(resizeIframes, 150);
How about using an approach like
http://flwebsites.biz/posts/proportional-responsive-iframes-youtube-videos for
this?

[mdjones, 2015-03-14 02:04:44]

https://codereview.chromium.org/880983007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right):

https://codereview.chromium.org/880983007/diff/40001/components/dom_distiller...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:5: function
addToPage(html) {
On 2015/03/12 02:39:22, cjhopman wrote:
> Is this only called for pages 2+? Maybe we should change that...

Done.

https://codereview.chromium.org/880983007/diff/40001/components/dom_distiller...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:118:
setTimeout(resizeIframes, 150);
On 2015/03/12 02:39:22, cjhopman wrote:
> How about using an approach like
> http://flwebsites.biz/posts/proportional-responsive-iframes-youtube-videos for
> this?

Done.

[cjhopman, 2015-03-16 20:09:48]

could we make this two separate changes, one with the iframe/youtube stuff and
one with the addtopage/error page changes?

[mdjones, 2015-03-17 17:13:16]

On 2015/03/16 20:09:48, cjhopman wrote:
> could we make this two separate changes, one with the iframe/youtube stuff and
> one with the addtopage/error page changes?

Done

Link to other CL: https://codereview.chromium.org/1015463004

[cjhopman, 2015-03-19 03:34:47]

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:280: padding-bottom: 60%;
this is based on the video aspect ratio, right? comment here about that.

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:291: }
Could you add a screenshot to the description so I can see how this looks?

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right):

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:13: var
placeholders = document.getElementsByClassName("embed-placeholder");
s/"/'/g

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:32:
embed.setAttribute('height', width * hdMult);
Are these needed? Aren't they set by css now?

[mdjones, 2015-03-19 18:08:40]

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:280: padding-bottom: 60%;
On 2015/03/19 03:34:47, cjhopman wrote:
> this is based on the video aspect ratio, right? comment here about that.

Done.

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:291: }
On 2015/03/19 03:34:47, cjhopman wrote:
> Could you add a screenshot to the description so I can see how this looks?

Done. See https://code.google.com/p/chromium/issues/detail?id=468862

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right):

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:13: var
placeholders = document.getElementsByClassName("embed-placeholder");
On 2015/03/19 03:34:47, cjhopman wrote:
> s/"/'/g

Done.

https://codereview.chromium.org/880983007/diff/100001/components/dom_distille...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:32:
embed.setAttribute('height', width * hdMult);
On 2015/03/19 03:34:47, cjhopman wrote:
> Are these needed? Aren't they set by css now?

Done.

[cjhopman, 2015-03-21 02:33:48]

lgtm

[mdjones, 2015-04-06 18:58:10]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-04-06 18:58:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org
Link to the patchset: https://codereview.chromium.org/880983007/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-06 18:58:32]

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

[commit-bot: I haz the power, 2015-04-06 19:48:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-06 20:03:51]

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