Bug 80392

Summary:

[Forms] Introduce LabelableElement to share "labels" attribute implementation

Product:

WebKit

Reporter:

yosin

Component:

Forms

Assignee:

yosin

Status:

RESOLVED FIXED

Severity:

Normal

CC:

rakuco, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

80240, 80380, 80381, 80403, 80466

Attachments:

Description	Flags
Patch 1	none
Patch 2	none
Patch 3	none
Patch 4	none
Patch 5	none
Patch 6	none
Patch 7	none
Patch 8	none

yosin

Reported 2012-03-06 00:08:56 PST

This bug is part of HTMLFormControlElement re-factoring (https://bugs.webkit.org/show_bug.cgi?id=80381). At this time, "labels" attribute is implemented in HTMLFormControlElement. However, meter, and progress will be derived from HTMLElement instead of HTMLFormControlElement. To share code of "labels" attribute implementation among HTMLFormControlElement, HTMLMeterElement and HTMLProgressElement, we introduce new base class LabeledElement as below: class LabelableElement : public HTMLElement { ... PassRefPtr<NodeList> labels(); ... }; class HTMLFormControlElement : public LabelableElement, public FormAssociateElement { ... }; class HTMLMeterElement : public LabelableElement { ... }; class HTMLProgressElement : public LabelableElement { ... };

Attachments
Patch 1 (23.69 KB, patch) 2012-03-06 01:51 PST, yosin	no flags	Details Formatted Diff Diff
Patch 2 (23.32 KB, patch) 2012-03-06 02:00 PST, yosin	no flags	Details Formatted Diff Diff
Patch 3 (23.29 KB, patch) 2012-03-06 02:06 PST, yosin	no flags	Details Formatted Diff Diff
Patch 4 (23.29 KB, patch) 2012-03-06 02:50 PST, yosin	no flags	Details Formatted Diff Diff
Patch 5 (22.36 KB, patch) 2012-03-06 19:28 PST, yosin	no flags	Details Formatted Diff Diff
Patch 6 (22.93 KB, patch) 2012-03-06 21:31 PST, yosin	no flags	Details Formatted Diff Diff
Patch 7 (22.70 KB, patch) 2012-03-06 22:54 PST, yosin	no flags	Details Formatted Diff Diff
Patch 8 (22.74 KB, patch) 2012-03-07 00:40 PST, yosin	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

yosin

Comment 1 2012-03-06 00:19:34 PST

List of all labelable elements in the specification[1]: 1. button 2. input other than “hidden” 3. keygen 4. meter 5. output 6. progress 7. select 8. textarea == References == [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-label

yosin

Comment 2 2012-03-06 01:51:57 PST

Created attachment 130334 [details] Patch 1

yosin

Comment 3 2012-03-06 02:00:28 PST

Created attachment 130338 [details] Patch 2

yosin

Comment 4 2012-03-06 02:06:01 PST

Created attachment 130340 [details] Patch 3

yosin

Comment 5 2012-03-06 02:50:50 PST

Created attachment 130347 [details] Patch 4

Kent Tamura

Comment 6 2012-03-06 03:09:34 PST

Comment on attachment 130347 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=130347&action=review > Source/WebCore/ChangeLog:11 > + This patch introduces new class LabelableElement for base class of > + HTMLFormControlElement and moves isLabelable method to HTMLElement. > + > + No new tests are required. No behavior changes. You should explain a reason why you want to introduce LabelableElement in ChangeLog. > Source/WebCore/html/HTMLElement.h:96 > + virtual bool isLabelable() const { return false; } > + You don't need to define isLabelable() in HTMLElement. Defining it in LabelableElement is enough. > Source/WebCore/html/HTMLFormControlElement.cpp:-470 > -bool HTMLFormControlElement::isLabelable() const Because you remove some code, you can remove some #include in this file. > Source/WebCore/html/HTMLOutputElement.h:61 > + virtual bool isLabelable() const OVERRIDE { return true; } The original code doesn't support HTMLOutputElement. This is a behavior change and you need to write a test. > Source/WebCore/html/LabelableElement.h:38 > +// LabelableElement is the default implementation of labels attribute. This comment is confusing. Usually an element is not an implementation of an element, not an attribute. The comment should be something like the following: LabelableElement represents "labelable element" defined in the HTML specification, and provides the implementation of the "labels" attribute. > Source/WebCore/html/LabelableElement.h:48 > +} // namespace The commet is confusing. This doesn't close the anonymous namespace. Please remove the comment, or make it "namespace WebCore".

Kent Tamura

Comment 7 2012-03-06 03:12:20 PST

(In reply to comment #6) > This comment is confusing. Usually an element is not an implementation of an element, not an attribute. => an element class is an implementation of an element, not...

yosin

Comment 8 2012-03-06 19:28:31 PST

Created attachment 130522 [details] Patch 5

Kent Tamura

Comment 9 2012-03-06 19:34:03 PST

(In reply to comment #6) > > Source/WebCore/ChangeLog:11 > > + This patch introduces new class LabelableElement for base class of > > + HTMLFormControlElement and moves isLabelable method to HTMLElement. > > + > > + No new tests are required. No behavior changes. > > You should explain a reason why you want to introduce LabelableElement in ChangeLog. Patch 5 has no update for this comment.

Build Bot

Comment 10 2012-03-06 20:41:32 PST

Comment on attachment 130522 [details] Patch 5 Attachment 130522 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11836717

yosin

Comment 11 2012-03-06 21:31:48 PST

Created attachment 130538 [details] Patch 6

Build Bot

Comment 12 2012-03-06 22:03:31 PST

Comment on attachment 130538 [details] Patch 6 Attachment 130538 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11836748

yosin

Comment 13 2012-03-06 22:54:40 PST

Created attachment 130549 [details] Patch 7

yosin

Comment 14 2012-03-06 22:56:56 PST

Take project.pbxproj generated by UI. Difference between UI generated and manually modified version are positions of entries. (><)

Build Bot

Comment 15 2012-03-07 00:13:43 PST

Comment on attachment 130549 [details] Patch 7 Attachment 130549 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11832842

yosin

Comment 16 2012-03-07 00:40:18 PST

Created attachment 130560 [details] Patch 8

yosin

Comment 17 2012-03-07 00:42:23 PST

Add settings = {ATTRIBUTES = (Private, );} for LabelableElement.h to project.pbxproj. It seems this attribute makes copying file to /WebKitBuild/Debug/WebCore.framework/PrivateHeaders

Kent Tamura

Comment 18 2012-03-07 04:37:15 PST

Comment on attachment 130560 [details] Patch 8 ok

WebKit Review Bot

Comment 19 2012-03-07 08:29:55 PST

Comment on attachment 130560 [details] Patch 8 Clearing flags on attachment: 130560 Committed r110055: <http://trac.webkit.org/changeset/110055>

WebKit Review Bot

Comment 20 2012-03-07 08:30:01 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.