Bug 107680 - [EFL][WK2] Use C API inside ewk_favicon_database
Summary: [EFL][WK2] Use C API inside 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 107666
  Show dependency treegraph
 
Reported: 2013-01-23 05:35 PST by Chris Dumez
Modified: 2013-01-24 10:04 PST (History)
11 users (show)

See Also:


Attachments
Patch (12.21 KB, patch)
2013-01-23 06:23 PST, Chris Dumez
beidson: review-
Details | Formatted Diff | Diff
Patch (9.66 KB, patch)
2013-01-24 02:47 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-23 05:35:15 PST
As per Bug 107657, we should start using the C API internally to avoid violating layering.
Comment 1 Chris Dumez 2013-01-23 06:23:20 PST
Created attachment 184216 [details]
Patch
Comment 2 Alexey Proskuryakov 2013-01-23 10:40:42 PST
Comment on attachment 184216 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:77
> +    toImpl(iconDatabaseRef)->synchronousIconURLForPageURL(toWTFString(pageURL), iconURL);

I don't know if this should be exposed via an API.
Comment 3 Brady Eidson 2013-01-23 11:07:02 PST
Comment on attachment 184216 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:77
>> +    toImpl(iconDatabaseRef)->synchronousIconURLForPageURL(toWTFString(pageURL), iconURL);
> 
> I don't know if this should be exposed via an API.

It should not be.  Can you explain more why you need it?

> Source/WebKit2/UIProcess/API/C/WKIconDatabase.cpp:85
> +bool WKIconDatabaseIsURLImportCompleted(WKIconDatabaseRef iconDatabaseRef)
> +{
> +    return toImpl(iconDatabaseRef)->isUrlImportCompleted();
> +}

The "url import" is an *extremely* fragile implementation detail of the icon database and has no business being exposed as API.

Could you share details why you think you need it?
Comment 4 Brady Eidson 2013-01-23 11:07:49 PST
(In reply to comment #0)
> As per Bug 107657, we should start using the C API internally to avoid violating layering.

The solution to solving layering problems is not to expose more internals as API.
Comment 5 Chris Dumez 2013-01-24 02:47:12 PST
Created attachment 184451 [details]
Patch

Do not add C API anymore. Further refactoring will be done in a later patch to stop relying on the few C++ methods that we currently use and have no C API alternative.
Comment 6 Brady Eidson 2013-01-24 08:51:55 PST
Comment on attachment 184451 [details]
Patch

Looking briefly, I definitely have no reason to r- this patch.

Since it's entirely contained within EFL, I'll let an EFL reviewer take a look.
Comment 7 Kenneth Rohde Christiansen 2013-01-24 08:57:41 PST
Comment on attachment 184451 [details]
Patch

OK, I am OK with this.
Comment 8 WebKit Review Bot 2013-01-24 09:10:36 PST
Comment on attachment 184451 [details]
Patch

Rejecting attachment 184451 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 184451, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
-queue

Parsed 4 diffs from patch file(s).
patch: **** Can't create file /tmp/pptQe127 : No space left on device
patch: **** Can't create file /tmp/ppIK0MM8 : No space left on device
patch: **** Can't create file /tmp/ppTdTd08 : No space left on device
patch: **** Can't create file /tmp/pp8tZQ8b : No space left on device

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kenneth Rohde Christiansen']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16073864
Comment 9 WebKit Review Bot 2013-01-24 10:01:24 PST
Comment on attachment 184451 [details]
Patch

Rejecting attachment 184451 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 184451, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
-queue

Parsed 4 diffs from patch file(s).
patch: **** Can't create file /tmp/ppMW9zDm : No space left on device
patch: **** Can't create file /tmp/ppdP3Ahm : No space left on device
patch: **** Can't create file /tmp/ppcKpp5m : No space left on device
patch: **** Can't create file /tmp/ppPJ0nln : No space left on device

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kenneth Rohde Christiansen']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16078798
Comment 10 Chris Dumez 2013-01-24 10:03:37 PST
Committed r140692: <http://trac.webkit.org/changeset/140692>