See summary.
Created attachment 30252 [details] patch
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 :)
Created attachment 30286 [details] patch2
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 on attachment 30286 [details] patch2 Want a new patch :)
Created attachment 30291 [details] patch3 Sorry for the delay...
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 ..
Created attachment 30303 [details] patch4
Comment on attachment 30303 [details] patch4 Looks sane enough to me, but Dimitri is really your man here.
Comment on attachment 30303 [details] patch4 Looking good!
Landed as http://trac.webkit.org/changeset/43787.