Bug 25739 - Upstream V8 bindings for HTMLOptionsCollection and HTMLSelectElementCollection
Summary: Upstream V8 bindings for HTMLOptionsCollection and HTMLSelectElementCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-12 15:56 PDT by Nate Chapin
Modified: 2009-05-15 15:28 PDT (History)
1 user (show)

See Also:


Attachments
patch (8.90 KB, patch)
2009-05-12 16:01 PDT, Nate Chapin
dglazkov: review-
Details | Formatted Diff | Diff
patch2 (8.54 KB, patch)
2009-05-13 14:18 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch3 (9.23 KB, patch)
2009-05-13 15:46 PDT, Nate Chapin
dglazkov: review-
Details | Formatted Diff | Diff
patch4 (9.52 KB, patch)
2009-05-13 17:23 PDT, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-05-12 15:56:08 PDT
See summary.
Comment 1 Nate Chapin 2009-05-12 16:01:42 PDT
Created attachment 30252 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-05-13 10:27:26 PDT
Comment on attachment 30252 [details]
patch

> +INDEXED_PROPERTY_SETTER(HTMLOptionsCollection) {

brace on new line.

> +v8::Handle<v8::Value> getOptionsCollectionSetter(uint32_t index, v8::Handle<v8::Value> value, HTMLSelectElement* base) {

brace on new line, and to prefix instead of get. Use "get" when the result is a ptr argument. http://webkit.org/coding/coding-style.html

I wonder though if this method really belongs in V8Collection.h (perhaps need a V8Collection.cpp added?)
  

> Property changes on: WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.h
> ___________________________________________________________________
> Name: svn:executable
>    + *
> 

Whoops! What are these props doing here?

> +NAMED_PROPERTY_GETTER(HTMLSelectElementCollection) {

brace on new line.

> +    // Search local callback properties next to find IDL defined  properties.
> +    if (info.Holder()->HasRealNamedCallbackProperty(name))
> +            return notHandledByInterceptor();

Something funny with indent here. 4 spaces.

> +    if (!items.size())
> +        return v8::Handle<v8::Value>();

return notHandledByInterceptor();

> +INDEXED_PROPERTY_SETTER(HTMLSelectElementCollection) {

brace on new line.

> +    return WebCore::getOptionsCollectionSetter(index, value, select);

No need for namespace prefix here. We're in WebCore.

> Property changes on: WebCore/bindings/v8/custom/V8HTMLSelectElementCollectionCustom.cpp
> ___________________________________________________________________
> Name: svn:executable
>    + *
> 

No extra props, pls :)
Comment 3 Nate Chapin 2009-05-13 14:18:55 PDT
Created attachment 30286 [details]
patch2
Comment 4 David Levin 2009-05-13 14:59:15 PDT
Minor comments:

custom/V8HTMLOptionsCollectionCustom.h

   v8::Handle<v8::Value> getOptionsCollectionSetter(uint32_t index, v8::Handle<v8::Value> value, HTMLSelectElement* element);

* value and element should be removed since they don't add any info.


custom/V8HTMLSelectElementCollectionCustom.cpp
 
* Two blank lines after the namespace start.
* Extra space after defined in this comment
"Search local callback properties next to find IDL defined  properties."

Comment 5 Dimitri Glazkov (Google) 2009-05-13 15:41:28 PDT
Comment on attachment 30286 [details]
patch2

Want a new patch :)
Comment 6 Nate Chapin 2009-05-13 15:46:08 PDT
Created attachment 30291 [details]
patch3

Sorry for the delay...
Comment 7 Dimitri Glazkov (Google) 2009-05-13 15:58:06 PDT
Comment on attachment 30291 [details]
patch3

Don't rush :)

> +        * bindings/v8/custom/V8ElementCustom.cpp:
> +        (WebCore::ACCESSOR_SETTER): Moved from v8_custom.cpp.

That doesn't sound right. We haven't moved anything like this here.

> -#include "HTMLOptionsCollection.h"
> +#include "V8HTMLOptionsCollectionCustom.h"

be careful here. There is no V8HTMLOptionsCollectionCustom in this CL.
  
> +INDEXED_PROPERTY_SETTER(HTMLOptionsCollection)
> +{
> +    INC_STATS("DOM.HTMLOptionsCollection.IndexedPropertySetter");
> +    HTMLOptionsCollection* collection = V8Proxy::ToNativeObject<HTMLOptionsCollection>(V8ClassIndex::HTMLOPTIONSCOLLECTION, info.Holder());
> +    HTMLSelectElement* base = static_cast<HTMLSelectElement*>(collection->base());
> +    return getOptionsCollectionSetter(index, value, base);

toOptions...

> +    HTMLSelectElement* select = V8Proxy::DOMWrapperToNode<HTMLSelectElement>(info.Holder());
> +    return getOptionsCollectionSetter(index, value, select);

toOptions ..
Comment 8 Nate Chapin 2009-05-13 17:23:50 PDT
Created attachment 30303 [details]
patch4
Comment 9 Eric Seidel (no email) 2009-05-14 22:00:11 PDT
Comment on attachment 30303 [details]
patch4

Looks sane enough to me, but Dimitri is really your man here.
Comment 10 Dimitri Glazkov (Google) 2009-05-15 09:14:53 PDT
Comment on attachment 30303 [details]
patch4

Looking good!
Comment 11 Dimitri Glazkov (Google) 2009-05-15 15:28:30 PDT
Landed as http://trac.webkit.org/changeset/43787.