[Webkit-unassigned] [Bug 67109] [EFL] Weak scroll feature

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67109





--- Comment #7 from Kamil Blank <k.blank at samsung.com>  2011-11-03 01:26:08 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.