Bug 41384 – Allow Chromium access to Select control choices

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[George Yakovlev]

Need access to the items of the Select element through WebKit::WebSelectElement. Patch is pending.

[George Yakovlev]

Created attachment 60150 [details]
Patch

[Adam Barth]

I hate that static_cast pattern, even though I know we use it a lot.

+fishd for kr API review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 60150 [details]
Patch

WebKit/chromium/src/WebSelectElement.cpp:65
 +  
nit: only one new line

WebKit/chromium/src/WebSelectElement.cpp:60
 +              items[i] = static_cast<HTMLOptionElement*>(sourceItems[i])->value();
instead of returning a list of the values of the items, how
about returning WebOptionElement objects instead?  otherwise,
i think the name of this method should be listItemValues to
better correspond to what it is returning.

WebOptionElement seems better to me.

[James Hawkins]

(In reply to comment #3)
> (From update of attachment 60150 [details])
> WebKit/chromium/src/WebSelectElement.cpp:65
>  +  
> nit: only one new line
> 
> WebKit/chromium/src/WebSelectElement.cpp:60
>  +              items[i] = static_cast<HTMLOptionElement*>(sourceItems[i])->value();
> instead of returning a list of the values of the items, how
> about returning WebOptionElement objects instead?  otherwise,
> i think the name of this method should be listItemValues to
> better correspond to what it is returning.
> 
> WebOptionElement seems better to me.

I agree. Implementing WebOptionElement is ideal.

[George Yakovlev]

Created attachment 61066 [details]
Patch

[WebKit Review Bot]

Attachment 61066 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/public/WebOptionElement.h:49:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61066 [details]
Patch

WebKit/chromium/public/WebOptionElement.h:60
 +      WEBKIT_API bool disabled() const;
hmm... this 'disabled' method seems similar to the inherited
WebFormControlElement::isEnabled?  i see from reading the
implementation that it does a bit more (checks the parent's
disabled state as well).  it seems to me that it would be
better to name this one isEnabled as well.

WebKit/chromium/src/WebSelectElement.cpp:59
 +          items[i] = WebOptionElement(static_cast<HTMLOptionElement*>(sourceItems[i]));
I think you need to check that sourcesItems[i]->hasLocalName(optionTag)
is true before casting to HTMLOptionElement.

I'm basing this on the implementation of HTMLSelectElement::value().
You could also just return a WebVector<WebElement>, and then leave
it up to the consumer to use WebElement::hasTagName.

[George Yakovlev]

Created attachment 61261 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 61261 [details]
Patch

WebKit/chromium/public/WebOptionElement.h:64
 +      WEBKIT_API bool disabled() const;
previously, i suggested that you rename disabled to isEnabled, flipping
the sense, to be consistent with WebFormControlElement::isEnabled.  any
reason not to do this?

[George Yakovlev]

Created attachment 61270 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 61270 [details]
Patch

WebKit/chromium/src/WebSelectElement.cpp:59
 +        if (sourceItems[i]->hasLocalName(HTMLNames::optionTag))
^^^ indentation is wrong here and on the next line


Since the WebSelectElement::listItems method is only returning the
options contained in the SELECT element (excluding other types of
elements), I think it would be better to call this method "options":

  WebVector<WebOptionElement> WebSelectElement::options()

If someone wanted to expose the full set of list items, then that
method could return a vector of WebElement and be named listItems.


LGTM with that change.

[George Yakovlev]

Created attachment 61562 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 61562 [details]
Patch

Clearing flags on attachment: 61562

Committed r63401: <http://trac.webkit.org/changeset/63401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.