Bug 124981 - Nix Upstream: Updating WebCore files
Summary: Nix Upstream: Updating WebCore files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago de Barros Lacerda
URL:
Keywords:
Depends on:
Blocks: 124950
  Show dependency treegraph
 
Reported: 2013-11-28 10:24 PST by Thiago de Barros Lacerda
Modified: 2013-12-04 10:00 PST (History)
5 users (show)

See Also:


Attachments
Patch (63.53 KB, patch)
2013-11-28 10:32 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (53.22 KB, patch)
2013-12-02 06:18 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (48.83 KB, patch)
2013-12-03 14:00 PST, Thiago de Barros Lacerda
benjamin: review+
Details | Formatted Diff | Diff
Patch for landing (48.86 KB, patch)
2013-12-03 14:11 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-11-28 10:24:40 PST
Just to sync our private repo files and the trunk, as part of the upstream process
Comment 1 Thiago de Barros Lacerda 2013-11-28 10:32:33 PST
Created attachment 218013 [details]
Patch
Comment 2 Csaba Osztrogonác 2013-11-29 01:19:56 PST
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
Comment 3 Thiago de Barros Lacerda 2013-11-29 09:53:49 PST
(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
Comment 4 Thiago de Barros Lacerda 2013-12-02 06:18:11 PST
Created attachment 218167 [details]
Patch
Comment 5 Benjamin Poulain 2013-12-02 14:20:19 PST
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.
Comment 6 Thiago de Barros Lacerda 2013-12-03 14:00:28 PST
Created attachment 218336 [details]
Patch
Comment 7 Benjamin Poulain 2013-12-03 14:04:27 PST
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.
Comment 8 Thiago de Barros Lacerda 2013-12-03 14:11:54 PST
Created attachment 218337 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2013-12-03 14:47:32 PST
Comment on attachment 218337 [details]
Patch for landing

Clearing flags on attachment: 218337

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