Bug 108035 - [EFL][WK2] Rely more on C API in ewk_favicon_database
Summary: [EFL][WK2] Rely more on C API in ewk_favicon_database
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 107657
  Show dependency treegraph
 
Reported: 2013-01-27 11:02 PST by Chris Dumez
Modified: 2013-01-28 01:04 PST (History)
9 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2013-01-27 11:15 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.83 KB, patch)
2013-01-27 11:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-01-27 11:02:01 PST
As per Bug 107657, we should start using the C API internally to avoid violating layering. ewk_favicon_database still has a few calls to the internal C++ API.
Comment 1 Chris Dumez 2013-01-27 11:15:37 PST
Created attachment 184917 [details]
Patch
Comment 2 Chris Dumez 2013-01-27 11:24:21 PST
Created attachment 184918 [details]
Patch
Comment 3 Benjamin Poulain 2013-01-27 16:15:00 PST
Comment on attachment 184918 [details]
Patch

I am not entirely convinced this is necessary.

For port specific API, there is no need to force a C API between public APIs and Proxys (in my opinion). If GTK were to use the new, WKIconDatabaseTryGetCairoSurfaceForURL, then this would be helping consolidate WebKit2.

I r+ because the patch seems correct and you handle your port abstractions as you see fit.
Comment 4 Chris Dumez 2013-01-27 23:20:36 PST
Comment on attachment 184918 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/cairo/WKIconDatabaseCairo.h:34
> +WK_EXPORT cairo_surface_t* WKIconDatabaseTryGetCairoSurfaceForURL(WKIconDatabaseRef iconDatabase, WKURLRef url);

Kenneth, I'm having doubts about this API now. Should we have an API that returns an Evas Object instead of a cairo surface? This makes more sense for EFL port and we really shouldn't expose cairo surfaces to applications.
Comment 5 Kenneth Rohde Christiansen 2013-01-28 00:32:25 PST
Comment on attachment 184918 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/cairo/WKIconDatabaseCairo.h:34
>> +WK_EXPORT cairo_surface_t* WKIconDatabaseTryGetCairoSurfaceForURL(WKIconDatabaseRef iconDatabase, WKURLRef url);
> 
> Kenneth, I'm having doubts about this API now. Should we have an API that returns an Evas Object instead of a cairo surface? This makes more sense for EFL port and we really shouldn't expose cairo surfaces to applications.

In the EFL API it might make more sense to return an Evas Object, but our C API is slightly lower level so returning a cairo_surface_t seems fine, especially for special browser cases or so where the browser/client might want to store the images locally or so.
Comment 6 Chris Dumez 2013-01-28 00:35:43 PST
(In reply to comment #5)
> (From update of attachment 184918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184918&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/cairo/WKIconDatabaseCairo.h:34
> >> +WK_EXPORT cairo_surface_t* WKIconDatabaseTryGetCairoSurfaceForURL(WKIconDatabaseRef iconDatabase, WKURLRef url);
> > 
> > Kenneth, I'm having doubts about this API now. Should we have an API that returns an Evas Object instead of a cairo surface? This makes more sense for EFL port and we really shouldn't expose cairo surfaces to applications.
> 
> In the EFL API it might make more sense to return an Evas Object, but our C API is slightly lower level so returning a cairo_surface_t seems fine, especially for special browser cases or so where the browser/client might want to store the images locally or so.

Ok, thanks for confirming. Let's land this then.
Comment 7 WebKit Review Bot 2013-01-28 01:04:03 PST
Comment on attachment 184918 [details]
Patch

Clearing flags on attachment: 184918

Committed r140952: <http://trac.webkit.org/changeset/140952>
Comment 8 WebKit Review Bot 2013-01-28 01:04:09 PST
All reviewed patches have been landed.  Closing bug.