[webkit-reviews] review granted: [Bug 88837] [Chromium] Add WebDocument.images : [Attachment 147184] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Jesse Greenwald
<jgreenwald at chromium.org>'s request for review:
Bug 88837: [Chromium] Add WebDocument.images
https://bugs.webkit.org/show_bug.cgi?id=88837

Attachment 147184: Patch
https://bugs.webkit.org/attachment.cgi?id=147184&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.