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

[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 206020: New patch with updated Changelog files
https://bugs.webkit.org/attachment.cgi?id=206020&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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?