Bug 101193 - [EFL][WK2] Add ewk_database_manager APIs
Summary: [EFL][WK2] Add ewk_database_manager APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-11-05 01:32 PST by Jihye Kang
Modified: 2012-11-06 16:33 PST (History)
10 users (show)

See Also:


Attachments
Patch (23.06 KB, patch)
2012-11-05 17:32 PST, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (23.56 KB, patch)
2012-11-06 04:38 PST, Jihye Kang
no flags Details | Formatted Diff | Diff
Patch (23.52 KB, patch)
2012-11-06 06:29 PST, Jihye Kang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jihye Kang 2012-11-05 01:32:29 PST
Add ewk_database_manager API to manage web database on webkit2 efl.
Comment 1 Jihye Kang 2012-11-05 17:32:00 PST
Created attachment 172452 [details]
Patch
Comment 2 Gyuyoung Kim 2012-11-06 00:12:54 PST
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 3 Chris Dumez 2012-11-06 00:55:23 PST
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 4 Mikhail Pozdnyakov 2012-11-06 01:27:45 PST
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
Comment 5 Jihye Kang 2012-11-06 01:35:35 PST
(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.
Comment 6 Jihye Kang 2012-11-06 01:43:11 PST
(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?
Comment 7 Jihye Kang 2012-11-06 01:50:50 PST
(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?
Comment 8 Mikhail Pozdnyakov 2012-11-06 02:08:49 PST
(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 9 Chris Dumez 2012-11-06 02:36:55 PST
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?
Comment 10 Jihye Kang 2012-11-06 03:14:01 PST
(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.
Comment 11 Jihye Kang 2012-11-06 03:19:00 PST
(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". :)
Comment 12 Jihye Kang 2012-11-06 04:38:34 PST
Created attachment 172550 [details]
Patch
Comment 13 Gyuyoung Kim 2012-11-06 04:55:53 PST
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 14 Chris Dumez 2012-11-06 05:04:51 PST
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.
Comment 15 Jihye Kang 2012-11-06 05:30:43 PST
(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.
Comment 16 Jihye Kang 2012-11-06 06:29:53 PST
Created attachment 172571 [details]
Patch
Comment 17 WebKit Review Bot 2012-11-06 16:33:20 PST
Comment on attachment 172571 [details]
Patch

Clearing flags on attachment: 172571

Committed r133692: <http://trac.webkit.org/changeset/133692>
Comment 18 WebKit Review Bot 2012-11-06 16:33:26 PST
All reviewed patches have been landed.  Closing bug.