Bug 23493 – Introduce FormControlElementWithState abstraction

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 26955 [details]
Initial patch

[Eric Seidel (no email)]

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.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Landed in r40204.