Need access to the items of the Select element through WebKit::WebSelectElement. Patch is pending.
Created attachment 60150 [details] Patch
I hate that static_cast pattern, even though I know we use it a lot. +fishd for kr API review.
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.
(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.
Created attachment 61066 [details] Patch
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.
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.
Created attachment 61261 [details] Patch
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?
Created attachment 61270 [details] Patch
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.
Created attachment 61562 [details] Patch
Comment on attachment 61562 [details] Patch Clearing flags on attachment: 61562 Committed r63401: <http://trac.webkit.org/changeset/63401>
All reviewed patches have been landed. Closing bug.