Bug 107680

Summary: [EFL][WK2] Use C API inside ewk_favicon_database
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, beidson, benjamin, gyuyoung.kim, kenneth, lucas.de.marchi, mikhail.pozdnyakov, rakuco, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107657, 107666    
Attachments:
Description Flags
Patch
beidson: review-
Patch none

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>