Created attachment 147184 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

(In reply to comment #0) > This is an API needed for Chrome. Why?

Comment on attachment 147184 [details] Patch 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.

Comment on attachment 147184 [details] Patch I'll expand on the ChangeLog when I upload the next patch. Thanks for taking a look! View in context: https://bugs.webkit.org/attachment.cgi?id=147184&action=review >> 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. I originally modeled this after forms(), which is const. Actually, isn't body() const as well? Either way, I can remove const from this method. >> 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. Ok. For reference, the comment was copied from forms().

Yeah, you're discovering that WebDocument.cpp was written a long time ago and we've learned a bunch since then. :)

Created attachment 147374 [details] Patch

Comment on attachment 147374 [details] Patch Thanks!

Comment on attachment 147374 [details] Patch Clearing flags on attachment: 147374 Committed r120284: <http://trac.webkit.org/changeset/120284>

All reviewed patches have been landed. Closing bug.