Bug 75930

Created attachment 125250 [details] Patch

Comment on attachment 125250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125250&action=review Looks almost fine. Let us pick some nits. > Source/WebCore/ChangeLog:9 > + We don't need to recreate DOM even if SUMMARY is removed or added from/into DETAILS. "removed from added into" > Source/WebCore/html/HTMLDetailsElement.cpp:83 > + ExceptionCode ec = 0; Could you use ASSERT_NO_EXCEPTION here? > Source/WebCore/html/HTMLDetailsElement.cpp:127 > ExceptionCode ec = 0; Could you get rid of |ec| using ASSERT_NO_EXCEPTION? > Source/WebCore/html/HTMLDetailsElement.cpp:139 > + m_mainSummary = findMainSummaryFor(this); It looks we are now able to get rid of m_mainSummary at all. Could you investigate it?

Created attachment 125261 [details] Patch

Comment on attachment 125261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125261&action=review > Source/WebCore/html/HTMLDetailsElement.cpp:123 > + return static_cast<DetailsSummaryElement*>(shadowRoot()->firstChild())->fallbackSummary(); I hope we have some safe net ASSERT here considering we are going to have multiple shadows... How about to give an unique tag name to DetailsSummaryElement and check it here or it will done by coming built-in check? > Source/WebCore/html/HTMLSummaryElement.cpp:93 > return 0; return false? (Well, it has been wrong...)

Created attachment 125268 [details] Patch

(In reply to comment #4) > (From update of attachment 125261 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125261&action=review > > > Source/WebCore/html/HTMLDetailsElement.cpp:123 > > + return static_cast<DetailsSummaryElement*>(shadowRoot()->firstChild())->fallbackSummary(); > > I hope we have some safe net ASSERT here considering we are going to have multiple shadows... > How about to give an unique tag name to DetailsSummaryElement and check it here or it will done by coming built-in check? Hmm... fallbackSummary() checks it is really a summary tag, so it checks some of errors actually. When forgetting to change shadowRoot() to a method getting a built-in shadow root method, it can be easily detected. Of course I'll add built-in check here when adding built-in flag. # When adding a method to get bulit-in shadow root, I'll do: - temporarily remove shadowRoot() method - add builtinShadowRoot() (maybe this name?), - and compile webkit to catch all the appearance of shadowRoot(). > > > Source/WebCore/html/HTMLSummaryElement.cpp:93 > > return 0; > > return false? (Well, it has been wrong...) Done.

Comment on attachment 125268 [details] Patch Clearing flags on attachment: 125268 Committed r106637: <http://trac.webkit.org/changeset/106637>

All reviewed patches have been landed. Closing bug.