Bug 90928 – [EFL] Add APIs to support ORIENTATION_EVENTS

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

[Ryuan Choi]

Enable ORIENTATION_EVENTS and add APIs to get/set orientation to webkit/efl.

[Ryuan Choi]

Created attachment 151571 [details]
Patch

[Ryuan Choi]

Created attachment 151648 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 151648 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:2015
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData);

Is it better to use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);

> Source/WebKit/efl/ewk/ewk_view.cpp:2033
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData);

Is it better to use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1) ?

> Source/WebKit/efl/ewk/ewk_view.h:1507
> + * @return @c EINA_TRUE on success or @c EINA_FALSE otherwise.

When target degree is same with current degree, false is returned. So, I think this is better to mention it to here as well.

[Gyuyoung Kim]

CC'ing Grzegorz, could you take a look this API description ?

[Ryuan Choi]

Created attachment 151946 [details]
Patch

[Ryuan Choi]

(In reply to comment #3)
> (From update of attachment 151648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151648&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:2015
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData);
> 
> Is it better to use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);
It's my mistake. fixed

> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:2033
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData);
> 
> Is it better to use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1) ?
Ditto.

> 
> > Source/WebKit/efl/ewk/ewk_view.h:1507
> > + * @return @c EINA_TRUE on success or @c EINA_FALSE otherwise.
> 
> When target degree is same with current degree, false is returned. So, I think this is better to mention it to here as well.
Agree. I tried to mention it.

[Thiago Marcos P. Santos]

C++ unit tests?

[Grzegorz Czajkowski]

Comment on attachment 151946 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Could you add a brief description of this API? its purpose etc.

> Source/WebKit/efl/ewk/ewk_view.cpp:2019
> +        return false;

I think it's wrong practice to return false (the same value is returned in case of errors here) when we want to set a new value that has been already set before. We shouldn't treat it as the error.

> Source/WebKit/efl/ewk/ewk_view.h:1510
> +EAPI Eina_Bool ewk_view_orientation_set(Evas_Object *o, int orientation);

If this API allows to set four kind of values wouldn't it be better to expose those values only? for instance enums ? Then API usage and its description will be simpler.

[Ryuan Choi]

(In reply to comment #7)
> C++ unit tests?

Do you mean the gtest? I will try it!

(In reply to comment #8)
> (From update of attachment 151946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151946&action=review
> 
> > Source/WebKit/efl/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> Could you add a brief description of this API? its purpose etc.
> 
OK, I will.

> > Source/WebKit/efl/ewk/ewk_view.cpp:2019
> > +        return false;
> 
> I think it's wrong practice to return false (the same value is returned in case of errors here) when we want to set a new value that has been already set before. We shouldn't treat it as the error.
> 
Hmm, this is really what I intended.
This API is for generating onorientationchange javascript event to web contents.
Because I want not to send onorientationchange event when same orientation was set, I thought that this API should notify false to application developers.
 
But, while thinking more, we don't do that because it is extra intention which javascript doesn't do.

If then, I think taht ewk_view_orientation_send looks enought.
How do you think about this?

> > Source/WebKit/efl/ewk/ewk_view.h:1510
> > +EAPI Eina_Bool ewk_view_orientation_set(Evas_Object *o, int orientation);
> 
> If this API allows to set four kind of values wouldn't it be better to expose those values only? for instance enums ? Then API usage and its description will be simpler.

As I commented Bug 90950, It can be limited as EWK_ORIENTATION_0, EWK_ORIENTATION_90, EWK_ORIENTATION_180, EWK_ORIENTATION_270.

How do you think about them?

[Ryuan Choi]

Created attachment 152383 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 152383 [details]
Patch

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

Thanks for pioneering writing utests for EFL. :)

> Source/WebKit/efl/tests/test_ewk_view.cpp:65
> +    ewk_view_orientation_send(o, 0);
> +
> +    // FIXME: We need a better way to receive the result from web content.
> +    const Ewk_Text_With_Direction* title = ewk_view_title_get(o);
> +    EXPECT_STREQ("window.orientation = 0", title->string);

Using title changes in utests has become a standard practice. Qt and GTK does that, although it might sound hackish, I see no problem. IMO you don't need the FIXME.

Maybe it is a good idea to protect this part with: "#if ENABLE(ORIENTATION_EVENTS)". Otherwise tests will fail when the feature is disabled.

[Ryuan Choi]

Created attachment 152444 [details]
Patch

[Ryuan Choi]

(In reply to comment #11)
> (From update of attachment 152383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152383&action=review
> 
> Thanks for pioneering writing utests for EFL. :)
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:65
> > +    ewk_view_orientation_send(o, 0);
> > +
> > +    // FIXME: We need a better way to receive the result from web content.
> > +    const Ewk_Text_With_Direction* title = ewk_view_title_get(o);
> > +    EXPECT_STREQ("window.orientation = 0", title->string);
> 
> Using title changes in utests has become a standard practice. Qt and GTK does that, although it might sound hackish, I see no problem. IMO you don't need the FIXME.

OK. I removed.

> 
> Maybe it is a good idea to protect this part with: "#if ENABLE(ORIENTATION_EVENTS)". Otherwise tests will fail when the feature is disabled.

My mistake. fixed.

[Gyuyoung Kim]

Comment on attachment 152444 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:2017
> +

If current orientation is same with orientation, I think we don't need to send orientation event.

It seems to me you need to add below code.

if (priv->mainFrame->orientation() == orientation)
    return;

> Source/WebKit/efl/ewk/ewk_view.h:1503
> + *                      90 degrees when it's left side is at the top,

if possible, is it better to adhere indentation for comment ?

As this below,

orientation will be  0 degrees when the device is oriented to natural position,
                              90 degrees when it's left side is at the top,
                              -90 degrees when it's right side is at the top,
                              180 degrees when it is upside down.

In addition, there are two spaces in *be   0*.

[Ryuan Choi]

(In reply to comment #14)
> (From update of attachment 152444 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152444&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:2017
> > +
> 
> If current orientation is same with orientation, I think we don't need to send orientation event.
> 
> It seems to me you need to add below code.
> 
> if (priv->mainFrame->orientation() == orientation)
>     return;
> 
It was in my previous patch and this limitation is helpful to avoid logical error

But I change my mind because I am not sure whether we should limit only for orientationchange.

If I am right, We didn't limit other events such as mouse down.
For example, applications may send ewk_frame_feed_mouse_down twice without mouse_up.

And also, we can generate events in javascript side similarly.
In web page, they can generate same events twice including mouse, keyboad and orientationchange also.

Anyway, because it's not important, I will add this if you want.

> > Source/WebKit/efl/ewk/ewk_view.h:1503
> > + *                      90 degrees when it's left side is at the top,
> 
> if possible, is it better to adhere indentation for comment ?
> 
> As this below,
> 
> orientation will be  0 degrees when the device is oriented to natural position,
>                               90 degrees when it's left side is at the top,
>                               -90 degrees when it's right side is at the top,
>                               180 degrees when it is upside down.
> 
> In addition, there are two spaces in *be   0*.

I thought it is more readable. but I will fix it.

Thank you.

[Gyuyoung Kim]

Comment on attachment 152444 [details]
Patch

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

>>> Source/WebKit/efl/ewk/ewk_view.cpp:2017
>>> +
>> 
>> If current orientation is same with orientation, I think we don't need to send orientation event.
>> 
>> It seems to me you need to add below code.
>> 
>> if (priv->mainFrame->orientation() == orientation)
>>     return;
> 
> It was in my previous patch and this limitation is helpful to avoid logical error
> 
> But I change my mind because I am not sure whether we should limit only for orientationchange.
> 
> If I am right, We didn't limit other events such as mouse down.
> For example, applications may send ewk_frame_feed_mouse_down twice without mouse_up.
> 
> And also, we can generate events in javascript side similarly.
> In web page, they can generate same events twice including mouse, keyboad and orientationchange also.
> 
> Anyway, because it's not important, I will add this if you want.

In my opinion, we don't need to send events when orientation is already same with target orientation. In addition, blackberry port has same logic.

[Ryuan Choi]

Created attachment 152674 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152674 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.h:1502
> + * orientation will be 0 degrees when the device is oriented to natural position,

I just wanted to use below,

orientation will be 0 degrees when the device is oriented to natural position,
                    90 degrees when it's left side is at the top,
                   -90 degrees when it's right side is at the top,
                    180 degrees when it is upside down

[Ryuan Choi]

Created attachment 152765 [details]
Patch

[Ryuan Choi]

(In reply to comment #18)
> (From update of attachment 152674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152674&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:1502
> > + * orientation will be 0 degrees when the device is oriented to natural position,
> 
> I just wanted to use below,
> 
> orientation will be 0 degrees when the device is oriented to natural position,
>                     90 degrees when it's left side is at the top,
>                    -90 degrees when it's right side is at the top,
>                     180 degrees when it is upside down

I misunderstand your comment.
Updated like you mentioned.

[Gyuyoung Kim]

Comment on attachment 152765 [details]
Patch

Looks good to me now. Thanks.

[Gyuyoung Kim]

Comment on attachment 152765 [details]
Patch

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

> Source/WebKit/efl/tests/test_ewk_view.cpp:58
> +* @brief Checking whether function send orientation to contents.

Add a space before *.

> Source/WebKit/efl/tests/test_ewk_view.cpp:60
> +static void ewkViewOrientationSendCb(void* eventInfo, Evas_Object* o, void* data)

Please use full name for parameter name.

[Gyuyoung Kim]

Comment on attachment 152765 [details]
Patch

Cleared review? from attachment 152765 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Ryuan Choi]

ewebkit1 was dropped