Bug 41384 - Allow Chromium access to Select control choices
Summary: Allow Chromium access to Select control choices
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-29 16:50 PDT by George Yakovlev
Modified: 2010-07-14 21:05 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.38 KB, patch)
2010-06-30 14:11 PDT, George Yakovlev
no flags Details | Formatted Diff | Diff
Patch (10.54 KB, patch)
2010-07-09 11:29 PDT, George Yakovlev
no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2010-07-12 13:20 PDT, George Yakovlev
no flags Details | Formatted Diff | Diff
Patch (11.11 KB, patch)
2010-07-12 14:28 PDT, George Yakovlev
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2010-07-14 14:12 PDT, George Yakovlev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Yakovlev 2010-06-29 16:50:49 PDT
Need access to the items of the Select element through WebKit::WebSelectElement. Patch is pending.
Comment 1 George Yakovlev 2010-06-30 14:11:53 PDT
Created attachment 60150 [details]
Patch
Comment 2 Adam Barth 2010-06-30 14:16:21 PDT
I hate that static_cast pattern, even though I know we use it a lot.

+fishd for kr API review.
Comment 3 Darin Fisher (:fishd, Google) 2010-07-02 16:41:20 PDT
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.
Comment 4 James Hawkins 2010-07-08 12:58:58 PDT
(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.
Comment 5 George Yakovlev 2010-07-09 11:29:32 PDT
Created attachment 61066 [details]
Patch
Comment 6 WebKit Review Bot 2010-07-09 11:32:53 PDT
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 7 Darin Fisher (:fishd, Google) 2010-07-09 11:59:46 PDT
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.
Comment 8 George Yakovlev 2010-07-12 13:20:25 PDT
Created attachment 61261 [details]
Patch
Comment 9 Darin Fisher (:fishd, Google) 2010-07-12 14:13:49 PDT
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?
Comment 10 George Yakovlev 2010-07-12 14:28:18 PDT
Created attachment 61270 [details]
Patch
Comment 11 Darin Fisher (:fishd, Google) 2010-07-12 14:46:13 PDT
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.
Comment 12 George Yakovlev 2010-07-14 14:12:19 PDT
Created attachment 61562 [details]
Patch
Comment 13 WebKit Commit Bot 2010-07-14 21:05:25 PDT
Comment on attachment 61562 [details]
Patch

Clearing flags on attachment: 61562

Committed r63401: <http://trac.webkit.org/changeset/63401>
Comment 14 WebKit Commit Bot 2010-07-14 21:05:31 PDT
All reviewed patches have been landed.  Closing bug.