WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
75930
Reimplement DETAILS and SUMMARY using selector query.
https://bugs.webkit.org/show_bug.cgi?id=75930
Summary
Reimplement DETAILS and SUMMARY using selector query.
Shinya Kawanaka
Reported
2012-01-09 19:23:57 PST
When
Bug 75306
is resolved, DETAILS and SUMMARY can be re-implemented using selector query.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1
2012-02-02 20:49:02 PST
Created
attachment 125250
[details]
Patch
Hajime Morrita
Comment 2
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?
Shinya Kawanaka
Comment 3
2012-02-02 22:27:11 PST
Created
attachment 125261
[details]
Patch
Hajime Morrita
Comment 4
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...)
Shinya Kawanaka
Comment 5
2012-02-02 23:13:44 PST
Created
attachment 125268
[details]
Patch
Shinya Kawanaka
Comment 6
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.
WebKit Review Bot
Comment 7
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
>
WebKit Review Bot
Comment 8
2012-02-03 00:31:18 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug