[chromium] Add API to make it possible to request several variants of a WebImage
Created attachment 155948 [details] Patch
Comment on attachment 155948 [details] Patch just wip
Created attachment 156181 [details] Patch
Created attachment 156463 [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.
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.
Is this function specific to favicons? If so, we should put that in the name somewhere.
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?
Created attachment 156466 [details] Patch
Comment on attachment 156466 [details] Patch ok
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.
Created attachment 156476 [details] Patch for landing
> 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.
Comment on attachment 156476 [details] Patch for landing Clearing flags on attachment: 156476 Committed r124688: <http://trac.webkit.org/changeset/124688>
All reviewed patches have been landed. Closing bug.