Bug 26246 - Implement WML specific features for <select>
Summary: Implement WML specific features for <select>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (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-06-07 17:18 PDT by Nikolas Zimmermann
Modified: 2009-06-29 12:11 PDT (History)
3 users (show)

See Also:


Attachments
Initial patch (20.17 KB, patch)
2009-06-07 17:46 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (34.40 KB, patch)
2009-06-24 12:18 PDT, Nikolas Zimmermann
manyoso: review-
Details | Formatted Diff | Diff
Updated patch v2 (33.80 KB, patch)
2009-06-26 07:15 PDT, Nikolas Zimmermann
manyoso: 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-06-07 17:18:56 PDT
Implement WML specific features for <select>, related to variable handling.
Process the 'iname' / 'ivalue' attributes, and process them according to the WML specification.
Comment 1 Nikolas Zimmermann 2009-06-07 17:46:39 PDT
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.
Comment 2 Arun Patole 2009-06-10 23:46:29 PDT
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
Comment 3 Nikolas Zimmermann 2009-06-23 02:17:28 PDT
(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
Comment 4 Nikolas Zimmermann 2009-06-24 12:18:29 PDT
Created attachment 31802 [details]
Updated patch

Updated patch, including layout tests.
Comment 5 Adam Treat 2009-06-24 13:36:53 PDT
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...
Comment 6 Nikolas Zimmermann 2009-06-25 06:28:12 PDT
(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 7 Adam Treat 2009-06-29 11:49:05 PDT
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.
Comment 8 Nikolas Zimmermann 2009-06-29 12:11:38 PDT
(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.