Bug 68610

Created attachment 108313 [details] Patch

Created attachment 108457 [details] Updated patch

Created attachment 108656 [details] Adding layout test Layout test for Microdata document.getItems() DOM API.

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.

(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.

(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?

Created attachment 108977 [details] Updated patch

(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.

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.

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.)

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.

(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 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.

(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.

(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.

(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!

(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?

(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.

Created attachment 110685 [details] Updated patch

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.

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.

s/not/now/, sorry!

(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.

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

(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!

(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.

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

Created attachment 110812 [details] Updated patch

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

(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.

(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. :)

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

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