Bug 101193 – [EFL][WK2] Add ewk_database_manager APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jihye Kang]

Add ewk_database_manager API to manage web database on webkit2 efl.

[Jihye Kang]

Created attachment 172452 [details]
Patch

[Gyuyoung Kim]

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.

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

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

[Jihye Kang]

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

[Jihye Kang]

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

[Jihye Kang]

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

[Mikhail Pozdnyakov]

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

[Chris Dumez]

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?

[Jihye Kang]

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

[Jihye Kang]

(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". :)

[Jihye Kang]

Created attachment 172550 [details]
Patch

[Gyuyoung Kim]

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.

[Chris Dumez]

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.

[Jihye Kang]

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

[Jihye Kang]

Created attachment 172571 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172571 [details]
Patch

Clearing flags on attachment: 172571

Committed r133692: <http://trac.webkit.org/changeset/133692>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.