Bug 101193

Created attachment 172452 [details] Patch

Comment on attachment 172452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 > +struct Ewk_Database_Origins_Async_Get_Context { Can't we use class instead of struct ? As you know, EFL port start to use class for internal usage. > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.h:69 > + Unneeded line. > Source/WebKit2/UIProcess/API/efl/ewk_database_manager_private.h:46 > + explicit Ewk_Database_Manager(WKDatabaseManagerRef wkDatabaseManagerRef); It looks wkDatabaseManagerRef is not meaningful. Generally, WebKit has been omitted parameter name when there is no meaning.

Comment on attachment 172452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 > + Ewk_Database_Manager* databaseManager() const; Should not be const as per WebKit coding style. > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:44 > +void Ewk_Database_Manager::getDatabaseOrigins(void* context, WKDatabaseManagerGetDatabaseOriginsFunction callback) const We usually pass user data *after* the callback, not before. >> Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 >> +struct Ewk_Database_Origins_Async_Get_Context { > > Can't we use class instead of struct ? As you know, EFL port start to use class for internal usage. It's only a callback struct, I think it is OK to leave it as is. We still use structs for this case in the rest of the code I believe. > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:81 > + Eina_List* originList = 0; Should be declared when it is used below. This is nicer C++ style. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:29 > +#include <EWebKit2.h> You forgot to add ewk_database_manager.h to <EWebKit2.h>. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:31 > +#include <gtest/gtest.h> Do we really need this header? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:40 > + unsigned timeToCheck; Bad variable name.

Comment on attachment 172452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 >> + Ewk_Database_Manager* databaseManager() const; > > Should not be const as per WebKit coding style. or should return const pointer, as far as I see there are only const methods in the interface

(In reply to comment #2) > (From update of attachment 172452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 > > +struct Ewk_Database_Origins_Async_Get_Context { > > Can't we use class instead of struct ? As you know, EFL port start to use class for internal usage. Can I keep this as struct? As Christopher said, it's a callback struct and it is the same style on other codes. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.h:69 > > + > > Unneeded line. Will remove. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager_private.h:46 > > + explicit Ewk_Database_Manager(WKDatabaseManagerRef wkDatabaseManagerRef); > > It looks wkDatabaseManagerRef is not meaningful. Generally, WebKit has been omitted parameter name when there is no meaning. I will remove wk prefix on variable/parameter names.

(In reply to comment #4) > (From update of attachment 172452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 > >> + Ewk_Database_Manager* databaseManager() const; > > > > Should not be const as per WebKit coding style. > > or should return const pointer, as far as I see there are only const methods in the interface But I might add non-const methods in database manager interface later. So I think I need to use const_cast on getter for database manager rather than making databaseManager() as const function. Is it webkit preferred style?

(In reply to comment #3) > (From update of attachment 172452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 > > + Ewk_Database_Manager* databaseManager() const; > > Should not be const as per WebKit coding style. Ok. Then I need to use const_cast on getter of database manager. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:44 > > +void Ewk_Database_Manager::getDatabaseOrigins(void* context, WKDatabaseManagerGetDatabaseOriginsFunction callback) const > > We usually pass user data *after* the callback, not before. I will change the parameter's order. > > >> Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 > >> +struct Ewk_Database_Origins_Async_Get_Context { > > > > Can't we use class instead of struct ? As you know, EFL port start to use class for internal usage. > > It's only a callback struct, I think it is OK to leave it as is. We still use structs for this case in the rest of the code I believe. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:81 > > + Eina_List* originList = 0; > > Should be declared when it is used below. This is nicer C++ style. > I will change to decleare and use in the same place below. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:29 > > +#include <EWebKit2.h> > > You forgot to add ewk_database_manager.h to <EWebKit2.h>. > Thanks! I'll add it to EWebKit2.h. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:31 > > +#include <gtest/gtest.h> > > Do we really need this header? I will remove it. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:40 > > + unsigned timeToCheck; > > Bad variable name. I'm trying to find a good name. Do you have any suggestion?

(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 172452 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > > > > >> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 > > >> + Ewk_Database_Manager* databaseManager() const; > > > > > > Should not be const as per WebKit coding style. > > > > or should return const pointer, as far as I see there are only const methods in the interface > > But I might add non-const methods in database manager interface later. So I think I need to use const_cast on getter for database manager rather than making databaseManager() as const function. Is it webkit preferred style? which other methods would you need? if so, you will add another method: Ewk_Database_Manager* databaseManager(); and invoke it from non-const pointer to context. In general const_cast is better, as it makes it obvious that you break constness.

Comment on attachment 172452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:40 >>> + unsigned timeToCheck; >> >> Bad variable name. > > I'm trying to find a good name. Do you have any suggestion? This is basically a timeout in seconds right? (since you're using a timer with 1 second). How about "timeoutSeconds" then?

(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #4) > > > (From update of attachment 172452 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > > > > > > >> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 > > > >> + Ewk_Database_Manager* databaseManager() const; > > > > > > > > Should not be const as per WebKit coding style. > > > > > > or should return const pointer, as far as I see there are only const methods in the interface > > > > But I might add non-const methods in database manager interface later. So I think I need to use const_cast on getter for database manager rather than making databaseManager() as const function. Is it webkit preferred style? > > which other methods would you need? > if so, you will add another method: Ewk_Database_Manager* databaseManager(); > and invoke it from non-const pointer to context. > > In general const_cast is better, as it makes it obvious that you break constness. some kind of clear and setting methods. So I need to use const cast. Thanks for the advice.

(In reply to comment #9) > (From update of attachment 172452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172452&action=review > > >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_database_manager.cpp:40 > >>> + unsigned timeToCheck; > >> > >> Bad variable name. > > > > I'm trying to find a good name. Do you have any suggestion? > > This is basically a timeout in seconds right? (since you're using a timer with 1 second). How about "timeoutSeconds" then? Yes. It is basically a timeout in seconds and it is reduced every seconds. Thanks for the suggestion. I will use "timeoutSeconds". :)

Created attachment 172550 [details] Patch

Comment on attachment 172550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172550&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.h:190 > + * @param context context object to query. Do not use . at the end of line. We haven't been used . at @ field. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:192 > + * @return Ewk_Database_Manager object instance or @c NULL in case of failure. ditto. > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 > +struct Ewk_Database_Origins_Async_Get_Context { If this struct is used for callback, don't you need to add this to below ? http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h#L34 > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.h:48 > + * @brief Callback type for use with ewk_database_manager_origins_get(). ditto ? > Source/WebKit2/UIProcess/API/efl/ewk_database_manager_private.h:46 > + explicit Ewk_Database_Manager(WKDatabaseManagerRef databaseManagerRef); It looks databseManagerRef is not meaningful. I think Data type is enough.

Comment on attachment 172550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172550&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 >> +struct Ewk_Database_Origins_Async_Get_Context { > > If this struct is used for callback, don't you need to add this to below ? > > http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h#L34 EwkViewCallbacks.h is only for smart callbacks (and only for the view). Ewk_Database_Origins_Async_Get_Context is merely internal user data for a WK API callback. I think it is correct as it is.

(In reply to comment #13) > (From update of attachment 172550 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172550&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:190 > > + * @param context context object to query. > > Do not use . at the end of line. We haven't been used . at @ field. I will follow the policy. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:192 > > + * @return Ewk_Database_Manager object instance or @c NULL in case of failure. > > ditto. I will follow the policy. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.cpp:67 > > +struct Ewk_Database_Origins_Async_Get_Context { > > If this struct is used for callback, don't you need to add this to below ? > > http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h#L34 This struct is used for storing data for callback function, static getDatabaseOriginsCallback() which is needed to get asynchronous data from ewk_database_manager_origins_get(). Because it is not a structure for ewk view callback, I think I don't need to add this to EwkViewCallbacks.h. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager.h:48 > > + * @brief Callback type for use with ewk_database_manager_origins_get(). > > ditto ? I will follow the policy. > > > Source/WebKit2/UIProcess/API/efl/ewk_database_manager_private.h:46 > > + explicit Ewk_Database_Manager(WKDatabaseManagerRef databaseManagerRef); > > It looks databseManagerRef is not meaningful. I think Data type is enough. I will remove databaseManagerRef. And let only data type exists on the param.

Created attachment 172571 [details] Patch

Comment on attachment 172571 [details] Patch Clearing flags on attachment: 172571 Committed r133692: <http://trac.webkit.org/changeset/133692>

All reviewed patches have been landed. Closing bug.