[webkit-reviews] review denied: [Bug 124981] Nix Upstream: Updating WebCore files : [Attachment 218167] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Thiago de Barros Lacerda
<thiago.lacerda at openbossa.org>'s request for review:
Bug 124981: Nix Upstream: Updating WebCore files
https://bugs.webkit.org/show_bug.cgi?id=124981

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=218167&action=review


Most of it is great. Let's move the bug fix and new features in common code
separate.

> Source/WebCore/loader/HistoryController.cpp:156
>  
> +	   double currentScaleFactor = m_currentItem->pageScaleFactor();
> +	   IntPoint currentScrollPoint = m_currentItem->scrollPoint();
> +#if PLATFORM(NIX)
> +	   double previousScaleFactor = m_previousItem ?
m_previousItem->pageScaleFactor() : currentScaleFactor;
> +	   IntPoint previousScrollPoint = m_previousItem ?
m_previousItem->scrollPoint() : currentScrollPoint;
> +
> +	   if (previousScaleFactor != currentScaleFactor || previousScrollPoint
!= currentScrollPoint) {
> +#else
>	   if (!view->wasScrolledByUser()) {
> -	       if (page && m_frame.isMainFrame() &&
m_currentItem->pageScaleFactor())
> -		   page->setPageScaleFactor(m_currentItem->pageScaleFactor(),
m_currentItem->scrollPoint());
> +#endif // PLATFORM(NIX)
> +	       if (page && m_frame.isMainFrame() && currentScaleFactor)
> +		   page->setPageScaleFactor(currentScaleFactor,
currentScrollPoint);
>	       else
> -		   view->setScrollPosition(m_currentItem->scrollPoint());
> +		   view->setScrollPosition(currentScrollPoint);
>	   }

Let's do this in a separate patch with tests.

> Source/WebCore/page/EventHandler.cpp:2597
> +#if PLATFORM(NIX)
> +void EventHandler::handleSingleTap(double timestamp, const
PlatformTouchPoint& point)
> +{
> +    IntPoint targetPoint;
> +#if ENABLE(TOUCH_ADJUSTMENT)
> +    bool positionAdjusted = false;
> +    if (m_frame.settings().touchAdjustmentEnabled()) {
> +	   Node* targetNode = 0;
> +	   positionAdjusted = bestClickableNodeForTouchPoint(point.pos(),
IntSize(point.radiusX(), point.radiusY()), targetPoint, targetNode);
> +    }
> +    if (!positionAdjusted)
> +	   targetPoint = point.pos();
> +#else
> +    targetPoint = point.pos();
> +#endif
> +
> +    PlatformMouseEvent fakeMouseMove(targetPoint, point.screenPos(),
NoButton, PlatformEvent::MouseMoved, /* clickCount */ 0,
> +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */,
timestamp);
> +    mouseMoved(fakeMouseMove);
> +
> +    PlatformMouseEvent fakeMouseDown(targetPoint, point.screenPos(),
LeftButton, PlatformEvent::MousePressed, 1 /* tapCount */,
> +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */,
timestamp);
> +    handleMousePressEvent(fakeMouseDown);
> +
> +    PlatformMouseEvent fakeMouseUp(targetPoint, point.screenPos(),
LeftButton, PlatformEvent::MouseReleased, 1 /* tapCount */,
> +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */,
timestamp);
> +    handleMouseReleaseEvent(fakeMouseUp);
> +}
> +#endif // PLATFORM(NIX)
> +

Let's not do this here. Any code like this should be unified with the iOS code
that was upstreamed recently.

> Source/WebCore/page/EventHandler.h:75
> +#if PLATFORM(NIX)
> +class PlatformTouchPoint;
> +#endif

Please remove this.

> Source/WebCore/page/EventHandler.h:183
> +#if PLATFORM(NIX)
> +    void handleSingleTap(double timestamp, const PlatformTouchPoint&);
> +#endif

Please remove this.