Bug 67109

Summary: [EFL] Queued scroll feature
Product: WebKit Reporter: Kamil Blank <k.blank>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: gyuyoung.kim, hyuki.kim, lucas.de.marchi, rakuco, ryuan.choi, sangseok.lim
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
patch
none
patch
gyuyoung.kim: review-
patch
none
patch
none
patch
none
patch
none
patch none

Description Kamil Blank 2011-08-29 00:00:02 PDT
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.
Comment 1 Kamil Blank 2011-08-29 00:02:42 PDT
Created attachment 105463 [details]
patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-08-29 05:23:07 PDT
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?
Comment 3 Kamil Blank 2011-08-30 03:31:02 PDT
(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.
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-08-30 07:50:13 PDT
(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 :(
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-09-20 09:57:00 PDT
Comment on attachment 105463 [details]
patch

Clearing flags while things are sorted out.
Comment 6 Kamil Blank 2011-11-03 01:21:59 PDT
Created attachment 113442 [details]
patch
Comment 7 Kamil Blank 2011-11-03 01:26:08 PDT
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.
Comment 8 Kamil Blank 2012-01-02 05:21:31 PST
Hi,

Could someone please take a look on this patch?
Comment 9 KwangHyuk 2012-01-12 06:59:55 PST
> 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.
Comment 10 KwangHyuk 2012-01-12 07:00:49 PST
Why don't you change the title ?
Comment 11 Gyuyoung Kim 2012-01-12 16:23:36 PST
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.
Comment 12 Kamil Blank 2012-01-13 00:23:18 PST
Created attachment 122387 [details]
patch
Comment 13 Kamil Blank 2012-01-13 00:25:08 PST
Thanks for comments. I adjusted patch to new EFL WebKit coding style.
Comment 14 KwangHyuk 2012-01-13 13:28:38 PST
> 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 ?
Comment 15 Kamil Blank 2012-01-18 00:44:28 PST
(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.
Comment 16 Kamil Blank 2012-01-18 00:49:15 PST
Created attachment 122883 [details]
patch
Comment 17 Kamil Blank 2012-01-18 02:40:41 PST
Created attachment 122895 [details]
patch

Patch rebased.
Comment 18 KwangHyuk 2012-01-25 01:17:29 PST
> 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 ?
Comment 19 Kamil Blank 2012-01-27 02:40:42 PST
Created attachment 124281 [details]
patch
Comment 20 Kamil Blank 2012-01-27 02:43:38 PST
(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().
Comment 21 KwangHyuk 2012-01-27 03:11:26 PST
(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 ?
Comment 22 Kamil Blank 2012-01-31 05:47:45 PST
(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.
Comment 23 KwangHyuk 2012-02-01 05:05:49 PST
(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 ? :)
Comment 24 Kamil Blank 2012-02-06 05:14:45 PST
> 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.
Comment 25 KwangHyuk 2012-02-09 18:37:19 PST
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 ?
Comment 26 Kamil Blank 2012-02-10 02:49:26 PST
> > 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.
Comment 27 KwangHyuk 2012-02-12 16:59:37 PST
(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.
Comment 28 KwangHyuk 2012-02-12 17:21:30 PST
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.
Comment 29 Kamil Blank 2012-02-14 07:16:59 PST
(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.
Comment 30 KwangHyuk 2012-02-18 07:07:40 PST
(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 ?
Comment 31 Kamil Blank 2012-02-27 01:42:53 PST
(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).
Comment 32 KwangHyuk 2012-04-04 00:52:26 PDT
(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. :)
Comment 33 Kamil Blank 2012-04-04 02:54:38 PDT
Created attachment 135538 [details]
patch

Patch rebased
Comment 34 Kamil Blank 2012-04-04 02:55:28 PDT
> Kamil, sorry for the late feedback.
> Would you rebase the patch ? and then let me double check. :)

No problem :)
Comment 35 KwangHyuk 2012-04-30 19:59:27 PDT
(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.
Comment 36 KwangHyuk 2012-04-30 20:10:10 PDT
> > 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 ?
Comment 37 Gyuyoung Kim 2012-11-23 22:16:45 PST
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).
Comment 38 Ryuan Choi 2014-02-07 22:38:09 PST
It's quite long without any action.

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

So I closed this as invalid.