Bug 68610

Summary: Microdata: Basic implementation of document.getItems() method
Product: WebKit Reporter: Arko Saha <arko>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dbates, dglazkov, donggwan.kim, ian, joepeck, philipj, rniwa, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 68609, 69839    
Attachments:
Description Flags
Patch
none
Updated patch
rniwa: review-
Adding layout test
none
Updated patch
none
Updated patch
rniwa: review-
Updated patch
rniwa: review+
Updated patch rniwa: review+, rniwa: commit-queue-

Description Arko Saha 2011-09-22 04:33:42 PDT
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.
Comment 1 Arko Saha 2011-09-22 04:45:26 PDT
Created attachment 108313 [details]
Patch
Comment 2 Arko Saha 2011-09-23 03:39:50 PDT
Created attachment 108457 [details]
Updated patch
Comment 3 Arko Saha 2011-09-26 05:49:36 PDT
Created attachment 108656 [details]
Adding layout test

Layout test for Microdata document.getItems() DOM API.
Comment 4 Ryosuke Niwa 2011-09-26 11:17:35 PDT
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.
Comment 5 Arko Saha 2011-09-28 00:14:11 PDT
(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.
Comment 6 Ryosuke Niwa 2011-09-28 00:17:29 PDT
(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?
Comment 7 Arko Saha 2011-09-28 00:18:40 PDT
Created attachment 108977 [details]
Updated patch
Comment 8 Arko Saha 2011-09-28 00:33:55 PDT
(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.
Comment 9 Ryosuke Niwa 2011-09-28 10:29:35 PDT
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.
Comment 10 Ian 'Hixie' Hickson 2011-09-28 13:54:38 PDT
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.)
Comment 11 Arko Saha 2011-10-10 04:15:33 PDT
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.
Comment 12 Ryosuke Niwa 2011-10-11 11:23:02 PDT
(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.
Comment 13 Ryosuke Niwa 2011-10-11 12:05:38 PDT
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.
Comment 14 Ryosuke Niwa 2011-10-11 12:06:46 PDT
(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.
Comment 15 Sam Weinig 2011-10-11 13:34:16 PDT
(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.
Comment 16 Ryosuke Niwa 2011-10-11 13:39:14 PDT
(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!
Comment 17 Arko Saha 2011-10-12 00:44:38 PDT
(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?
Comment 18 Ryosuke Niwa 2011-10-12 00:51:48 PDT
(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.
Comment 19 Arko Saha 2011-10-12 08:18:58 PDT
Created attachment 110685 [details]
Updated patch
Comment 20 Ryosuke Niwa 2011-10-12 15:34:40 PDT
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.
Comment 21 Ian 'Hixie' Hickson 2011-10-12 15:37:00 PDT
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.
Comment 22 Ian 'Hixie' Hickson 2011-10-12 15:40:38 PDT
s/not/now/, sorry!
Comment 23 Ryosuke Niwa 2011-10-12 15:44:20 PDT
(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.
Comment 24 Ryosuke Niwa 2011-10-12 15:44:42 PDT
We can probably land this patch and follow up in a separate patch.
Comment 25 Arko Saha 2011-10-13 01:35:01 PDT
(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!
Comment 26 Arko Saha 2011-10-13 01:36:36 PDT
(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.
Comment 27 Philip Jägenstedt 2011-10-13 01:42:35 PDT
Points of possible interest:

http://krijnhoetmer.nl/irc-logs/whatwg/20111013#l-938
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14444
Comment 28 Arko Saha 2011-10-13 01:49:14 PDT
Created attachment 110812 [details]
Updated patch
Comment 29 Arko Saha 2011-10-13 02:09:16 PDT
Please can anyone help me to add the Microdata feature flag i.e, ENABLE(MICRODATA) and new files MicroDataItemList.cpp/h to xcode project?
Comment 30 Ryosuke Niwa 2011-10-13 02:14:05 PDT
(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.
Comment 31 Arko Saha 2011-10-13 02:19:52 PDT
(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. :)
Comment 32 Ryosuke Niwa 2011-10-14 09:12:02 PDT
Committed r97471: <http://trac.webkit.org/changeset/97471>
Comment 33 Ryosuke Niwa 2011-10-14 09:45:33 PDT
Skipped tests on Mac and WinCairo in http://trac.webkit.org/changeset/97472.