Bug 50309

Summary: HTML5 <details> and <summary> initial implementation
Product: WebKit Reporter: Luiz Agostini <luiz>
Component: WebCore Misc.Assignee: Luiz Agostini <luiz>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, darin, gustavo, paulirish, peter, tkent, webkit.review.bot, xan.lopez, yael
Priority: P2 Keywords: HTML5
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 32934    
Attachments:
Description Flags
patch
none
patch
darin: review-
patch
none
patch
darin: review+
checking build systems before landing. please do not review. none

Luiz Agostini
Reported 2010-11-30 21:42:12 PST
HTML5 <details> and <summary> elements initial implementation. The main objective is to add the files for html elements and renderers, and to get rid of build system issues in future patches.
Attachments
patch (65.94 KB, patch)
2010-11-30 21:50 PST, Luiz Agostini 		no flags
DetailsFormatted DiffDiff
patch (65.90 KB, patch)
2010-12-02 15:52 PST, Luiz Agostini 		darin: review-
DetailsFormatted DiffDiff
patch (48.28 KB, patch)
2010-12-02 20:40 PST, Luiz Agostini 		no flags
DetailsFormatted DiffDiff
patch (48.82 KB, patch)
2010-12-02 21:49 PST, Luiz Agostini 		darin: review+
DetailsFormatted DiffDiff
checking build systems before landing. please do not review. (49.70 KB, patch)
2010-12-05 15:59 PST, Luiz Agostini 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Luiz Agostini
Comment 1 2010-11-30 21:50:07 PST
Created attachment 75248 [details] patch
WebKit Review Bot
Comment 2 2010-11-30 22:33:08 PST
Attachment 75248 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6678001
Luiz Agostini
Comment 4 2010-12-02 15:52:52 PST
Created attachment 75427 [details] patch
Darin Adler
Comment 5 2010-12-02 17:48:55 PST
Comment on attachment 75427 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=75427&action=review I don’t want to see these renderer classes added until we are sure we need them. Are you certain you will need all three? > WebCore/html/HTMLDetailsElement.cpp:55 > +bool HTMLDetailsElement::open() const > +{ > + return !getAttribute(openAttr).isNull(); > +} > + > +void HTMLDetailsElement::setOpen(bool value) > +{ > + setAttribute(openAttr, value ? "" : 0); > +} These functions are not needed. > WebCore/html/HTMLDetailsElement.h:31 > + static PassRefPtr<HTMLDetailsElement> create(Document* document); We should not add this function unless we have a callsite that needs it. > WebCore/html/HTMLDetailsElement.h:34 > + bool open() const; > + void setOpen(bool); These functions are not needed. > WebCore/html/HTMLDetailsElement.idl:23 > + attribute boolean open; This should use [Reflect] so we don’t need open and canOpen functions in the C++ class. > WebCore/html/HTMLTagNames.in:118 > -summary interfaceName=HTMLElement > +summary This change is incorrect. See the HTML5 specification. It specifically says the DOM interface uses HTMLElement. The class HTMLSummaryElement is not needed.
Luiz Agostini
Comment 6 2010-12-02 19:07:08 PST
(In reply to comment #5) > (From update of attachment 75427 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75427&action=review > > I don’t want to see these renderer classes added until we are sure we need them. Are you certain you will need all three? The plan is to RenderDetails to use RenderBlock::layoutSpecialExcludedChild() to set the position of its first RenderSummary child. And this first RenderSummary creates a RenderDetailsMarker in a way similar to what is done by RenderListItem. What do you think?
Luiz Agostini
Comment 7 2010-12-02 20:40:08 PST
Created attachment 75459 [details] patch
Build Bot
Comment 8 2010-12-02 21:31:41 PST
Attachment 75459 [details] did not build on win: Build output: http://queues.webkit.org/results/6727025
Luiz Agostini
Comment 9 2010-12-02 21:49:10 PST
Created attachment 75464 [details] patch
Build Bot
Comment 10 2010-12-02 22:39:14 PST
Attachment 75464 [details] did not build on win: Build output: http://queues.webkit.org/results/6736030
Darin Adler
Comment 11 2010-12-03 09:42:19 PST
Comment on attachment 75464 [details] patch I’m still not sure I understand if this will require all three unique renderer classes. At the moment we are just checking them in as dead code which seems a little weak. r=me, but please find out why the Windows build is failing and resolve that before checking in
Luiz Agostini
Comment 12 2010-12-05 15:59:33 PST
Created attachment 75639 [details] checking build systems before landing. please do not review.
Luiz Agostini
Comment 13 2010-12-05 17:15:59 PST
Committed r73346: <http://trac.webkit.org/changeset/73346>
Note You need to log in before you can comment on or make changes to this bug.