Bug 75930 - Reimplement DETAILS and SUMMARY using selector query.
: Reimplement DETAILS and SUMMARY using selector query.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on: 75301 75306
Blocks: 72957 77608
  Show dependency treegraph
 
Reported: 2012-01-09 19:23 PST by Shinya Kawanaka
Modified: 2012-02-03 00:31 PST (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-01-09 19:23:57 PST
When Bug 75306 is resolved, DETAILS and SUMMARY can be re-implemented using selector query.
Comment 1 Shinya Kawanaka 2012-02-02 20:49:02 PST
Created attachment 125250 [details]
Patch
Comment 2 Hajime Morrita 2012-02-02 21:12:56 PST
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?
Comment 3 Shinya Kawanaka 2012-02-02 22:27:11 PST
Created attachment 125261 [details]
Patch
Comment 4 Hajime Morrita 2012-02-02 22:48:15 PST
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...)
Comment 5 Shinya Kawanaka 2012-02-02 23:13:44 PST
Created attachment 125268 [details]
Patch
Comment 6 Shinya Kawanaka 2012-02-02 23:16:16 PST
(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 7 WebKit Review Bot 2012-02-03 00:31:13 PST
Comment on attachment 125268 [details]
Patch

Clearing flags on attachment: 125268

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