Bug 68610 – Microdata: Basic implementation of document.getItems() method

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Arko Saha]

Basic implementation of document.getItems() method:

1. Add Microdata compilation guards for Linux, Chromium and Windows.
2. Implement itemId, itemScope, itemType idl attributes.
3. Implement document.getItems([types]) method.

[Arko Saha]

Created attachment 108313 [details]
Patch

[Arko Saha]

Created attachment 108457 [details]
Updated patch

[Arko Saha]

Created attachment 108656 [details]
Adding layout test

Layout test for Microdata document.getItems() DOM API.

[Ryosuke Niwa]

Comment on attachment 108457 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108457&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

Clearly, we need a test for this feature.

> Source/WebCore/dom/ItemNodeList.h:37
> +class ItemNodeList : public DynamicNodeList {

I'm not convinced that ItemNodeList is a good name for this class. It sounds too generic.

Now about MicroDataItemList?

> Source/WebCore/dom/Node.cpp:1137
> +void Node::removeCachedItemNodeList(ItemNodeList* list, const String& typeName)

I'd like this function name to include microData somewhere.

> Source/WebCore/dom/Node.cpp:1733
> +PassRefPtr<NodeList> Node::getItems(const String& typeNames)

Why should this function live in Node instead of Document?

> Source/WebCore/html/HTMLElement.cpp:986
> +    dispatchSubtreeModifiedEvent();

I don't think we should be dispatching subtree modified here. As far as I understand it, this function is called in parseMappedAttribute, which means nothing in the DOM tree has changed. r- because of this.

[Arko Saha]

(In reply to comment #4)
Thanks for the review.

> Clearly, we need a test for this feature.

Done! Combined test-cases and code changes in a single patch.

> I'm not convinced that ItemNodeList is a good name for this class. It sounds too generic.
> 
> Now about MicroDataItemList?

Yes you are right ItemNodeList is generic class name, Changed the class name to MicroDataItemList.

> I'd like this function name to include microData somewhere.

Done! removeCachedItemNodeList() medthod renamed to removeCachedMicroDataItemList().

> Why should this function live in Node instead of Document?

I have placed getItems() method in Document so that they can use the caching mechanism already used by getElementsByClassName and getElementsByName. getElementsByClassName and getElementsByName were moved to Node some time back (https://bugs.webkit.org/show_bug.cgi?id=16511).

> I don't think we should be dispatching subtree modified here. As far as I understand it, this function is called in parseMappedAttribute, which means nothing in the DOM tree has changed. r- because of this.

Agreed! Modified the code so that it will only invalidate the MicroDataItemList cache while itemType attribte changed by JavaScript.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > Why should this function live in Node instead of Document?
> 
> I have placed getItems() method in Document so that they can use the caching mechanism already used by getElementsByClassName and getElementsByName. getElementsByClassName and getElementsByName were moved to Node some time back (https://bugs.webkit.org/show_bug.cgi?id=16511).

But Document inherits from Node?

[Arko Saha]

Created attachment 108977 [details]
Updated patch

[Arko Saha]

(In reply to comment #6)
> 
> But Document inherits from Node?

Yes, you are right. But there were some reasons mentioned in bug (16511) because of which these functions were moved to Node. I have kept it in node to keep it inline with the existing code and also use the same caching mechanism. Please let me know your suggestions.

[Ryosuke Niwa]

Could you tell us which spec you're trying to implement?  Is it http://www.w3.org/TR/microdata/ ?

You should also add the link to change log.

[Ian 'Hixie' Hickson]

That spec is obsolete. You should reference:
http://www.whatwg.org/specs/web-apps/current-work/complete/microdata.html

(Microdata is just a part of HTML.)

[Arko Saha]

Created attachment 110347 [details]
Updated patch

Added specification URL "http://www.whatwg.org/specs/web-apps/current-work/complete/microdata.html" in the change log.

[Ryosuke Niwa]

(In reply to comment #8)
> (In reply to comment #6)
> > 
> > But Document inherits from Node?
> 
> Yes, you are right. But there were some reasons mentioned in bug (16511) because of which these functions were moved to Node. I have kept it in node to keep it inline with the existing code and also use the same caching mechanism. Please let me know your suggestions.

I don't think that comment is applicable here. Please move it to Document.

[Ryosuke Niwa]

Comment on attachment 110347 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110347&action=review

> Source/WebCore/dom/MicroDataItemList.cpp:58
> +    if (m_typeNames.contains("undefined") || !m_typeNames.size())

Are you trying to detect a case where the script didn't pass type? I don't think comparing it to the string 'undefined' is the right approach.

> Source/WebCore/dom/MicroDataItemList.h:49
> +    virtual bool nodeMatches(Element*) const;

Please use OVERRIDE macro.

> Source/WebCore/dom/NodeRareData.h:68
> +#if ENABLE(MICRODATA)
> +    typedef HashMap<String, MicroDataItemList*> MicroDataItemListCache;
> +    MicroDataItemListCache m_microDataItemListCache;
> +#endif

You also need to modify NodeListsNodeData::isEmpty.

> Source/WebCore/html/HTMLElement.cpp:201
> +#if ENABLE(MICRODATA)
> +    } else if (attr->name() == itemtypeAttr) {
> +        itemTypeAttributeChanged();
> +#endif

Please modify invalidateCachesThatDependOnAttributes instead. m_classNodeListCache is a good one to mimic. r- because of this.

[Ryosuke Niwa]

(In reply to comment #13)
> (From update of attachment 110347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110347&action=review
> 
> > Source/WebCore/dom/MicroDataItemList.cpp:58
> > +    if (m_typeNames.contains("undefined") || !m_typeNames.size())
> 
> Are you trying to detect a case where the script didn't pass type? I don't think comparing it to the string 'undefined' is the right approach.

Sam, do you know a better way of doing this? We have a function that takes DOMString and we want to detect a case where the author omitted the argument.

[Sam Weinig]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 110347 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=110347&action=review
> > 
> > > Source/WebCore/dom/MicroDataItemList.cpp:58
> > > +    if (m_typeNames.contains("undefined") || !m_typeNames.size())
> > 
> > Are you trying to detect a case where the script didn't pass type? I don't think comparing it to the string 'undefined' is the right approach.
> 
> Sam, do you know a better way of doing this? We have a function that takes DOMString and we want to detect a case where the author omitted the argument.

We usually handle this by adding a modifier to the string argument in the IDL which converts undefined to the null string [ConvertUndefinedOrNullToNullString].  If you need a different semantic, you either need to use [Optional] or [Custom].  Using the string "undefined" is clearly wrong, as you could pass that string.

[Ryosuke Niwa]

(In reply to comment #15)
> We usually handle this by adding a modifier to the string argument in the IDL which converts undefined to the null string [ConvertUndefinedOrNullToNullString].  If you need a different semantic, you either need to use [Optional] or [Custom].  Using the string "undefined" is clearly wrong, as you could pass that string.

Yes! ConvertUndefinedOrNullToNullString is what I was looking for. Thanks!

[Arko Saha]

(In reply to comment #13)
> Are you trying to detect a case where the script didn't pass type? I don't think comparing it to the string 'undefined' is the right approach.

Ok, we will use ConvertUndefinedOrNullToNullString which converts undefined to null string.
 
> Please use OVERRIDE macro.
> 
I am not aware of this macro. Could you please help me to understand this.

> You also need to modify NodeListsNodeData::isEmpty.
> 
Ok, we will modify this.

> Please modify invalidateCachesThatDependOnAttributes instead. m_classNodeListCache is a good one to mimic. r- because of this.

In case of class attribute when classAttr changes it calls classAttributeChanged() from StyledElement::parseMappedAttribute().
Now classAttributeChanged() invokes dispatchSubtreeModifiedEvent() which internally calls invalidateCachesThatDependOnAttributes() to invalidate m_classNodeListCache.

Earlier I did the same for itemtype attribute change. We were calling dispatchSubtreeModifiedEvent() from itemTypeAttributeChanged()
which internally invoked invalidateCachesThatDependOnAttributes() to invalidate m_microDataItemListCache.

Now we modified the code so that it will only invalidate the m_microDataItemListCache when itemType attribute changes, it should not send the dispatchSubtreeModifiedEvent() as you suggested.
Do we need to follow the same as earlier case?

[Ryosuke Niwa]

(In reply to comment #17)
> > Please modify invalidateCachesThatDependOnAttributes instead. m_classNodeListCache is a good one to mimic. r- because of this.
> 
> In case of class attribute when classAttr changes it calls classAttributeChanged() from StyledElement::parseMappedAttribute().
> Now classAttributeChanged() invokes dispatchSubtreeModifiedEvent() which internally calls invalidateCachesThatDependOnAttributes() to invalidate m_classNodeListCache.

I wasn't aware of this but that sounds wrong. I don't think we should be dispatching SubtreeModified when attributes change like that. Thanks for the clarification!

> Now we modified the code so that it will only invalidate the m_microDataItemListCache when itemType attribute changes, it should not send the dispatchSubtreeModifiedEvent() as you suggested.
> Do we need to follow the same as earlier case?

Probably not. I think your current approach is fine. We should probably fix other implementations not to fire SubtreeModified when attributes change.

[Arko Saha]

Created attachment 110685 [details]
Updated patch

[Ryosuke Niwa]

Comment on attachment 110685 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110685&action=review

I know there's some discussion about whether we should prefix this API but that can be added later if we strongly feel about. r=me provided you change UndefinedMicroDataItemType to some unique url under webkit.org.

> Source/WebCore/dom/Document.cpp:5212
> +    // Since documet.getItem() is allowed for microdata, typeNames will be null string.
> +    // In this case we need to create an unique string identifier to map such request in the cache.
> +    String localTypeNames = typeNames.isNull() ? String("UndefinedMicroDataItemType") : typeNames;

I don't think UndefinedMicroDataItemType is unique enough. Authors can certainly define UndefinedMicroDataItemType as a type, right?  If anything, I'd suggest to use http://webkit.org/microdata/undefined since type can be an url.

> Source/WebCore/dom/Node.cpp:2289
> +    // We need to invalidate the microDataItemListCache when itemtype attribute changed

This comment repeats what the code says. Please remove.

[Ian 'Hixie' Hickson]

Apologies for the poor timing, but FYI, the spec just changed so that itemtype="" is not a space-separated list of types rather than a single type. I hope that doesn't cause too many difficulties.

[Ian 'Hixie' Hickson]

s/not/now/, sorry!

[Ryosuke Niwa]

(In reply to comment #21)
> Apologies for the poor timing, but FYI, the spec just changed so that itemtype="" is now a space-separated list of types rather than a single type. I hope that doesn't cause too many difficulties.

That sounds like a easy change to adopt. We just need to update nodeMatches.

[Ryosuke Niwa]

We can probably land this patch and follow up in a separate patch.

[Arko Saha]

(In reply to comment #20)
> 
> I don't think UndefinedMicroDataItemType is unique enough. Authors can certainly define UndefinedMicroDataItemType as a type, right?  If anything, I'd suggest to use http://webkit.org/microdata/undefined since type can be an url.
>
Done!
 
> > Source/WebCore/dom/Node.cpp:2289
> > +    // We need to invalidate the microDataItemListCache when itemtype attribute changed
> 
> This comment repeats what the code says. Please remove.
Done!

[Arko Saha]

(In reply to comment #23)
> (In reply to comment #21)
> > Apologies for the poor timing, but FYI, the spec just changed so that itemtype="" is now a space-separated list of types rather than a single type. I hope that doesn't cause too many difficulties.
> 
> That sounds like a easy change to adopt. We just need to update nodeMatches.
I will log a separate bug for this issue.

[Philip Jägenstedt]

Points of possible interest:

http://krijnhoetmer.nl/irc-logs/whatwg/20111013#l-938
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14444

[Arko Saha]

Created attachment 110812 [details]
Updated patch

[Arko Saha]

Please can anyone help me to add the Microdata feature flag i.e, ENABLE(MICRODATA) and new files MicroDataItemList.cpp/h to xcode project?

[Ryosuke Niwa]

(In reply to comment #29)
> Please can anyone help me to add the Microdata feature flag i.e, ENABLE(MICRODATA) and new files MicroDataItemList.cpp/h to xcode project?

I'll do that and land it manually.

[Arko Saha]

(In reply to comment #30)
> (In reply to comment #29)
> > Please can anyone help me to add the Microdata feature flag i.e, ENABLE(MICRODATA) and new files MicroDataItemList.cpp/h to xcode project?
> 
> I'll do that and land it manually.

Thanks Ryosuke for the help. :)

[Ryosuke Niwa]

Committed r97471: <http://trac.webkit.org/changeset/97471>

[Ryosuke Niwa]

Skipped tests on Mac and WinCairo in http://trac.webkit.org/changeset/97472.