Bug 63491 – [EFL] Remove IconDatabase initialization in _ewk_init_body()

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jaehun Lim]

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.

[Jaehun Lim]

Created attachment 98811 [details]
Proposed patch

[Ryuan Choi]

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?

[Jaehun Lim]

(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.

[Gyuyoung Kim]

(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 ?

[Jaehun Lim]

Created attachment 98827 [details]
Change API comments

[Jaehun Lim]

(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.

[Jaehun Lim]

(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.

[Jaehun Lim]

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.

[Lucas De Marchi]

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
> [phone]  Jaehun Lim  <[e-mail]>
> +
> +        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?

[Jaehun Lim]

Created attachment 98835 [details]
Change patch description.

[Jaehun Lim]

(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
> > [phone]  Jaehun Lim  <[e-mail]>
> > +
> > +        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

[Raphael Kubo da Costa (:rakuco)]

> 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

[Jaehun Lim]

Created attachment 98984 [details]
Fix comments

Fix comments. Thanks.

[Raphael Kubo da Costa (:rakuco)]

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

[Jaehun Lim]

Created attachment 99167 [details]
Modified patch

Fix it. Thanks again.

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Jaehun Lim]

Could anyone review this patch?

[WebKit Review Bot]

Comment on attachment 99167 [details]
Modified patch

Clearing flags on attachment: 99167

Committed r91051: <http://trac.webkit.org/changeset/91051>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.