Bug 99232

I think the corresponding bug in the spec is here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 It seems we should resolve the bug in the spec at first.

(In reply to comment #1) > I think the corresponding bug in the spec is here: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 > > It seems we should resolve the bug in the spec at first. Sure, but just like with the rest of reprojection, the spec bug doesn't have to block this bug. Besides, you already have this accessor on HTMLContentElement.

Okay. Let's expose the attribute. That won't break anything. (In reply to comment #2) > (In reply to comment #1) > > I think the corresponding bug in the spec is here: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 > > > > It seems we should resolve the bug in the spec at first. > > Sure, but just like with the rest of reprojection, the spec bug doesn't have to block this bug. Besides, you already have this accessor on HTMLContentElement.

(In reply to comment #3) > Okay. Let's expose the attribute. That won't break anything. > > (In reply to comment #2) > > (In reply to comment #1) > > > I think the corresponding bug in the spec is here: > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 > > > > > > It seems we should resolve the bug in the spec at first. > > > > Sure, but just like with the rest of reprojection, the spec bug doesn't have to block this bug. Besides, you already have this accessor on HTMLContentElement. Great! Also -- the item in this nodelist could certainly be an node, so it should be called distributedNodes.

It looks that the spec is going to make it a dynamic list, but this one isn't. so it will change the behavior once it's written out to the standard. Is it in your mind?

(In reply to comment #5) > It looks that the spec is going to make it a dynamic list, but this one isn't. > so it will change the behavior once it's written out to the standard. > Is it in your mind? I am not going to make this a dynamic list. Dynamic lists are evil.

The current implementation assumes that an element is attached when distributedNode() is called. So it won't work if it's called like: var sr = new WebKitShadowRoot(host); var content = document.createElement('content'); sr.appendChild(content); content.distributedNodes This will always return null. It's ok if we use setTimeout like the following: var sr = new WebKitShadowRoot(host); var content = document.createElement('content'); sr.appendChild(content); setTimeout(function() content.distributedNodes }, 0) Maybe we have to call ensureDistribution() when distributedNodes is called. We also have to ensure distribution for younger shadows and so on.

> > I am not going to make this a dynamic list. Dynamic lists are evil. Hmm. Added comment on W3C bug.

Created attachment 169132 [details] Add a distributedNodes().

Comment on attachment 169132 [details] Add a distributedNodes(). View in context: https://bugs.webkit.org/attachment.cgi?id=169132&action=review > Source/WebCore/html/shadow/InsertionPoint.cpp:94 > + root->owner()->ensureDistribution(); Could you add a test case for nested shadow dom? I'm afraid that this causes an error (or returns nonsense result) If distributedNodes() is called before attaching parent shadow dom.

Okay. It seems we should call ensureDistribution *recursively*. Let me update the patch. (In reply to comment #10) > (From update of attachment 169132 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169132&action=review > > > Source/WebCore/html/shadow/InsertionPoint.cpp:94 > > + root->owner()->ensureDistribution(); > > Could you add a test case for nested shadow dom? > I'm afraid that this causes an error (or returns nonsense result) If distributedNodes() is called before attaching parent shadow dom.

Created attachment 169159 [details] Call updateLayout() explicitly.

Comment on attachment 169159 [details] Call updateLayout() explicitly. View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review > Source/WebCore/html/shadow/HTMLContentElement.idl:33 > + NodeList distributedNodes(); I don't understand why this is a method? It should be an attribute, just like Node.childNodes or MutationRecord.addedNodes/removedNodes. We may want to optimize not to re-create the array on each access though.

Another thing -- probably should include removing Internals::distributedNodes in this patch?

Comment on attachment 169159 [details] Call updateLayout() explicitly. View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review >> Source/WebCore/html/shadow/HTMLContentElement.idl:33 >> + NodeList distributedNodes(); > > I don't understand why this is a method? It should be an attribute, just like Node.childNodes or MutationRecord.addedNodes/removedNodes. We may want to optimize not to re-create the array on each access though. Is this a static NodeList? If so, we should make it so. Also, once it's static, I think we want it to be a method since each call will return a different list.

(In reply to comment #15) > (From update of attachment 169159 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review > > >> Source/WebCore/html/shadow/HTMLContentElement.idl:33 > >> + NodeList distributedNodes(); > Is this a static NodeList? If so, we should make it so. Also, once it's static, I think we want it to be a method since each call will return a different list. I spoke with Ojan, and he convinced me it needs to be a method. Can we name it getDistributedNodes to make it action-oriented?

(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 169159 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review > > > > >> Source/WebCore/html/shadow/HTMLContentElement.idl:33 > > >> + NodeList distributedNodes(); > > > Is this a static NodeList? If so, we should make it so. Also, once it's static, I think we want it to be a method since each call will return a different list. > > I spoke with Ojan, and he convinced me it needs to be a method. Can we name it getDistributedNodes to make it action-oriented? For the record we are having a related discussion regarding ES6 APIs. One thing that we managed to agree on is that property getters must be idempotent. node.distributedNodes === node.distributedNodes so if it returns different static NodeLists every time it MUST not be a property (WebIDL attribute).

Created attachment 169325 [details] Renamed to getDistributedNodes(). Remove distributedNodes() from Internals.

Comment on attachment 169325 [details] Renamed to getDistributedNodes(). Remove distributedNodes() from Internals. yay!

Comment on attachment 169325 [details] Renamed to getDistributedNodes(). Remove distributedNodes() from Internals. Attachment 169325 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14396829

Created attachment 169326 [details] Patch for landing

Comment on attachment 169326 [details] Patch for landing Clearing flags on attachment: 169326 Committed r131701: <http://trac.webkit.org/changeset/131701>

All reviewed patches have been landed. Closing bug.

It looks like this has broken the Windows build: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/

(In reply to comment #25) > It looks like this has broken the Windows build: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/ Oops, sorry about that! It's looking for a symbol that was removed, and it looks like the internals lib was not recompiled. Smells like VS build didn't understand this change in dependency. Can we clobber?

Clean build is failing locally on my machine. Bots still haven't finished it seems. I got rid of he distributedNodes error by getting rid of the export line in WebKit2.def. Not sure why the OpaqueJSString errors are there though. THey don'y seem related?

(In reply to comment #27) > Clean build is failing locally on my machine. Bots still haven't finished it seems. > > I got rid of he distributedNodes error by getting rid of the export line in WebKit2.def. That was it! I am so sorry. > > Not sure why the OpaqueJSString errors are there though. THey don'y seem related? No, those aren't related.

No worries! Yeah, the other two failures look like they're caused by a different patch that landed this morning so we're all clear here :) Thanks

Ops, sorry about that. commit-queue could not catch such an error. Lesson: I have to update a list of exported symbols when we remove an entry from Internals.idl. (In reply to comment #25) > It looks like this has broken the Windows build: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/

*** Bug 104229 has been marked as a duplicate of this bug. ***