Bug 132709 - Expose functions necessary for copying favicon between IconDatabases.
Summary: Expose functions necessary for copying favicon between IconDatabases.
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: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-08 15:09 PDT by Gordon Sheridan
Modified: 2014-05-14 11:01 PDT (History)
5 users (show)

See Also:

Attachments
Patch (3.29 KB, patch)
2014-05-08 15:29 PDT, Gordon Sheridan 		no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2014-05-08 21:02 PDT, Gordon Sheridan 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gordon Sheridan 2014-05-08 15:09:02 PDT 
In order to copy a favicon between icon databases, we need to surface WebIconDatabase::setIconURLForPageURL() and WebIconDatabase::synchronousIconURLForPageURL().
Comment 1 Gordon Sheridan 2014-05-08 15:29:05 PDT 
Created attachment 231104 [details]
Patch
Comment 2 Jessie Berlin 2014-05-08 18:55:41 PDT 
<rdar://problem/16138065>
Comment 3 Jessie Berlin 2014-05-08 20:17:58 PDT 
Comment on attachment 231104 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:65
> +WKURLRef WKIconDatabaseSynchronousIconURLForPageURL(WKIconDatabaseRef iconDatabaseRef, WKURLRef pageURLRef)

This should be something like WKIconDatabaseSynchronousCopyIconURLForPageURL since it returns a copy.

> Source/WebKit2/UIProcess/API/C/WKIconDatabase.h:84
> +WK_EXPORT void WKIconDatabaseSetIconURLForPageURL(WKIconDatabaseRef iconDatabaseRef, WKURLRef iconURLRef, WKURLRef pageURLRef);

Is there a reason you called out "ForPageURL"? The other functions are just "ForURL".

> Source/WebKit2/UIProcess/API/C/WKIconDatabase.h:85
> +WK_EXPORT WKURLRef WKIconDatabaseSynchronousIconURLForPageURL(WKIconDatabaseRef iconDatabaseRef, WKURLRef pageURLRef);

Is it important and/or right for it to have "Synchronous" in the title? It seems like that should be more of a comment, or just entirely be implied by the fact that it returns the WKURLRef. I don't see any of our other WK2 C API doing this.
Comment 4 Gordon Sheridan 2014-05-08 21:02:40 PDT 
Created attachment 231128 [details]
Patch
Comment 5 Gordon Sheridan 2014-05-08 21:10:36 PDT 
Jessie, I updated the patch to incorporate your suggestion to replace 'Synchronous' with 'Copy' in WKIconDatabaseSynchronousIconURLForPageURL().

I'm keeping the 'PageURL' portion instead of condensing it to just 'URL' because there are two URLs in these parameter lists, it matches the underlying method names these are wrapping, and there seemed to be a precedent of using 'PageURL' in two of the IconDatabase client callbacks (WKIconDatabaseDidChangeIconForPageURLCallback and WKIconDatabaseIconDataReadyForPageURLCallback).

That said, I'm fine with changing them to whatever the WebKit folks want.
Comment 6 Brady Eidson 2014-05-09 13:49:40 PDT 
Comment on attachment 231128 [details]
Patch

These names are fine.
Comment 7 WebKit Commit Bot 2014-05-09 14:20:41 PDT 
Comment on attachment 231128 [details]
Patch

Clearing flags on attachment: 231128

Committed r168553: <http://trac.webkit.org/changeset/168553>
Comment 8 WebKit Commit Bot 2014-05-09 14:20:43 PDT 
All reviewed patches have been landed.  Closing bug.