Bug 78313 - Element::removeShadowRoot(), setShadowRoot() should be moved into ShadowTree
Summary: Element::removeShadowRoot(), setShadowRoot() should be moved into ShadowTree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on: 78069 78455
Blocks: 77931 79197
  Show dependency treegraph
 
Reported: 2012-02-09 18:58 PST by Shinya Kawanaka
Modified: 2012-02-28 00:06 PST (History)
5 users (show)

See Also:


Attachments
Patch (15.81 KB, patch)
2012-02-27 00:17 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (16.03 KB, patch)
2012-02-27 01:30 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch for landing (16.20 KB, patch)
2012-02-27 22:21 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch for landing (16.43 KB, patch)
2012-02-27 23:05 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-09 18:58:34 PST
Patch for Bug 78069 will emulate shadowRoot(), removeShadowRoot(), and setShadowRoot() using shadowRootList().
These API should be removed and replaced using ShadowRootList API.
Comment 1 Shinya Kawanaka 2012-02-27 00:17:42 PST
Created attachment 128976 [details]
Patch
Comment 2 Hajime Morrita 2012-02-27 00:42:00 PST
Comment on attachment 128976 [details]
Patch

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

Basically looks good. Could you polish a bit?

> Source/WebCore/ChangeLog:9
> +

Could you mention now ShadowTree is on its own heap?

> Source/WebCore/dom/Element.cpp:125
> +    if (hasShadowRoot())

Can be hasShadowTree()?

> Source/WebCore/dom/Element.cpp:126
> +        shadowTree()->removeAllShadowRoots();

It looks we can do removeAllShadowRoots() in the ShadowTree dtor then we can just delete the ShadowTree instance here.
Comment 3 Shinya Kawanaka 2012-02-27 01:25:08 PST
(In reply to comment #2)
> (From update of attachment 128976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128976&action=review
> 
> Basically looks good. Could you polish a bit?
> 
> > Source/WebCore/ChangeLog:9
> > +
> 
> Could you mention now ShadowTree is on its own heap?
> 
> > Source/WebCore/dom/Element.cpp:125
> > +    if (hasShadowRoot())
> 
> Can be hasShadowTree()?
> 
> > Source/WebCore/dom/Element.cpp:126
> > +        shadowTree()->removeAllShadowRoots();
> 
> It looks we can do removeAllShadowRoots() in the ShadowTree dtor then we can just delete the ShadowTree instance here.

Unfortunately removeAllShadowRoots() now requires the element. So what I can do here is just to call rareData()->m_shadowTree.clear().

Currently our code assumes that a shadow root could be removed from an element now, and we have tests for it. However, since we should not have such API, we can remove that assumption. This will reduce the code and make it clear. I think it should be done in another bug though.
Comment 4 Shinya Kawanaka 2012-02-27 01:30:37 PST
Created attachment 128982 [details]
Patch
Comment 5 Hajime Morrita 2012-02-27 01:56:50 PST
Comment on attachment 128982 [details]
Patch

>Unfortunately removeAllShadowRoots() now requires the element. So what I can do here is just to call rareData()->m_shadowTree.clear().
>
>Currently our code assumes that a shadow root could be removed from an element now, and we have tests for it. However, since we should not have such API, we can remove that assumption. This will reduce the code and make it clear. I think it should be done in another bug though.

This looks reasonable. When we need the host element, there should be at least one shadow root.
If there is no shadow root. We won't need the reference the the host.
Comment 6 Shinya Kawanaka 2012-02-27 22:21:25 PST
Created attachment 129189 [details]
Patch for landing
Comment 7 Early Warning System Bot 2012-02-27 23:05:24 PST
Comment on attachment 129189 [details]
Patch for landing

Attachment 129189 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11644419
Comment 8 Shinya Kawanaka 2012-02-27 23:05:32 PST
Created attachment 129192 [details]
Patch for landing
Comment 9 Shinya Kawanaka 2012-02-28 00:06:16 PST
Committed r109084: <http://trac.webkit.org/changeset/109084>