Bug 118358 – Nix upstreaming - Adding stubs and Nix specific platform files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Luciano Wolf]

Basically adding all files that have "Nix" in their names and are related to WebCore module.

[Luciano Wolf]

Created attachment 206001 [details]
Patch containing new Nix specific files and stubs

[Luciano Wolf]

Created attachment 206003 [details]
New patch avoiding WebAudio and RenderTheme stuff

[Benjamin Poulain]

Please change the changelog to only mention filename.

Are you okay with a real review or do you want to land as-is and fix later?

[Luciano Wolf]

(In reply to comment #3)
> Please change the changelog to only mention filename.
> 
> Are you okay with a real review or do you want to land as-is and fix later?

Thanks, I'll update the changelog. Right now it's better to "land as-is". This patch is just a small part of a bigger patch and doesn't provide a full context so a real review would require lots of missing information that is distributed through a series of other patches. The most important is to guarantee that this patch doesn't hurt other ports.

[Luciano Wolf]

Created attachment 206020 [details]
New patch with updated Changelog files

[Benjamin Poulain]

Comment on attachment 206020 [details]
New patch with updated Changelog files

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

> Source/WTF/wtf/nix/PlatformNix.h:32
> +// Some bits can't be changed at all

This comment makes no sense. Improve it or remove it.

> Source/WTF/wtf/nix/PlatformNix.h:51
> +// EGL/OpenGLES2 vs GLX

Flesh out this comment or remove it. Missing period.

> Source/WTF/wtf/nix/PlatformNix.h:61
> +// Soup vs libCurl

Flesh out this comment or remove it. Missing period.

> Source/WebCore/platform/graphics/egl/GLContextFromCurrentEGL.h:54
> +    // TODO: Add support for Offscreen buffers.
> +    static PassOwnPtr<GLContextFromCurrentEGL> createFromCurrentGLContext();
> +
> +    virtual bool makeContextCurrent();
> +
> +    // These are not used by Nix.
> +    virtual void swapBuffers() { }
> +    virtual IntSize defaultFrameBufferSize() { return IntSize(); }
> +    virtual cairo_device_t* cairoDevice() { return 0; }
> +
> +    // TODO: This is not used in GLContext interface, it's used by
> +    // GLContextGLX only as an implementation detail.
> +    virtual bool canRenderToDefaultFramebuffer() { return false; }
> +
> +    // TODO: Used only as a key in HashMaps, see if we can change WebKit code to not
> +    // rely on this anymore (at least in our platform).
> +    virtual PlatformGraphicsContext3D platformContext() { return this; }
> +    virtual void waitNative() { return; }

WebKit uses FIXME, not TODO.

All the virtual misses OVERRIDE.

This file is not a Nix file, the comments need to be clearer.

> Source/WebCore/platform/graphics/nix/ImageNix.cpp:44
> +    return SharedBuffer::create(); // TODO: fallback image?

TODO -> FIXME.

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

All the literals in this file should be ASCIILiteral.

> Source/WebCore/platform/nix/GamepadsNix.cpp:33
> +#include "GamepadList.h"
> +
> +#include <public/Platform.h>

Those #include should be together.

> Source/WebCore/platform/nix/LocalizedStringsNix.cpp:38
> +#include "config.h"
> +#include "LocalizedStrings.h"
> +
> +#include "NotImplemented.h"
> +#include <wtf/text/WTFString.h>

I am opposed to this.

Nix should use LocalizedStrings.cpp. Certain other ports have their own file for historical reason and it is a pain in the ass to work with that.

All you should have to do is implement WebCore::localizedString.

> Source/WebCore/platform/nix/MIMETypeRegistryNix.cpp:75
> +static const ExtensionMap extensionMap[] = {
> +    { "bmp", "image/bmp" },
> +    { "css", "text/css" },
> +    { "gif", "image/gif" },
> +    { "html", "text/html" },
> +    { "htm", "text/html" },
> +    { "ico", "image/x-icon" },
> +    { "jpeg", "image/jpeg" },
> +    { "jpg", "image/jpeg" },
> +    { "js", "application/x-javascript" },
> +    { "mng", "video/x-mng" },
> +    { "pbm", "image/x-portable-bitmap" },
> +    { "pgm", "image/x-portable-graymap" },
> +    { "pdf", "application/pdf" },
> +    { "png", "image/png" },
> +    { "ppm", "image/x-portable-pixmap" },
> +    { "rss", "application/rss+xml" },
> +    { "svg", "image/svg+xml" },
> +    { "text", "text/plain" },
> +    { "tif", "image/tiff" },
> +    { "tiff", "image/tiff" },
> +    { "txt", "text/plain" },
> +    { "xbm", "image/x-xbitmap" },
> +    { "xml", "text/xml" },
> +    { "xpm", "image/x-xpm" },
> +    { "xsl", "text/xsl" },
> +    { "xhtml", "application/xhtml+xml" },
> +    { "wml", "text/vnd.wap.wml" },
> +    { "wmlc", "application/vnd.wap.wmlc" },
> +    { 0, 0 }
> +};

Those looks standard…Why do you need to repeat them in getMIMETypeForExtension?

[Luciano Wolf]

Comment on attachment 206020 [details]
New patch with updated Changelog files

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

>> Source/WebCore/platform/nix/ErrorsNix.cpp:28
>> +#include "ErrorsNix.h"
> 
> All the literals in this file should be ASCIILiteral.

Sorry, I didn't get this one. This file is a plain copy of other port. Is it about some weird character in this file? Or is it about strings passed to the ResourceError method?

>> Source/WebCore/platform/nix/LocalizedStringsNix.cpp:38
>> +#include <wtf/text/WTFString.h>
> 
> I am opposed to this.
> 
> Nix should use LocalizedStrings.cpp. Certain other ports have their own file for historical reason and it is a pain in the ass to work with that.
> 
> All you should have to do is implement WebCore::localizedString.

I just left the implementation of localizedString into this file so every other method will come from LocalizedStrings.cpp.

>> Source/WebCore/platform/nix/MIMETypeRegistryNix.cpp:75
>> +};
> 
> Those looks standard…Why do you need to repeat them in getMIMETypeForExtension?

No reason to repeat. Was just a blind copy of other port. I've left just the {0, 0} line.

[Luciano Wolf]

Created attachment 206105 [details]
New patch with corrections

[Benjamin Poulain]

Comment on attachment 206105 [details]
New patch with corrections

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?

[Benjamin Poulain]

(In reply to comment #7)
> (From update of attachment 206020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206020&action=review
> 
> >> Source/WebCore/platform/nix/ErrorsNix.cpp:28
> >> +#include "ErrorsNix.h"
> > 
> > All the literals in this file should be ASCIILiteral.
> 
> Sorry, I didn't get this one. This file is a plain copy of other port. Is it about some weird character in this file? Or is it about strings passed to the ResourceError method?

WTF::String create from literals should use ASCIILiteral() unless they are translated.

[Luciano Wolf]

Comment on attachment 206105 [details]
New patch with corrections

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

>> Source/WebCore/platform/nix/GamepadsNix.cpp:60
>> +
> 
> Looking at Qt and GTK, I suspect this is dead code. You don't have any code to get the devices.

Nix architecture delegates all platform specifics to the application via our Platform API (WebKit::Platform in public/Platform.h). So, the logic to get device info isn't into WebCore but it forwards the sampleGamepads call to the application (line #42).

[Luciano Wolf]

Created attachment 206168 [details]
New patch with corrections and shared files

[Benjamin Poulain]

Comment on attachment 206168 [details]
New patch with corrections and shared files

I think you forgot to remove the GTK files you moved to glib.
Please do those in a separate patch, that would be much easier.

[Luciano Wolf]

Created attachment 206241 [details]
New patch with corrections - no mention to GTK

[Benjamin Poulain]

(In reply to comment #14)
> Created an attachment (id=206241) [details]
> New patch with corrections - no mention to GTK

I think there is a big misunderstanding.

Get me on IRC, and let's fix and land everything.

[Luciano Wolf]

Created attachment 206410 [details]
New patch without glib directory.

[Benjamin Poulain]

Comment on attachment 206410 [details]
New patch without glib directory.

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

> Source/WebCore/platform/nix/LanguageNix.cpp:44
> +        return String("c");

String("c") -> ASCIIString("c")

> Source/WebCore/platform/nix/NixKeyboardUtilities.h:34
> +namespace WTF {
> +class String;
> +}

WTF::String is not used in this header.

[Luciano Wolf]

Created attachment 206457 [details]
New reviewed patch.

[Luciano Wolf]

My bad.. sorry. Forgot to include --no-obsolete in the last webkit-patch upload. :/

[Build Bot]

Comment on attachment 206457 [details]
New reviewed patch.

Attachment 206457 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1034517

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206465 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Luciano Wolf]

Created attachment 211038 [details]
Rebased patch

[WebKit Commit Bot]

Comment on attachment 211038 [details]
Rebased patch

Clearing flags on attachment: 211038

Committed r155360: <http://trac.webkit.org/changeset/155360>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.