Bug 75930 - Reimplement DETAILS and SUMMARY using selector query.
: Reimplement DETAILS and SUMMARY using selector query.
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 75301 75306
: 72957 77608
  Show dependency treegraph
 
Reported: 2012-01-09 19:23 PST by
Modified: 2012-02-03 00:31 PST (History)


Attachments
Patch (16.53 KB, patch)
2012-02-02 20:49 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.11 KB, patch)
2012-02-02 22:27 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.17 KB, patch)
2012-02-02 23:13 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-09 19:23:57 PST
When Bug 75306 is resolved, DETAILS and SUMMARY can be re-implemented using selector query.
------- Comment #1 From 2012-02-02 20:49:02 PST -------
Created an attachment (id=125250) [details]
Patch
------- Comment #2 From 2012-02-02 21:12:56 PST -------
(From update of attachment 125250 [details])
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?
------- Comment #3 From 2012-02-02 22:27:11 PST -------
Created an attachment (id=125261) [details]
Patch
------- Comment #4 From 2012-02-02 22:48:15 PST -------
(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?

> Source/WebCore/html/HTMLSummaryElement.cpp:93
>      return 0;

return false? (Well, it has been wrong...)
------- Comment #5 From 2012-02-02 23:13:44 PST -------
Created an attachment (id=125268) [details]
Patch
------- Comment #6 From 2012-02-02 23:16:16 PST -------
(In reply to comment #4)
> (From update of attachment 125261 [details] [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 #7 From 2012-02-03 00:31:13 PST -------
(From update of attachment 125268 [details])
Clearing flags on attachment: 125268

Committed r106637: <http://trac.webkit.org/changeset/106637>
------- Comment #8 From 2012-02-03 00:31:18 PST -------
All reviewed patches have been landed.  Closing bug.