Bug 98793 – [EFL][WK2] Revisit setting API names and documentation

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

[Jinwoo Song]

API names are inconsistent and some comment of parameters are missing.

[Jinwoo Song]

Created attachment 167834 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167834 [details]
Patch

Attachment 167834 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14249098

New failing tests:
accessibility/media-controls.html

[Ryuan Choi]

Thank you.

I didn't follow this decision while upstreaming tizen code.

LGTM.

[Jinwoo Song]

Created attachment 167900 [details]
patch for landing.

[Laszlo Gombos]

Comment on attachment 167900 [details]
patch for landing.

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Refine the setting API names and comments

Technically it might be a comment in the code, but I think it is more descriptive to use the word documentation instead of comment - as the patch fixes API documentation.

How about "Revisit setting API names and documentation"

> Source/WebKit2/ChangeLog:9
> +        and some comment of parameters are missing.

--> "add missing documentation for API arguments"

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:74
> + * By default, the javascript excuting is enabled.

--> "JavaScript execution is enabled".

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:85
> + * Returns whether the javascript is executable or not.

Returns whether JavaScript execution is enabled.

[Grzegorz Czajkowski]

Frankly speaking I like the original API syntax which you are changing at the moment.

IMHO ewk_settings_enable_frame_flattening_set sounds/looks better than
ewk_settings_frame_flattening_enabled_set because:

1) WK1 uses similar API, for example: ewk_view_setting_enable_frame_flattening_set
2) Verb next to another verb (enabled_set) doesn't looks/sounds well.
3) I'd rather prefer not using plural in API (enabled -> enable)
4) The Spelling API  bug 91854 proposes API syntax based on above suggestions (as during work on this there wasn't ewk2-settings yet). So we will have consistent API.

I suppose author of an origin API has created it mainly based on WKPreferences.h: 
WKPreferencesSetFrameFlatteningEnabled -> ewk_settings_frame_flattening_enabled_set

If there is no objections I'd rather adjust the other API similar to ewk_settings_enable_frame_flattening_set

Feel free to express your opinion about it.

[Jinwoo Song]

(In reply to comment #5)
> (From update of attachment 167900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167900&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] Refine the setting API names and comments
> 
> Technically it might be a comment in the code, but I think it is more descriptive to use the word documentation instead of comment - as the patch fixes API documentation.
> 
> How about "Revisit setting API names and documentation"
> 
Exactly right, I'll change the title as your suggestion. 

> > Source/WebKit2/ChangeLog:9
> > +        and some comment of parameters are missing.
> 
> --> "add missing documentation for API arguments"
> 
I'll fix it.

> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:74
> > + * By default, the javascript excuting is enabled.
> 
> --> "JavaScript execution is enabled".
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:85
> > + * Returns whether the javascript is executable or not.
> 
> Returns whether JavaScript execution is enabled.
I'll fix it.

[Jinwoo Song]

(In reply to comment #6)
> Frankly speaking I like the original API syntax which you are changing at the moment.
> 
> IMHO ewk_settings_enable_frame_flattening_set sounds/looks better than
> ewk_settings_frame_flattening_enabled_set because:
> 
> 1) WK1 uses similar API, for example: ewk_view_setting_enable_frame_flattening_set
> 2) Verb next to another verb (enabled_set) doesn't looks/sounds well.
> 3) I'd rather prefer not using plural in API (enabled -> enable)
> 4) The Spelling API  bug 91854 proposes API syntax based on above suggestions (as during work on this there wasn't ewk2-settings yet). So we will have consistent API.
> 
> I suppose author of an origin API has created it mainly based on WKPreferences.h: 
> WKPreferencesSetFrameFlatteningEnabled -> ewk_settings_frame_flattening_enabled_set
> 
> If there is no objections I'd rather adjust the other API similar to ewk_settings_enable_frame_flattening_set
> 
> Feel free to express your opinion about it.

I think your guessing is right that API authors referenced the WKPreferences APIs.
Then I'll ask the opinions about the naming through the webkit-efl mailing list and change the API names according to the consensus.
Actually, I vote to your opinion. :) 
FYI, GTK uses set_enable_frame_flattening() and Qt uses setFrameFlatteningEnabled().

[Grzegorz Czajkowski]

(In reply to comment #8)
> (In reply to comment #6)
> > Frankly speaking I like the original API syntax which you are changing at the moment.
> > 
> > IMHO ewk_settings_enable_frame_flattening_set sounds/looks better than
> > ewk_settings_frame_flattening_enabled_set because:
> > 
> > 1) WK1 uses similar API, for example: ewk_view_setting_enable_frame_flattening_set
> > 2) Verb next to another verb (enabled_set) doesn't looks/sounds well.
> > 3) I'd rather prefer not using plural in API (enabled -> enable)
> > 4) The Spelling API  bug 91854 proposes API syntax based on above suggestions (as during work on this there wasn't ewk2-settings yet). So we will have consistent API.
> > 
> > I suppose author of an origin API has created it mainly based on WKPreferences.h: 
> > WKPreferencesSetFrameFlatteningEnabled -> ewk_settings_frame_flattening_enabled_set
> > 
> > If there is no objections I'd rather adjust the other API similar to ewk_settings_enable_frame_flattening_set
> > 
> > Feel free to express your opinion about it.
> 
> I think your guessing is right that API authors referenced the WKPreferences APIs.
> Then I'll ask the opinions about the naming through the webkit-efl mailing list and change the API names according to the consensus.
> Actually, I vote to your opinion. :) 
Ok thanks, I will support you :)

[Grzegorz Czajkowski]

Comment on attachment 167900 [details]
patch for landing.

Clearing flags as API syntax is being discussed.

[Grzegorz Czajkowski]

Summarizing discussion (http://lists.webkit.org/pipermail/webkit-efl/2012-October/000336.html) WK2-EFL should keep consistent with the vast majority of Efl libraries:

ewk_foo_bar_enabled_{get,set}

Jinwoo,
Your idea was ok and we have approval to change the API as you proposed so please apply Laszlo's suggestions and submit the patch.

[Jinwoo Song]

(In reply to comment #11)
> Summarizing discussion (http://lists.webkit.org/pipermail/webkit-efl/2012-October/000336.html) WK2-EFL should keep consistent with the vast majority of Efl libraries:
> 
> ewk_foo_bar_enabled_{get,set}
> 
> Jinwoo,
> Your idea was ok and we have approval to change the API as you proposed so please apply Laszlo's suggestions and submit the patch.

Okay, I'll upload patch.

[Jinwoo Song]

Created attachment 168161 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168161 [details]
Patch

Clearing flags on attachment: 168161

Committed r131031: <http://trac.webkit.org/changeset/131031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.