Bug 110367

Created attachment 189376 [details] proposed patch

Attachment 189376 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/MicroData/names-property-add-remove-itemref-expected.txt', u'LayoutTests/fast/dom/MicroData/names-property-add-remove-itemref.html', u'LayoutTests/fast/dom/MicroData/names-property-add-remove-itemscope-expected.txt', u'LayoutTests/fast/dom/MicroData/names-property-add-remove-itemscope.html', u'LayoutTests/fast/dom/MicroData/names-property-must-be-live-expected.txt', u'LayoutTests/fast/dom/MicroData/names-property-must-be-live.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/dom/DOMStringList.h', u'Source/WebCore/dom/DOMStringList.idl', u'Source/WebCore/dom/MicroDataPropertyNames.cpp', u'Source/WebCore/dom/MicroDataPropertyNames.h', u'Source/WebCore/html/HTMLPropertiesCollection.cpp', u'Source/WebCore/html/HTMLPropertiesCollection.h']" exit_code: 1 Source/WebCore/dom/DOMStringList.h:53: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 189376 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=189376&action=review >> Source/WebCore/dom/DOMStringList.h:53 >> + virtual size_t length() const { return m_strings.size(); } >> + virtual String item(unsigned index) const; >> + virtual bool contains(const String& str) const; > > The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] I'm worried that this will regress performance. Can we avoid inheriting from DOMStringList in C++ side (we should to continue to inherit on IDL side)? > Source/WebCore/dom/MicroDataPropertyNames.cpp:76 > +HTMLElement* MicroDataPropertyNames::itemAfter(HTMLElement* rootNode, Element* previous) const It seems like all of this code is duplicated from HTMLPropertiesCollection?

(In reply to comment #3) > I'm worried that this will regress performance. Can we avoid inheriting from DOMStringList in C++ side (we should to continue to inherit on IDL side)? > I am not sure if we can avoid inheriting from DOMStringList in C++ side. Can you please guide me how to do this by inheriting on IDL side? > > Source/WebCore/dom/MicroDataPropertyNames.cpp:76 > > +HTMLElement* MicroDataPropertyNames::itemAfter(HTMLElement* rootNode, Element* previous) const > > It seems like all of this code is duplicated from HTMLPropertiesCollection? Yes, I have moved virtualItemAfter(), updateNameCache() and nextNodeWithProperty() methods from HTMLPropertiesCollection to MicroDataPropertyNames. Also renamed virtualItemAfter() to itemAfter().

> Can we avoid inheriting from DOMStringList in C++ side (we should to continue to inherit on IDL side)? haraken do you have any thoughts how this can be achieved?

(In reply to comment #5) > > Can we avoid inheriting from DOMStringList in C++ side (we should to continue to inherit on IDL side)? > > haraken do you have any thoughts how this can be achieved? Do you really need to inherit DOMStringList from MicroDataPropertyNames? For example, wouldn't it be possible to avoid the inheritance like this? class MicroDataPropertyNames { size_t length() { return stringList->length(); } OwnPtr<DOMStringList> stringList; };

(In reply to comment #5) > > Can we avoid inheriting from DOMStringList in C++ side (we should to continue to inherit on IDL side)? > > haraken do you have any thoughts how this can be achieved? I don't think you need to do anything special. Just not inherit in C++ but still inherit in IDL. There's nothing that mandates C++/IDL inherence trees to match.

(In reply to comment #7) > > I don't think you need to do anything special. Just not inherit in C++ but still inherit in IDL. There's nothing that mandates C++/IDL inherence trees to match. I tried to inherit only in IDL side. Below JS binding code is generated in JSMicroDataPropertyNames.cpp : JSValue jsMicroDataPropertyNamesLength(ExecState* exec, JSValue slotBase, PropertyName) { JSMicroDataPropertyNames* castedThis = jsCast<JSMicroDataPropertyNames*>(asObject(slotBase)); UNUSED_PARAM(exec); MicroDataPropertyNames* impl = static_cast<MicroDataPropertyNames*>(castedThis->impl()); JSValue result = jsNumber(impl->length()); return result; } Here it is giving compilation error like : DerivedSources/WebCore/JSMicroDataPropertyNames.cpp:181:91: error: invalid static_cast from type 'WebCore::DOMStringList*' to type 'WebCore::MicroDataPropertyNames*' As MicroDataPropertyNames is not inherited from DOMStringList in C++ side. Please note that castedThis->impl() returns DOMStringList in this case. JSMicroDataPropertyNames::JSMicroDataPropertyNames(Structure* structure, JSDOMGlobalObject* globalObject, PassRefPtr<MicroDataPropertyNames> impl) : JSDOMStringList(structure, globalObject, impl) { } This causes below error as JSDOMStringList takes PassRefPtr<DOMStringList> as third parameter JSDOMStringList(JSC::Structure*, JSDOMGlobalObject*, PassRefPtr<DOMStringList>); ../../Source/WTF/wtf/PassRefPtr.h: In instantiation of 'WTF::PassRefPtr<T>::PassRefPtr(const WTF::PassRefPtr<U>&) [with U = WebCore::MicroDataPropertyNames; T = WebCore::DOMStringList]': DerivedSources/WebCore/JSMicroDataPropertyNames.cpp:113:52: required from here ../../Source/WTF/wtf/PassRefPtr.h:66:84: error: cannot convert 'WebCore::MicroDataPropertyNames*' to 'WebCore::DOMStringList*' in initialization

Also tried with "JSGenerateToNativeObject" IDL attribute which generates to below JS binding code: MicroDataPropertyNames* impl() const { return static_cast<MicroDataPropertyNames*>(Base::impl()); } Still same error: invalid static_cast from type 'WebCore::DOMStringList*' to type 'WebCore::MicroDataPropertyNames*' Can you please point me out if I am missing something?

Comment on attachment 189376 [details] proposed patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

MicroData was removed here: https://github.com/WebKit/WebKit/commit/cd0400986f42a280613126c60e2ffe028ae6c7aa Do we need this? Thanks!