Bug 23493 - Introduce FormControlElementWithState abstraction
Summary: Introduce FormControlElementWithState abstraction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20393
  Show dependency treegraph
 
Reported: 2009-01-22 20:23 PST by Nikolas Zimmermann
Modified: 2009-01-23 18:01 PST (History)
2 users (show)

See Also:


Attachments
Initial patch (58.96 KB, patch)
2009-01-22 20:27 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2009-01-22 20:23:59 PST
Introduce a shared FormControlElementWithState abstraction, and rework Document to operate on this class instead of HTMLFormControlElementWithState, to support save/restore state of form control elements from non-HTML languages, ie. WML.

WML needs some more tweaks to be able to use that abstraction -> most noticeable: WMLOptionElement inheriting from WMLEventHandlingElement & FormControlElement. Obviously WMLOptionElement can't inherit from the new WMLFormControlElement(WithState), as it inherits from WMLElement and FormControlElement(WithState). The easiest way to solve is to remove the WMLElement base-class from WMLEventHandlingElement, and make it behave just like the current FormControlElement does - this way we can let WMLOptionElement / WMLOptGroupElement / WMLInputElement all inherit from WMLFormControlElement.

See the attached patch.
Comment 1 Nikolas Zimmermann 2009-01-22 20:27:31 PST
Created attachment 26955 [details]
Initial patch
Comment 2 Eric Seidel (no email) 2009-01-23 17:41:25 PST
Comment on attachment 26955 [details]
Initial patch

  if (!element->isFormControlElement())
 37         return false;

Seems a bit strange when it's supposed to return a FormControlElement*

Strange indent:
FormControlElementWithState* toFormControlElementWithState(Element* element)
 59 {
 60 	if (!element->isFormControlElementWithState())
 61 		return 0;
 62

Tabs?
 {
260      doc->registerFormElementWithState(this);
 260 	FormControlElementWithState::registerFormControlElementWithState(this, document());
261261 }

It's a little confusing that there are both member and free versions of the toFormControlElement function.

I'm not sure what the EventHandlingElement changes are for.

In general this looks fine though.  Please be sure to fix the tabs/indent.
Comment 3 Nikolas Zimmermann 2009-01-23 17:55:07 PST
(In reply to comment #2)
> (From update of attachment 26955 [details] [review])
>   if (!element->isFormControlElement())
>  37         return false;
> 
> Seems a bit strange when it's supposed to return a FormControlElement*
Oops, changed to return 0;

> Strange indent:
> FormControlElementWithState* toFormControlElementWithState(Element* element)
>  59 {
>  60     if (!element->isFormControlElementWithState())
>  61             return 0;
>  62
> 
> Tabs?
>  {
> 260      doc->registerFormElementWithState(this);
>  260    FormControlElementWithState::registerFormControlElementWithState(this,
> document());
> 261261 }
Yes tabs, fixed.

> It's a little confusing that there are both member and free versions of the
> toFormControlElement function.
The free version of toFormControlElement() casts Element* to FormControlElement*.
The member version FormControlElementWithState::toFormControlElement() handles conversion of FormControlElementWithState -> FormControlElement (FormControlElementWithState does not inherit from FormControlElement, that's why I've used this special version here).

> 
> I'm not sure what the EventHandlingElement changes are for.
The ChangeLog covers this, no?

> 
> In general this looks fine though.  Please be sure to fix the tabs/indent.
Thanks for the review, landing soon.
Comment 4 Nikolas Zimmermann 2009-01-23 18:01:11 PST
Landed in r40204.