Bug 74768

Summary: Cache and reuse the HTMLAllCollection returned by document.all.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, koivisto, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
kling: review-
Patch v2 koivisto: review+

Description Andreas Kling 2011-12-16 16:41:50 PST
document.all should behave like the other collections on document (multiple calls return the same objects.)
Comment 1 Andreas Kling 2011-12-16 16:50:24 PST
Created attachment 119699 [details]
Patch
Comment 2 Andreas Kling 2011-12-16 19:30:38 PST
Comment on attachment 119699 [details]
Patch

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

> Source/WebCore/html/HTMLCollection.cpp:70
> -    if (m_baseIsRetained)
> +    if (!m_base->isDocumentNode())

Gnarf! This isn't correct, isDocumentNode() will be false after ~Document() runs (if the HTMLCollection is being destroyed after the Document removed the last ref.)
Comment 3 Andreas Kling 2011-12-17 09:06:28 PST
Created attachment 119726 [details]
Patch v2

Keep the m_baseIsRetained logic in HTMLCollection, since we can't depend on calling Node::isDocumentNode() after ~Document().
m_baseIsRetained can go away by making all HTMLCollections cached instead, since the collection will then never hold a ref on its base node.
Comment 4 Antti Koivisto 2011-12-17 18:45:16 PST
Comment on attachment 119726 [details]
Patch v2

r=me

The ChangeLog could use a sentence or two about why this is a good thing. Bonus points for some performance measurements.
Comment 5 Andreas Kling 2011-12-17 19:58:05 PST
Committed r103166: <http://trac.webkit.org/changeset/103166>