Bug 96974 – [EFL][WK2] Add setting to toggle developer extensions

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

[Seokju Kwon]

This is required to allow inspecting page.

[Seokju Kwon]

Created attachment 164490 [details]
Patch

[Chris Dumez]

Comment on attachment 164490 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Add APIs to enable or disable developerExtraEnabled

How about "Add setting to toggle developer extensions" ?

> Source/WebKit2/ChangeLog:8
> +        This is required to allow inspecting page.

"inspecting page" ? Web Inspector page?

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

You're adding public API, please add unit tests for this.

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

We should indicate the default value for this setting.

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

We should indicate the default value for this setting.

[Jinwoo Song]

Comment on attachment 164490 [details]
Patch

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

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

s/const Ewk_Settings* settings/Ewk_Settings *settings

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

s/const Ewk_Settings/Ewk_Settings

[Seokju Kwon]

Created attachment 164684 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164684 [details]
Patch

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

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

I think "ewk_settings_developer_extras_enabled_set" is consistent with existing WK2's ewk_settings API naming.

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

ditto.

[Seokju Kwon]

(In reply to comment #5)
> (From update of attachment 164684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164684&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:123
> > +EAPI Eina_Bool ewk_settings_enable_developer_extras_set(Ewk_Settings *settings, Eina_Bool enable);
> 
> I think "ewk_settings_developer_extras_enabled_set" is consistent with existing WK2's ewk_settings API naming.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:134
> > +EAPI Eina_Bool ewk_settings_enable_developer_extras_get(const Ewk_Settings *settings);
> 
> ditto.

Ok. I will rename it for consistency

[Seokju Kwon]

Created attachment 164830 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164830 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:116
> + * Default value for developer extensions setting is @c EINA_FALSE.

Looks better to follow WK1's comment style,

 * By default, the value is @c ~/.webkit
 * By default, this value is 1MB.
 * By default, applications are allowed unlimited storage space.
 * By default, the cache is enabled.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:128
> + * Default value for developer extensions setting is @c EINA_FALSE.

ditto.

[Seokju Kwon]

(In reply to comment #8)
> (From update of attachment 164830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164830&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:116
> > + * Default value for developer extensions setting is @c EINA_FALSE.
> 
> Looks better to follow WK1's comment style,
> 
>  * By default, the value is @c ~/.webkit
>  * By default, this value is 1MB.
>  * By default, applications are allowed unlimited storage space.
>  * By default, the cache is enabled.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:128
> > + * Default value for developer extensions setting is @c EINA_FALSE.
> 
> ditto.

All done.

[Seokju Kwon]

Created attachment 165003 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 165003 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:123
> + * @return @c EINA_TRUE on success or @EINA_FALSE on failure
> + */
> +EAPI Eina_Bool ewk_settings_developer_extras_enabled_set(Ewk_Settings *settings, Eina_Bool enable);

I know this is consistent with the rest of the api, but it actually only retursn false when the API is *used wrongly*. A crash or assert would even be better. I think we should change all these _set apis to not return anything

[Ryuan Choi]

Comment on attachment 165003 [details]
Patch

Personally, I have same opinion with kenneth about returning of _set api.

But, I think that we can land this and need to discuss in mailing lists for it.

[WebKit Review Bot]

Comment on attachment 165003 [details]
Patch

Clearing flags on attachment: 165003

Committed r129327: <http://trac.webkit.org/changeset/129327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.