Bug 124981 – Nix Upstream: Updating WebCore files

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Thiago de Barros Lacerda]

Just to sync our private repo files and the trunk, as part of the upstream process

[Thiago de Barros Lacerda]

Created attachment 218013 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 218013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218013&action=review

The patch looks good to me in general, except some parts which add 
maybe intrusive Nix specific/only changes to platform independent 
code. I suggest extracting these parts from this patch and uploading 
for review as smaller patches:

- WorkerScriptController changes - it adds PLATFORM(NIX) ifdefs, but there weren't
  any platform ifdefs in these files before. I'm not sure if it is a good idea.
- loader/HistoryController.cpp change - Can't we avoid platform specific behaviour here?
- adding Nix only handleSingleTap() to EventHandler
- texmap changes

> Source/WebCore/css/mediaControlsNix.css:4
> - * Copyright (C) 2009, 2010, 2011 Apple Inc.  All rights reserved.
> - * Copyright (C) 2011 Samsung Electronics
> - * Copyright (C) 2012-2013 Nokia Corporation and/or its subsidiary(-ies).
> + * Copyright (C) 2009 Apple Inc.  All rights reserved.
> + * Copyright (C) 2009 Google Inc.
> + * Copyright (C) 2012, 2013 Nokia Corporation and/or its subsidiary(-ies).

Why did you change these copyright entries? Did you replaced the whole file?

> Source/WebCore/page/EventHandler.cpp:2585
> +    PlatformMouseEvent fakeMouseMove(targetPoint, point.screenPos(), NoButton, PlatformEvent::MouseMoved, /* clickCount */ 0,
> +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */, timestamp);

Do we really need this extra parameter notation?

> Source/WebCore/page/EventHandler.cpp:2589
> +    PlatformMouseEvent fakeMouseDown(targetPoint, point.screenPos(), LeftButton, PlatformEvent::MousePressed, 1 /* tapCount */,
> +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */, timestamp);

ditto

> Source/WebCore/page/EventHandler.cpp:2593
> +    PlatformMouseEvent fakeMouseUp(targetPoint, point.screenPos(), LeftButton, PlatformEvent::MouseReleased, 1 /* tapCount */,
> +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */, timestamp);

ditto

[Thiago de Barros Lacerda]

(In reply to comment #2)
> (From update of attachment 218013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218013&action=review
> 
> The patch looks good to me in general, except some parts which add 
> maybe intrusive Nix specific/only changes to platform independent 
> code. I suggest extracting these parts from this patch and uploading 
> for review as smaller patches:
> 
> - WorkerScriptController changes - it adds PLATFORM(NIX) ifdefs, but there weren't
>   any platform ifdefs in these files before. I'm not sure if it is a good idea.

This is a WIP in https://bugs.webkit.org/show_bug.cgi?id=117523, which we need in Nix. I will remove it now and wait for the patch to be approved.

> - loader/HistoryController.cpp change - Can't we avoid platform specific behaviour here?

This is also a reported bug happening with fixed layout only (https://bugs.webkit.org/show_bug.cgi?id=119063), which I couldn't provide a LayoutTest for that. But we also need it in nix.

> - adding Nix only handleSingleTap() to EventHandler

After the removal of gesture events, this was the only way we found to get touch adjustment working. We are the only port that supports touch adjustment (there was Qt too).

> - texmap changes

This is related to ACCELERATED_OVERFLOW_SCROLLING, I will put in another patch

> 
> > Source/WebCore/css/mediaControlsNix.css:4
> > - * Copyright (C) 2009, 2010, 2011 Apple Inc.  All rights reserved.
> > - * Copyright (C) 2011 Samsung Electronics
> > - * Copyright (C) 2012-2013 Nokia Corporation and/or its subsidiary(-ies).
> > + * Copyright (C) 2009 Apple Inc.  All rights reserved.
> > + * Copyright (C) 2009 Google Inc.
> > + * Copyright (C) 2012, 2013 Nokia Corporation and/or its subsidiary(-ies).
> 
> Why did you change these copyright entries? Did you replaced the whole file?

Yes, this file was copied from other and replaced it entirely.

> 
> > Source/WebCore/page/EventHandler.cpp:2585
> > +    PlatformMouseEvent fakeMouseMove(targetPoint, point.screenPos(), NoButton, PlatformEvent::MouseMoved, /* clickCount */ 0,
> > +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */, timestamp);
> 
> Do we really need this extra parameter notation?
> 
> > Source/WebCore/page/EventHandler.cpp:2589
> > +    PlatformMouseEvent fakeMouseDown(targetPoint, point.screenPos(), LeftButton, PlatformEvent::MousePressed, 1 /* tapCount */,
> > +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */, timestamp);
> 
> ditto
> 
> > Source/WebCore/page/EventHandler.cpp:2593
> > +    PlatformMouseEvent fakeMouseUp(targetPoint, point.screenPos(), LeftButton, PlatformEvent::MouseReleased, 1 /* tapCount */,
> > +    false /* shift */, false /* ctrl */, false /* alt */, false /* meta */, timestamp);
> 
> ditto

Those are only to improve code readability, instead of having a lot of "false" arguments with no meaning to whom is reading the code. Do you think we should remove them?

Thanks

[Thiago de Barros Lacerda]

Created attachment 218167 [details]
Patch

[Benjamin Poulain]

Comment on attachment 218167 [details]
Patch

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.

[Thiago de Barros Lacerda]

Created attachment 218336 [details]
Patch

[Benjamin Poulain]

Comment on attachment 218336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218336&action=review

> Source/WebCore/platform/audio/nix/AudioBusNix.cpp:70
> -    size_t bytesRead = fread(&fileContents[0], fileContents.size(), 1, file);
> +    size_t bytesRead = fread(&fileContents[0], 1, fileContents.size(), file);

Silly fread() :)

> Source/WebCore/platform/nix/RenderThemeNix.cpp:44
> +#include "UserAgentStyleSheets.h"

You should put this just after PlatformContextCairo.h.

[Thiago de Barros Lacerda]

Created attachment 218337 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 218337 [details]
Patch for landing

Clearing flags on attachment: 218337

Committed r160045: <http://trac.webkit.org/changeset/160045>