WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89757
Microdata: document.getItems(typeNames) is not returning Microdata items when typeNames argument is not specified
https://bugs.webkit.org/show_bug.cgi?id=89757
Summary
Microdata: document.getItems(typeNames) is not returning Microdata items when...
Arko Saha
Reported
2012-06-22 06:15:24 PDT
document.getItems(typeNames) should return all Microdata top level items if there are no typeNames specified in the aurgument i.e, document.getItems(). Failing test cases: fast/dom/MicroData/007.html fast/dom/MicroData/009.html fast/dom/MicroData/002.html fast/dom/MicroData/003.html fast/dom/MicroData/properties-collection-must-see-the-properties-added-in-itemref.html In case of documet.getItem() typeNames will be null string. Earlier we used to create MicroDataItemList with m_typeNames = null string. And we used to create an unique string identifier (
http://webkit.org/microdata/undefinedItemType
) to map such request in the cache. With
r120979
this change it creates MicroDataItemList with m_typeNames = "
http://webkit.org/microdata/undefinedItemType
", when typeNames argument is not specified. So we need to modify the check in nodeMatches() accordingly.
Attachments
Patch
(2.50 KB, patch)
2012-06-22 06:23 PDT
,
Arko Saha
no flags
Details
Formatted Diff
Diff
Updated patch
(2.65 KB, patch)
2012-06-22 10:54 PDT
,
Arko Saha
darin
: review+
Details
Formatted Diff
Diff
Updated patch
(3.27 KB, patch)
2012-06-22 11:55 PDT
,
Arko Saha
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Arko Saha
Comment 1
2012-06-22 06:23:48 PDT
Created
attachment 149014
[details]
Patch
Ryosuke Niwa
Comment 2
2012-06-22 09:11:35 PDT
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?
Arko Saha
Comment 3
2012-06-22 10:54:11 PDT
Created
attachment 149062
[details]
Updated patch
Darin Adler
Comment 4
2012-06-22 10:55:49 PDT
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?
Darin Adler
Comment 5
2012-06-22 10:58:30 PDT
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.
Darin Adler
Comment 6
2012-06-22 10:58:48 PDT
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
Arko Saha
Comment 7
2012-06-22 11:21:10 PDT
(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.
Arko Saha
Comment 8
2012-06-22 11:55:05 PDT
Created
attachment 149078
[details]
Updated patch Incorporated review comments.
Ryosuke Niwa
Comment 9
2012-06-24 23:22:07 PDT
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.
Arko Saha
Comment 10
2012-06-25 05:12:44 PDT
Committed
r121149
: <
http://trac.webkit.org/changeset/121149
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug