Bug 64251 - [ShadowContentElement] forwarded node should be able to access its hosting content element.
Summary: [ShadowContentElement] forwarded node should be able to access its hosting co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 64252 64517
Blocks: 64072
  Show dependency treegraph
 
Reported: 2011-07-10 23:30 PDT by Hajime Morrita
Modified: 2011-07-18 19:55 PDT (History)
3 users (show)

See Also:


Attachments
Patch (26.54 KB, patch)
2011-07-13 06:00 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.74 MB, application/zip)
2011-07-13 06:23 PDT, WebKit Review Bot
no flags Details
Remove wrong comment, fixed build and test breakage (31.71 KB, patch)
2011-07-13 18:34 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
For CQ (31.74 KB, patch)
2011-07-13 19:44 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
For asking EWS (33.36 KB, patch)
2011-07-15 00:51 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-07-10 23:30:21 PDT
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.
Comment 1 Hajime Morrita 2011-07-13 06:00:58 PDT
Created attachment 100666 [details]
Patch
Comment 2 Hajime Morrita 2011-07-13 06:04:49 PDT
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.
Comment 3 WebKit Review Bot 2011-07-13 06:22:56 PDT
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
Comment 4 WebKit Review Bot 2011-07-13 06:23:02 PDT
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
Comment 5 Dimitri Glazkov (Google) 2011-07-13 08:25:42 PDT
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 :)
Comment 6 Hajime Morrita 2011-07-13 18:34:31 PDT
Created attachment 100750 [details]
Remove wrong comment, fixed build and test breakage
Comment 7 Hajime Morrita 2011-07-13 18:40:56 PDT
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.
Comment 8 Dimitri Glazkov (Google) 2011-07-13 19:17:39 PDT
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?
Comment 9 Hajime Morrita 2011-07-13 19:29:49 PDT
(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!
Comment 10 Hajime Morrita 2011-07-13 19:44:05 PDT
Created attachment 100758 [details]
For CQ
Comment 11 WebKit Review Bot 2011-07-13 20:39:46 PDT
Comment on attachment 100758 [details]
For CQ

Clearing flags on attachment: 100758

Committed r90976: <http://trac.webkit.org/changeset/90976>
Comment 12 WebKit Review Bot 2011-07-13 20:39:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Hajime Morrita 2011-07-14 02:00:46 PDT
Committed r90987: <http://trac.webkit.org/changeset/90987>
Comment 15 Hajime Morrita 2011-07-15 00:51:38 PDT
Created attachment 100944 [details]
For asking EWS
Comment 16 Philippe Normand 2011-07-15 08:54:38 PDT
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.
Comment 17 Philippe Normand 2011-07-15 08:57:01 PDT
(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.
Comment 18 Hajime Morrita 2011-07-18 19:29:57 PDT
> > 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.
Comment 19 WebKit Review Bot 2011-07-18 19:55:37 PDT
Comment on attachment 100944 [details]
For asking EWS

Clearing flags on attachment: 100944

Committed r91235: <http://trac.webkit.org/changeset/91235>
Comment 20 WebKit Review Bot 2011-07-18 19:55:53 PDT
All reviewed patches have been landed.  Closing bug.