Adding CSP attribute on HTMLIFrameElement

Adding CSP attribute on HTMLIFrameElement

As defined in https://w3c.github.io/webappsec-csp/embedded/#dom-htmliframeelement-csp.

BUG=647588
Committed: https://crrev.com/28ea58f34c71c6af9044f16e342ce395f8b2c5c1
Cr-Commit-Position: refs/heads/master@{#421182}

[amalika, 2016-09-26 12:39:31]

The CQ bit was checked by amalika@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 12:39:40]

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

[amalika, 2016-09-26 12:39:59]

Description was changed from

==========
Adding CSP attribute on HTMon HTMLIFrameElement

r=mkwst@chromium.org
BUG=647588
==========

to

==========
Adding CSP attribute on HTMon HTMLIFrameElement

BUG=647588
==========

[amalika, 2016-09-26 12:40:28]

amalika@google.com changed reviewers:
+ mkwst@chromium.org

[amalika, 2016-09-26 12:40:29]

Next Patch would be on Embedder-CSP header

[amalika, 2016-09-26 13:43:30]

Patchset #1 (id:1) has been deleted

[Mike West, 2016-09-26 14:20:55]

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

[commit-bot: I haz the power, 2016-09-26 14:21:11]

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

[Mike West, 2016-09-26 14:55:02]

Thanks! This is a great start. I have only a few small comments.

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html
(right):

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html:12:
assert_equals('string', typeof (i.csp));
Nit: Can you add `assert_true('csp' in i)`?

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html:25:
}, "<iframe> has a 'csp' attibute.");
Nit: Perhaps "<iframe>'s 'csp' content attribute reflects the IDL attribute."

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html:36:
}, "'csp' attribute of <iframe> has proper value.");
Nit: Perhaps "<iframe>'s IDL attribute reflects the DOM attribute."

Why did you add a second frame? It's not clear what it's testing that isn't
tested with the first frame.

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:124: m_csp = value;
I think you need to notify the client that this value was updated. You don't
_need_ to do that in this patch, as you're not actually replicating
`FrameOwnerProperties` up to the host yet, but you might as well call into
`frameOwnerPropertiesChanged()` now so that you don't have to look for it again
later. :)

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right):

https://codereview.chromium.org/2370783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:48:
[RuntimeEnabled=EmbedderCSPEnforcement, CEReactions, Reflect,
TreatNullAs=EmptyString] attribute DOMString csp;
1. Please move this up out of the `// obsolete members` section, and add a
comment pointing to the spec
(`https://w3c.github.io/webappsec-csp/embedded/#dom-htmliframeelement-csp`).

2. Please drop the `TreatNullAs` bit. I don't think we need it here. It means
that `i.csp = null` would be treated as `i.csp = ""`, which doesn't seem like a
restriction we need to add. :)

[amalika, 2016-09-26 15:18:54]

Patchset #3 (id:60001) has been deleted

[amalika, 2016-09-26 15:19:45]

Addressing comments

[Mike West, 2016-09-27 07:52:11]

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

[commit-bot: I haz the power, 2016-09-27 07:52:22]

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

[Mike West, 2016-09-27 07:54:22]

Tiny nits, otherwise LGTM! Land it. :)

https://codereview.chromium.org/2370783002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html
(right):

https://codereview.chromium.org/2370783002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html:13:
assert_equals('string', typeof (i.csp));
Nit: I don't think you need `()` here.

https://codereview.chromium.org/2370783002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/iframe-csp-attribute.html:30:

Nit: You don't have this newline in any other test block.

[amalika, 2016-09-27 08:34:35]

The CQ bit was checked by amalika@google.com

[amalika, 2016-09-27 08:34:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2370783002/#ps100001
(title: "Correcting style in test file")

[commit-bot: I haz the power, 2016-09-27 08:34:52]

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

[Mike West, 2016-09-27 08:43:29]

Description was changed from

==========
Adding CSP attribute on HTMon HTMLIFrameElement

BUG=647588
==========

to

==========
Adding CSP attribute on HTMLIFrameElement

As defined in
https://w3c.github.io/webappsec-csp/embedded/#dom-htmliframeelement-csp.

BUG=647588
==========

[commit-bot: I haz the power, 2016-09-27 10:23:00]

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

[commit-bot: I haz the power, 2016-09-27 10:23:01]

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

[amalika, 2016-09-27 11:03:26]

The CQ bit was checked by amalika@google.com

[amalika, 2016-09-27 11:03:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2370783002/#ps120001
(title: "Adjusting expectations")

[commit-bot: I haz the power, 2016-09-27 11:03:38]

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-27 12:38:37]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-09-27 12:42:18]

Description was changed from

==========
Adding CSP attribute on HTMLIFrameElement

As defined in
https://w3c.github.io/webappsec-csp/embedded/#dom-htmliframeelement-csp.

BUG=647588
==========

to

==========
Adding CSP attribute on HTMLIFrameElement

As defined in
https://w3c.github.io/webappsec-csp/embedded/#dom-htmliframeelement-csp.

BUG=647588

Committed: https://crrev.com/28ea58f34c71c6af9044f16e342ce395f8b2c5c1
Cr-Commit-Position: refs/heads/master@{#421182}
==========

[commit-bot: I haz the power, 2016-09-27 12:42:19]

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