Bug 65595

Created attachment 102749 [details] Patch

Comment on attachment 102749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102749&action=review Another idea is to maintain a separate list for shadow frame. > Source/WebCore/page/FrameTree.cpp:26 > +#include "HTMLFrameOwnerElement.h" Is this required? > Source/WebCore/page/FrameTree.cpp:70 > + while (frame->isInShadowTree()) is fram always non-null? > Source/WebCore/page/FrameTree.cpp:346 > + if (frame) Do we need this check? > Source/WebCore/page/FrameTree.h:55 > + // The number of child frames excluding shadow frames. This is obvious from the code.

AFAICT, with this patch one cannot use firstChild(), nextChild(), previousSibling and nextSibling() within shadow trees at all. Rather than skipping frames where isInShadowTree() is true, it might be better to skip frames where the treeScope is different (?). Also, in case the HTML of an <iframe> within a shadow tree includes itself an <iframe>, isInShadowTree() for the nested <iframe> would be false, and the frame thus visible. However, since the nested <iframe> got included by a shadow tree, this seems to run counter to the intent of the patch (?). In the long run I wonder if it wasn't possible to have frame trees at the TreeScope level instead.

Comment on attachment 102749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102749&action=review Thank you for reviewing. > Another idea is to maintain a separate list for shadow frame. It hink the current idea is better to maintain. We might want to change the behavior in the future so that loading for shadow iframes updates the browser history. >> Source/WebCore/page/FrameTree.cpp:26 > > Is this required? It's not required. Removed. >> Source/WebCore/page/FrameTree.cpp:70 >> + while (frame->isInShadowTree()) > > is fram always non-null? frame can be null. But it's safe to call 0->isInShadowTree(). >> Source/WebCore/page/FrameTree.cpp:346 >> + if (frame) > > Do we need this check? It's not needed. frame is always non-0 in this case. Fixed. >> Source/WebCore/page/FrameTree.h:55 >> + // The number of child frames excluding shadow frames. > > This is obvious from the code. Removed.

Comment on attachment 102749 [details] Patch Could you add test for dynamic frame tree modification like - from main document to shadow tree (and vice versa) - from a shadow tree to another shadow tree ?

(In reply to comment #3) > AFAICT, with this patch one cannot use firstChild(), nextChild(), previousSibling and nextSibling() within shadow trees at all. Rather than skipping frames where isInShadowTree() is true, it might be better to skip frames where the treeScope is different (?). It's reasonable. I need to investigate what we should do for Frame::ownerElement() == 0. > Also, in case the HTML of an <iframe> within a shadow tree includes itself an <iframe>, isInShadowTree() for the nested <iframe> would be false, and the frame thus visible. However, since the nested <iframe> got included by a shadow tree, this seems to run counter to the intent of the patch (?). In this case, a deeper <iframe> might update the browser history. I'll make a test and check the behavior.

(In reply to comment #5) > (From update of attachment 102749 [details]) > Could you add test for dynamic frame tree modification like > - from main document to shadow tree (and vice versa) > - from a shadow tree to another shadow tree > ? Do they make sense for the component model? If such cases can be made only by window.internals, I don't think we should take care of them.

What's the use case for having an iframe in a shadow tree? Can we just disallow them instead?

(In reply to comment #8) > What's the use case for having an iframe in a shadow tree? Can we just disallow them instead? iframes in shadow tree have been discussed as one way to achieve strong isolation between the document and (parts of) a component.

(In reply to comment #8) > What's the use case for having an iframe in a shadow tree? Can we just disallow them instead? A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree. I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context.

(In reply to comment #10) > (In reply to comment #8) > > What's the use case for having an iframe in a shadow tree? Can we just disallow them instead? > > A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree. > I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context. That sounds like a massive hack. I know you really want that calendar to land, but shouldn't we try to solve this problem properly?

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #8) > > > What's the use case for having an iframe in a shadow tree? Can we just disallow them instead? > > > > A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree. > > I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context. > > That sounds like a massive hack. I know you really want that calendar to land, but shouldn't we try to solve this problem properly? This might be a great opportunity to think about isolation: If you want to create components in non intrusive way, what do we need? This sounds worth to investigate.

I thought that the whole purpose of shadow trees was to provide isolation. If we need iframes for that, we could just as well remove shadow tree support and use iframes everywhere, couldn't we? :)

(In reply to comment #11) > > A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree. > > I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context. > > That sounds like a massive hack. I know you really want that calendar to land, but shouldn't we try to solve this problem properly? Yes, I really want to make the progress of the calendar picker. I don't think implementing "the proper way" will be done in a few months, and the iframe-in-shadow might be needed anyway.

Created attachment 103051 [details] Patch 2

Comment on attachment 103051 [details] Patch 2 Attachment 103051 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9321002

Created attachment 103053 [details] Patch 3 Include Document.h for Chromium

Comment on attachment 103053 [details] Patch 3 Attachment 103053 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9306789 New failing tests: fast/frames/layout-after-destruction.html

Created attachment 103059 [details] Patch 4 Fix a re-parenting bug

I still don't understand what the plan is here. Having iframes in shadow trees is a long term liability, and in the end, they should probably not be supported at all.

(In reply to comment #20) > I still don't understand what the plan is here. Having iframes in shadow trees is a long term liability, and in the end, they should probably not be supported at all. Having iframes in shadow DOM is not a problem in general. None of the component model proposals currently in existence place any limitations like this. However, using iframes in the context of 53961 definitely seems like a hack. If what you're looking for is a global JS context (which I don't think you do -- you could just use closure), then we should do this explicitly for JS running in a component. If you're trying to avoid stylesheet pollution, then use scoped styles. Does this make sense?

(In reply to comment #0) > <iframe> in a shadow tree have some problems for now. > > - hide shadow frames from windows.frames > - Loading in a shadow frame shouldn't update the history. Where do these rules come from?

(In reply to comment #21) > However, using iframes in the context of 53961 definitely seems like a hack. If what you're looking for is a global JS context (which I don't think you do -- you could just use closure), then we should do this explicitly for JS running in a component. If you're trying to avoid stylesheet pollution, then use scoped styles. I confirmed <script> in a shadow tree worked. Do you have any idea how to access the shadow tree from the script in the shadow tree? We can't use document.getElementById(), etc.

(In reply to comment #22) > > - hide shadow frames from windows.frames > > - Loading in a shadow frame shouldn't update the history. > > Where do these rules come from? They are my opinions. Others might object to them.

(In reply to comment #22) > (In reply to comment #0) > > <iframe> in a shadow tree have some problems for now. > > > > - hide shadow frames from windows.frames > > - Loading in a shadow frame shouldn't update the history. > > Where do these rules come from? In my understanding these are a natural consequence of the isolation discussion of components: if a component is supposed to be isolated from the page by default, then the page should have no way of accessing those iframes unless the component opens them up. Conversely, the component should not "pollute" the page's history by virtue of it utilizing iframes.

Comment on attachment 103059 [details] Patch 4 I'm trying another way to implement the calendar picker.

Now that ShadowRoot is exposed to script, ClusterFuzz demonstrated a crash adding an IFRAME to a ShadowRoot <http://code.google.com/p/chromium/issues/detail?id=113167>.

Created attachment 128159 [details] Patch

This change just hides iframes from accessor on document. History stuff is out of scope. I don't think we need to hide navigation in shadowed iframes because we don't do that for nested iframes. The concept of shadow DOM is further clarified since the last patch was written. <iframe> in shadow is no longer a big deal.

Comment on attachment 128159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128159&action=review > Source/WebCore/ChangeLog:8 > + This change introduced FrameTree::documentChild() and "introduces" > Source/WebCore/ChangeLog:10 > + frame lookup. Using these, now the named accessor and the indexed remove "now" here ... > Source/WebCore/ChangeLog:11 > + acceccor on Document, and Window.length is TreeScope aware. They and replace "is" with "are now". > Source/WebCore/page/Frame.cpp:1128 > +bool Frame::isScopedBy(TreeScope* scope) const isScopedby -> inScope > Source/WebCore/page/FrameTree.h:57 > Frame* previousSibling() const { return m_previousSibling; } > Frame* firstChild() const { return m_firstChild.get(); } > Frame* lastChild() const { return m_lastChild; } Are these guys still being used? > Source/WebCore/page/FrameTree.h:78 > + Frame* documentChild(unsigned index) const; I think the name is a bit unintuitive. It requires you to know that Document is a TreeScope. Also, the purpose of scoped* functions is limited to just supplying results for document* functions. Do we need the complete set of the two? Can we collapse them into one set, called "scopedChild"?

Created attachment 128328 [details] Patch

Comment on attachment 128159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128159&action=review Hi Dimitri, thanks for reviewing this! I addressed your point. Could you have another round? >> Source/WebCore/ChangeLog:8 >> + This change introduced FrameTree::documentChild() and > > "introduces" fixed. >> Source/WebCore/ChangeLog:10 >> + frame lookup. Using these, now the named accessor and the indexed > > remove "now" here ... removed... >> Source/WebCore/ChangeLog:11 >> + acceccor on Document, and Window.length is TreeScope aware. They > > and replace "is" with "are now". ... and fixed. >> Source/WebCore/page/Frame.cpp:1128 >> +bool Frame::isScopedBy(TreeScope* scope) const > > isScopedby -> inScope fixed. >> Source/WebCore/page/FrameTree.h:57 >> Frame* lastChild() const { return m_lastChild; } > > Are these guys still being used? Yes. People iterate over frame children for example in the history module. >> Source/WebCore/page/FrameTree.h:78 >> + Frame* documentChild(unsigned index) const; > > I think the name is a bit unintuitive. It requires you to know that Document is a TreeScope. > > Also, the purpose of scoped* functions is limited to just supplying results for document* functions. Do we need the complete set of the two? Can we collapse them into one set, called "scopedChild"? Sounds good. done.

Comment on attachment 128328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128328&action=review > Source/WebCore/ChangeLog:9 > + This change introduces FrameTree::documentChild() and > + FrameTree::documentChild(), which can be used for scope-aware Whoops! Still mentions documentChild!

Created attachment 128344 [details] Patch

(In reply to comment #33) > > Whoops! Still mentions documentChild! Whoaaaa. My text editor searched only under page/ and binding/ :-/

> > Whoops! Still mentions documentChild! > Whoaaaa. My text editor searched only under page/ and binding/ :-/ And bots are red... I need to fix them.

Created attachment 128347 [details] Patch

Comment on attachment 128347 [details] Patch r=me, make sure bots are green before landing.

Created attachment 128377 [details] Patch for landing

Comment on attachment 128377 [details] Patch for landing Waiting EWS greeness.

Comment on attachment 128377 [details] Patch for landing Clearing flags on attachment: 128377 Committed r108700: <http://trac.webkit.org/changeset/108700>

All reviewed patches have been landed. Closing bug.

The test added by this patch is failing on Mac port: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r109929%20(37823)/fast/dom/shadow/iframe-shadow-pretty-diff.html There are a bunch of other tests that have been failing for the same missing WebKitShadowRoot error :( Could you guys skip all of those on Mac port?