Bug 160682

Summary: getElementsByTagName() should take a qualifiedName in parameter
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, dbates, esprehn+autocc, gyuyoung.kim, kangil.han, rniwa, sam
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164705    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-08-08 19:30:11 PDT 
getElementsByTagName() should take a qualifiedName in parameter, not a localName:
- https://dom.spec.whatwg.org/#dom-document-getelementsbytagname
- https://dom.spec.whatwg.org/#concept-getelementsbytagname
Comment 1 Chris Dumez 2016-08-11 22:07:22 PDT 
Created attachment 285894 [details]
WIP Patch
Comment 2 Chris Dumez 2016-08-11 22:29:41 PDT 
Created attachment 285896 [details]
WIP Patch
Comment 3 Chris Dumez 2016-08-11 22:31:26 PDT 
Created attachment 285897 [details]
WIP Patch
Comment 4 Chris Dumez 2016-08-12 08:45:29 PDT 
Created attachment 285913 [details]
Patch
Comment 5 Chris Dumez 2016-08-12 08:46:00 PDT 
This passes all the tests. I'll evaluate performance on Speedometer and Dromaeo today.
Comment 6 Chris Dumez 2016-08-12 10:03:33 PDT 
(In reply to comment #5)
> This passes all the tests. I'll evaluate performance on Speedometer and
> Dromaeo today.

Seems to be perf-neutral on Dromaeo DOM core. However, it looks like this is a 1-3% regression on Speedometer. I'll see what I can do to avoid regressing Speedometer.
Comment 7 Chris Dumez 2016-08-12 10:56:40 PDT 
Created attachment 285920 [details]
Patch
Comment 8 Chris Dumez 2016-08-12 10:57:16 PDT 
Ok, this now seems to be perf-neutral on Speedometer as well. I'll closely monitor the perf bots when it lands though.
Comment 9 Chris Dumez 2016-08-12 15:55:59 PDT 
Relevant spec discussion:
https://github.com/whatwg/dom/issues/143
Comment 10 Ryosuke Niwa 2016-08-12 18:38:45 PDT 
Comment on attachment 285920 [details]
Patch

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

> Source/WebCore/dom/AllDescendantsCollection.h:32
> +// Collection that matches all element descendants.

I don't think this comment is necessary given the code below that says:
CachedHTMLCollection<AllDescendantsCollection,

> Source/WebCore/dom/AllDescendantsCollection.h:33
> +class AllDescendantsCollection final : public CachedHTMLCollection<AllDescendantsCollection, CollectionTypeTraits<AllDescendants>::traversalType> {

Can we use this in HTMLAllCollection as well?
Comment 11 Chris Dumez 2016-08-12 19:21:11 PDT 
Comment on attachment 285920 [details]
Patch

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

>> Source/WebCore/dom/AllDescendantsCollection.h:33
>> +class AllDescendantsCollection final : public CachedHTMLCollection<AllDescendantsCollection, CollectionTypeTraits<AllDescendants>::traversalType> {
> 
> Can we use this in HTMLAllCollection as well?

Do you mean have HTMLAllCollection subclass AllDescendantsCollection to avoid duplicating the elementMatches() implementation?
Comment 12 Chris Dumez 2016-08-12 20:23:22 PDT 
Created attachment 286000 [details]
Patch
Comment 13 WebKit Commit Bot 2016-08-12 21:22:02 PDT 
Comment on attachment 286000 [details]
Patch

Clearing flags on attachment: 286000

Committed r204441: <http://trac.webkit.org/changeset/204441>
Comment 14 WebKit Commit Bot 2016-08-12 21:22:08 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Chris Dumez 2016-08-13 15:42:46 PDT 
I monitored the perf bots and did not see any obvious regression from this change.