Bug 91206 – [EFL][WK2] Add ewk_settings.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 152196 [details]
patch for ewk_setting.

[WebKit Review Bot]

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.

[Eunmi Lee]

Created attachment 152198 [details]
patch for ewk_setting.

Sytle error is fixed.

[Chris Dumez]

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.

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 152408 [details]
patch for ewk_settings.

Fixed for Christophe's comment.

[Eunmi Lee]

Created attachment 152413 [details]
patch for ewk_settings.

rebased.

[Chris Dumez]

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?

[Eunmi Lee]

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

[Chris Dumez]

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

[Eunmi Lee]

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

[Gyuyoung Kim]

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.

[Eunmi Lee]

Created attachment 152420 [details]
patch for ewk_settings.

fixed for Christophe's comments.

[Gyuyoung Kim]

CC'ing Grzegorz, could you take a look this APIs comments ?

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 152422 [details]
patch for ewk_settings.

Fixed for Gyuyoung's comment.

[Chris Dumez]

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.

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 152424 [details]
patch for ewk_settings.

Fixed for Christophe's comments.

[Chris Dumez]

Comment on attachment 152424 [details]
patch for ewk_settings.

LGTM.

[Gyuyoung Kim]

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.

[Chris Dumez]

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

[Jongseok Yang]

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?

[Gyuyoung Kim]

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

[Chris Dumez]

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

[Chris Dumez]

(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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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

[Grzegorz Czajkowski]

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?

[Chris Dumez]

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

[Jongseok Yang]

(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

[Chris Dumez]

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

[Jongseok Yang]

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

[Chris Dumez]

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

[Eunmi Lee]

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

[Eunmi Lee]

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

[Ryuan Choi]

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

[Eunmi Lee]

(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

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 152938 [details]
Patch

[Eunmi Lee]

Created attachment 152951 [details]
Rebased patch.

[Alexander Shalamov]

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.

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 157896 [details]
Rebased

[Raphael Kubo da Costa (:rakuco)]

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.

[Eunmi Lee]

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 :)

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 157930 [details]
Patch

[Gyuyoung Kim]

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.

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 158004 [details]
Patch

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

I add this case(!!) to WebKit EFL coding style. :) If there is something wrong, please fix it. Thanks.

https://trac.webkit.org/wiki/EFLWebKitCodingStyle

[Eunmi Lee]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Gyuyoung Kim]

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

[Ryuan Choi]

LGTM, too.

[Gyuyoung Kim]

Comment on attachment 158004 [details]
Patch

Eunmi, could you add unit test for this patch ?

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 160600 [details]
Patch

[Gyuyoung Kim]

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()));

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 161895 [details]
Patch

[Eunmi Lee]

Created attachment 161898 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161898 [details]
Patch

LGTM.

[Kenneth Rohde Christiansen]

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

[Eunmi Lee]

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

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 161913 [details]
Patch

[Eunmi Lee]

Created attachment 161915 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161915 [details]
Patch

Clearing flags on attachment: 161915

Committed r127422: <http://trac.webkit.org/changeset/127422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.