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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 12 19:43:41 PDT 2012


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.

More information about the webkit-unassigned mailing list