Bug 100213 – [WK2][EFL] Add ewk_view_pagination_mode_set/get() APIs

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

[KyungTae Kim]

Add ewk_pagination_mode_set/ewk_view_pagination_mode_get APIs.
The  WK APIs for pagination mode set/get was existed, and we need ewk APIs for those.

[KyungTae Kim]

Created attachment 170340 [details]
Patch

[Jinwoo Song]

Comment on attachment 170340 [details]
Patch

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

> Source/WebKit2/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=100213

ewk_view_pagination_mode_set/get() APIs?

> Source/WebKit2/ChangeLog:5
> +

remove redundant space after 'Add'.

> Source/WebKit2/ChangeLog:8
> +        Add ewk_pagination_mode_set/ewk_view_pagination_mode_get APIs for paginated display.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:812
> + *

Remove redundant space after 'a'.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:814
> + * @param mode The  Ewk_Pagination_Mode to set

Remove redundant space in the line.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:821
> + * Get a  pagenation mode of the current web page.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:825
> + * @return The  pagination mode of the current web page

ditto.

[Jinwoo Song]

Comment on attachment 170340 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:2207
> +        paginationMode =WebCore::Pagination::LeftToRightPaginated;

add space after '='

[Chris Dumez]

Comment on attachment 170340 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:2200
> +    

The code in these functions should be moved inside methods in EwkViewImpl and the C function should merely call the new C++ method.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:2203
> +    case EWK_PAGINATION_MODE_UNPAGINATED:

You should probably use COMPILE_ASSERT_MATCHING_ENUM macro from ewk_private.h to avoid this switch(). A simple cast would be more efficient.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:2228
> +    Ewk_Pagination_Mode mode;

Unused variable?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:2230
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, EWK_PAGINATION_MODE_END);

Why return EWK_PAGINATION_MODE_END in case of error? Shouldn't you use a mode such as EWK_PAGINATION_MODE_INVALID with value of -1?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:275
> + * @info    Keep this in sync with WebCore::Pagination::Mode in Pagination.h

COMPILE_ASSERT_MATCHING_ENUM() will make sure of that. I don't think we should have such comment in the public header (this is implementation specific).

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:278
> +    EWK_PAGINATION_MODE_UNPAGINATED,

Lack of documentation for each enum value.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:283
> +    EWK_PAGINATION_MODE_END    

What is this mode?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:811
> + * Set a  pagenation mode to the current web page.

"pagenation" > "pagination"
remove the "a" in front of pagination (bad english)

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:818
> +EAPI Eina_Bool ewk_view_pagination_mode_set(Evas_Object* o, Ewk_Pagination_Mode mode);

Star on wrong side

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:821
>> + * Get a  pagenation mode of the current web page.
> 
> ditto.

"pagenation" > "pagination"
remove the "a" in front of pagination (bad english)

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:822
> + *

Please document what the default pagination mode is.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:823
> + * @param o view object toget the pagenation mode

"toget" -> "to get"
"pagenation" > "pagination"

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:827
> +EAPI Ewk_Pagination_Mode ewk_view_pagination_mode_get(const Evas_Object* o);

Star on wrong side

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:972
> +    // Reload page to check the pagination modeue.

"modeue" -> "mode"

[KyungTae Kim]

Created attachment 170572 [details]
Patch

[Chris Dumez]

Comment on attachment 170572 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1510
> +    return static_cast<Ewk_Pagination_Mode>(impl->pageProxy->paginationMode());

Blank line before return statement would be nice.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:283
> +    EWK_PAGINATION_MODE_INVALID /**< invalid pagination mode that will be returned when error occured. */

We usually put this one first with "= -1" value I believe.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:821
> + * Get pagination mode of the current web page. The default value is EWK_PAGINATION_MODE_UNPAGINATED.

"The default value is EWK_PAGINATION_MODE_UNPAGINATED." should probably not be in the title. Instead, I would put it under, after a blank line.

[KyungTae Kim]

Created attachment 170582 [details]
Patch

[Chris Dumez]

Comment on attachment 170582 [details]
Patch

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

LGTM but:

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:283
> +    EWK_PAGINATION_MODE_END /**< number of pagination mode */

Why do you need this EWK_PAGINATION_MODE_END ? I think we should remove it.

[Mikhail Pozdnyakov]

Comment on attachment 170582 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:283
>> +    EWK_PAGINATION_MODE_END /**< number of pagination mode */
> 
> Why do you need this EWK_PAGINATION_MODE_END ? I think we should remove it.

Agree with Chris.

[KyungTae Kim]

I thought EWK_PAGINATION_MODE_END is needed when someone needed to change the pagination mode step by step.
If it is not a common way, I'll remove it and let use "EWK_PAGINATION_MODE_BOTTOM_TO_TOP + 1" in that case.

[KyungTae Kim]

Created attachment 170603 [details]
Patch

[Chris Dumez]

Comment on attachment 170603 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 170603 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:808
> +

Nit : Unneeded line.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:970
> +

ditto.

[KyungTae Kim]

Created attachment 170621 [details]
Patch

[WebKit Review Bot]

Comment on attachment 170621 [details]
Patch

Clearing flags on attachment: 170621

Committed r132479: <http://trac.webkit.org/changeset/132479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.