Summary: | [Shadow] ShadowRoot should know the existence of element having ElementShadow. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||||||||
Component: | DOM | Assignee: | Shinya Kawanaka <shinyak> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cmarcelo, webcomponents-bugzilla, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 100451 | ||||||||||||||
Attachments: |
|
Description
Shinya Kawanaka
2012-10-31 23:59:22 PDT
The patterns where the number of elements having shadow may be changed are: (1) An element having shadow is inserted. (2) An element having shadow is removed. (3) ShadowRoot is added to an element (4) ShadowRoot is removed from an element. Actually we don't have to consider (4) case, since there is no API to remove it. Created attachment 172005 [details]
Patch
This patch will definitely be conflicted with a patch for Bug 100921. Created attachment 172250 [details]
Patch
Comment on attachment 172250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172250&action=review > Source/WebCore/dom/Element.cpp:1061 > + if (ElementShadow* elementShadow = shadow()) { ShadowRoot also has insertedInto(). Can we move this to there? insertedInto() is relatively hot function and better to keep slim if possible. Same can be said for removedFrom(). Comment on attachment 172250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172250&action=review >> Source/WebCore/dom/Element.cpp:1061 >> + if (ElementShadow* elementShadow = shadow()) { > > ShadowRoot also has insertedInto(). Can we move this to there? > insertedInto() is relatively hot function and better to keep slim if possible. > Same can be said for removedFrom(). OK. The number of calling ShadowRoot::insertedInto and ShadowRoot::removedFrom does not seem balanced correctly... Maybe we have another bug around there? Created attachment 172508 [details]
Patch
Comment on attachment 172508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172508&action=review > Source/WebCore/dom/ShadowRoot.cpp:263 > + // Note that host element might be being destroyed here. What is the point of this comment? > Source/WebCore/dom/ShadowRoot.h:102 > + void unregisterElementHavingShadow() { --m_numberOfElementsHavingShadow; } Let's ASSERT() the count cannot be zero. > Source/WebCore/dom/ShadowRoot.h:134 > + size_t m_numberOfElementsHavingShadow; This is basically a count of ElementShadow, is that right? If so, I'd prefer more brief name. "ElementHavingShadow" sound a bit cryptic. Created attachment 172514 [details]
Patch
(In reply to comment #10) > (From update of attachment 172508 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172508&action=review > > > Source/WebCore/dom/ShadowRoot.cpp:263 > > + // Note that host element might be being destroyed here. > > What is the point of this comment? Removed. > > > Source/WebCore/dom/ShadowRoot.h:102 > > + void unregisterElementHavingShadow() { --m_numberOfElementsHavingShadow; } > > Let's ASSERT() the count cannot be zero. Added. > > > Source/WebCore/dom/ShadowRoot.h:134 > > + size_t m_numberOfElementsHavingShadow; > > This is basically a count of ElementShadow, is that right? > If so, I'd prefer more brief name. "ElementHavingShadow" sound a bit cryptic. Yes. I used m_numberOfElementShadow... I couldn't come up with better name... Comment on attachment 172514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172514&action=review > Source/WebCore/testing/Internals.cpp:332 > + ec = INVALID_ACCESS_ERR; Let's take check-error-then do happy path pattern. Pushing error path to late is unusual thus confusing. Created attachment 172518 [details]
Patch for landing
Comment on attachment 172518 [details] Patch for landing Clearing flags on attachment: 172518 Committed r133575: <http://trac.webkit.org/changeset/133575> All reviewed patches have been landed. Closing bug. |