Bug 132709 – Expose functions necessary for copying favicon between IconDatabases.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Gordon Sheridan]

In order to copy a favicon between icon databases, we need to surface WebIconDatabase::setIconURLForPageURL() and WebIconDatabase::synchronousIconURLForPageURL().

[Gordon Sheridan]

Created attachment 231104 [details]
Patch

[Jessie Berlin]

<rdar://problem/16138065>

[Jessie Berlin]

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.

[Gordon Sheridan]

Created attachment 231128 [details]
Patch

[Gordon Sheridan]

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.

[Brady Eidson]

Comment on attachment 231128 [details]
Patch

These names are fine.

[WebKit Commit Bot]

Comment on attachment 231128 [details]
Patch

Clearing flags on attachment: 231128

Committed r168553: <http://trac.webkit.org/changeset/168553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.