[Webkit-unassigned] [Bug 88837] [Chromium] Add WebDocument.images

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88837


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #147184|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #4 from Adam Barth <abarth at webkit.org>  2012-06-12 19:43:40 PST ---
(From update of attachment 147184)
View in context: https://bugs.webkit.org/attachment.cgi?id=147184&action=review

A few minor nits below.  The main thing to improve about this patch is the ChangeLog.  The ChangeLog should explain why we're making this change.  In this case, I suspect the reason is that this API is needed by Chrome on Android for some purpose.  It would be nice to elaborate a bit about that purpose, if you're able to.

In the particular case of this patch, it's not a big deal because we're mirroring a long-established DOM API, but it's still a good practice when contributing to WebKit.

Thanks for the patch!

> Source/WebKit/chromium/src/WebDocument.cpp:142
> +void WebDocument::images(WebVector<WebElement>& results) const

Why "const"?  You're ending up const_casting the Document anyway.  It seems better to model this after body() and head() and skip the const.

> Source/WebKit/chromium/src/WebDocument.cpp:150
> +        // Strange but true, sometimes node can be 0.

I'd skip this comment.  WebKit usually only includes comments if they something about why the code is the way it is.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.