Bug 132709 - Expose functions necessary for copying favicon between IconDatabases.
Summary: Expose functions necessary for copying favicon between IconDatabases.
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Gordon Sheridan
Keywords: InRadar
Depends on:
Reported: 2014-05-08 15:09 PDT by Gordon Sheridan
Modified: 2014-05-14 11:01 PDT (History)
5 users (show)

See Also:

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

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]
Comment 2 Jessie Berlin 2014-05-08 18:55:41 PDT
Comment 3 Jessie Berlin 2014-05-08 20:17:58 PDT
Comment on attachment 231104 [details]

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]
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]

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

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.