[webkit-reviews] review denied: [Bug 118358] Nix upstreaming - Adding stubs and Nix specific platform files : [Attachment 206105] New patch with corrections

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Luciano Wolf
<luciano.wolf at openbossa.org>'s request for review:
Bug 118358: Nix upstreaming - Adding stubs and Nix specific platform files
https://bugs.webkit.org/show_bug.cgi?id=118358

Attachment 206105: New patch with corrections
https://bugs.webkit.org/attachment.cgi?id=206105&action=review

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


I am ok with this, but I want you to do one more pass and try to maximize the
code shared with GTK.
I suggest to find files that can be shared, remove them from this patch, and
make an other refactoring patch to make those common to GTK/Nix.

Martin Robinson agrees the code should be shared when possible. You can add new
platform directories if needed (e.g. platform/gobject).

> Source/WebCore/platform/graphics/egl/GLContextFromCurrentEGL.h:48
> +    virtual void waitNative() OVERRIDE { return; }

remove the "return;"

> Source/WebCore/platform/nix/ErrorsNix.cpp:28
> +#include "config.h"
> +#include "ErrorsNix.h"

Can't you share this file with GTK?

> Source/WebCore/platform/nix/FileSystemNix.cpp:24
> +#include "config.h"
> +#include "FileSystem.h"

Why can't you share this file with GTK?

> Source/WebCore/platform/nix/FileSystemNix.cpp:42
> +
> +/* On linux file names are just raw bytes, so also strings that cannot be
encoded in any way
> + * are valid file names. This mean that we cannot just store a file name
as-is in a String
> + * but we have to escape it.
> + * On Windows the GLib file name encoding is always UTF-8 so we can optimize
this case. */

Change this to WebKit-style comments.

> Source/WebCore/platform/nix/GamepadsNix.cpp:60
> +void sampleGamepads(GamepadList* into)
> +{
> +    WebKit::WebGamepads gamepads;
> +
> +    WebKit::Platform::current()->sampleGamepads(gamepads);
> +
> +    for (unsigned i = 0; i < WebKit::WebGamepads::itemsLengthCap; ++i) {
> +	   WebKit::WebGamepad& webGamepad = gamepads.items[i];
> +	   if (i < gamepads.length && webGamepad.connected) {
> +	       RefPtr<Gamepad> gamepad = into->item(i);
> +	       if (!gamepad)
> +		   gamepad = Gamepad::create();
> +	       gamepad->id(webGamepad.id);
> +	       gamepad->index(i);
> +	       gamepad->timestamp(webGamepad.timestamp);
> +	       gamepad->axes(webGamepad.axesLength, webGamepad.axes);
> +	       gamepad->buttons(webGamepad.buttonsLength, webGamepad.buttons);
> +	       into->set(i, gamepad);
> +	   } else
> +	       into->set(i, 0);
> +    }
> +}
> +

Looking at Qt and GTK, I suspect this is dead code. You don't have any code to
get the devices.

> Source/WebCore/platform/nix/LocalizedStringsNix.cpp:45
> +String localizedString(const char* key)
> +{
> +    return String::fromUTF8(key, strlen(key));
> +}

Much better!

> Source/WebCore/platform/nix/RunLoopNix.cpp:28
> +#include "config.h"
> +#include "RunLoop.h"

Why can't this be shared with GTK?

> Source/WebCore/platform/nix/SharedTimerNix.cpp:29
> +#include "config.h"
> +#include "SharedTimer.h"

Why can't this be shared with GTK?