Bug 67109 – [EFL] Queued scroll feature

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Kamil Blank]

Implementation of weak scroll feature which allows WebKit to move viewport without informing WebCore about this fact. 
Scroll requests will be processed when disabling weak scroll.

[Kamil Blank]

Created attachment 105463 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

The API needs some love IMO.

 * ewk_frame_scroll_add now seems to have a complete different code path depending on whether weak scrolling is enabled, and one of the code paths now relies on ewk_view (so far ewk_frame calls ewk_view as little as possible, usually only to propagate signals). It's a bit hard to follow the code with those 1, 2 and 3-letter variables with similar names.
 * It was not clear at first sight what ewk_frame_weak_scroll_moving_viewport_get was supposed to do.
 * How is one expected to use this API in an application?
 * The purpose of this code seems to be to queue scroll requests and do them all at once at some point. If the performance gains are worthwhile, doesn't it make sense to make this the only possible behavior? How about processing the scroll requests in the main loop?
 * Does any other port implement this? If so, might it be something to add to WebCore?

[Kamil Blank]

(In reply to comment #2)
> The API needs some love IMO.
> 
>  * ewk_frame_scroll_add now seems to have a complete different code path depending on whether weak scrolling is enabled, and one of the code paths now relies on ewk_view (so far ewk_frame calls ewk_view as little as possible, usually only to propagate signals). It's a bit hard to follow the code with those 1, 2 and 3-letter variables with similar names.

I followed WebKit-EFL convention when choosing names of variables:
vw, vh - viewport
sx sy - scroll
fw, fh - frame
sdx, sdy - scroll delta

>  * It was not clear at first sight what ewk_frame_weak_scroll_moving_viewport_get was supposed to do.

Do you mean lack of comments or the function's name could be better?
I guess if you look at it as a part of weak scroll feature and you realize how this feature works it's quite understandable but of course I'm ready to improve it if necessary :)

>  * How is one expected to use this API in an application?

It's dedicated to be used during panning to avoid processing many scroll requests following each other.
Typical scenario:
//start panning
ewk_frame_weak_scroll_enabled_set(..., EINA_TRUE) - from now on only viewport will be moved, no scroll actions
ewk_frame_scroll_add(...)
ewk_frame_scroll_add(...)
ewk_frame_scroll_add(...)
...
ewk_frame_scroll_add(...)
// finish panning
ewk_frame_weak_scroll_enabled_set(..., EINA_FALSE) - now all previous scroll requests will be processed by WebCore but EWK won't move viewport which was already done - to be clear, it'll be just 1 request contains sum of skipped requests. 

>  * The purpose of this code seems to be to queue scroll requests and do them all at once at some point. If the performance gains are worthwhile, doesn't it make sense to make this the only possible behavior? How about processing the scroll requests in the main loop?

It can't be used as natural behavior because during weak scroll some actions will be missed due to skipping scroll events passing into WebCore like repaints, java script code. Due to this reason I don't think it's possible to queue these requests in normal usage.
This feature is a kind of enhancement - just like fast mobile scrolling - but not for general use.

>  * Does any other port implement this? If so, might it be something to add to WebCore?

I don't know about other ports doing such things. Moreover it looks like a specific feature which can be used by EFL port which uses internal backing store.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> (In reply to comment #2)
> > The API needs some love IMO.
> > 
> >  * ewk_frame_scroll_add now seems to have a complete different code path depending on whether weak scrolling is enabled, and one of the code paths now relies on ewk_view (so far ewk_frame calls ewk_view as little as possible, usually only to propagate signals). It's a bit hard to follow the code with those 1, 2 and 3-letter variables with similar names.
> 
> I followed WebKit-EFL convention when choosing names of variables:
> vw, vh - viewport
> sx sy - scroll
> fw, fh - frame
> sdx, sdy - scroll delta

Trying to think from the point of view of other people working on the code, the convention might not immediately make sense. If you prefer to follow the conventions, I'd recommend at least adding some comments explaining either the variable names or what that block of code is supposed to do.
 
> >  * It was not clear at first sight what ewk_frame_weak_scroll_moving_viewport_get was supposed to do.
> 
> Do you mean lack of comments or the function's name could be better?
> I guess if you look at it as a part of weak scroll feature and you realize how this feature works it's quite understandable but of course I'm ready to improve it if necessary :)

The function name could be better. Does it mean the viewport is currently being moved? Does it only tell ewk_view_scroll it shouldn't do anything?

Similarly, now thinking from the point of view of the end user of this API, wouldn't it be clearer if you just had a call like "ewk_frame_scroll_policy_set(..., EWK_FRAME_QUEUE_SCROLLS)"?

> >  * How is one expected to use this API in an application?
> 
> It's dedicated to be used during panning to avoid processing many scroll requests following each other.
> Typical scenario:
> //start panning
> ewk_frame_weak_scroll_enabled_set(..., EINA_TRUE) - from now on only viewport will be moved, no scroll actions
> ewk_frame_scroll_add(...)
> ewk_frame_scroll_add(...)
> ewk_frame_scroll_add(...)
> ...
> ewk_frame_scroll_add(...)
> // finish panning
> ewk_frame_weak_scroll_enabled_set(..., EINA_FALSE) - now all previous scroll requests will be processed by WebCore but EWK won't move viewport which was already done - to be clear, it'll be just 1 request contains sum of skipped requests.

What I find weird in this example is that a function which was just supposed to change the value of a flag (ewk_frame_weak_scroll_enabled_set) ends up also triggering a scrolling event depending on the value of the boolean parameter. This is not what one would expect at first. Wouldn't it be better to do this kind of processing automatically during the main loop?

> >  * Does any other port implement this? If so, might it be something to add to WebCore?
> 
> I don't know about other ports doing such things. Moreover it looks like a specific feature which can be used by EFL port which uses internal backing store.

Sad :(

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 105463 [details]
patch

Clearing flags while things are sorted out.

[Kamil Blank]

Created attachment 113442 [details]
patch

[Kamil Blank]

Hi,

Thank you for your comments and sorry for the late reply.

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > The API needs some love IMO.
> > > 
> > >  * ewk_frame_scroll_add now seems to have a complete different code path depending on whether weak scrolling is enabled, and one of the code paths now relies on ewk_view (so far ewk_frame calls ewk_view as little as possible, usually only to propagate signals). It's a bit hard to follow the code with those 1, 2 and 3-letter variables with similar names.
> > 
> > I followed WebKit-EFL convention when choosing names of variables:
> > vw, vh - viewport
> > sx sy - scroll
> > fw, fh - frame
> > sdx, sdy - scroll delta
> 
> Trying to think from the point of view of other people working on the code, the convention might not immediately make sense. If you prefer to follow the conventions, I'd recommend at least adding some comments explaining either the variable names or what that block of code is supposed to do.

Due to name changes in ewk I also renamed used variables.

> 
> > >  * It was not clear at first sight what ewk_frame_weak_scroll_moving_viewport_get was supposed to do.
> > 
> > Do you mean lack of comments or the function's name could be better?
> > I guess if you look at it as a part of weak scroll feature and you realize how this feature works it's quite understandable but of course I'm ready to improve it if necessary :)
> 
> The function name could be better. Does it mean the viewport is currently being moved? Does it only tell ewk_view_scroll it shouldn't do anything?

I added "_allowed" suffix.

> 

> Similarly, now thinking from the point of view of the end user of this API, wouldn't it be clearer if you just had a call like "ewk_frame_scroll_policy_set(..., EWK_FRAME_QUEUE_SCROLLS)"?

Done. In ewk_frame_scroll_policy_set I used Eina_Bool instead of enum to set   whether scrolls will be queued or not.


> > >  * How is one expected to use this API in an application?
> > 
> > It's dedicated to be used during panning to avoid processing many scroll requests following each other.
> > Typical scenario:
> > //start panning
> > ewk_frame_weak_scroll_enabled_set(..., EINA_TRUE) - from now on only viewport will be moved, no scroll actions
> > ewk_frame_scroll_add(...)
> > ewk_frame_scroll_add(...)
> > ewk_frame_scroll_add(...)
> > ...
> > ewk_frame_scroll_add(...)
> > // finish panning
> > ewk_frame_weak_scroll_enabled_set(..., EINA_FALSE) - now all previous scroll requests will be processed by WebCore but EWK won't move viewport which was already done - to be clear, it'll be just 1 request contains sum of skipped requests.
> 
> What I find weird in this example is that a function which was just supposed to change the value of a flag (ewk_frame_weak_scroll_enabled_set) ends up also triggering a scrolling event depending on the value of the boolean parameter. This is not what one would expect at first. Wouldn't it be better to do this kind of processing automatically during the main loop?

Sounds good. In new patch WebKit is informed about scrolls on idler.

[Kamil Blank]

Hi,

Could someone please take a look on this patch?

[KwangHyuk]

> Source/WebKit/efl/ewk/ewk_frame.cpp:77
> +        Eina_Bool enabled : 1;

Use bool type.

> Source/WebKit/efl/ewk/ewk_frame.cpp:79
> +        Eina_Bool moving_viewport_allowed : 1;

Ditto.
Check the name of variable.

> Source/WebKit/efl/ewk/ewk_frame.cpp:80
> +        Ecore_Idler *idler;

idler for what ?

> Source/WebKit/efl/ewk/ewk_frame.cpp:193
> +    smartData->queued_scrolls.enabled = EINA_FALSE;

Use false;

> Source/WebKit/efl/ewk/ewk_frame.cpp:194
> +    smartData->queued_scrolls.moving_viewport_allowed = EINA_TRUE;

Use true;

> Source/WebKit/efl/ewk/ewk_frame.cpp:294
> +    Ewk_Frame_Smart_Data *smartData = (Ewk_Frame_Smart_Data*) priv;

Use cpp-style type casting.

> Source/WebKit/efl/ewk/ewk_frame.cpp:295
> +    smartData->queued_scrolls.moving_viewport_allowed = EINA_FALSE;

Use false,

> Source/WebKit/efl/ewk/ewk_frame.cpp:297
> +    smartData->queued_scrolls.moving_viewport_allowed = EINA_TRUE;

Use true;

> Source/WebKit/efl/ewk/ewk_frame.cpp:747
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->view, EINA_FALSE);

Why don't use false ?

> Source/WebKit/efl/ewk/ewk_frame.cpp:750
> +        int visibleW, visibleH, scrollX, scrollY, scrollDeltaX, scrollDeltaY, frameW, frameH;

For the W,H, why don't you use Widht or Height ?

> Source/WebKit/efl/ewk/ewk_frame.cpp:751
> +        ewk_frame_visible_content_geometry_get(ewkFrame, EINA_FALSE, 0, 0, &visibleW, &visibleH);

use false,

> Source/WebKit/efl/ewk/ewk_frame.cpp:779
> +                smartData->queued_scrolls.idler = ecore_idler_add(_ewk_frame_queued_scrolls_process_cb, smartData);

I couldn't make sure idler would be good for scroll operation ?
It may hardly have a chance to get a time slice.

> Source/WebKit/efl/ewk/ewk_frame.cpp:782
> +        return EINA_TRUE;

Use true or false,

> Source/WebKit/efl/ewk/ewk_frame.cpp:804
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->frame, EINA_FALSE);

Use true or false,

> Source/WebKit/efl/ewk/ewk_frame.cpp:806
> +        return EINA_TRUE;

Ditto,

> Source/WebKit/efl/ewk/ewk_frame.cpp:809
> +    return EINA_TRUE;

Ditto,

> Source/WebKit/efl/ewk/ewk_frame.cpp:814
> +    EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, smartData, EINA_FALSE);

Ditto,

> Source/WebKit/efl/ewk/ewk_frame.cpp:820
> +    EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, smartData, EINA_FALSE);

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.h:624
> +EAPI Eina_Bool    ewk_frame_scroll_policy_set(Evas_Object *o, Eina_Bool queue_scrolls);

Check whether there is tab between Eina_Bool and API name.

> Source/WebKit/efl/ewk/ewk_frame.h:633
> +EAPI Eina_Bool    ewk_frame_scroll_policy_get(const Evas_Object *o);

Ditto.

[KwangHyuk]

Why don't you change the title ?

[Gyuyoung Kim]

Comment on attachment 113442 [details]
patch

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

See WebKit EFL coding style : http://trac.webkit.org/wiki/EFLWebKitCodingStyle

> Source/WebKit/efl/ewk/ewk_frame.cpp:78
> +        int dx, dy;

Do not use abbreviation.

> Source/WebKit/efl/ewk/ewk_frame.cpp:79
> +        Eina_Bool moving_viewport_allowed : 1;

Do not use '_' except for public APIs.

> Source/WebKit/efl/ewk/ewk_frame.cpp:81
> +    } queued_scrolls;

s/queued_scrolls/queuedScrolls/g

> Source/WebKit/efl/ewk/ewk_private.h:229
> +Eina_Bool ewk_frame_scroll_moving_viewport_allowed_get(Evas_Object* o);

Use ewkFrame instead of o. It looks this patch needs to be rebased.

[Kamil Blank]

Created attachment 122387 [details]
patch

[Kamil Blank]

Thanks for comments. I adjusted patch to new EFL WebKit coding style.

[KwangHyuk]

> Source/WebKit/efl/ewk/ewk_frame.cpp:770
> +        int visibleWidth, visibleHeight, scrollX, scrollY, scrollDeltaX, scrollDeltaY, frameWidth, frameHeight;

If possible, put some variable when they are really needed.

> Source/WebKit/efl/ewk/ewk_frame.cpp:778
> +        else if (deltaX > frameWidth - scrollX - visibleWidth)

Reduce repetition of same pattern.

> Source/WebKit/efl/ewk/ewk_frame.cpp:782
> +            scrollDeltaX = deltaX;

This can be an initialization of scrollDeltaX, so else condition won't be required.

> Source/WebKit/efl/ewk/ewk_frame.cpp:784
> +        if (scrollY + deltaY < 0)

Check the webkit coding style, it might be two lines although one of them is comment.

> Source/WebKit/efl/ewk/ewk_frame.cpp:787
> +        else if (deltaY > frameHeight - scrollY - visibleHeight)

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:791
> +            scrollDeltaY = deltaY;

This can be an initialization of scrollDeltaY, so else condition won't be required.

> Source/WebKit/efl/ewk/ewk_frame.cpp:799
> +                smartData->queuedScrolls.idler = ecore_idler_add(_ewk_frame_queued_scrolls_process_cb, smartData);

I couldn't find ecore_idler_del ? isn't it required ?

[Kamil Blank]

(In reply to comment #14)
> > Source/WebKit/efl/ewk/ewk_frame.cpp:770
> > +        int visibleWidth, visibleHeight, scrollX, scrollY, scrollDeltaX, scrollDeltaY, frameWidth, frameHeight;
> 
> If possible, put some variable when they are really needed.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:778
> > +        else if (deltaX > frameWidth - scrollX - visibleWidth)
> 
> Reduce repetition of same pattern.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:782
> > +            scrollDeltaX = deltaX;
> 
> This can be an initialization of scrollDeltaX, so else condition won't be required.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:784
> > +        if (scrollY + deltaY < 0)
> 
> Check the webkit coding style, it might be two lines although one of them is comment.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:787
> > +        else if (deltaY > frameHeight - scrollY - visibleHeight)
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:791
> > +            scrollDeltaY = deltaY;
> 
> This can be an initialization of scrollDeltaY, so else condition won't be required.
> 
Thanks I'll fix it.

> > Source/WebKit/efl/ewk/ewk_frame.cpp:799
> > +                smartData->queuedScrolls.idler = ecore_idler_add(_ewk_frame_queued_scrolls_process_cb, smartData);
> 
> I couldn't find ecore_idler_del ? isn't it required ?
Returning ECORE_CALLBACK_CANCEL deletes the idler.

[Kamil Blank]

Created attachment 122883 [details]
patch

[Kamil Blank]

Created attachment 122895 [details]
patch

Patch rebased.

[KwangHyuk]

> Source/WebKit/efl/ewk/ewk_frame.cpp:794
> +            if (!smartData->queuedScrolls.idler)

Unfortunately, idler callback can cause two issues.
Idler callback can not be called when cpu is busy and it can cause crash if object is destroyed and callback isn't unregistered.
In addition, current implementation may not apply final scroll request.
What do you think ?

[Kamil Blank]

Created attachment 124281 [details]
patch

[Kamil Blank]

(In reply to comment #18)
> > Source/WebKit/efl/ewk/ewk_frame.cpp:794
> > +            if (!smartData->queuedScrolls.idler)
> 
> Unfortunately, idler callback can cause two issues.
> Idler callback can not be called when cpu is busy and it can cause crash if object is destroyed and callback isn't unregistered.
> In addition, current implementation may not apply final scroll request.
> What do you think ?

Yes, looks like you are right :) I added ecore_idler_del inside _ewk_frame_smart_del().

[KwangHyuk]

(In reply to comment #20)
> (In reply to comment #18)
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:794
> > > +            if (!smartData->queuedScrolls.idler)
> > 
> > Unfortunately, idler callback can cause two issues.
> > Idler callback can not be called when cpu is busy and it can cause crash if object is destroyed and callback isn't unregistered.
> > In addition, current implementation may not apply final scroll request.
> > What do you think ?
> 
> Yes, looks like you are right :) I added ecore_idler_del inside _ewk_frame_smart_del().

Ok for the change,
But, finally, I hope that  you will consider the things below one more time,
- Doesn't it matter if idler callback isn't called for a while while panning on page-loading ?
- Would it be ok although calling of callback is pending, so smartData->queuedScrolls.idler won't be 0 for a long time ?

[Kamil Blank]

(In reply to comment #21)
> - Doesn't it matter if idler callback isn't called for a while while panning on page-loading ?

Do you have in mind specific case which can give us trouble? We can consider disabling this feature while page loading if it's needed.

> - Would it be ok although calling of callback is pending, so smartData->queuedScrolls.idler won't be 0 for a long time ?

I don't see any issues there. As soon as callback is called it'll process all scroll events which were made until that time - not only these made before adding idler. When callback is pending it's not possible to make new scrolls so there is no risk caused by mixing queued scrolls and frame scrolls.

[KwangHyuk]

(In reply to comment #22)
> (In reply to comment #21)
> > - Doesn't it matter if idler callback isn't called for a while while panning on page-loading ?
> 
> Do you have in mind specific case which can give us trouble? We can consider disabling this feature while page loading if it's needed.
> 
> > - Would it be ok although calling of callback is pending, so smartData->queuedScrolls.idler won't be 0 for a long time ?
> 
> I don't see any issues there. As soon as callback is called it'll process all scroll events which were made until that time - not only these made before adding idler. When callback is pending it's not possible to make new scrolls so there is no risk caused by mixing queued scrolls and frame scrolls.

IMO, Looks ok for the code now.
Would you share the way to test this patch ? :)

[Kamil Blank]

> Would you share the way to test this patch ? :)
Well, the best way to test it is to use it :)
You can simply enable queued scroll policy by added API and call ewk_frame_scroll_add() few times.
Than you can do the same thing with the policy disabled and check whether the final visual effect is the same in both cases - scroll positions should be the same.

[KwangHyuk]

One more things !!

> Source/WebKit/efl/ewk/ewk_frame.cpp:780
> +        const int bottomBound = frameHeight - scrollY - visibleHeight;

What if both rightBound and bottomBound have minus result ?

[Kamil Blank]

> > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> 
> What if both rightBound and bottomBound have minus result ?

Is it possible?  
If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.

[KwangHyuk]

(In reply to comment #26)
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> > 
> > What if both rightBound and bottomBound have minus result ?
> 
> Is it possible?  
> If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.

It can be smaller then visible rect's height if frameHeight means contents's height because content size can be smaller than visible rect after the zoom or small size of contents can be created.

[KwangHyuk]

One more question,

> Source/WebKit/efl/ewk/ewk_frame.cpp:815
> +    smartData->queuedScrolls.deltaY = 0;

I am wondering whether idler cb will do unexpected behavior if this api is called before idler cb will work.

[Kamil Blank]

(In reply to comment #27)
> (In reply to comment #26)
> > > > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > > > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> > > 
> > > What if both rightBound and bottomBound have minus result ?
> > 
> > Is it possible?  
> > If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.
> 
> It can be smaller then visible rect's height if frameHeight means contents's height because content size can be smaller than visible rect after the zoom or small size of contents can be created.

The case you described means that whole content is smaller than its currently visible part. Could you please give me some example for this situation so I can check it? 

(In reply to comment #28)
> One more question,
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:815
> > +    smartData->queuedScrolls.deltaY = 0;
> 
> I am wondering whether idler cb will do unexpected behavior if this api is called before idler cb will work.

I guess you meant ewk_frame_scroll_set, right? That's why deltaY and deltaX are zeroed here to avoid calling scrollBy() by idler in case of calling this API.

[KwangHyuk]

(In reply to comment #29)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > > > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > > > > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> > > > 
> > > > What if both rightBound and bottomBound have minus result ?
> > > 
> > > Is it possible?  
> > > If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.
> > 
> > It can be smaller then visible rect's height if frameHeight means contents's height because content size can be smaller than visible rect after the zoom or small size of contents can be created.
> 
> The case you described means that whole content is smaller than its currently visible part. Could you please give me some example for this situation so I can check it? 
> 
I think that you can meet the same issue if you use target environment.
I just get a hint for this issue from it. :)

> (In reply to comment #28)
> > One more question,
> > 
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:815
> > > +    smartData->queuedScrolls.deltaY = 0;
> > 
> > I am wondering whether idler cb will do unexpected behavior if this api is called before idler cb will work.
> 
> I guess you meant ewk_frame_scroll_set, right? That's why deltaY and deltaX are zeroed here to avoid calling scrollBy() by idler in case of calling this API.

Yes, you are doing it, but ewk_freame_scroll_set doesn't seem to handle idler.
So, if idler is added and this api is called, won't idler work with interesting value ? what do you think ?

[Kamil Blank]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > > > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > > > > > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> > > > > 
> > > > > What if both rightBound and bottomBound have minus result ?
> > > > 
> > > > Is it possible?  
> > > > If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.
> > > 
> > > It can be smaller then visible rect's height if frameHeight means contents's height because content size can be smaller than visible rect after the zoom or small size of contents can be created.
> > 
> > The case you described means that whole content is smaller than its currently visible part. Could you please give me some example for this situation so I can check it? 
> > 
> I think that you can meet the same issue if you use target environment.
> I just get a hint for this issue from it. :)

Please, give me detailed example so I could check it and reproduce it if you potential issue here.

> 
> > (In reply to comment #28)
> > > One more question,
> > > 
> > > > Source/WebKit/efl/ewk/ewk_frame.cpp:815
> > > > +    smartData->queuedScrolls.deltaY = 0;
> > > 
> > > I am wondering whether idler cb will do unexpected behavior if this api is called before idler cb will work.
> > 
> > I guess you meant ewk_frame_scroll_set, right? That's why deltaY and deltaX are zeroed here to avoid calling scrollBy() by idler in case of calling this API.
> 
> Yes, you are doing it, but ewk_freame_scroll_set doesn't seem to handle idler.
> So, if idler is added and this api is called, won't idler work with interesting value ? what do you think ?

If ewk_frame_scroll_set is called then idler is not needed in this case as scroll is already set (setScrollPosition is called).

[KwangHyuk]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > (In reply to comment #27)
> > > > (In reply to comment #26)
> > > > > > > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > > > > > > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> > > > > > 
> > > > > > What if both rightBound and bottomBound have minus result ?
> > > > > 
> > > > > Is it possible?  
> > > > > If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.
> > > > 
> > > > It can be smaller then visible rect's height if frameHeight means contents's height because content size can be smaller than visible rect after the zoom or small size of contents can be created.
> > > 
> > > The case you described means that whole content is smaller than its currently visible part. Could you please give me some example for this situation so I can check it? 
> > > 
> > I think that you can meet the same issue if you use target environment.
> > I just get a hint for this issue from it. :)
> 
> Please, give me detailed example so I could check it and reproduce it if you potential issue here.
> 
> > 
> > > (In reply to comment #28)
> > > > One more question,
> > > > 
> > > > > Source/WebKit/efl/ewk/ewk_frame.cpp:815
> > > > > +    smartData->queuedScrolls.deltaY = 0;
> > > > 
> > > > I am wondering whether idler cb will do unexpected behavior if this api is called before idler cb will work.
> > > 
> > > I guess you meant ewk_frame_scroll_set, right? That's why deltaY and deltaX are zeroed here to avoid calling scrollBy() by idler in case of calling this API.
> > 
> > Yes, you are doing it, but ewk_freame_scroll_set doesn't seem to handle idler.
> > So, if idler is added and this api is called, won't idler work with interesting value ? what do you think ?
> 
> If ewk_frame_scroll_set is called then idler is not needed in this case as scroll is already set (setScrollPosition is called).

Kamil, sorry for the late feedback.
Would you rebase the patch ? and then let me double check. :)

[Kamil Blank]

Created attachment 135538 [details]
patch

Patch rebased

[Kamil Blank]

> Kamil, sorry for the late feedback.
> Would you rebase the patch ? and then let me double check. :)

No problem :)

[KwangHyuk]

(In reply to comment #34)
> > Kamil, sorry for the late feedback.
> > Would you rebase the patch ? and then let me double check. :)
> 
> No problem :)

(In reply to comment #26)
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:780
> > > +        const int bottomBound = frameHeight - scrollY - visibleHeight;
> > 
> > What if both rightBound and bottomBound have minus result ?
> 
> Is it possible?  
> If page is maximally down scrolled then frameHeight = scrollY + visibleHeight => bottomBound = 0. The same with rightBound when page is maximally scrolled to the right.

Kamil, 
Once i ran a simple test for your code, I couldn't meet the issue I expected.
Indeed frameHeight was equal to the visibleHeight for the case I mentioned.

LGTM.

[KwangHyuk]

> > One more question,
> > 
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:815
> > > +    smartData->queuedScrolls.deltaY = 0;
> > 
> > I am wondering whether idler cb will do unexpected behavior if this api is called before idler cb will work.
> 
> I guess you meant ewk_frame_scroll_set, right? That's why deltaY and deltaX are zeroed here to avoid calling scrollBy() by idler in case of calling this API.

I forgot about this.
After all, It looks reasonable, but why callback has to do the most of unnecessary things ?

[Gyuyoung Kim]

Comment on attachment 135538 [details]
patch

Cleared review? from attachment 135538 [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]

It's quite long without any action.

And I think that this is right way at least now.

So I closed this as invalid.