Reimplement DETAILS and SUMMARY using selector query.

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

When Bug 75306 is resolved, DETAILS and SUMMARY can be re-implemented using selector query.

[Shinya Kawanaka]

Created attachment 125250 [details]
Patch

[Hajime Morrita]

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?

[Shinya Kawanaka]

Created attachment 125261 [details]
Patch

[Hajime Morrita]

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...)

[Shinya Kawanaka]

Created attachment 125268 [details]
Patch

[Shinya Kawanaka]

(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.

[WebKit Review Bot]

Comment on attachment 125268 [details]
Patch

Clearing flags on attachment: 125268

Committed r106637: <http://trac.webkit.org/changeset/106637>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.