Add an iframe permissions= attribute for implementing permission delegation

Add an iframe permissions= attribute for implementing permission delegation

This adds a permissions= attribute to the iframe element. The delegation
information is parsed and then plumbed down to the WebFrameOwnerProperties
which will be replicated to the browser process.

BUG=614608
Committed: https://crrev.com/76dd6b4c688ae32a7a240dcb9da09678472a9e09
Cr-Commit-Position: refs/heads/master@{#402751}

[raymes, 2016-05-26 04:46:14]

raymes@chromium.org changed reviewers:
+ esprehn@chromium.org

[raymes, 2016-05-26 04:46:15]

Hey Elliot, please take a look at this initial attempt at the blink-side code.
I'm happy to add a unittest for the new HTMLIFrameElementPermissions class if it
makes sense. The rest is basically plumbing atm.

Thanks!

+cc mkwst,palmer

[raymes, 2016-06-01 00:55:04]

Friendly ping

[esprehn, 2016-06-01 05:05:57]

This needs tests :)

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

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:163: // DOMTokenList*
to valueWasSet()?
I think you want to split it into two observers instead? DOMTokenListObserver is
garbage collected, so you could create a SandboxObserver, and a
PermissionsObserver and as long as they have a Member<HTMLIframeElement> all the
lifetimes work out.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp
(right):

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:29:
supportedValues.set("midi", WebPermissionTypeMidiSysEx);
.add(...) not .set().

Also this is large in the binary size, we inline the hash function at the .add
and .set call sites. The sandbox code is also bad, we should probably use a loop
over an array of static strings.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:38: size_t
numTokenErrors = 0;
unsigned

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:47: if
(numTokenErrors > 0)
if (numTokenErrors)

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:48:
tokenErrors.appendLiteral(", '");
appendLiteral is gone, just use append() now:

https://chromium.googlesource.com/chromium/src.git/+/41584c4ff6bc7a0556ffbbb3...

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:57: if
(numTokenErrors > 0) {
remove >

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h (right):

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:27: bool
validateTokenValue(const AtomicString&, ExceptionState&) const override;
add arg name for the AtomicString, primitives like that need argument names so
you know what they do

[raymes, 2016-06-01 05:17:27]

Thanks! I haven't addressed the comments yet but for tests I wasn't sure what
would be appropriate. Is a unittest for HTMLIFrameElementPermissions sufficient?
Does this need a layout test even though it doesn't do anything yet? Other
suggestions?

Cheers!

[esprehn, 2016-06-01 05:29:12]

On 2016/06/01 at 05:17:27, raymes wrote:
> Thanks! I haven't addressed the comments yet but for tests I wasn't sure what
would be appropriate. Is a unittest for HTMLIFrameElementPermissions sufficient?
Does this need a layout test even though it doesn't do anything yet? Other
suggestions?
> 
> Cheers!

You can start with C++ unit tests for HTMLIframeElement and
HTMLIframeElementPermissions, you want to assert the attribute changes and such.
You can write a layout tests if you want. Ideally there'd be some test that
asserts about the remote frame replication stuff too, we probably need dcheng@
or cries@ to tell us how to do that.

[raymes, 2016-06-06 06:48:29]

Thanks! Tests added.

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

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:163: // DOMTokenList*
to valueWasSet()?
On 2016/06/01 05:05:57, esprehn wrote:
> I think you want to split it into two observers instead? DOMTokenListObserver
is
> garbage collected, so you could create a SandboxObserver, and a
> PermissionsObserver and as long as they have a Member<HTMLIframeElement> all
the
> lifetimes work out.

Done.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp
(right):

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:29:
supportedValues.set("midi", WebPermissionTypeMidiSysEx);
On 2016/06/01 05:05:57, esprehn wrote:
> .add(...) not .set().
> 
> Also this is large in the binary size, we inline the hash function at the .add
> and .set call sites. The sandbox code is also bad, we should probably use a
loop
> over an array of static strings.

Done.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:38: size_t
numTokenErrors = 0;
On 2016/06/01 05:05:57, esprehn wrote:
> unsigned

Done.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:47: if
(numTokenErrors > 0)
On 2016/06/01 05:05:57, esprehn wrote:
> if (numTokenErrors)

Done.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:48:
tokenErrors.appendLiteral(", '");
On 2016/06/01 05:05:57, esprehn wrote:
> appendLiteral is gone, just use append() now:
> 
>
https://chromium.googlesource.com/chromium/src.git/+/41584c4ff6bc7a0556ffbbb3...

Done.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:57: if
(numTokenErrors > 0) {
On 2016/06/01 05:05:57, esprehn wrote:
> remove >

Done.

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h (right):

https://codereview.chromium.org/2011763006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:27: bool
validateTokenValue(const AtomicString&, ExceptionState&) const override;
On 2016/06/01 05:05:57, esprehn wrote:
> add arg name for the AtomicString, primitives like that need argument names so
> you know what they do

Done.

[raymes, 2016-06-09 04:59:01]

Ping :)

[raymes, 2016-06-13 23:31:36]

esprehn: ping

[esprehn, 2016-06-14 21:17:48]

On 2016/06/13 at 23:31:36, raymes wrote:
> esprehn: ping

Sorry been swamped, I'll look tomorrow. :)

[raymes, 2016-06-16 22:54:33]

On 2016/06/14 21:17:48, esprehn wrote:
> On 2016/06/13 at 23:31:36, raymes wrote:
> > esprehn: ping
> 
> Sorry been swamped, I'll look tomorrow. :)

ping :) Let me know if you're too busy and I can find another reviewer :)

Thanks!

[esprehn, 2016-06-17 02:22:49]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, hayato@chromium.org

[esprehn, 2016-06-17 02:22:50]

haraken@ and hayato@ can you take this?

[dominicc (has gone to gerrit), 2016-06-17 04:55:05]

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

[dominicc (has gone to gerrit), 2016-06-17 04:55:06]

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

https://codereview.chromium.org/2011763006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:204: void
HTMLIFrameElement::permissionsValueWasSet()
Um, I might have missed something but it looks like you're actually shipping
this feature when it is accessed by setAttribute.

Could you add a layout test to webexposed that checks that setting the
permissions attribute with setAttribute doesn't have any special side effects,
like printing console messages?

I hope/assume that nothing is using m_delegatedPermissions without testing that
the runtime enabled feature is actually enabled. It looks like there's an ==
operator that might consult it; what does that do?

[raymes, 2016-06-22 06:43:32]

Thanks dominicc! 

haraken or hayato: ping :)

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

https://codereview.chromium.org/2011763006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:204: void
HTMLIFrameElement::permissionsValueWasSet()
On 2016/06/17 04:55:06, dominicc wrote:
> Um, I might have missed something but it looks like you're actually shipping
> this feature when it is accessed by setAttribute.

Thanks for catching this - I had assumed that something would automatically
handle this when I added the annotation (as with JS access) but I guess not
(sorry I haven't done much in blink).

> 
> Could you add a layout test to webexposed that checks that setting the
> permissions attribute with setAttribute doesn't have any special side effects,
> like printing console messages?

Done.

> 
> I hope/assume that nothing is using m_delegatedPermissions without testing
that
> the runtime enabled feature is actually enabled. It looks like there's an ==
> operator that might consult it; what does that do?

No nothing is actually using this. The equality operator is just for checking
whether to propagate changes, but they won't be used for anything. In another CL
I will be using this stuff in the chrome side but there will be a flag check for
that. I've added in a flag check here too and a webexposed test.

[haraken, 2016-06-22 06:50:33]

(I think dominicc@/hayato@/tkent@ will be a better reviewer for this one.)

[hayato, 2016-06-22 10:53:30]

Sorry for the late reply.

The CL looks good, but I am afraid that I am not familiar with this domain
(permission, sandbox, etc..).

Adding tkent@ to a reviewer.

[hayato, 2016-06-22 10:53:45]

hayato@chromium.org changed reviewers:
+ tkent@chromium.org

[dominicc (has gone to gerrit), 2016-06-23 03:51:49]

This LGTM but you will need tkent for platform/web etc.

[tkent, 2016-06-23 04:57:28]

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:38: class
SandboxObserver : public GarbageCollected<SandboxObserver>, public
DOMTokenListObserver {
nit: You may merge this class into HTMLIFrameSandbox.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:58: class
PermissionsObserver : public GarbageCollected<PermissionsObserver>, public
DOMTokenListObserver {
Ditto.  You may merge this into HTMLIFramePermission.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:38:
USING_GARBAGE_COLLECTED_MIXIN(HTMLIFrameElement);
This line should be removed because this doesn't inherit from
DOMTokenListObserver.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:36:
[RuntimeEnabled=PermissionDelegation, PutForwards=value] readonly attribute
DOMTokenList permissions;
Please add a comment about the specification URL.

    //
https://noncombatant.github.io/permission-delegation-api/#delegation-via-the-...

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp
(right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:26: bool
GetPermissionType(const AtomicString& name, WebPermissionType* result)
With the current Blink coding style, this should be |getPermissionType()|.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementSandbox.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementSandbox.cpp:22: bool
IsTokenSupported(const AtomicString& token)
With the current Blink coding style, this should be |isTokenSupported()|.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:939:
m_webFrame->client()->didChangeFrameOwnerProperties(WebFrame::fromFrame(frameElement->contentFrame()),
WebFrameOwnerProperties(frameElement->scrollingMode(),
frameElement->marginWidth(), frameElement->marginHeight(),
frameElement->allowFullscreen(), *frameElement->delegatedPermissions()));
Can frameElement->delegatedPermissions() be nullptr?

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1511:
WebFrameOwnerProperties ownerProperties(ownerElement->scrollingMode(),
ownerElement->marginWidth(), ownerElement->marginHeight(),
ownerElement->allowFullscreen(), *ownerElement->delegatedPermissions());
Can ownerElement->delegatePermission() be nullptr?

[raymes, 2016-06-27 07:25:57]

Thanks tkent! I haven't updated the CL because I have a question below. I'll
update it after, thanks!

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:38: class
SandboxObserver : public GarbageCollected<SandboxObserver>, public
DOMTokenListObserver {
I tried this (it makes the code nicer) but I can't get it to compile. When
HTMLIFrameElementPermissions inherits from both DOMTokenList and
DOMTokenListObserver I get compile errors:
../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:71:18: note:
unimplemented pure virtual method 'adjustAndMark' in
'HTMLIFrameElementPermissions'
    virtual void adjustAndMark(Visitor*) const = 0;

I tried adding USING_GARBAGE_COLLECTED_MIXIN but then I get:

../../third_party/WebKit/Source/platform/heap/TraceTraits.h:190:5: error:
static_assert failed "T should not be traced"
    static_assert(WTF::IsTraceable<T>::value, "T should not be traced");
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:18:5:
note: in instantiation of member function
'blink::TraceTrait<blink::HTMLIFrameElementPermissions>::trace' requested here
    USING_GARBAGE_COLLECTED_MIXIN(HTMLIFrameElementPermissions);
    ^
../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:159:5: note:
expanded from macro 'USING_GARBAGE_COLLECTED_MIXIN'
    DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::Visitor*, TYPE)       \
    ^
../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:87:35: note:
expanded from macro 'DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS'
                TraceTrait<TYPE>::trace(visitor, const_cast<TYPE*>(this)); \
                                  ^
...
../../third_party/WebKit/Source/platform/heap/TraceTraits.h:193:32: error:
member 'trace' found in multiple base classes of different types
       
static_cast<T*>(self)->trace(InlinedGlobalMarkingVisitor(visitor->state()));
                               ^
../../third_party/WebKit/Source/core/dom/DOMTokenList.h:83:5: note: member found
by ambiguous name lookup
    DEFINE_INLINE_VIRTUAL_TRACE() {
    ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:112:39: note: expanded
from macro 'DEFINE_INLINE_VIRTUAL_TRACE'
#define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)


Any ideas?

[sof, 2016-06-27 07:41:08]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-06-27 07:41:09]

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:38: class
SandboxObserver : public GarbageCollected<SandboxObserver>, public
DOMTokenListObserver {
On 2016/06/27 07:25:57, raymes wrote:
> I tried this (it makes the code nicer) but I can't get it to compile. When
> HTMLIFrameElementPermissions inherits from both DOMTokenList and
> DOMTokenListObserver I get compile errors:
> ../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:71:18: note:
> unimplemented pure virtual method 'adjustAndMark' in
> 'HTMLIFrameElementPermissions'
>     virtual void adjustAndMark(Visitor*) const = 0;
> 
> I tried adding USING_GARBAGE_COLLECTED_MIXIN but then I get:
> 
> ../../third_party/WebKit/Source/platform/heap/TraceTraits.h:190:5: error:
> static_assert failed "T should not be traced"
>     static_assert(WTF::IsTraceable<T>::value, "T should not be traced");
>     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:18:5:
> note: in instantiation of member function
> 'blink::TraceTrait<blink::HTMLIFrameElementPermissions>::trace' requested here
>     USING_GARBAGE_COLLECTED_MIXIN(HTMLIFrameElementPermissions);
>     ^
> ../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:159:5: note:
> expanded from macro 'USING_GARBAGE_COLLECTED_MIXIN'
>     DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::Visitor*, TYPE)       \
>     ^
> ../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:87:35: note:
> expanded from macro 'DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS'
>                 TraceTrait<TYPE>::trace(visitor, const_cast<TYPE*>(this)); \
>                                   ^
> ...
> ../../third_party/WebKit/Source/platform/heap/TraceTraits.h:193:32: error:
> member 'trace' found in multiple base classes of different types
>        
> static_cast<T*>(self)->trace(InlinedGlobalMarkingVisitor(visitor->state()));
>                                ^
> ../../third_party/WebKit/Source/core/dom/DOMTokenList.h:83:5: note: member
found
> by ambiguous name lookup
>     DEFINE_INLINE_VIRTUAL_TRACE() {
>     ^
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:112:39: note: expanded
> from macro 'DEFINE_INLINE_VIRTUAL_TRACE'
> #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
> 
> 
> Any ideas?

Not having a delegating trace method override on HTMLIFrameElementPermissions
perhaps, but it'd be a lot easier to diagnose if we could see the actual code
you're trying to compile - could you upload it ?

[tkent, 2016-06-27 07:42:04]

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:38: class
SandboxObserver : public GarbageCollected<SandboxObserver>, public
DOMTokenListObserver {
On 2016/06/27 at 07:25:57, raymes wrote:
> Any ideas?

Please post the current HTMLIFrameElementPermissions declaration with
USING_GARBAGE_COLLECTED_MIXIN.
Did you add *_VIRTUAL_TRACE() to it?

[raymes, 2016-06-27 08:09:50]

Thanks! I managed to get it working with those tips, please take a look :)

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:38:
USING_GARBAGE_COLLECTED_MIXIN(HTMLIFrameElement);
On 2016/06/23 04:57:27, tkent wrote:
> This line should be removed because this doesn't inherit from
> DOMTokenListObserver.

Done.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:36:
[RuntimeEnabled=PermissionDelegation, PutForwards=value] readonly attribute
DOMTokenList permissions;
On 2016/06/23 04:57:27, tkent wrote:
> Please add a comment about the specification URL.
> 
>     //
>
https://noncombatant.github.io/permission-delegation-api/#delegation-via-the-...

Done.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp
(right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.cpp:26: bool
GetPermissionType(const AtomicString& name, WebPermissionType* result)
On 2016/06/23 04:57:28, tkent wrote:
> With the current Blink coding style, this should be |getPermissionType()|.

Done.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementSandbox.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementSandbox.cpp:22: bool
IsTokenSupported(const AtomicString& token)
On 2016/06/23 04:57:28, tkent wrote:
> With the current Blink coding style, this should be |isTokenSupported()|.

Done.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:939:
m_webFrame->client()->didChangeFrameOwnerProperties(WebFrame::fromFrame(frameElement->contentFrame()),
WebFrameOwnerProperties(frameElement->scrollingMode(),
frameElement->marginWidth(), frameElement->marginHeight(),
frameElement->allowFullscreen(), *frameElement->delegatedPermissions()));
I think so now that I look at it closely. I've added a check here.

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2011763006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1511:
WebFrameOwnerProperties ownerProperties(ownerElement->scrollingMode(),
ownerElement->marginWidth(), ownerElement->marginHeight(),
ownerElement->allowFullscreen(), *ownerElement->delegatedPermissions());
Same here.

[tkent, 2016-06-27 08:16:08]

lgtm

[sof, 2016-06-27 08:38:52]

https://codereview.chromium.org/2011763006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2011763006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:42: ,
m_permissions(HTMLIFrameElementPermissions::create(this))
As this isn't commonly accessed (or enabled by default), could we lazily create
|m_permissions| ?

[raymes, 2016-06-28 01:19:04]

https://codereview.chromium.org/2011763006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2011763006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:42: ,
m_permissions(HTMLIFrameElementPermissions::create(this))
On 2016/06/27 08:38:52, sof wrote:
> As this isn't commonly accessed (or enabled by default), could we lazily
create
> |m_permissions| ?

Done.

[raymes, 2016-06-28 01:19:14]

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

[commit-bot: I haz the power, 2016-06-28 01:19:54]

Dry run: 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-06-28 02:25:56]

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

[commit-bot: I haz the power, 2016-06-28 02:25:57]

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

[sof, 2016-06-28 05:29:00]

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:69: const
WebVector<WebPermissionType>* delegatedPermissions() const override { return
&m_delegatedPermissions; }
What is the assumed lifetime of this value? i.e., you're returning a
non-traceable pointer into a Blink heap-allocated object (which might be garbage
collected.)

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:10: #include
"platform/heap/GarbageCollected.h"
Instead of these two, just include "platform/heap/Handle.h"

[raymes, 2016-06-28 06:49:46]

Thanks!

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:69: const
WebVector<WebPermissionType>* delegatedPermissions() const override { return
&m_delegatedPermissions; }
On 2016/06/28 05:29:00, sof wrote:
> What is the assumed lifetime of this value? i.e., you're returning a
> non-traceable pointer into a Blink heap-allocated object (which might be
garbage
> collected.)

The pointer is just used to reduce copied but the value is used synchronously
and will get copied into the frame owner properties struct.

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:10: #include
"platform/heap/GarbageCollected.h"
On 2016/06/28 05:29:00, sof wrote:
> Instead of these two, just include "platform/heap/Handle.h"

Done.

Is there documentation about this somewhere? Generally I would apply the
"include what you use" rule.

[sof, 2016-06-28 06:57:48]

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:69: const
WebVector<WebPermissionType>* delegatedPermissions() const override { return
&m_delegatedPermissions; }
On 2016/06/28 06:49:46, raymes wrote:
> On 2016/06/28 05:29:00, sof wrote:
> > What is the assumed lifetime of this value? i.e., you're returning a
> > non-traceable pointer into a Blink heap-allocated object (which might be
> garbage
> > collected.)
> 
> The pointer is just used to reduce copied but the value is used synchronously
> and will get copied into the frame owner properties struct.

ok, it seems unnecessary to resort to returning a pointer here if the call site
substitutes |nullptr| with an empty WebVector.

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementPermissions.h:10: #include
"platform/heap/GarbageCollected.h"
On 2016/06/28 06:49:46, raymes wrote:
> On 2016/06/28 05:29:00, sof wrote:
> > Instead of these two, just include "platform/heap/Handle.h"
> 
> Done.
> 
> Is there documentation about this somewhere? Generally I would apply the
> "include what you use" rule.

platform/heap/BlinkGCAPIReference.md is where to look for Blink GC details.

[raymes, 2016-06-29 05:54:47]

Thanks

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:69: const
WebVector<WebPermissionType>* delegatedPermissions() const override { return
&m_delegatedPermissions; }
On 2016/06/28 06:57:48, sof wrote:
> On 2016/06/28 06:49:46, raymes wrote:
> > On 2016/06/28 05:29:00, sof wrote:
> > > What is the assumed lifetime of this value? i.e., you're returning a
> > > non-traceable pointer into a Blink heap-allocated object (which might be
> > garbage
> > > collected.)
> > 
> > The pointer is just used to reduce copied but the value is used
synchronously
> > and will get copied into the frame owner properties struct.
> 
> ok, it seems unnecessary to resort to returning a pointer here if the call
site
> substitutes |nullptr| with an empty WebVector.

I was trying to reduce copies :) I've returned a const reference instead, which
requires using a static local in the places where null is currently returned.
Otherwise I can return by copy if you feel strongly.

[raymes, 2016-06-29 05:57:34]

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

[commit-bot: I haz the power, 2016-06-29 05:57:47]

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

[sof, 2016-06-29 06:10:28]

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/2011763006/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.h:69: const
WebVector<WebPermissionType>* delegatedPermissions() const override { return
&m_delegatedPermissions; }
On 2016/06/29 05:54:47, raymes wrote:
> On 2016/06/28 06:57:48, sof wrote:
> > On 2016/06/28 06:49:46, raymes wrote:
> > > On 2016/06/28 05:29:00, sof wrote:
> > > > What is the assumed lifetime of this value? i.e., you're returning a
> > > > non-traceable pointer into a Blink heap-allocated object (which might be
> > > garbage
> > > > collected.)
> > > 
> > > The pointer is just used to reduce copied but the value is used
> synchronously
> > > and will get copied into the frame owner properties struct.
> > 
> > ok, it seems unnecessary to resort to returning a pointer here if the call
> site
> > substitutes |nullptr| with an empty WebVector.
> 
> I was trying to reduce copies :) I've returned a const reference instead,
which
> requires using a static local in the places where null is currently returned.
> Otherwise I can return by copy if you feel strongly.

When & where to optimize is a perennial question, but I think this is fine.

[raymes, 2016-06-29 06:22:10]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2016-06-29 06:22:21]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-06-29 06:22:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2011763006/#ps260001
(title: "Blink-side")

[commit-bot: I haz the power, 2016-06-29 06:22:45]

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-06-29 07:40:21]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-29 07:42:11]

Description was changed from

==========
Add an iframe permissions= attribute for implementing permission delegation

This adds a permissions= attribute to the iframe element. The delegation
information is parsed and then plumbed down to the WebFrameOwnerProperties
which will be replicated to the browser process.

BUG=614608
==========

to

==========
Add an iframe permissions= attribute for implementing permission delegation

This adds a permissions= attribute to the iframe element. The delegation
information is parsed and then plumbed down to the WebFrameOwnerProperties
which will be replicated to the browser process.

BUG=614608

Committed: https://crrev.com/76dd6b4c688ae32a7a240dcb9da09678472a9e09
Cr-Commit-Position: refs/heads/master@{#402751}
==========

[commit-bot: I haz the power, 2016-06-29 07:42:17]

Patchset 14 (id:??) landed as
https://crrev.com/76dd6b4c688ae32a7a240dcb9da09678472a9e09
Cr-Commit-Position: refs/heads/master@{#402751}