Bug 96974

Summary: [EFL][WK2] Add setting to toggle developer extensions
Product: WebKit Reporter: Seokju Kwon <seokju.kwon>
Component: WebKit EFLAssignee: Seokju Kwon <seokju.kwon>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Seokju Kwon 2012-09-17 20:25:58 PDT
This is required to allow inspecting page.
Comment 1 Seokju Kwon 2012-09-17 21:44:53 PDT
Created attachment 164490 [details]
Patch
Comment 2 Chris Dumez 2012-09-18 06:00:28 PDT
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.
Comment 3 Jinwoo Song 2012-09-18 06:06:01 PDT
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
Comment 4 Seokju Kwon 2012-09-19 01:09:07 PDT
Created attachment 164684 [details]
Patch
Comment 5 Gyuyoung Kim 2012-09-19 01:14:23 PDT
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.
Comment 6 Seokju Kwon 2012-09-19 16:46:41 PDT
(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
Comment 7 Seokju Kwon 2012-09-19 20:09:11 PDT
Created attachment 164830 [details]
Patch
Comment 8 Gyuyoung Kim 2012-09-19 21:44:26 PDT
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.
Comment 9 Seokju Kwon 2012-09-20 15:45:21 PDT
(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.
Comment 10 Seokju Kwon 2012-09-20 15:47:40 PDT
Created attachment 165003 [details]
Patch
Comment 11 Kenneth Rohde Christiansen 2012-09-20 23:15:27 PDT
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
Comment 12 Ryuan Choi 2012-09-23 23:22:21 PDT
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.
Comment 13 WebKit Review Bot 2012-09-23 23:27:29 PDT
Comment on attachment 165003 [details]
Patch

Clearing flags on attachment: 165003

Committed r129327: <http://trac.webkit.org/changeset/129327>
Comment 14 WebKit Review Bot 2012-09-23 23:27:33 PDT
All reviewed patches have been landed.  Closing bug.