Implement WML specific features for <select>, related to variable handling. Process the 'iname' / 'ivalue' attributes, and process them according to the WML specification.
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.
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
(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
Created attachment 31802 [details] Updated patch Updated patch, including layout tests.
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...
(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.
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.
(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.