[webkit-reviews] review granted: [Bug 88837] [Chromium] Add WebDocument.images : [Attachment 147184] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 12 19:43:40 PDT 2012
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.
More information about the webkit-reviews
mailing list