Bug 26246 – Implement WML specific features for <select>

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Implement WML specific features for <select>, related to variable handling.
Process the 'iname' / 'ivalue' attributes, and process them according to the WML specification.

[Nikolas Zimmermann]

Created attachment 31037 [details]
Initial patch

Doesn't contain any layout tests yet. Need to write some tomorrow. Uploading patch so George can have a look.

[Arun Patole]

Hi Nikolas,

Just thought of having a quick look at the patch and here are some comments that might help if I am right saying it:

1. in void WMLSelectElement::calculateDefaultOptionIndices()
// Spec: iF the default option index is empty and the select is a multi-choice, then the
// default option index is set to zero. If the select is single-choice, then the default
// option index is set to one.
if (m_defaultOptionIndices.isEmpty())
m_defaultOptionIndices.append((unsigned) m_data.multiple()); 

[Arun]: I think you will have to negate multiples() return value:
 m_defaultOptionIndices.append((unsigned) !m_data.multiple()); 


2. at String WMLSelectElement::optionIndicesToValueString() const

[Arun] I think this function will fail at assert if m_defaultOptionIndices contains "0".

m_defaultOptionIndices will contain "0" for the following case i think:
// Spec: iF the default option index is empty and the select is a multi-choice, then the
// default option index is set to zero. If the select is single-choice, then the default
// option index is set to one.
 if (m_defaultOptionIndices.isEmpty())
 m_defaultOptionIndices.append((unsigned) m_data.multiple());


Thanks,
Arun

[Nikolas Zimmermann]

(In reply to comment #2)
> Hi Nikolas,
> 
> Just thought of having a quick look at the patch and here are some comments
> that might help if I am right saying it:
> 
> 1. in void WMLSelectElement::calculateDefaultOptionIndices()
> // Spec: iF the default option index is empty and the select is a multi-choice,
> then the
> // default option index is set to zero. If the select is single-choice, then
> the default
> // option index is set to one.
> if (m_defaultOptionIndices.isEmpty())
> m_defaultOptionIndices.append((unsigned) m_data.multiple()); 
> 
> [Arun]: I think you will have to negate multiples() return value:
>  m_defaultOptionIndices.append((unsigned) !m_data.multiple()); 
> 
> 
> 2. at String WMLSelectElement::optionIndicesToValueString() const
> 
> [Arun] I think this function will fail at assert if m_defaultOptionIndices
> contains "0".
> 
> m_defaultOptionIndices will contain "0" for the following case i think:
> // Spec: iF the default option index is empty and the select is a multi-choice,
> then the
> // default option index is set to zero. If the select is single-choice, then
> the default
> // option index is set to one.
>  if (m_defaultOptionIndices.isEmpty())
>  m_defaultOptionIndices.append((unsigned) m_data.multiple());

Hi Arun,

thanks for your comments, that's appreciated! You may have noticed this patch doesn't contain any layout tests so far, thus I couldn't specifically test it so far, that's why it wasn't up for review so far :-)

Going to write layout tests that cover these situations.

Cheers,
Niko

[Nikolas Zimmermann]

Created attachment 31802 [details]
Updated patch

Updated patch, including layout tests.

[Adam Treat]

Comment on attachment 31802 [details]
Updated patch

> +        * dom/SelectElement.cpp:
> +        (WebCore::SelectElement::optionCount):
> +        (WebCore::SelectElementData::name):
> +        * dom/SelectElement.h:
> +        (WebCore::SelectElementData::setName):
> +        * html/HTMLSelectElement.cpp:
> +        (WebCore::HTMLSelectElement::formControlName):
> +        (WebCore::HTMLSelectElement::length):
> +        (WebCore::HTMLSelectElement::parseMappedAttribute):
> +        * html/HTMLSelectElement.h:
> +        * wml/WMLCardElement.cpp:
> +        (WebCore::WMLCardElement::handleIntrinsicEventIfNeeded):
> +        * wml/WMLSelectElement.cpp:
> +        (WebCore::WMLSelectElement::title):
> +        (WebCore::WMLSelectElement::formControlName):
> +        (WebCore::WMLSelectElement::parseMappedAttribute):
> +        (WebCore::WMLSelectElement::defaultEventHandler):
> +        (WebCore::WMLSelectElement::selectInitialOptions):
> +        (WebCore::WMLSelectElement::calculateDefaultOptionIndices):
> +        (WebCore::WMLSelectElement::selectDefaultOptions):
> +        (WebCore::WMLSelectElement::initializeVariables):
> +        (WebCore::WMLSelectElement::updateVariables):
> +        (WebCore::WMLSelectElement::parseIndexValueString):
> +        (WebCore::WMLSelectElement::valueStringToOptionIndices):
> +        (WebCore::WMLSelectElement::optionIndicesToValueString):
> +        (WebCore::WMLSelectElement::optionIndicesToString):
> +        * wml/WMLSelectElement.h:

This is a big change to review.  I'd appreciate it if you could go through the ChangeLog's and document specifically what you change to each method and why.  And also why you add methods in a couple of cases.  From looking, it seems that you add 'optionCount' to SelectElement so that both HTMLSelectElement and WMLSelectElement can use it whereas before it was implemented in HTMLSelectElement.  You've also changed it so that it uses 'isOptionElement' as the test rather than 'hasLocalName(optionTag).'  In general, it'd be great to have these changes and the reasoning behind them documented in the ChangeLog so I know specifically what you were thinking without guessing.

This is best practices anyway...

[Nikolas Zimmermann]

(In reply to comment #5)
> This is a big change to review.  I'd appreciate it if you could go through the
> ChangeLog's and document specifically what you change to each method and why. 
> And also why you add methods in a couple of cases.  From looking, it seems that
> you add 'optionCount' to SelectElement so that both HTMLSelectElement and
> WMLSelectElement can use it whereas before it was implemented in
> HTMLSelectElement.  You've also changed it so that it uses 'isOptionElement' as
> the test rather than 'hasLocalName(optionTag).'  In general, it'd be great to
> have these changes and the reasoning behind them documented in the ChangeLog so
> I know specifically what you were thinking without guessing.
> 
> This is best practices anyway...

Right, I should update it. This happens if women distract you to finish your work ;-)
Coming up with a detailed ChangeLog tonight.

[Adam Treat]

Comment on attachment 31932 [details]
Updated patch v2

There are some weird symbols showing up in the formatted diff:

//Spec: Step 2 – initialise variables

Otherwise, looks good! r+ with those removed.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 31932 [details] [review])
> There are some weird symbols showing up in the formatted diff:
> 
> //Spec: Step 2 – initialise variables
> 
> Otherwise, looks good! r+ with those removed.

Landed in r45342.