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

[bugzilla-daemon at webkit.org]

Csaba Osztrogonác <ossy 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 218013: Patch
https://bugs.webkit.org/attachment.cgi?id=218013&action=review

------- Additional Comments from Csaba Osztrogonác <ossy at webkit.org>
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