Bug 64251 – [ShadowContentElement] forwarded node should be able to access its hosting content element.

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

To provide flattened-tree traversal, we need to traverse from forwarded nodes to hosting content element.
But we currently maintain only references from content element to forwarded nodes.

[Hajime Morrita]

Created attachment 100666 [details]
Patch

[Hajime Morrita]

Hi Dimitri, could you take a look?
This patch introduces the included node to content element hash.

--
I feel relationship between ShadowContentElement, ShadowInclusion, ShadowContentSelector
becomes complicated, so I'll do some reorg after this patch.
My plan is to
- move ShadowContentSelector to a member variable of ShadowRoot,
- move ShadowContentSet to ShadowContentSelector
Then we can hide content-forwarding mechanism behind ShadowContentSelector.

[WebKit Review Bot]

Comment on attachment 100666 [details]
Patch

Attachment 100666 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9016532

New failing tests:
fast/html/details-nested-2.html

[WebKit Review Bot]

Created attachment 100668 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dimitri Glazkov (Google)]

Comment on attachment 100666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100666&action=review

I am ok with the approach as an iteration, but the bots are angry.

> Source/WebCore/dom/ShadowRoot.cpp:33
> +#include "ShadowContentElement.h" // XXX: should be ShadowInclusion.h or something

Probably don't want that comment left in :)

[Hajime Morrita]

Created attachment 100750 [details]
Remove wrong comment, fixed build and test breakage

[Hajime Morrita]

Hi Dimitri, thanks for reviewing this!

> > Source/WebCore/dom/ShadowRoot.cpp:33
> > +#include "ShadowContentElement.h" // XXX: should be ShadowInclusion.h or something
> 
> Probably don't want that comment left in :)
Oops... thanks for the catch. Removed. I hope triple-x makes style bot uncomfortable.

[Dimitri Glazkov (Google)]

Comment on attachment 100750 [details]
Remove wrong comment, fixed build and test breakage

View in context: https://bugs.webkit.org/attachment.cgi?id=100750&action=review

> Source/WebKit2/win/WebKit2.def:140
> +	??0NodeRenderingContext@WebCore@@QAE@PAVNode@1@@Z
> +	??1NodeRenderingContext@WebCore@@QAE@XZ
> +	?toNode@WebCore@@YAPAVNode@1@VJSValue@JSC@@@Z

Is this indent correct?

[Hajime Morrita]

(In reply to comment #8)
> (From update of attachment 100750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100750&action=review
> 
> > Source/WebKit2/win/WebKit2.def:140
> > +	??0NodeRenderingContext@WebCore@@QAE@PAVNode@1@@Z
> > +	??1NodeRenderingContext@WebCore@@QAE@XZ
> > +	?toNode@WebCore@@YAPAVNode@1@VJSValue@JSC@@@Z
> 
> Is this indent correct?
Ouch, I'll untabify them before landing.
Thanks for take a look again, Dimitri!

[Hajime Morrita]

Created attachment 100758 [details]
For CQ

[WebKit Review Bot]

Comment on attachment 100758 [details]
For CQ

Clearing flags on attachment: 100758

Committed r90976: <http://trac.webkit.org/changeset/90976>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

Committed r90987: <http://trac.webkit.org/changeset/90987>

[Hajime Morrita]

Rolled out at http://trac.webkit.org/changeset/90987
Due to gtk build error
http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/15574/steps/compile-webkit/logs/stdio

[Hajime Morrita]

Created attachment 100944 [details]
For asking EWS

[Philippe Normand]

I finally had time to look at this. Adding _ZN7WebCore6toNodeEN3JSC7JSValueE; in Source/autotools/symbols.filter should fix this build issue.

So, please re-land the patch with that little update^^ :)

No clean build should be needed.
Thanks to mrobinson for the help on this.

[Philippe Normand]

(In reply to comment #16)
> I finally had time to look at this. Adding _ZN7WebCore6toNodeEN3JSC7JSValueE; in Source/autotools/symbols.filter should fix this build issue.
> 
> So, please re-land the patch with that little update^^ :)
> 
> No clean build should be needed.
> Thanks to mrobinson for the help on this.

I didn't see comment 15. So yeah, this new patch should build indeed on GTK.

[Hajime Morrita]

> > No clean build should be needed.
> > Thanks to mrobinson for the help on this.
> 
> I didn't see comment 15. So yeah, this new patch should build indeed on GTK.

Hi Philippe,
Thanks so much for your investigation!
That's really helpful because I don't have a way to do GTK build.
I'll retry this patch now.

[WebKit Review Bot]

Comment on attachment 100944 [details]
For asking EWS

Clearing flags on attachment: 100944

Committed r91235: <http://trac.webkit.org/changeset/91235>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.