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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Raphael Kubo da Costa <kubo at profusion.mobi>  2011-08-30 07:50:13 PST ---
(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 :(

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