Summary: | [EFL] Remove IconDatabase initialization in _ewk_init_body() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jaehun Lim <ljaehun.lim> | ||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Jaehun Lim
2011-06-27 15:57:57 PDT
Created attachment 98811 [details]
Proposed patch
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? (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. (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 ? Created attachment 98827 [details]
Change API comments
(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. (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. 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 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? Created attachment 98835 [details]
Change patch description.
(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 > 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 Created attachment 98984 [details]
Fix comments
Fix comments. Thanks.
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 Created attachment 99167 [details]
Modified patch
Fix it. Thanks again.
LGTM. Could anyone review this patch? Comment on attachment 99167 [details] Modified patch Clearing flags on attachment: 99167 Committed r91051: <http://trac.webkit.org/changeset/91051> All reviewed patches have been landed. Closing bug. |