Summary: | Need function to copy favicon data without image conversion. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gordon Sheridan <gordon_sheridan> | ||||||||
Component: | WebKit2 | Assignee: | Gordon Sheridan <gordon_sheridan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Gordon Sheridan
2014-05-11 13:01:52 PDT
Created attachment 231264 [details]
Patch
Comment on attachment 231264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231264&action=review > Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:77 > + WebCore::Image *image = toImpl(iconDatabaseRef)->imageForPageURL(toWTFString(pageURL)); Formatted wrong. Should be Image* rather than Image * with a space. > Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:79 > + return 0; Should be nullptr. > Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:81 > + WebCore::SharedBuffer *iconData = image->data(); Formatted wrong. Should be SharedBuffer* rather than SharedBuffer * with a space. Thanks Darin! I'll update it and post a new patch. Created attachment 231265 [details]
Patch
Comment on attachment 231265 [details]
Patch
I need to update the patch handle when there is no image data.
Created attachment 231268 [details]
Patch
Comment on attachment 231268 [details] Patch Clearing flags on attachment: 231268 Committed r168609: <http://trac.webkit.org/changeset/168609> All reviewed patches have been landed. Closing bug. Comment on attachment 231268 [details]
Patch
There is one thing wrong with this patch that I did not notice until after it was landed.
Generally, API files are not supposed to have substantive code in them. They just have bindings.
So this patch should have added a copyIconDataForPageURL member function to WebIconDatabase, rather than putting the code into the bindings file itself. Then the bindings file would just have the binding.
I can submit a new patch. Can this bug be reopened for that, or should I create a new bug? (In reply to comment #10) > I can submit a new patch. Can this bug be reopened for that, or should I create a new bug? I took care of it and did some other icon database improvements. It’s in bug 132812. |