[Webkit-unassigned] [Bug 49345] [Chromium] Add a basic GestureManager to Chromium

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #75402|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #6 from Adam Barth <abarth at webkit.org>  2010-12-07 13:53:07 PST ---
(From update of attachment 75402)
View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review

My review comments are mostly just style issues.  I don't know too much about touch, so I'm not able to review the substance of your change.  We'll want to find someone who's familiar with this kind of thing to do a review of the substance.

> WebCore/page/GestureManager.h:68
> +    void addEdgeFunction(State,
> +                         unsigned,
> +                         PlatformTouchPoint::State,
> +                         bool,
> +                         GestureTransitionFunction);
> +  

Generally, we omit formal parameter names in header files if the meaning is obvious.  For example "Frame* frame" is kind of redundant. However, here, it looks like you've gone a bit overboard.  What does the second parameter mean?  Also, the comment seems to refer to parameter names that are no longer present.  (Also, these declarations should be on one line.)

> WebCore/page/GestureManager.h:95
> +    inline void setState(State s)
> +    {
> +      m_state = s;
> +    }

No need to declare these functions inline.  Also, you can just write the whole function on one line.  (You've also got a bad indent here, but that will get fixed when you put the whole thing on one line.)  The general rule is that simple setter/getter functions should take up as little space as possible.

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