Bug 69828 - Move the content of SelectElementData into HTMLSelectElement, and remove SelectElement.{cpp,h}
: Move the content of SelectElementData into HTMLSelectElement, and remove Sele...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P3 Minor
Assigned To: Kent Tamura
:
Depends on: 69812
Blocks: 69908
  Show dependency treegraph
 
Reported: 2011-10-11 03:30 PDT by Kent Tamura
Modified: 2011-10-12 00:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (45.50 KB, patch)
2011-10-11 03:38 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (46.73 KB, patch)
2011-10-11 05:51 PDT, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-10-11 03:30:56 PDT
Move the content of SelectElementData into HTMLSelectElement, and remove SelectElement.{cpp,h}
Comment 1 Kent Tamura 2011-10-11 03:38:35 PDT
Created attachment 110493 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2011-10-11 04:44:49 PDT
Comment on attachment 110493 [details]
Patch

Attachment 110493 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10027494
Comment 3 Collabora GTK+ EWS bot 2011-10-11 05:26:38 PDT
Comment on attachment 110493 [details]
Patch

Attachment 110493 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10031417
Comment 4 Kent Tamura 2011-10-11 05:51:46 PDT
Created attachment 110503 [details]
Patch 2

Probably fixed GTK and Windows bulid
Comment 5 Darin Adler 2011-10-11 08:40:19 PDT
Comment on attachment 110503 [details]
Patch 2

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

> Source/WebCore/html/HTMLSelectElement.h:164
> +    bool usesMenuList() const
> +    {
> +#if ENABLE(NO_LISTBOX_RENDERING)
> +        return true;
> +#else
> +        return !m_multiple && m_size <= 1;
> +#endif
> +    }

It’s good to have this inline, but it’s a little long and ugly to have here in the class definition. I’d put it separately after the class definition.

> Source/WebCore/html/HTMLSelectElement.h:178
>      Vector<Element*> m_listItems;

These should be HTMLFormControlElement*. The only reason they are Element* was WML support.

> Source/WebCore/html/HTMLSelectElement.h:191
>      bool m_recalcListItems;

I think this should be renamed (later) m_shouldRecalcListItems or something even better.
Comment 6 Kent Tamura 2011-10-12 00:10:19 PDT
Comment on attachment 110503 [details]
Patch 2

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

>> Source/WebCore/html/HTMLSelectElement.h:164
>> +    }
> 
> It’s good to have this inline, but it’s a little long and ugly to have here in the class definition. I’d put it separately after the class definition.

ok, I move it.

>> Source/WebCore/html/HTMLSelectElement.h:178
>>      Vector<Element*> m_listItems;
> 
> These should be HTMLFormControlElement*. The only reason they are Element* was WML support.

Unfortunately, m_listItems contains <hr> elements.
We can change Element* to HTMLElement*.

>> Source/WebCore/html/HTMLSelectElement.h:191
>>      bool m_recalcListItems;
> 
> I think this should be renamed (later) m_shouldRecalcListItems or something even better.

I'll address this later.
Comment 7 Kent Tamura 2011-10-12 00:13:19 PDT
Committed r97238: <http://trac.webkit.org/changeset/97238>