[webkit-reviews] review denied: [Bug 26695] Windows 7: Dragging Scrollbar doesn't work properly on inner divs with scrollbars or Frames on Touch Screen : [Attachment 31873] Inner Scrollbar Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 26695: Windows 7: Dragging Scrollbar doesn't work properly on inner divs
with scrollbars or Frames on Touch Screen
https://bugs.webkit.org/show_bug.cgi?id=26695

Attachment 31873: Inner Scrollbar Patch
https://bugs.webkit.org/attachment.cgi?id=31873&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 45184)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,24 @@
> +2009-06-25  Brian Weinstein	<bweinstein at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +	   
> +	   https://bugs.webkit.org/show_bug.cgi?id=26695
> +	   
> +	   This patch adds the ability to get have the Scrollbar variable set
> +	   in EventHandler::hitTestResultAtPoint, and if the mouse is over a
> +	   scrollbar, the m_scrollbar variable will be set.

I'm not sure how the independent clauses of this sentence are different

> -HitTestResult EventHandler::hitTestResultAtPoint(const IntPoint& point, bool
allowShadowContent, bool ignoreClipping)
> +HitTestResult EventHandler::hitTestResultAtPoint(const IntPoint& point, bool
allowShadowContent, bool ignoreClipping, bool shouldSetScrollbar)

We're trying to avoid adding boolean parameters, because they are not very
clear (when reading code like "hitTestResultAtPoint(point, true, false, true)",
what do the booleans mean?). One strategy to replace a boolean parameter is to
use a two-value enum, like this:

enum HitTestScrollbars { ShouldHitTestScrollbars, DontHitTestScrollbars };
HitTestResult hitTestResultAtPoint(const IntPoint&, bool allowShadowContent,
bool ignoreClipping, HitTestScrollbars);

> +	   if (shouldSetScrollbar) {
> +	       PlatformMouseEvent mouseEvent(point, point, NoButton,
MouseEventPressed, 0, false, false, false, false, 0);
> +	       Scrollbar* eventScrollbar =
view->scrollbarUnderMouse(mouseEvent);
> +	       if (eventScrollbar)
> +		   result.setScrollbar(eventScrollbar);
> +	   }

Can we change scrollbarUnderMouse to take an IntPoint instead of a
PlatformMouseEvent? If we do that, we might want to change its name to
something like scrollbarAtPoint or scrollbarAtWindowPoint.

> +++ WebCore/platform/PlatformWheelEvent.h	(working copy)
> @@ -27,6 +27,8 @@
>  #define PlatformWheelEvent_h
>  
>  #include "IntPoint.h"
> +#include "FloatPoint.h"
> +#include "FloatSize.h"

You don't need these #includes here. You can just declare the classes.


> +++ WebCore/platform/win/WheelEventWin.cpp	(working copy)
> @@ -23,6 +23,8 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>   */
>  
> +#include "FloatPoint.h"
> +#include "FloatSize.h"
>  #include "PlatformWheelEvent.h"
>  
>  #include <windows.h>

The #includes should be formatted like this:

#include "config.h"
#include "PlatformWheelEvent.h"

#include "FloatPoint.h"
#include "FloatSize.h"
#include <windows.h>

(It's a mistake that config.h wasn't listed before!)

> +	   ScrollView* view = coreFrame->view();
> +	   if (!view) {
> +	       CloseGestureInfoHandlePtr()(gestureHandle);
> +	       return false;
> +	   }
> +	   Scrollbar* vertScrollbar = view->verticalScrollbar();
> +	   if (!vertScrollbar) {
> +	       CloseGestureInfoHandlePtr()(gestureHandle);
> +	       return true;
> +	   }

Why "return false" in one case and "return true" in another?

I think ideally this change would be done as 3 patches:

1) Change the parameters to the PlatformWheelEvent constructor
2) Change the parameter to scrollbarUnderMouse (and maybe change its name, too)

3) Everything else

r- so that we can at least get the style issues fixed. But please consider
splitting up the patch, too!