Summary: | Microdata: document.getItems(typeNames) is not returning Microdata items when typeNames argument is not specified | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Arko Saha <arko> | ||||||||
Component: | DOM | Assignee: | Arko Saha <arko> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 68609 | ||||||||||
Attachments: |
|
Description
Arko Saha
2012-06-22 06:15:24 PDT
Created attachment 149014 [details]
Patch
Comment on attachment 149014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149014&action=review > Source/WebCore/dom/MicroDataItemList.cpp:62 > - if (!m_typeNames.size()) > + if (m_originalTypeNames == "http://webkit.org/microdata/undefinedItemType") Can we define this url in some static function of MicroDataItemList (using DEFINE_STATIC_LOCAL) so that we don't have to do String conversion here? Created attachment 149062 [details]
Updated patch
Comment on attachment 149062 [details]
Updated patch
Do we have a regression test that covers this? If not, can we add a new test that shows the bug?
Comment on attachment 149062 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=149062&action=review > Source/WebCore/ChangeLog:9 > + With r120979 change, it creates MicroDataItemList with m_typeNames = "http://webkit.org/microdata/undefinedItemType", > + when typeNames argument is not specified. Modified the check in nodeMatches() accordingly. Change log should list the regression tests that are failing to make it clear this is covered by tests. > Source/WebCore/ChangeLog:16 > + (WebCore::undefinedItemType): > + (WebCore): > + (WebCore::MicroDataItemList::MicroDataItemList): > + (WebCore::MicroDataItemList::~MicroDataItemList): > + (WebCore::MicroDataItemList::nodeMatches): Patches with per-function comments are better. Bogus lines like (WebCore): should be deleted before landing a change. > Source/WebCore/dom/MicroDataItemList.cpp:41 > +static String& undefinedItemType() Return type should be const String& because we don’t want the caller to be able to modify it. > Source/WebCore/dom/MicroDataItemList.cpp:49 > - , m_typeNames(typeNames, node()->document()->inQuirksMode()) > + , m_typeNames(typeNames, document()->inQuirksMode()) Seems like unrelated cleanup. > Source/WebCore/dom/MicroDataItemList.cpp:56 > - String localTypeNames = m_originalTypeNames.isNull() ? String("http://webkit.org/microdata/undefinedItemType") : m_originalTypeNames; > - m_node->nodeLists()->removeCacheWithName(this, DynamicNodeList::MicroDataItemListType, localTypeNames); > + ownerNode()->nodeLists()->removeCacheWithName(this, DynamicNodeList::MicroDataItemListType, m_originalTypeNames); Using ownerNode() instead of m_node seems like unrelated cleanup. Comment on attachment 149062 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=149062&action=review > Source/WebCore/ChangeLog:3 > + Microdata: document.getItems(typeNames) is not returning Microdata items when typeNames aurgument is not specified. Typo here: aurgument (In reply to comment #5) Thanks Darin for the review. > > Source/WebCore/ChangeLog:9 > > + With r120979 change, it creates MicroDataItemList with m_typeNames = "http://webkit.org/microdata/undefinedItemType", > > + when typeNames argument is not specified. Modified the check in nodeMatches() accordingly. > > Change log should list the regression tests that are failing to make it clear this is covered by tests. Ok. I will update the Changelog with failing test cases. > > Source/WebCore/ChangeLog:16 > > + (WebCore::undefinedItemType): > > + (WebCore): > > + (WebCore::MicroDataItemList::MicroDataItemList): > > + (WebCore::MicroDataItemList::~MicroDataItemList): > > + (WebCore::MicroDataItemList::nodeMatches): > > Patches with per-function comments are better. Bogus lines like (WebCore): should be deleted before landing a change. Ok. > > Source/WebCore/dom/MicroDataItemList.cpp:41 > > +static String& undefinedItemType() > > Return type should be const String& because we don’t want the caller to be able to modify it. Yes you are right. Will modify. > > Source/WebCore/dom/MicroDataItemList.cpp:49 > > - , m_typeNames(typeNames, node()->document()->inQuirksMode()) > > + , m_typeNames(typeNames, document()->inQuirksMode()) > > Seems like unrelated cleanup. r121003 revision added document() method which returns the document of owner node. > > Source/WebCore/dom/MicroDataItemList.cpp:56 > > - String localTypeNames = m_originalTypeNames.isNull() ? String("http://webkit.org/microdata/undefinedItemType") : m_originalTypeNames; > > - m_node->nodeLists()->removeCacheWithName(this, DynamicNodeList::MicroDataItemListType, localTypeNames); > > + ownerNode()->nodeLists()->removeCacheWithName(this, DynamicNodeList::MicroDataItemListType, m_originalTypeNames); > > Using ownerNode() instead of m_node seems like unrelated cleanup. r121003 revision renamed DynamicNodeList's m_node to m_ownerNode. So we need to use ownerNode() as m_ownerNode private to DynamicNodeList. Made changes accordingly. Created attachment 149078 [details]
Updated patch
Incorporated review comments.
Comment on attachment 149078 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=149078&action=review > Source/WebCore/dom/MicroDataItemList.cpp:41 > +static const String& undefinedItemType() We should also use this function in Document::getItems. Committed r121149: <http://trac.webkit.org/changeset/121149> |