Only adding the compilation directives that are Nix specific, in the existent WebCore files
Created attachment 205945 [details] Patch
Comment on attachment 205945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205945&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:492 > -#if USE(ACCELERATED_COMPOSITING) > +#if USE(ACCELERATED_COMPOSITING) typo
(In reply to comment #2) > (From update of attachment 205945 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205945&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:492 > > -#if USE(ACCELERATED_COMPOSITING) > > +#if USE(ACCELERATED_COMPOSITING) > > typo Sorry, I didn't get it. Shouldn't that trailing space be removed by this patch?
Comment on attachment 205945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205945&action=review > Source/WebCore/platform/graphics/cairo/GLContext.cpp:22 > +#if USE(OPENGL) || USE(OPENGL_ES_2) I think you should add "|| (PLATFORM(NIX) && USE(OPENGL_ES_2))" to make sure you don't affect other ports. It's not clear for me why this change is nix specific.
Comment on attachment 205945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205945&action=review > Source/WebCore/platform/DragImage.h:64 > -#elif PLATFORM(GTK) > +#elif PLATFORM(GTK) || PLATFORM(NIX) > typedef cairo_surface_t* DragImageRef; I wonder why this isn't under USE(CAIRO). > Source/WebCore/platform/FileSystem.h:124 > -#elif PLATFORM(GTK) > +#elif PLATFORM(GTK) || PLATFORM(NIX) > typedef GFileIOStream* PlatformFileHandle; I wonder if we should add USE(GLIB). It's quite confusing that "G" types are used by two platforms, one of which doesn't have a "G" in its name. > Source/WebCore/platform/PlatformMenuDescription.h:38 > +#elif PLATFORM(NIX) > +#include <wtf/Vector.h> Why is this needed? You are not changing this file in any other way, and this file is not using Vector. > Source/WebCore/platform/audio/FFTFrame.h:189 > +#if PLATFORM(NIX) > + void scalePlanarData(float scale); > + OwnPtr<WebKit::WebFFTFrame> m_fftFrame; > +#endif // PLATFORM(NIX) Ugh, WebCore is not supposed to know anything about WebKit. This is a terrible layering violation.
Hi, thanks for spend time reviewing the code. (In reply to comment #5) > (From update of attachment 205945 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205945&action=review > > > Source/WebCore/platform/DragImage.h:64 > > -#elif PLATFORM(GTK) > > +#elif PLATFORM(GTK) || PLATFORM(NIX) > > typedef cairo_surface_t* DragImageRef; > > I wonder why this isn't under USE(CAIRO). Efl port also use Cairo and it's not included in this #if. > > Source/WebCore/platform/FileSystem.h:124 > > -#elif PLATFORM(GTK) > > +#elif PLATFORM(GTK) || PLATFORM(NIX) > > typedef GFileIOStream* PlatformFileHandle; > > I wonder if we should add USE(GLIB). It's quite confusing that "G" types are used by two platforms, one of which doesn't have a "G" in its name. Efl port also uses Glib in some places and it's not named GEfl, so Nix doesn't need to be named GNix ;-). For sure some Efl/Gtk/Nix code can be unified under a common glib directory/prefix, this is planned by us but we need to go one step at time. > > Source/WebCore/platform/PlatformMenuDescription.h:38 > > +#elif PLATFORM(NIX) > > +#include <wtf/Vector.h> > > Why is this needed? You are not changing this file in any other way, and this file is not using Vector. True, this change must go on the cpp file using it. > > Source/WebCore/platform/audio/FFTFrame.h:189 > > +#if PLATFORM(NIX) > > + void scalePlanarData(float scale); > > + OwnPtr<WebKit::WebFFTFrame> m_fftFrame; > > +#endif // PLATFORM(NIX) > > Ugh, WebCore is not supposed to know anything about WebKit. This is a terrible layering violation. The name may be confused you, this WebKit namespace isn't WebKit, it's the code located on our Platform library, similar to what Chromium had, the idea is to delegate the backend logic out of WebCore to an external library, and the external library knows nothing about WebCore, WebKit, etc.. the namespace name was indeed inherited from the name Chromium used in their API's.
(In reply to comment #4) > (From update of attachment 205945 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205945&action=review > > > Source/WebCore/platform/graphics/cairo/GLContext.cpp:22 > > +#if USE(OPENGL) || USE(OPENGL_ES_2) > > I think you should add "|| (PLATFORM(NIX) && USE(OPENGL_ES_2))" to make sure you don't affect other ports. It's not clear for me why this change is nix specific. Makes sense.
Created attachment 206164 [details] Patch
Comment on attachment 206164 [details] Patch > The name may be confused you, this WebKit namespace isn't WebKit Please don't do that.
> Efl port also use Cairo and it's not included in this #if. Following precedent is not a good enough excuse. Adding a new port has a significant cost for other WebKit contributors, so it's best to reduce it by fixing confusing code, instead of adding more.
(In reply to comment #9) > (From update of attachment 206164 [details]) > > The name may be confused you, this WebKit namespace isn't WebKit > > Please don't do that. We did that to keep compatibility with Chromium that use this exact namespace name for the same purposes as us, in fact we shared some Chromium platform files at some time. I agree with you the name would be better but this namespace is part of our public API so change it implies in replicate the changes in other projects using it, then my proposal is to keep the namespace name and later the Nix team could decide for a better name for the namespace in the API and do the necessary changes in other projects using Nix.
(In reply to comment #10) > > Efl port also use Cairo and it's not included in this #if. > > Following precedent is not a good enough excuse. Adding a new port has a significant cost for other WebKit contributors, so it's best to reduce it by fixing confusing code, instead of adding more. We can move the #if to define DragImageRef to a void*, but just because we aren't really using DragImageRef right now, so the problem will possible appear again when we spend some love with drag support, our DragImageRef will probably be a cairo_surface_t. Other than that what's your suggestion? do: #if USE(CAIRO) && !PLATFORM(EFL) ? Thanks.
Created attachment 206679 [details] Patch WebKit namespace renamed; Some #ifs merged into USE(GLIB)
So, any comments?
Comment on attachment 206679 [details] Patch Let see if the commit queue still can apply it without a manual rebase.
Comment on attachment 206679 [details] Patch Rejecting attachment 206679 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 206679, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /platform/graphics/texmap/TextureMapper.h patching file Source/WebCore/plugins/PluginPackage.cpp patching file Source/WebCore/plugins/PluginView.cpp Hunk #1 succeeded at 830 (offset -1 lines). patching file Source/WebCore/plugins/PluginView.h Hunk #1 succeeded at 369 (offset -14 lines). patching file Source/WebCore/plugins/PluginViewNone.cpp Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Eric Carlson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/1728378
Created attachment 211040 [details] Patch
(In reply to comment #17) > Created an attachment (id=211040) [details] > Patch Did you mean to mark this r?
Comment on attachment 211040 [details] Patch I guess I can just cq+, I was just waiting for the EWS, well.. let's see.
Comment on attachment 211040 [details] Patch Clearing flags on attachment: 211040 Committed r155358: <http://trac.webkit.org/changeset/155358>
All reviewed patches have been landed. Closing bug.