Bug 110367 - [Microdata] HTMLPropertiesCollection's names attribute must be live
Summary: [Microdata] HTMLPropertiesCollection's names attribute must be live
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arko Saha
URL:
Keywords:
Depends on:
Blocks: 92986
  Show dependency treegraph
 
Reported: 2013-02-20 12:36 PST by Arko Saha
Modified: 2014-02-05 11:11 PST (History)
10 users (show)

See Also:


Attachments
proposed patch (32.81 KB, patch)
2013-02-20 14:20 PST, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 2013-02-20 12:36:38 PST
The names attribute of HTMLPropertiesCollection must be live

Test:

<div itemscope>
<span itemprop="foo"></span>
</div>
<script>
var item = document.getItems()[0];
var names = item.properties.names;
item.children[0].itemProp.add("bar");
document.write(names.length == 2 ? "PASS" : "FAIL");
</script>

Expected result: PASS
Actual result: FAIL as names.length returns 1
Comment 1 Arko Saha 2013-02-20 14:20:15 PST
Created attachment 189376 [details]
proposed patch
Comment 2 WebKit Review Bot 2013-02-20 14:24:03 PST
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 3 Ryosuke Niwa 2013-02-20 14:29:58 PST
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?
Comment 4 Arko Saha 2013-02-20 14:41:41 PST
(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().
Comment 5 Arko Saha 2013-02-21 11:30:03 PST
> 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?
Comment 6 Kentaro Hara 2013-02-21 11:38:40 PST
(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;
};
Comment 7 Ryosuke Niwa 2013-02-21 16:48:09 PST
(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.
Comment 8 Arko Saha 2013-02-21 23:02:00 PST
(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
Comment 9 Arko Saha 2013-02-21 23:10:35 PST
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 10 Andreas Kling 2014-02-05 11:11:42 PST
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.