Bug 132805 - Need function to copy favicon data without image conversion.
Summary: Need function to copy favicon data without image conversion.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Gordon Sheridan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-11 13:01 PDT by Gordon Sheridan
Modified: 2014-05-12 00:53 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.01 KB, patch)
2014-05-11 13:25 PDT, Gordon Sheridan
no flags Details | Formatted Diff | Diff
Patch (3.01 KB, patch)
2014-05-11 13:50 PDT, Gordon Sheridan
no flags Details | Formatted Diff | Diff
Patch (3.06 KB, patch)
2014-05-11 15:27 PDT, Gordon Sheridan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gordon Sheridan 2014-05-11 13:01:52 PDT
With WKIconDatabaseSetIconDataForIconURL(), we have a function that can *set* the data for an icon in the destination database.

In order to copy a favicon between icon databases, without potential image conversion, we need a function that can get a copy of the data for an icon in the source database.
Comment 1 Gordon Sheridan 2014-05-11 13:25:01 PDT
Created attachment 231264 [details]
Patch
Comment 2 Darin Adler 2014-05-11 13:29:16 PDT
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.
Comment 3 Gordon Sheridan 2014-05-11 13:43:05 PDT
Thanks Darin!

I'll update it and post a new patch.
Comment 4 Gordon Sheridan 2014-05-11 13:50:07 PDT
Created attachment 231265 [details]
Patch
Comment 5 Gordon Sheridan 2014-05-11 15:00:43 PDT
Comment on attachment 231265 [details]
Patch

I need to update the patch handle when there is no image data.
Comment 6 Gordon Sheridan 2014-05-11 15:27:20 PDT
Created attachment 231268 [details]
Patch
Comment 7 WebKit Commit Bot 2014-05-11 23:31:09 PDT
Comment on attachment 231268 [details]
Patch

Clearing flags on attachment: 231268

Committed r168609: <http://trac.webkit.org/changeset/168609>
Comment 8 WebKit Commit Bot 2014-05-11 23:31:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2014-05-11 23:39:24 PDT
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.
Comment 10 Gordon Sheridan 2014-05-11 23:41:25 PDT
I can submit a new patch. Can this bug be reopened for that, or should I create a new bug?
Comment 11 Darin Adler 2014-05-12 00:53:06 PDT
(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.