Bug 149698 - Rewrite HTMLDetailsElement using HTMLSlotElement
Summary: Rewrite HTMLDetailsElement using HTMLSlotElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 149701 150322
Blocks: 148695 150036 150004
  Show dependency treegraph
 
Reported: 2015-09-30 23:28 PDT by Ryosuke Niwa
Modified: 2015-10-27 20:10 PDT (History)
11 users (show)

See Also:


Attachments
WIP (71.84 KB, patch)
2015-09-30 23:30 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
patch (48.68 KB, patch)
2015-10-10 06:37 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (632.16 KB, application/zip)
2015-10-10 07:13 PDT, Build Bot
no flags Details
patch (54.11 KB, patch)
2015-10-10 07:27 PDT, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-09-30 23:28:48 PDT
Now that the basic infrastructure for shadow DOM is available, rewrite details element using the new API.
Comment 1 Ryosuke Niwa 2015-09-30 23:30:01 PDT
Created attachment 262237 [details]
WIP
Comment 2 Ryosuke Niwa 2015-09-30 23:31:08 PDT
Maybe I should split the removal into a separate patch?
Comment 3 Antti Koivisto 2015-10-01 10:23:20 PDT
Seems fine like this too.
Comment 4 Ryosuke Niwa 2015-10-01 11:26:34 PDT
(In reply to comment #3)
> Seems fine like this too.

Okay. FWIW, I need to land my patch on https://bugs.webkit.org/show_bug.cgi?id=149701 first.
Comment 5 Antti Koivisto 2015-10-10 06:37:35 PDT
Created attachment 262824 [details]
patch
Comment 6 WebKit Commit Bot 2015-10-10 06:39:49 PDT
Attachment 262824 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SlotAssignment.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Build Bot 2015-10-10 07:12:56 PDT
Comment on attachment 262824 [details]
patch

Attachment 262824 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/264687

New failing tests:
fast/html/details-open2.html
Comment 8 Build Bot 2015-10-10 07:13:01 PDT
Created attachment 262825 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 9 Antti Koivisto 2015-10-10 07:27:07 PDT
Created attachment 262826 [details]
patch
Comment 10 WebKit Commit Bot 2015-10-10 07:29:53 PDT
Attachment 262826 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SlotAssignment.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Andreas Kling 2015-10-10 10:54:37 PDT
Comment on attachment 262826 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262826&action=review

r=me

> Source/WebCore/html/HTMLDetailsElement.cpp:76
> +    addShadowRoot(ShadowRoot::create(document, std::make_unique<SlotAssignment>(slotNameFunction)));

As discussed on IRC, calling this virtual function from a constructor is a bit lame, even though this is a leaf class.
I think we should just leave it in create() like you did with HTMLSummaryElement.

> Source/WebCore/html/HTMLDetailsElement.h:49
>      bool m_isOpen;
> +    HTMLSlotElement* m_summarySlot;
> +    HTMLSummaryElement* m_defaultSummary;

Would be nice to initialize all of these explicitly here.
Comment 12 Antti Koivisto 2015-10-10 11:17:43 PDT
https://trac.webkit.org/r190840
Comment 13 Brent Fulgham 2015-10-12 10:59:54 PDT
This change introduced several Windows test failures because the baselines changed in the same way as the Mac port.

I committed a rebaseline under <https://trac.webkit.org/r190861>.
Comment 14 Brent Fulgham 2015-10-12 11:06:41 PDT
There are a number of new test failures on EFL/GTK and Windows after this change was committed. See Bug 150036 for details.

New failing tests are:

fast/html/details-add-details-child-2.html
fast/html/details-add-summary-child-1.html
fast/html/details-add-summary-child-2.html
Comment 15 Radar WebKit Bug Importer 2015-10-27 20:10:35 PDT
<rdar://problem/23289343>