WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
78313
Element::removeShadowRoot(), setShadowRoot() should be moved into ShadowTree
https://bugs.webkit.org/show_bug.cgi?id=78313
Summary
Element::removeShadowRoot(), setShadowRoot() should be moved into ShadowTree
Shinya Kawanaka
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1
2012-02-27 00:17:42 PST
Created
attachment 128976
[details]
Patch
Hajime Morrita
Comment 2
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.
Shinya Kawanaka
Comment 3
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.
Shinya Kawanaka
Comment 4
2012-02-27 01:30:37 PST
Created
attachment 128982
[details]
Patch
Hajime Morrita
Comment 5
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.
Shinya Kawanaka
Comment 6
2012-02-27 22:21:25 PST
Created
attachment 129189
[details]
Patch for landing
Early Warning System Bot
Comment 7
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
Shinya Kawanaka
Comment 8
2012-02-27 23:05:32 PST
Created
attachment 129192
[details]
Patch for landing
Shinya Kawanaka
Comment 9
2012-02-28 00:06:16 PST
Committed
r109084
: <
http://trac.webkit.org/changeset/109084
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug