Bug 63491 - [EFL] Remove IconDatabase initialization in _ewk_init_body()
Summary: [EFL] Remove IconDatabase initialization in _ewk_init_body()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-27 15:57 PDT by Jaehun Lim
Modified: 2011-07-14 23:07 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (2.04 KB, patch)
2011-06-27 16:22 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Change API comments (2.47 KB, patch)
2011-06-27 17:35 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Change patch description. (2.53 KB, patch)
2011-06-27 18:07 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Fix comments (2.53 KB, patch)
2011-06-28 15:41 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Modified patch (2.56 KB, patch)
2011-06-29 15:59 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaehun Lim 2011-06-27 15:57:57 PDT
IconDatabase is an optional feature. But EFL port enables it in _ewk_init_body() as default.

So, when an application enables IconDatabse after ewk_init(), WebKit prints an warning message.
(See another bugs, 60361, 60148, 59238)

I think it's better that an application should decide whether to use IconDatabase or not,
and be responsible for using IconDatabase.
Comment 1 Jaehun Lim 2011-06-27 16:22:01 PDT
Created attachment 98811 [details]
Proposed patch
Comment 2 Ryuan Choi 2011-06-27 16:29:50 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=98811&action=review

> Source/WebKit/efl/ewk/ewk_settings.cpp:156
>      if (directory) {
> +        if (WebCore::iconDatabase().isEnabled()) {
> +            ERR("IconDatabase is enabled already: %s", _ewk_icon_database_path);
> +            return EINA_FALSE;
> +        }

If then, can we close iconDatabase?
Does we need to update comments?
Comment 3 Jaehun Lim 2011-06-27 17:07:10 PDT
(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=98811&action=review
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:156
> >      if (directory) {
> > +        if (WebCore::iconDatabase().isEnabled()) {
> > +            ERR("IconDatabase is enabled already: %s", _ewk_icon_database_path);
> > +            return EINA_FALSE;
> > +        }
> 
> If then, can we close iconDatabase?
> Does we need to update comments?

If there is no directory value, we can close.
Comment 4 Gyuyoung Kim 2011-06-27 17:32:21 PDT
(In reply to comment #0)
> IconDatabase is an optional feature. But EFL port enables it in _ewk_init_body() as default.
> 
> So, when an application enables IconDatabse after ewk_init(), WebKit prints an warning message.
> (See another bugs, 60361, 60148, 59238)
> 
> I think it's better that an application should decide whether to use IconDatabase or not,
> and be responsible for using IconDatabase.

Then, when we start to open IconDatabase ?
Comment 5 Jaehun Lim 2011-06-27 17:35:02 PDT
Created attachment 98827 [details]
Change API comments
Comment 6 Jaehun Lim 2011-06-27 17:41:17 PDT
(In reply to comment #4)
> (In reply to comment #0)
> > IconDatabase is an optional feature. But EFL port enables it in _ewk_init_body() as default.
> > 
> > So, when an application enables IconDatabse after ewk_init(), WebKit prints an warning message.
> > (See another bugs, 60361, 60148, 59238)
> > 
> > I think it's better that an application should decide whether to use IconDatabase or not,
> > and be responsible for using IconDatabase.
> 
> Then, when we start to open IconDatabase ?

After this patch, icon database must be fully managed by an application.

If an application doesn't want to use icon database, nothing to do for it.

But if another application wants to use favicon feature, just call ewk_settings_icon_database_path_set() with directory path.
Comment 7 Jaehun Lim 2011-06-27 17:41:50 PDT
(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=98811&action=review
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:156
> >      if (directory) {
> > +        if (WebCore::iconDatabase().isEnabled()) {
> > +            ERR("IconDatabase is enabled already: %s", _ewk_icon_database_path);
> > +            return EINA_FALSE;
> > +        }
> 
> If then, can we close iconDatabase?
> Does we need to update comments?

I chnaged API's comments.
Comment 8 Jaehun Lim 2011-06-27 17:58:30 PDT
After discussion with ryuan, we concludes that icondatabase APIs need to be changed. When I prepare, I'll make another bug.
This patch is the first step.
Comment 9 Lucas De Marchi 2011-06-27 17:59:19 PDT
Comment on attachment 98811 [details]
Proposed patch

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

> Source/WebKit/efl/ChangeLog:11
> +2011-06-27  Jaehun Lim  <ljaehun.lim@samsung.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [EFL] Remove IconDatabase initialization in _ewk_init_body()
> +        https://bugs.webkit.org/show_bug.cgi?id=63491
> +
> +        IconDatabase is an optional feature. But EFL port enables it in _ewk_init_body()
> +        as default.
> +        This patch removes IconDatabase enabling while ewk initialzation and
> +        prevents an application from re-opening IconDatabase.

I think a better description would be:

Make IconDatabase feature optional by removing its initialization from _ewk_init_body(). Now IconDatabase must be fully managed by an application: if it doesn't want to use it, there's nothing to do. Otherwise, just call ewk_settings_icon_database_path_set() with the directory path.

> Source/WebKit/efl/ewk/ewk_main.cpp:-201
>          ewk_settings_web_database_path_set(wkdir.utf8().data());
> -        ewk_settings_icon_database_path_set(wkdir.utf8().data());

What do you think about taking the same approach for web database?
Comment 10 Jaehun Lim 2011-06-27 18:07:00 PDT
Created attachment 98835 [details]
Change patch description.
Comment 11 Jaehun Lim 2011-06-27 18:20:57 PDT
(In reply to comment #9)
> (From update of attachment 98811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98811&action=review
> 
> > Source/WebKit/efl/ChangeLog:11
> > +2011-06-27  Jaehun Lim  <ljaehun.lim@samsung.com>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        [EFL] Remove IconDatabase initialization in _ewk_init_body()
> > +        https://bugs.webkit.org/show_bug.cgi?id=63491
> > +
> > +        IconDatabase is an optional feature. But EFL port enables it in _ewk_init_body()
> > +        as default.
> > +        This patch removes IconDatabase enabling while ewk initialzation and
> > +        prevents an application from re-opening IconDatabase.
> 
> I think a better description would be:
> 
> Make IconDatabase feature optional by removing its initialization from _ewk_init_body(). Now IconDatabase must be fully managed by an application: if it doesn't want to use it, there's nothing to do. Otherwise, just call ewk_settings_icon_database_path_set() with the directory path.

I changed, thanks.

> 
> > Source/WebKit/efl/ewk/ewk_main.cpp:-201
> >          ewk_settings_web_database_path_set(wkdir.utf8().data());
> > -        ewk_settings_icon_database_path_set(wkdir.utf8().data());
> 
> What do you think about taking the same approach for web database?

I made this patch because of IconDatabase's re-opening problem. I didn't care about web database.
At a glance, it looks no problem to take the same approach. After I analyze more, I'll try to do
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-06-28 05:40:09 PDT
> Source/WebKit/efl/ewk/ewk_settings.cpp:144
> + * database was opened already, this funciton returns EINA_FALSE.

was opened already -> is already open
funciton -> function
EINA_FALSE -> @c EINA_FALSE

> Source/WebKit/efl/ewk/ewk_settings.cpp:157
> +            ERR("IconDatabase is enabled already: %s", _ewk_icon_database_path);

enabled already -> already enabled
Comment 13 Jaehun Lim 2011-06-28 15:41:26 PDT
Created attachment 98984 [details]
Fix comments

Fix comments. Thanks.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-06-29 06:10:16 PDT
Almost there :)

> Source/WebKit/efl/ewk/ewk_settings.cpp:143
> + * Icon database must be opened only once. If you try to set a path when icon

when icon -> when the icon

> Source/WebKit/efl/ewk/ewk_settings.cpp:144
> + * database is already opend, this function returns @c EINA_FALSE.

opend -> open

> Source/WebKit/efl/ewk/ewk_settings.cpp:157
> +            ERR("IconDatabase is already opened: %s", _ewk_icon_database_path);

opened -> open
Comment 15 Jaehun Lim 2011-06-29 15:59:48 PDT
Created attachment 99167 [details]
Modified patch

Fix it. Thanks again.
Comment 16 Raphael Kubo da Costa (:rakuco) 2011-07-04 07:20:07 PDT
LGTM.
Comment 17 Jaehun Lim 2011-07-14 21:53:53 PDT
Could anyone review this patch?
Comment 18 WebKit Review Bot 2011-07-14 23:07:41 PDT
Comment on attachment 99167 [details]
Modified patch

Clearing flags on attachment: 99167

Committed r91051: <http://trac.webkit.org/changeset/91051>
Comment 19 WebKit Review Bot 2011-07-14 23:07:47 PDT
All reviewed patches have been landed.  Closing bug.