Bug 91206

Summary: [EFL][WK2] Add ewk_settings.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit EFLAssignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: alexander.shalamov, cdumez, g.czajkowski, gyuyoung.kim, gyuyoung.kim, js45.yang, kihong.kwon, lucas.de.marchi, rakuco, ryuan.choi, sw0524.lee, vimff0, webkit.review.bot, zoltan.nyul
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
patch for ewk_setting.
none
patch for ewk_setting.
none
patch for ewk_settings.
none
patch for ewk_settings.
gyuyoung.kim: review-, gyuyoung.kim: commit-queue-
patch for ewk_settings.
none
patch for ewk_settings.
none
patch for ewk_settings.
none
Patch
none
Rebased patch.
none
Rebased
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eunmi Lee 2012-07-13 02:30:58 PDT
Add ewk_setting which is wrapping WKPreferencesRef.
The ewk_setting will be created by ewk_view and it will be destroyed when ewk_view is destroyed.
Comment 1 Eunmi Lee 2012-07-13 02:43:10 PDT
Created attachment 152196 [details]
patch for ewk_setting.
Comment 2 WebKit Review Bot 2012-07-13 02:46:39 PDT
Attachment 152196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Eunmi Lee 2012-07-13 02:50:54 PDT
Created attachment 152198 [details]
patch for ewk_setting.

Sytle error is fixed.
Comment 4 Chris Dumez 2012-07-13 06:23:30 PDT
Comment on attachment 152198 [details]
patch for ewk_setting.

View in context: https://bugs.webkit.org/attachment.cgi?id=152198&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_setting.cpp:29
> +struct _Ewk_Setting {

I think it should be "Ewk_Settings" (plural).

> Source/WebKit2/UIProcess/API/efl/ewk_setting.cpp:36
> +    setting->preferences = wkPreferences;

It would be nice if you could use a constructor like for the other Ewk classes.

> Source/WebKit2/UIProcess/API/efl/ewk_setting.h:29
> +

I think we should add a few important settings in this patch already. It looks bad to add an empty wrapper.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:265
> +EAPI Ewk_Setting* ewk_view_setting_get(Evas_Object* o);

Stars are on the wrong side.
Comment 5 Eunmi Lee 2012-07-13 22:45:21 PDT
(In reply to comment #4)
> (From update of attachment 152198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152198&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_setting.cpp:29
> > +struct _Ewk_Setting {
> 
> I think it should be "Ewk_Settings" (plural).
> 
OK, I will change it to ewk_settings.

> > Source/WebKit2/UIProcess/API/efl/ewk_setting.cpp:36
> > +    setting->preferences = wkPreferences;
> 
> It would be nice if you could use a constructor like for the other Ewk classes.
> 

Thanks, I will use constructor.

> > Source/WebKit2/UIProcess/API/efl/ewk_setting.h:29
> > +
> 
> I think we should add a few important settings in this patch already. It looks bad to add an empty wrapper.
> 

Yes, so I will add a few settings APIs.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:265
> > +EAPI Ewk_Setting* ewk_view_setting_get(Evas_Object* o);
> 
> Stars are on the wrong side.

My mistake. I will fix that.
Comment 6 Eunmi Lee 2012-07-13 22:47:27 PDT
Created attachment 152408 [details]
patch for ewk_settings.

Fixed for Christophe's comment.
Comment 7 Eunmi Lee 2012-07-14 00:45:37 PDT
Created attachment 152413 [details]
patch for ewk_settings.

rebased.
Comment 8 Chris Dumez 2012-07-14 00:51:37 PDT
Comment on attachment 152413 [details]
patch for ewk_settings.

View in context: https://bugs.webkit.org/attachment.cgi?id=152413&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:24
> +#include <WebKit2/WKPreferences.h>

Shouldn't we include ewk_settings_private.h ?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:37
> +Ewk_Settings* ewk_settings_new(WKPreferencesRef wkPreferences)

Missing @internal documentation.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:39
> +    Ewk_Settings* settings = new Ewk_Settings(wkPreferences);

No EINA_SAFETY check for wkPreferences? and you could return the result directly, without the extra assignment.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:44
> +void ewk_settings_free(Ewk_Settings* settings)

Missing @internal documentation.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:29
> +

/** Creates a type name for _Ewk_Settings */

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:55
> +    Ewk_Settings* settings;

Should be initialized to 0 in the constructor.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:288
> +    ewk_settings_free(priv->settings);

null check since it is set to 0 in the constructor?
Comment 9 Eunmi Lee 2012-07-14 04:28:14 PDT
(In reply to comment #8)
> (From update of attachment 152413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152413&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:24
> > +#include <WebKit2/WKPreferences.h>
> 
> Shouldn't we include ewk_settings_private.h ?
> 

Actually, I don't know we should include private header or not.
but, other cpp files have its private header, so I will add like them.

> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:37
> > +Ewk_Settings* ewk_settings_new(WKPreferencesRef wkPreferences)
> 
> Missing @internal documentation.
> 

I will add.

> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:39
> > +    Ewk_Settings* settings = new Ewk_Settings(wkPreferences);
> 
> No EINA_SAFETY check for wkPreferences? and you could return the result directly, without the extra assignment.
> 

Yes, right. I missed that. I will add EINA_SAFETY.

> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:44
> > +void ewk_settings_free(Ewk_Settings* settings)
> 
> Missing @internal documentation.
> 

I will add.

> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:29
> > +
> 
> /** Creates a type name for _Ewk_Settings */
> 

Dotto. thanks.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:55
> > +    Ewk_Settings* settings;
> 
> Should be initialized to 0 in the constructor.
> 

yes. I will add.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:288
> > +    ewk_settings_free(priv->settings);
> 
> null check since it is set to 0 in the constructor?

We don't have to check null here because ewk_settings_free() checks the null. :_)
Comment 10 Chris Dumez 2012-07-14 04:33:23 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 152413 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152413&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:24
> > > +#include <WebKit2/WKPreferences.h>
> > 
> > Shouldn't we include ewk_settings_private.h ?
> > 
> 
> Actually, I don't know we should include private header or not.
> but, other cpp files have its private header, so I will add like them.
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:37
> > > +Ewk_Settings* ewk_settings_new(WKPreferencesRef wkPreferences)
> > 
> > Missing @internal documentation.
> > 
> 
> I will add.
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:39
> > > +    Ewk_Settings* settings = new Ewk_Settings(wkPreferences);
> > 
> > No EINA_SAFETY check for wkPreferences? and you could return the result directly, without the extra assignment.
> > 
> 
> Yes, right. I missed that. I will add EINA_SAFETY.
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:44
> > > +void ewk_settings_free(Ewk_Settings* settings)
> > 
> > Missing @internal documentation.
> > 
> 
> I will add.
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:29
> > > +
> > 
> > /** Creates a type name for _Ewk_Settings */
> > 
> 
> Dotto. thanks.
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:55
> > > +    Ewk_Settings* settings;
> > 
> > Should be initialized to 0 in the constructor.
> > 
> 
> yes. I will add.
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:288
> > > +    ewk_settings_free(priv->settings);
> > 
> > null check since it is set to 0 in the constructor?
> 
> We don't have to check null here because ewk_settings_free() checks the null. :_)
It does but it will print a warning I believe.
Comment 11 Eunmi Lee 2012-07-14 04:56:41 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 152413 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=152413&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:24
> > > > +#include <WebKit2/WKPreferences.h>
> > > 
> > > Shouldn't we include ewk_settings_private.h ?
> > > 
> > 
> > Actually, I don't know we should include private header or not.
> > but, other cpp files have its private header, so I will add like them.
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:37
> > > > +Ewk_Settings* ewk_settings_new(WKPreferencesRef wkPreferences)
> > > 
> > > Missing @internal documentation.
> > > 
> > 
> > I will add.
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:39
> > > > +    Ewk_Settings* settings = new Ewk_Settings(wkPreferences);
> > > 
> > > No EINA_SAFETY check for wkPreferences? and you could return the result directly, without the extra assignment.
> > > 
> > 
> > Yes, right. I missed that. I will add EINA_SAFETY.
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:44
> > > > +void ewk_settings_free(Ewk_Settings* settings)
> > > 
> > > Missing @internal documentation.
> > > 
> > 
> > I will add.
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:29
> > > > +
> > > 
> > > /** Creates a type name for _Ewk_Settings */
> > > 
> > 
> > Dotto. thanks.
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:55
> > > > +    Ewk_Settings* settings;
> > > 
> > > Should be initialized to 0 in the constructor.
> > > 
> > 
> > yes. I will add.
> > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:288
> > > > +    ewk_settings_free(priv->settings);
> > > 
> > > null check since it is set to 0 in the constructor?
> > 
> > We don't have to check null here because ewk_settings_free() checks the null. :_)
> It does but it will print a warning I believe.

Yes, a warining will be printed for null.
and, If the priv is allocated and ewk_view is created correctly, the priv->settings should not be null.
So, I think it is good to let applications know that something was wrong by warning. If there is no warning, applications don't know what happens.
How do you think?
Comment 12 Gyuyoung Kim 2012-07-14 05:09:08 PDT
Comment on attachment 152413 [details]
patch for ewk_settings.

View in context: https://bugs.webkit.org/attachment.cgi?id=152413&action=review

Informal r- because of many comments.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:21
> +#ifndef ewk_settings_h

File description is missing.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:577
> +Ewk_Settings* ewk_view_settings_get(Evas_Object* ewkView)

Use *const* keyword for _get() function.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> + * @return the Ewk_Settings of this view

Need to mention what is return on failure.
Comment 13 Eunmi Lee 2012-07-14 05:09:30 PDT
Created attachment 152420 [details]
patch for ewk_settings.

fixed for Christophe's comments.
Comment 14 Gyuyoung Kim 2012-07-14 05:12:00 PDT
CC'ing Grzegorz, could you take a look this APIs comments ?
Comment 15 Eunmi Lee 2012-07-14 05:23:20 PDT
(In reply to comment #12)
> (From update of attachment 152413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152413&action=review
> 
> Informal r- because of many comments.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:21
> > +#ifndef ewk_settings_h
> 
> File description is missing.
> 
oops. I have to check doxygen comments again and again!
Thanks for your comments.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:577
> > +Ewk_Settings* ewk_view_settings_get(Evas_Object* ewkView)
> 
> Use *const* keyword for _get() function.
> 
my mistake. I have to add const.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > + * @return the Ewk_Settings of this view
> 
> Need to mention what is return on failure.

sure. I will add it.
Comment 16 Eunmi Lee 2012-07-14 05:27:50 PDT
Created attachment 152422 [details]
patch for ewk_settings.

Fixed for Gyuyoung's comment.
Comment 17 Chris Dumez 2012-07-14 07:55:44 PDT
Comment on attachment 152422 [details]
patch for ewk_settings.

View in context: https://bugs.webkit.org/attachment.cgi?id=152422&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:50
> +    Ewk_Settings* settings = new Ewk_Settings(wkPreferences);

Useless assignment. You could return directly.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:511
> +    priv->settings = ewk_settings_new(WKPageGroupGetPreferences(WKPageGetPageGroup(toAPI(priv->pageClient->page()))));

why not use pageGroupRef directly instead of retrieving it again from the pageClient you have just created?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> + * @return the Ewk_Settings of this view or @c 0 on failure

I would prefer NULL instead of 0, since this is a C API.
Comment 18 Eunmi Lee 2012-07-14 08:23:06 PDT
(In reply to comment #17)
> (From update of attachment 152422 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152422&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:50
> > +    Ewk_Settings* settings = new Ewk_Settings(wkPreferences);
> 
> Useless assignment. You could return directly.

Yes, I will change that.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:511
> > +    priv->settings = ewk_settings_new(WKPageGroupGetPreferences(WKPageGetPageGroup(toAPI(priv->pageClient->page()))));
> 
> why not use pageGroupRef directly instead of retrieving it again from the pageClient you have just created?
> 

We can not use pageGroupRef directly because it can be 0.
If pageGroupRef is 0, newly created WebPageProxy's PageGroup will be the default PageGroup.
so, we have to get PageGroup from the created WebPageProxy.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > + * @return the Ewk_Settings of this view or @c 0 on failure
> 
> I would prefer NULL instead of 0, since this is a C API.

OK, I will change that.
Comment 19 Eunmi Lee 2012-07-14 08:33:45 PDT
Created attachment 152424 [details]
patch for ewk_settings.

Fixed for Christophe's comments.
Comment 20 Chris Dumez 2012-07-14 08:36:00 PDT
Comment on attachment 152424 [details]
patch for ewk_settings.

LGTM.
Comment 21 Gyuyoung Kim 2012-07-14 18:09:10 PDT
Comment on attachment 152424 [details]
patch for ewk_settings.

View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> + * @return the Ewk_Settings of this view or @c NULL on failure

Use 0 instead of NULL.
Comment 22 Chris Dumez 2012-07-14 23:40:43 PDT
(In reply to comment #21)
> (From update of attachment 152424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > + * @return the Ewk_Settings of this view or @c NULL on failure
> 
> Use 0 instead of NULL.

gyuyoung, I asked her to do the exact opposite :) Could you please explain why?
In my opinion, this is a public C API so NULL is not appropriate than 0. If you look at EVAS documentation, they seem to use NULL in the doc as well.
Comment 23 Jongseok Yang 2012-07-15 21:43:15 PDT
How about using the similar function name to webkit1?

Webkit1:
ewk_view_setting_auto_load_images_get
ewk_view_setting_auto_load_images_set
ewk_view_setting_enable_scripts_get
ewk_view_setting_enable_scripts_set

So, I think it's better to use the below function name for webkit2
ewk_settings_auto_load_images_get
ewk_settings_auto_load_images_set
ewk_settings_enable_scripts_get
ewk_settings_enable_scripts_set

Could you please express your opinion?
Comment 24 Gyuyoung Kim 2012-07-15 22:52:38 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 152424 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > > + * @return the Ewk_Settings of this view or @c NULL on failure
> > 
> > Use 0 instead of NULL.
> 
> gyuyoung, I asked her to do the exact opposite :) Could you please explain why?
> In my opinion, this is a public C API so NULL is not appropriate than 0. If you look at EVAS documentation, they seem to use NULL in the doc as well.

EFL port has used 0 instead of NULL. This is webkit style. If you think we should use NULL instead of 0, you have to discuss it with webkit-efl mailing list first. I think we need to adhere this style until deciding it.
Comment 25 Chris Dumez 2012-07-15 23:12:08 PDT
(In reply to comment #24)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (From update of attachment 152424 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > > > + * @return the Ewk_Settings of this view or @c NULL on failure
> > > 
> > > Use 0 instead of NULL.
> > 
> > gyuyoung, I asked her to do the exact opposite :) Could you please explain why?
> > In my opinion, this is a public C API so NULL is not appropriate than 0. If you look at EVAS documentation, they seem to use NULL in the doc as well.
> 
> EFL port has used 0 instead of NULL. This is webkit style. If you think we should use NULL instead of 0, you have to discuss it with webkit-efl mailing list first. I think we need to adhere this style until deciding it.

Since when are we following WebKit style in public C headers?
Then, let's move the stars towards the data types too :)

Using 0 here is wrong: http://lwn.net/Articles/93574/
If you really want to take this to the mailing list, we can.
Comment 26 Chris Dumez 2012-07-15 23:13:46 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > (In reply to comment #21)
> > > > (From update of attachment 152424 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review
> > > > 
> > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > > > > + * @return the Ewk_Settings of this view or @c NULL on failure
> > > > 
> > > > Use 0 instead of NULL.
> > > 
> > > gyuyoung, I asked her to do the exact opposite :) Could you please explain why?
> > > In my opinion, this is a public C API so NULL is not appropriate than 0. If you look at EVAS documentation, they seem to use NULL in the doc as well.
> > 
> > EFL port has used 0 instead of NULL. This is webkit style. If you think we should use NULL instead of 0, you have to discuss it with webkit-efl mailing list first. I think we need to adhere this style until deciding it.
> 
> Since when are we following WebKit style in public C headers?
> Then, let's move the stars towards the data types too :)
> 
> Using 0 here is wrong: http://lwn.net/Articles/93574/
> If you really want to take this to the mailing list, we can.

Your rule is undocumented it seems by the way:
http://trac.webkit.org/wiki/EFLWebKitCodingStyle
Comment 27 Gyuyoung Kim 2012-07-15 23:25:23 PDT
(In reply to comment #23)
> How about using the similar function name to webkit1?
> 
> Webkit1:
> ewk_view_setting_auto_load_images_get
> ewk_view_setting_auto_load_images_set
> ewk_view_setting_enable_scripts_get
> ewk_view_setting_enable_scripts_set
> 
> So, I think it's better to use the below function name for webkit2
> ewk_settings_auto_load_images_get
> ewk_settings_auto_load_images_set
> ewk_settings_enable_scripts_get
> ewk_settings_enable_scripts_set
> 
> Could you please express your opinion?

There was similar issue on other bug. I think we need to decide if we follow WK1 EFL coding style first. So, I sent an email to webkit-efl dev. Let's discuss this on mailing list.
Comment 28 Gyuyoung Kim 2012-07-15 23:27:49 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #22)
> > > > (In reply to comment #21)
> > > > > (From update of attachment 152424 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review
> > > > > 
> > > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:263
> > > > > > + * @return the Ewk_Settings of this view or @c NULL on failure
> > > > > 
> > > > > Use 0 instead of NULL.
> > > > 
> > > > gyuyoung, I asked her to do the exact opposite :) Could you please explain why?
> > > > In my opinion, this is a public C API so NULL is not appropriate than 0. If you look at EVAS documentation, they seem to use NULL in the doc as well.
> > > 
> > > EFL port has used 0 instead of NULL. This is webkit style. If you think we should use NULL instead of 0, you have to discuss it with webkit-efl mailing list first. I think we need to adhere this style until deciding it.
> > 
> > Since when are we following WebKit style in public C headers?
> > Then, let's move the stars towards the data types too :)
> > 
> > Using 0 here is wrong: http://lwn.net/Articles/93574/
> > If you really want to take this to the mailing list, we can.
> 
> Your rule is undocumented it seems by the way:
> http://trac.webkit.org/wiki/EFLWebKitCodingStyle

Unfortunately, it seems this is implicit rule so far. As you know, all EFL APIs have used 0 instead of NULL so far. So, I think we need to fix it for all APIs as once. If not, this can make confusion.
Comment 29 Grzegorz Czajkowski 2012-07-16 00:11:59 PDT
Comment on attachment 152424 [details]
patch for ewk_settings.

View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:23
> + * @brief   Describes the settings API.

From API from of view there is a lack of detailed description how those settings are connected with ewk view object. Does ewk_settings exist one object peer application or is it created for each view? If the setting was changed for instance, ewk_settings_javascript_enabled_set(setting, EINA_TRUE) does it mean that javascript is enabled for all views?
Comment 30 Chris Dumez 2012-07-16 00:13:49 PDT
(In reply to comment #23)
> How about using the similar function name to webkit1?
> 
> Webkit1:
> ewk_view_setting_auto_load_images_get
> ewk_view_setting_auto_load_images_set
> ewk_view_setting_enable_scripts_get
> ewk_view_setting_enable_scripts_set
> 
> So, I think it's better to use the below function name for webkit2
> ewk_settings_auto_load_images_get
> ewk_settings_auto_load_images_set
> ewk_settings_enable_scripts_get
> ewk_settings_enable_scripts_set
> 
> Could you please express your opinion?

It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.

I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
Comment 31 Jongseok Yang 2012-07-16 00:21:21 PDT
(In reply to comment #30)
> (In reply to comment #23)
> > How about using the similar function name to webkit1?
> > 
> > Webkit1:
> > ewk_view_setting_auto_load_images_get
> > ewk_view_setting_auto_load_images_set
> > ewk_view_setting_enable_scripts_get
> > ewk_view_setting_enable_scripts_set
> > 
> > So, I think it's better to use the below function name for webkit2
> > ewk_settings_auto_load_images_get
> > ewk_settings_auto_load_images_set
> > ewk_settings_enable_scripts_get
> > ewk_settings_enable_scripts_set
> > 
> > Could you please express your opinion?
> 
> It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> 
> I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.


I'm sorry that it was confused.
I think that ewk_setting or ewk_settings is good and my point is just function name.

ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get
Comment 32 Chris Dumez 2012-07-16 00:24:04 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #23)
> > > How about using the similar function name to webkit1?
> > > 
> > > Webkit1:
> > > ewk_view_setting_auto_load_images_get
> > > ewk_view_setting_auto_load_images_set
> > > ewk_view_setting_enable_scripts_get
> > > ewk_view_setting_enable_scripts_set
> > > 
> > > So, I think it's better to use the below function name for webkit2
> > > ewk_settings_auto_load_images_get
> > > ewk_settings_auto_load_images_set
> > > ewk_settings_enable_scripts_get
> > > ewk_settings_enable_scripts_set
> > > 
> > > Could you please express your opinion?
> > 
> > It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> > 
> > I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
> 
> 
> I'm sorry that it was confused.
> I think that ewk_setting or ewk_settings is good and my point is just function name.
> 
> ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get

Oh right, I misread your comment, sorry. Well, then I disagree :)
Those are view-specific settings and right now they can be confused as "global" settings.
Comment 33 Jongseok Yang 2012-07-16 00:36:22 PDT
(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > (In reply to comment #23)
> > > > How about using the similar function name to webkit1?
> > > > 
> > > > Webkit1:
> > > > ewk_view_setting_auto_load_images_get
> > > > ewk_view_setting_auto_load_images_set
> > > > ewk_view_setting_enable_scripts_get
> > > > ewk_view_setting_enable_scripts_set
> > > > 
> > > > So, I think it's better to use the below function name for webkit2
> > > > ewk_settings_auto_load_images_get
> > > > ewk_settings_auto_load_images_set
> > > > ewk_settings_enable_scripts_get
> > > > ewk_settings_enable_scripts_set
> > > > 
> > > > Could you please express your opinion?
> > > 
> > > It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> > > 
> > > I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
> > 
> > 
> > I'm sorry that it was confused.
> > I think that ewk_setting or ewk_settings is good and my point is just function name.
> > 
> > ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get
> 
> Oh right, I misread your comment, sorry. Well, then I disagree :)
> Those are view-specific settings and right now they can be confused as "global" settings.

GTK port uses "settings" word for view settings.
static void webkit_web_settings_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec);

They use "context" word for "global" settings like the below
WEBKIT_API void webkit_web_context_set_cache_model(WebKitWebContext *context, WebKitCacheModel cache_model);

So, I think ewk_setting is good.
Comment 34 Chris Dumez 2012-07-16 01:14:02 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (In reply to comment #30)
> > > > (In reply to comment #23)
> > > > > How about using the similar function name to webkit1?
> > > > > 
> > > > > Webkit1:
> > > > > ewk_view_setting_auto_load_images_get
> > > > > ewk_view_setting_auto_load_images_set
> > > > > ewk_view_setting_enable_scripts_get
> > > > > ewk_view_setting_enable_scripts_set
> > > > > 
> > > > > So, I think it's better to use the below function name for webkit2
> > > > > ewk_settings_auto_load_images_get
> > > > > ewk_settings_auto_load_images_set
> > > > > ewk_settings_enable_scripts_get
> > > > > ewk_settings_enable_scripts_set
> > > > > 
> > > > > Could you please express your opinion?
> > > > 
> > > > It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> > > > 
> > > > I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
> > > 
> > > 
> > > I'm sorry that it was confused.
> > > I think that ewk_setting or ewk_settings is good and my point is just function name.
> > > 
> > > ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get
> > 
> > Oh right, I misread your comment, sorry. Well, then I disagree :)
> > Those are view-specific settings and right now they can be confused as "global" settings.
> 
> GTK port uses "settings" word for view settings.
> static void webkit_web_settings_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec);
> 
> They use "context" word for "global" settings like the below
> WEBKIT_API void webkit_web_context_set_cache_model(WebKitWebContext *context, WebKitCacheModel cache_model);
> 
> So, I think ewk_setting is good.

Right, both the GTK and Qt ports are omitting "view" from the name. It's fine by me then if no one else has any issue with "ewk_settings".
Comment 35 Eunmi Lee 2012-07-16 01:37:36 PDT
(In reply to comment #29)
> (From update of attachment 152424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152424&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:23
> > + * @brief   Describes the settings API.
> 
> From API from of view there is a lack of detailed description how those settings are connected with ewk view object. Does ewk_settings exist one object peer application or is it created for each view? If the setting was changed for instance, ewk_settings_javascript_enabled_set(setting, EINA_TRUE) does it mean that javascript is enabled for all views?

Thanks for your comments.
I have to add detailed description.

The ewk_settings is connected with page group. That means the ewk_settings_javascript_enabled_set() will change the views in the same page group.
This patch can cause the confusion because the ewk_view has the ewk_settings.
so, I have to think about this patch again.
Comment 36 Eunmi Lee 2012-07-16 01:51:32 PDT
(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > (In reply to comment #30)
> > > > > (In reply to comment #23)
> > > > > > How about using the similar function name to webkit1?
> > > > > > 
> > > > > > Could you please express your opinion?
> > > > > 
> > > > > It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> > > > > 
> > > > > I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
> > > > 
> > > > 
> > > > I'm sorry that it was confused.
> > > > I think that ewk_setting or ewk_settings is good and my point is just function name.
> > > > 
> > > > ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get
> > > 
> > > Oh right, I misread your comment, sorry. Well, then I disagree :)
> > > Those are view-specific settings and right now they can be confused as "global" settings.
> > 
> > GTK port uses "settings" word for view settings.
> > static void webkit_web_settings_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec);
> > 
> > They use "context" word for "global" settings like the below
> > WEBKIT_API void webkit_web_context_set_cache_model(WebKitWebContext *context, WebKitCacheModel cache_model);
> > 
> > So, I think ewk_setting is good.
> 
> Right, both the GTK and Qt ports are omitting "view" from the name. It's fine by me then if no one else has any issue with "ewk_settings".

Dear Christophe, Jongseok and all,

I think this patch causes the confusion because ewk_settings is connected to the ewk_view.
so, someone can think ewk_settings set the specific ewk_view and someone can think ewk_settings is for global settings.
However, both are wrong. ewk_settings is for page group and views in the page group.

So, how about moving settings to the ewk_page_group?
The page group (WKBrowsingContextGroup.mm) has the WKPreferences in the mac port, and the qwebpreferences.cpp uses the WKPreference and the view has qwebpreferences like this patch.

I think mac port's style is more clear, so I want to modify this patch for that.

How do you think?
Comment 37 Ryuan Choi 2012-07-16 20:23:32 PDT
(In reply to comment #36)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > (In reply to comment #32)
> > > > (In reply to comment #31)
> > > > > (In reply to comment #30)
> > > > > > (In reply to comment #23)
> > > > > > > How about using the similar function name to webkit1?
> > > > > > > 
> > > > > > > Could you please express your opinion?
> > > > > > 
> > > > > > It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> > > > > > 
> > > > > > I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
> > > > > 
> > > > > 
> > > > > I'm sorry that it was confused.
> > > > > I think that ewk_setting or ewk_settings is good and my point is just function name.
> > > > > 
> > > > > ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get
> > > > 
> > > > Oh right, I misread your comment, sorry. Well, then I disagree :)
> > > > Those are view-specific settings and right now they can be confused as "global" settings.
> > > 
> > > GTK port uses "settings" word for view settings.
> > > static void webkit_web_settings_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec);
> > > 
> > > They use "context" word for "global" settings like the below
> > > WEBKIT_API void webkit_web_context_set_cache_model(WebKitWebContext *context, WebKitCacheModel cache_model);
> > > 
> > > So, I think ewk_setting is good.
> > 
> > Right, both the GTK and Qt ports are omitting "view" from the name. It's fine by me then if no one else has any issue with "ewk_settings".
> 
> Dear Christophe, Jongseok and all,
> 
> I think this patch causes the confusion because ewk_settings is connected to the ewk_view.
> so, someone can think ewk_settings set the specific ewk_view and someone can think ewk_settings is for global settings.
> However, both are wrong. ewk_settings is for page group and views in the page group.
> 
> So, how about moving settings to the ewk_page_group?
> The page group (WKBrowsingContextGroup.mm) has the WKPreferences in the mac port, and the qwebpreferences.cpp uses the WKPreference and the view has qwebpreferences like this patch.
> 
> I think mac port's style is more clear, so I want to modify this patch for that.
> 
> How do you think?

It make sense to me.
If then, can you make a bug for ewk_page_group first?
Comment 38 Eunmi Lee 2012-07-16 22:07:42 PDT
(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #34)
> > > (In reply to comment #33)
> > > > (In reply to comment #32)
> > > > > (In reply to comment #31)
> > > > > > (In reply to comment #30)
> > > > > > > (In reply to comment #23)
> > > > > > > > How about using the similar function name to webkit1?
> > > > > > > > 
> > > > > > > > Could you please express your opinion?
> > > > > > > 
> > > > > > > It is a good point that all the WKPreferences are associated to a PageGroup and for us to a view since we don't have page groups in Ewk APIs. As a consequence, all the preferences in this patch (and the others to come) are specific to a view.
> > > > > > > 
> > > > > > > I believe Jongseok Yang is right, we need to rename the files and methods to "ewk_view_setting" or "ewk_view_settings". Personally, I have a preference for the plural version but since we used ewk_view_setting in WK1, we can use that for consistency.
> > > > > > 
> > > > > > 
> > > > > > I'm sorry that it was confused.
> > > > > > I think that ewk_setting or ewk_settings is good and my point is just function name.
> > > > > > 
> > > > > > ewk_settings_javascript_enabled_get -> ewk_settings_enable_scripts_get
> > > > > 
> > > > > Oh right, I misread your comment, sorry. Well, then I disagree :)
> > > > > Those are view-specific settings and right now they can be confused as "global" settings.
> > > > 
> > > > GTK port uses "settings" word for view settings.
> > > > static void webkit_web_settings_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec);
> > > > 
> > > > They use "context" word for "global" settings like the below
> > > > WEBKIT_API void webkit_web_context_set_cache_model(WebKitWebContext *context, WebKitCacheModel cache_model);
> > > > 
> > > > So, I think ewk_setting is good.
> > > 
> > > Right, both the GTK and Qt ports are omitting "view" from the name. It's fine by me then if no one else has any issue with "ewk_settings".
> > 
> > Dear Christophe, Jongseok and all,
> > 
> > I think this patch causes the confusion because ewk_settings is connected to the ewk_view.
> > so, someone can think ewk_settings set the specific ewk_view and someone can think ewk_settings is for global settings.
> > However, both are wrong. ewk_settings is for page group and views in the page group.
> > 
> > So, how about moving settings to the ewk_page_group?
> > The page group (WKBrowsingContextGroup.mm) has the WKPreferences in the mac port, and the qwebpreferences.cpp uses the WKPreference and the view has qwebpreferences like this patch.
> > 
> > I think mac port's style is more clear, so I want to modify this patch for that.
> > 
> > How do you think?
> 
> It make sense to me.
> If then, can you make a bug for ewk_page_group first?

I think ewk_page_group is necessary not only settings, but WK2 EFL port.
so, I've created new Bug for that.
https://bugs.webkit.org/show_bug.cgi?id=91468
Comment 39 Eunmi Lee 2012-07-17 22:57:54 PDT
The confusion from ewk_settings can be solved if I make each view in its own group like Qt port.

So, I don't want to make ewk_view_group and I'm going to add code to create WebPageGroup per ewk_view instead of that.
That means, ewk_settings can set preferences for only one ewk_view.

I'm going to upload new patch for that.
Comment 40 Eunmi Lee 2012-07-17 23:33:00 PDT
Created attachment 152938 [details]
Patch
Comment 41 Eunmi Lee 2012-07-18 00:30:13 PDT
Created attachment 152951 [details]
Rebased patch.
Comment 42 Alexander Shalamov 2012-08-03 02:27:28 PDT
I have a question related to this patch. Why did you choose class name "ewk_settings"? All other ports have "preferences", moreover, WK API is also called WKPreferences.
Comment 43 Eunmi Lee 2012-08-12 06:20:59 PDT
(In reply to comment #42)
> I have a question related to this patch. Why did you choose class name "ewk_settings"? All other ports have "preferences", moreover, WK API is also called WKPreferences.

I choose the "ewk_settings" because WebKit1 EFL port also using that name.
I think "ewk_settings" is more familiar to WebKit EFL developers. :)
Comment 44 Eunmi Lee 2012-08-12 06:51:25 PDT
Created attachment 157896 [details]
Rebased
Comment 45 Raphael Kubo da Costa (:rakuco) 2012-08-12 09:12:11 PDT
Comment on attachment 157896 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=157896&action=review

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:53
> -    m_page = context->createWebPage(this, pageGroup);
> +    if (pageGroup)
> +        m_page = context->createWebPage(this, pageGroup);
> +    else
> +        m_page = context->createWebPage(this, WebPageGroup::create().get());

Is this really needed? WebContext::createWebPage already falls back to a default page group if pageGroup is 0.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:682
> +    priv->settings = ewk_settings_new(WKPageGroupGetPreferences(WKPageGetPageGroup(toAPI(priv->pageClient->page()))));

Since ewk_settings_new is private and simply creates a new instance on the heap, isn't it better to move the structure definition to ewk_settings_private.h and use an OwnPtr here. You then do not need to manually manage the allocated memory anymore.
Comment 46 Eunmi Lee 2012-08-12 23:53:39 PDT
Comment on attachment 157896 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=157896&action=review

Thank you for your advice :)

>> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:53
>> +        m_page = context->createWebPage(this, WebPageGroup::create().get());
> 
> Is this really needed? WebContext::createWebPage already falls back to a default page group if pageGroup is 0.

I really needed above codes because I want that one ewk_view has one WebPageGroup.
If we set the default page group when pageGroup is 0, some ewk_views will share the one WebPageGropup (default page group),
and it will make users of ewk_view confused.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:682
>> +    priv->settings = ewk_settings_new(WKPageGroupGetPreferences(WKPageGetPageGroup(toAPI(priv->pageClient->page()))));
> 
> Since ewk_settings_new is private and simply creates a new instance on the heap, isn't it better to move the structure definition to ewk_settings_private.h and use an OwnPtr here. You then do not need to manually manage the allocated memory anymore.

I think that is a good idea, so I will change codes like that :)
Comment 47 Eunmi Lee 2012-08-12 23:56:27 PDT
Comment on attachment 157896 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=157896&action=review

>>> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:53
>>> -    m_page = context->createWebPage(this, pageGroup);
>>> +    if (pageGroup)
>>> +        m_page = context->createWebPage(this, pageGroup);
>>> +    else
>>> +        m_page = context->createWebPage(this, WebPageGroup::create().get());
>> 
>> Is this really needed? WebContext::createWebPage already falls back to a default page group if pageGroup is 0.
> 
> I really needed above codes because I want that one ewk_view has one WebPageGroup.
> If we set the default page group when pageGroup is 0, some ewk_views will share the one WebPageGropup (default page group),
> and it will make users of ewk_view confused.

Additionally, the WebPageGroup has one WebPreferences, so one WebPageGroup means one WebPreferences.
Comment 48 Eunmi Lee 2012-08-13 00:00:48 PDT
Created attachment 157930 [details]
Patch
Comment 49 Gyuyoung Kim 2012-08-13 01:32:07 PDT
Comment on attachment 157930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157930&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:2
> + * Copyright (C) 2012 Samsung Electronics

Eunmi, I was told BSD license has more benefits than LGPL. How do you think about it?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:32
> +

Don't you need to use "enable = !! enable" ? EFL port has used double not operator(!!) in order to support bit field operation in Eina_Bool.

See also, http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_settings.cpp#L261

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:48
> +

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:18
> + *

Nit : Unneeded line.

> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:39
> +    _Ewk_Settings(WKPreferencesRef wkPreferences)

Please add *explicit* keyword.
Comment 50 Eunmi Lee 2012-08-13 07:19:36 PDT
Comment on attachment 157930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157930&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:2
>> + * Copyright (C) 2012 Samsung Electronics
> 
> Eunmi, I was told BSD license has more benefits than LGPL. How do you think about it?

I agree with you and I will change the copyright :)

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:32
>> +
> 
> Don't you need to use "enable = !! enable" ? EFL port has used double not operator(!!) in order to support bit field operation in Eina_Bool.
> 
> See also, http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_settings.cpp#L261

In this case, the parameter "Eina_Bool enable" is set to the "bool" type variable in the WKPreferencesSetJavaScriptEnabled(), so we don't have to use double not operator here because bool type variable can have only true or false.

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:18
>> + *
> 
> Nit : Unneeded line.

I will change this copyright to BSD :)

>> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:39
>> +    _Ewk_Settings(WKPreferencesRef wkPreferences)
> 
> Please add *explicit* keyword.

Thanks, I will add it.
Comment 51 Eunmi Lee 2012-08-13 08:22:46 PDT
Created attachment 158004 [details]
Patch
Comment 52 Gyuyoung Kim 2012-08-13 08:54:44 PDT
(In reply to comment #50)
> In this case, the parameter "Eina_Bool enable" is set to the "bool" type variable in the WKPreferencesSetJavaScriptEnabled(), so we don't have to use double not operator here because bool type variable can have only true or false.

Yes, it looks this is not strict rule in WebKit. But, this is can be useful when variable was initialized as bit operation as below,

   Eina_Bool enable : 1;

If application passes a bool variable which is set as bit operation, API needs to use "= !!". 

I would like to know how does Kubo think about this. CC'ing Kubo.
Comment 53 Raphael Kubo da Costa (:rakuco) 2012-08-13 11:20:07 PDT
(In reply to comment #52)
> (In reply to comment #50)
> > In this case, the parameter "Eina_Bool enable" is set to the "bool" type variable in the WKPreferencesSetJavaScriptEnabled(), so we don't have to use double not operator here because bool type variable can have only true or false.
> 
> Yes, it looks this is not strict rule in WebKit. But, this is can be useful when variable was initialized as bit operation as below,
> 
>    Eina_Bool enable : 1;
> 
> If application passes a bool variable which is set as bit operation, API needs to use "= !!". 

Sorry, when you poked me on IRC I didn't realize you meant this case here.

Using the double negation is useful if you end up passing a value other than 0 or 1 to Eina_Bool (which is possible, since it's an unsigned char typedef) and keep manipulating it.

In this case here, we're directly passing the Eina_Bool to a function that takes a bool, which, according to the C++ standard, can only assume the values 0 or 1, so what EunMi is doing is safe.

This also applies to the example you linked to in WK1's ewk_setting.cpp; it makes sense to do this where we do store an Eina_Bool in a bitfield, such as in <http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.cpp#L2584>.
Comment 54 Gyuyoung Kim 2012-08-13 18:00:22 PDT
(In reply to comment #53)
 
> Sorry, when you poked me on IRC I didn't realize you meant this case here.
> 
> Using the double negation is useful if you end up passing a value other than 0 or 1 to Eina_Bool (which is possible, since it's an unsigned char typedef) and keep manipulating it.
> 
> In this case here, we're directly passing the Eina_Bool to a function that takes a bool, which, according to the C++ standard, can only assume the values 0 or 1, so what EunMi is doing is safe.
> 
> This also applies to the example you linked to in WK1's ewk_setting.cpp; it makes sense to do this where we do store an Eina_Bool in a bitfield, such as in <http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.cpp#L2584>.

Kubo, thank you for your comment. It looks I thought unnecessary things. Ok, I agree with your explanation.

LGTM on latest patch. Thank you Eunmi.
Comment 55 Gyuyoung Kim 2012-08-13 18:40:12 PDT
I add this case(!!) to WebKit EFL coding style. :) If there is something wrong, please fix it. Thanks.

https://trac.webkit.org/wiki/EFLWebKitCodingStyle
Comment 56 Eunmi Lee 2012-08-13 18:52:41 PDT
(In reply to comment #55)
> I add this case(!!) to WebKit EFL coding style. :) If there is something wrong, please fix it. Thanks.
> 
> https://trac.webkit.org/wiki/EFLWebKitCodingStyle

Thanks Gyuyoung,
We have to be careful to use C style boolean type! :)

I've add "compare with" in the your sentence of EFLWebKitCodingStyle page.
so, the sentence is as follows:
"Use double not operator(!!) when you modify or compare with a Eina Bool member variable which stores a value in a bit field."
Comment 57 Raphael Kubo da Costa (:rakuco) 2012-08-13 19:34:01 PDT
(In reply to comment #55)
> I add this case(!!) to WebKit EFL coding style. :) If there is something wrong, please fix it. Thanks.
> 
> https://trac.webkit.org/wiki/EFLWebKitCodingStyle

I've added a better explanation for the issue (and corrected the correct example :-) and added some syntax highlighting.

To be clear, bools in bitfields work fine; Eina_Bools in bitfields need extra caution (see my explanation in the wiki). At the end, though, one just needs to understand what's going on and have some common sense.
Comment 58 Gyuyoung Kim 2012-08-13 19:39:07 PDT
(In reply to comment #57)

> To be clear, bools in bitfields work fine; Eina_Bools in bitfields need extra caution (see my explanation in the wiki). At the end, though, one just needs to understand what's going on and have some common sense.

Looks much better than before. Thank you.
Comment 59 Ryuan Choi 2012-08-14 05:03:18 PDT
LGTM, too.
Comment 60 Gyuyoung Kim 2012-08-24 00:41:27 PDT
Comment on attachment 158004 [details]
Patch

Eunmi, could you add unit test for this patch ?
Comment 61 Eunmi Lee 2012-08-24 00:47:19 PDT
(In reply to comment #60)
> (From update of attachment 158004 [details])
> Eunmi, could you add unit test for this patch ?

sure, I will add it :)
Comment 62 Eunmi Lee 2012-08-26 04:20:11 PDT
Created attachment 160600 [details]
Patch
Comment 63 Gyuyoung Kim 2012-08-26 05:40:54 PDT
Comment on attachment 160600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160600&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:55
> +EAPI Eina_Bool ewk_settings_enable_scripts_set(Ewk_Settings* settings, Eina_Bool enable);

Nit : Wrong '*' operator place.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:65
> +EAPI Eina_Bool ewk_settings_enable_scripts_get(const Ewk_Settings* settings);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:76
> +EAPI Eina_Bool ewk_settings_auto_load_images_set(Ewk_Settings* settings, Eina_Bool automatic);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:86
> +EAPI Eina_Bool ewk_settings_auto_load_images_get(const Ewk_Settings* settings);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:353
> + * @param o the view object to get Ewk_Settings

s/the view/view/g

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_settings.cpp:38
> +    ASSERT_TRUE(ewk_settings_enable_scripts_set(settings, EINA_TRUE));

Please use standard boolean type. Other are same.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:194
> +    ASSERT_TRUE(ewk_view_settings_get(webView()));

Is below test is more proper ?

Ewk_Settings* settings = ewk_view_settings_get(webView());
ASSERT_TRUE(settings);
ASSERT_EQ(settings, ewk_view_settings_get(webView()));
Comment 64 Eunmi Lee 2012-08-29 19:57:18 PDT
Comment on attachment 160600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160600&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:55
>> +EAPI Eina_Bool ewk_settings_enable_scripts_set(Ewk_Settings* settings, Eina_Bool enable);
> 
> Nit : Wrong '*' operator place.

oops. I will fix it.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:353
>> + * @param o the view object to get Ewk_Settings
> 
> s/the view/view/g

Thanks. I will fix it.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_settings.cpp:38
>> +    ASSERT_TRUE(ewk_settings_enable_scripts_set(settings, EINA_TRUE));
> 
> Please use standard boolean type. Other are same.

The ewk_settings_enable_scripts_set() API gets Eina_Bool type as a parameter, so I used it.
Do I have to use bool type for test cases?

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:194
>> +    ASSERT_TRUE(ewk_view_settings_get(webView()));
> 
> Is below test is more proper ?
> 
> Ewk_Settings* settings = ewk_view_settings_get(webView());
> ASSERT_TRUE(settings);
> ASSERT_EQ(settings, ewk_view_settings_get(webView()));

Thanks, I will change like that.
Comment 65 Eunmi Lee 2012-09-03 04:10:58 PDT
Created attachment 161895 [details]
Patch
Comment 66 Eunmi Lee 2012-09-03 04:16:16 PDT
Created attachment 161898 [details]
Patch
Comment 67 Gyuyoung Kim 2012-09-03 04:34:04 PDT
Comment on attachment 161898 [details]
Patch

LGTM.
Comment 68 Kenneth Rohde Christiansen 2012-09-03 05:06:05 PDT
Comment on attachment 161898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161898&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:34
> +Eina_Bool ewk_settings_enable_scripts_set(Ewk_Settings* settings, Eina_Bool enable)

Not the best name. Scripts? which scripts? enable_scripting makes a lot more sense!

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:50
> +Eina_Bool ewk_settings_auto_load_images_set(Ewk_Settings* settings, Eina_Bool automatic)

why not load_images_automatically? It is a lot easier to understand. It is ok though
Comment 69 Eunmi Lee 2012-09-03 05:13:25 PDT
(In reply to comment #68)
> (From update of attachment 161898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161898&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:34
> > +Eina_Bool ewk_settings_enable_scripts_set(Ewk_Settings* settings, Eina_Bool enable)
> 
> Not the best name. Scripts? which scripts? enable_scripting makes a lot more sense!
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:50
> > +Eina_Bool ewk_settings_auto_load_images_set(Ewk_Settings* settings, Eina_Bool automatic)
> 
> why not load_images_automatically? It is a lot easier to understand. It is ok though

I've chosen those names to synchronize with WK1 APIs.
Do you think it is better to choose new name for WK2 APIs?
Comment 70 Eunmi Lee 2012-09-03 06:09:49 PDT
(In reply to comment #69)
> (In reply to comment #68)
> > (From update of attachment 161898 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161898&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:34
> > > +Eina_Bool ewk_settings_enable_scripts_set(Ewk_Settings* settings, Eina_Bool enable)
> > 
> > Not the best name. Scripts? which scripts? enable_scripting makes a lot more sense!
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:50
> > > +Eina_Bool ewk_settings_auto_load_images_set(Ewk_Settings* settings, Eina_Bool automatic)
> > 
> > why not load_images_automatically? It is a lot easier to understand. It is ok though
> 
> I've chosen those names to synchronize with WK1 APIs.
> Do you think it is better to choose new name for WK2 APIs?

I've decided to give better name to WK2 API without matching name with WK1 API.
Comment 71 Eunmi Lee 2012-09-03 06:13:40 PDT
Created attachment 161913 [details]
Patch
Comment 72 Eunmi Lee 2012-09-03 06:17:31 PDT
Created attachment 161915 [details]
Patch
Comment 73 WebKit Review Bot 2012-09-03 07:48:22 PDT
Comment on attachment 161915 [details]
Patch

Clearing flags on attachment: 161915

Committed r127422: <http://trac.webkit.org/changeset/127422>
Comment 74 WebKit Review Bot 2012-09-03 07:48:30 PDT
All reviewed patches have been landed.  Closing bug.