Bug 118358

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

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

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?

(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.

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

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?

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.

Created attachment 206105 [details] New patch with corrections

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?

(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.

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).

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

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.

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

(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.

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

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.

Created attachment 206457 [details] New reviewed patch.

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

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

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

Created attachment 211038 [details] Rebased patch

Comment on attachment 211038 [details] Rebased patch Clearing flags on attachment: 211038 Committed r155360: <http://trac.webkit.org/changeset/155360>

All reviewed patches have been landed. Closing bug.