Bug 25739 – Upstream V8 bindings for HTMLOptionsCollection and HTMLSelectElementCollection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

See summary.

[Nate Chapin]

Created attachment 30252 [details]
patch

[Dimitri Glazkov (Google)]

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 :)

[Nate Chapin]

Created attachment 30286 [details]
patch2

[David Levin]

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."

[Dimitri Glazkov (Google)]

Comment on attachment 30286 [details]
patch2

Want a new patch :)

[Nate Chapin]

Created attachment 30291 [details]
patch3

Sorry for the delay...

[Dimitri Glazkov (Google)]

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 ..

[Nate Chapin]

Created attachment 30303 [details]
patch4

[Eric Seidel (no email)]

Comment on attachment 30303 [details]
patch4

Looks sane enough to me, but Dimitri is really your man here.

[Dimitri Glazkov (Google)]

Comment on attachment 30303 [details]
patch4

Looking good!

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/43787.