Bug 92933 – [chromium] Add API to make it possible to request several variants of a WebImage

• Component: New Bugs
• Status: RESOLVED FIXED

[Nico Weber]

[chromium] Add API to make it possible to request several variants of a WebImage

[Nico Weber]

Created attachment 155948 [details]
Patch

[Nico Weber]

Comment on attachment 155948 [details]
Patch

just wip

[Nico Weber]

Created attachment 156181 [details]
Patch

[Nico Weber]

Created attachment 156463 [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.

[Adam Barth]

Comment on attachment 156463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156463&action=review

> Source/WebKit/chromium/src/WebImageSkia.cpp:92
> +    // This is to protect from malicious favicons. It should be big enough that it's never hit in pracice.
> +    const size_t kMaxFaviconCount = 8;

As written, this function doesn't have anything to do with favicons.  Maybe we should use a different name for this constant?  Also, WebKit doesn't use the k prefix.

> Source/WebKit/chromium/src/WebImageSkia.cpp:100
> +    // Frames are arranged by decreasing size, then decreasing bit depth.
> +    // Keep the first frame at every size, has the highest bit depth.

Is this true of things other than favicons?  I would imagine that a GIF might not follow this rule.

[Adam Barth]

Is this function specific to favicons?  If so, we should put that in the name somewhere.

[Nico Weber]

Thanks!

(In reply to comment #6)
> (From update of attachment 156463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156463&action=review
> 
> > Source/WebKit/chromium/src/WebImageSkia.cpp:92
> > +    // This is to protect from malicious favicons. It should be big enough that it's never hit in pracice.
> > +    const size_t kMaxFaviconCount = 8;
> 
> As written, this function doesn't have anything to do with favicons.  Maybe we should use a different name for this constant?  Also, WebKit doesn't use the k prefix.

Change "favicon" to "frame".

WebKit/Source/WTF/wtf/ThreadSpecific.h and a few other places do seem to use it. But constantNamesLikeThis seem most common, so changed to that.

> > Source/WebKit/chromium/src/WebImageSkia.cpp:100
> > +    // Frames are arranged by decreasing size, then decreasing bit depth.
> > +    // Keep the first frame at every size, has the highest bit depth.
> 
> Is this true of things other than favicons?  I would imagine that a GIF might not follow this rule.

WebImage::fromData() above has the same comment.


In practice, these functions are only used for favicon-like functionality at the moment, but they aren't inherently favicon-specific.

The APIs can't be used to get all frames from a gif, but since that's not needed anywhere at the moment I think that's fine?

[Nico Weber]

Created attachment 156466 [details]
Patch

[Adam Barth]

Comment on attachment 156466 [details]
Patch

ok

[Darin Fisher (:fishd, Google)]

Comment on attachment 156466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156466&action=review

> Source/WebKit/chromium/src/WebImageSkia.cpp:119
> +    return result;

nit: you should just be able to write "return frames;" here.  It should auto-convert to WebVector for you.  See the WebVector(const C&) constructor.

[Nico Weber]

Created attachment 156476 [details]
Patch for landing

[Nico Weber]

> nit: you should just be able to write "return frames;" here.  It should auto-convert to WebVector for you.  See the WebVector(const C&) constructor.

Neat! Done.

[WebKit Review Bot]

Comment on attachment 156476 [details]
Patch for landing

Clearing flags on attachment: 156476

Committed r124688: <http://trac.webkit.org/changeset/124688>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.