Bug 118326 – Preparing WebCore to receive Nix Port

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

[Thiago de Barros Lacerda]

Only adding the compilation directives that are Nix specific, in the existent WebCore files

[Thiago de Barros Lacerda]

Created attachment 205945 [details]
Patch

[Csaba Osztrogonác]

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

[Luciano Wolf]

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

[Rafael Brandao]

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.

[Alexey Proskuryakov]

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.

[Hugo Parente Lima]

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.

[Thiago de Barros Lacerda]

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

[Hugo Parente Lima]

Created attachment 206164 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 206164 [details]
Patch

> The name may be confused you, this WebKit namespace isn't WebKit

Please don't do that.

[Alexey Proskuryakov]

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

[Hugo Parente Lima]

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

[Hugo Parente Lima]

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

[Hugo Parente Lima]

Created attachment 206679 [details]
Patch

WebKit namespace renamed; Some #ifs merged into USE(GLIB)

[Hugo Parente Lima]

So, any comments?

[Hugo Parente Lima]

Comment on attachment 206679 [details]
Patch

Let see if the commit queue still can apply it without a manual rebase.

[WebKit Commit Bot]

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

[Hugo Parente Lima]

Created attachment 211040 [details]
Patch

[Eric Carlson]

(In reply to comment #17)
> Created an attachment (id=211040) [details]
> Patch

Did you mean to mark this r?

[Hugo Parente Lima]

Comment on attachment 211040 [details]
Patch

I guess I can just cq+, I was just waiting for the EWS, well.. let's see.

[WebKit Commit Bot]

Comment on attachment 211040 [details]
Patch

Clearing flags on attachment: 211040

Committed r155358: <http://trac.webkit.org/changeset/155358>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.