Bug 88837 – [Chromium] Add WebDocument.images

• Component: New Bugs
• Status: RESOLVED FIXED

[Jesse Greenwald]

This is an API needed for Chrome.

[Jesse Greenwald]

Created attachment 147184 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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

Why?

[Adam Barth]

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.

[Jesse Greenwald]

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().

[Adam Barth]

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

[Jesse Greenwald]

Created attachment 147374 [details]
Patch

[Adam Barth]

Comment on attachment 147374 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 147374 [details]
Patch

Clearing flags on attachment: 147374

Committed r120284: <http://trac.webkit.org/changeset/120284>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.