Bug 80392 - [Forms] Introduce LabelableElement to share "labels" attribute implementation
Summary: [Forms] Introduce LabelableElement to share "labels" attribute implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 80240 80380 80381 80403 80466
  Show dependency treegraph
 
Reported: 2012-03-06 00:08 PST by yosin
Modified: 2012-03-07 08:30 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 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 { ... };
Comment 1 yosin 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
Comment 2 yosin 2012-03-06 01:51:57 PST
Created attachment 130334 [details]
Patch 1
Comment 3 yosin 2012-03-06 02:00:28 PST
Created attachment 130338 [details]
Patch 2
Comment 4 yosin 2012-03-06 02:06:01 PST
Created attachment 130340 [details]
Patch 3
Comment 5 yosin 2012-03-06 02:50:50 PST
Created attachment 130347 [details]
Patch 4
Comment 6 Kent Tamura 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".
Comment 7 Kent Tamura 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...
Comment 8 yosin 2012-03-06 19:28:31 PST
Created attachment 130522 [details]
Patch 5
Comment 9 Kent Tamura 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.
Comment 10 Build Bot 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
Comment 11 yosin 2012-03-06 21:31:48 PST
Created attachment 130538 [details]
Patch 6
Comment 12 Build Bot 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
Comment 13 yosin 2012-03-06 22:54:40 PST
Created attachment 130549 [details]
Patch 7
Comment 14 yosin 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. (><)
Comment 15 Build Bot 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
Comment 16 yosin 2012-03-07 00:40:18 PST
Created attachment 130560 [details]
Patch 8
Comment 17 yosin 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
Comment 18 Kent Tamura 2012-03-07 04:37:15 PST
Comment on attachment 130560 [details]
Patch 8

ok
Comment 19 WebKit Review Bot 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>
Comment 20 WebKit Review Bot 2012-03-07 08:30:01 PST
All reviewed patches have been landed.  Closing bug.