Bug 35272 - [Chromium] Implement WebDocument::getElementsByTagName
Summary: [Chromium] Implement WebDocument::getElementsByTagName
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-22 16:44 PST by James Hawkins
Modified: 2010-02-23 14:17 PST (History)
5 users (show)

See Also:

Attachments
[Chromium] Implement WebDocument::getElementsByTagName (1.93 KB, patch)
2010-02-22 16:45 PST, James Hawkins 		no flags Details | Formatted Diff | Diff
[Chromium] Implement WebDocument::getElementsByTagName [try2] (2.11 KB, patch)
2010-02-22 16:53 PST, James Hawkins 		fishd: review-
Details | Formatted Diff | Diff
[Chromium] Implement WebNode::getElementsByTagName [try2] (1.69 KB, patch)
2010-02-23 11:02 PST, James Hawkins 		no flags Details | Formatted Diff | Diff
[Chromium] Implement WebDocument::getElementsByTagName [try3] (2.11 KB, patch)
2010-02-23 13:36 PST, James Hawkins 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description James Hawkins 2010-02-22 16:44:18 PST 
Patch will follow.
Comment 1 James Hawkins 2010-02-22 16:45:29 PST 
Created attachment 49247 [details]
[Chromium] Implement WebDocument::getElementsByTagName
Comment 2 WebKit Review Bot 2010-02-22 16:51:55 PST 
Attachment 49247 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/299294
Comment 3 James Hawkins 2010-02-22 16:53:12 PST 
Created attachment 49248 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try2]

Added missing include.
Comment 4 Darin Fisher (:fishd, Google) 2010-02-22 22:12:40 PST 
Comment on attachment 49248 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try2]

> Index: WebKit/chromium/public/WebDocument.h
...
>      WEBKIT_API WebElement getElementById(const WebString& id) const;
> +    WEBKIT_API WebNodeList getElementsByTagName(const WebString&);

hmm, it seems like getElementsByTagName (like getElementById) should be
a const method since it does not mutate WebDocument.
Comment 5 James Hawkins 2010-02-23 10:52:06 PST 
(In reply to comment #4)
> (From update of attachment 49248 [details])
> > Index: WebKit/chromium/public/WebDocument.h
> ...
> >      WEBKIT_API WebElement getElementById(const WebString& id) const;
> > +    WEBKIT_API WebNodeList getElementsByTagName(const WebString&);
> 
> hmm, it seems like getElementsByTagName (like getElementById) should be
> a const method since it does not mutate WebDocument.

More importantly, getElementsByTagName belongs in WebNode, not WebDocument.  The original problem remains though: I can't make WebNode::getElementsByTagName const because WebCore::Node::getElementsByTagName is non-const.  The Node method caches the NodeList in the document for future reference.  Patch will follow.
Comment 6 James Hawkins 2010-02-23 11:02:09 PST 
Created attachment 49305 [details]
[Chromium] Implement WebNode::getElementsByTagName [try2]
Comment 7 Darin Fisher (:fishd, Google) 2010-02-23 13:35:26 PST 
Ah, yeah... it does return a live node list.  OK.  Anyways, the constness stuff is a little wacky since we are not returning a list of const WebNodes.  One thing, I think you should keep getElementsByTagName on WebDocument to align with Document.idl.  I'd consider it an implementation detail of WebCore that getElementsByTagName happens to be defined on class Node.
Comment 8 James Hawkins 2010-02-23 13:36:34 PST 
Created attachment 49323 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try3]

Moved the implementation back to WebDocument.  This is a copy of attachment 49248 [details].  That comment about non-const still applies.
Comment 9 WebKit Commit Bot 2010-02-23 14:17:07 PST 
Comment on attachment 49323 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try3]

Clearing flags on attachment: 49323

Committed r55171: <http://trac.webkit.org/changeset/55171>
Comment 10 WebKit Commit Bot 2010-02-23 14:17:13 PST 
All reviewed patches have been landed.  Closing bug.