Bug 58030 - [WK2][Qt][GTK] Introduce common use flag for the shared UNIX domain socket IPC implementation
Summary: [WK2][Qt][GTK] Introduce common use flag for the shared UNIX domain socket IP...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on: 49791
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-07 05:07 PDT by Balazs Kelemen
Modified: 2011-04-08 10:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.65 KB, patch)
2011-04-07 05:54 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-04-07 05:07:08 PDT
A common flag would be more readable then using "PLATFORM(QT) || PLATFORM(GTK)" everywhere in the IPC code.
Furthermore it would be useful if a new platform would like to use the infrastructure.
Comment 1 Benjamin Poulain 2011-04-07 05:09:27 PDT
I like the idea. The keyword "Unix" is not gonna do the trick since Mac OS X is also Unix. :(
So maybe a keyword by the type of IPC used?
Comment 2 Kenneth Rohde Christiansen 2011-04-07 05:15:44 PDT
Mac is Unix, and doesn't this work for Mac as well? Mac is just a specialization of Unix which has another more platform close implementation
Comment 3 Zoltan Horvath 2011-04-07 05:27:20 PDT
Does it make sense to use OS(LINUX)?
Comment 4 Balazs Kelemen 2011-04-07 05:45:38 PDT
(In reply to comment #3)
> Does it make sense to use OS(LINUX)?
It's too restrictive. I think it works on most of the OS-s that is in the group of OS(UNIX) (in Platform.h) - at least on Linux+*BSD+Mac.
Comment 5 Balazs Kelemen 2011-04-07 05:54:53 PDT
Created attachment 88613 [details]
Patch

Note: the define has been placed in Platform.h instead of WebKit2/config.h because in the final build step the include order is broken and we pick up WebCore's config.h even for the WebKit2 API files. Annoying bug but I do not see any solution for that.
Comment 6 Kenneth Rohde Christiansen 2011-04-07 06:12:00 PDT
Comment on attachment 88613 [details]
Patch

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

> Source/JavaScriptCore/wtf/Platform.h:1199
> +// This one is for WebKit2.

Strange comment; I would just remove it
Comment 7 Andras Becsi 2011-04-07 06:12:33 PDT
(In reply to comment #5)
> Note: the define has been placed in Platform.h instead of WebKit2/config.h because in the final build step the include order is broken and we pick up WebCore's config.h even for the WebKit2 API files. Annoying bug but I do not see any solution for that.

One solution would be to change the config.h includes to WebKit2/config.h and WebCore/config.h respectively.
Comment 8 Balazs Kelemen 2011-04-07 08:49:23 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > Note: the define has been placed in Platform.h instead of WebKit2/config.h because in the final build step the include order is broken and we pick up WebCore's config.h even for the WebKit2 API files. Annoying bug but I do not see any solution for that.
> 
> One solution would be to change the config.h includes to WebKit2/config.h and WebCore/config.h respectively.

Or rename each config.h to represent the component that it belongs to. (i.e. WebCoreConfig.h, WebKit2Config.h, ...). The problem is that it would require a huge change and furthermore I guess not everybody would like that style.
Comment 9 Balazs Kelemen 2011-04-07 08:53:08 PDT
(In reply to comment #6)
> (From update of attachment 88613 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88613&action=review
> 
> > Source/JavaScriptCore/wtf/Platform.h:1199
> > +// This one is for WebKit2.
> 
> Strange comment; I would just remove it

I will do it on commit. However I forget to set the dependency. We need to wait for the GTK SharedMemory patch to be landed first.
Comment 10 Martin Robinson 2011-04-07 16:58:13 PDT
Comment on attachment 88613 [details]
Patch

Thanks for cleaning this up!
Comment 11 Balazs Kelemen 2011-04-08 10:05:26 PDT
Committed r83307: <http://trac.webkit.org/changeset/83307>
Comment 12 WebKit Review Bot 2011-04-08 10:53:24 PDT
http://trac.webkit.org/changeset/83307 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/5354455-1.html
editing/selection/5354455-2.html
editing/selection/button-right-click.html
editing/selection/context-menu-on-text.html
editing/selection/context-menu-text-selection.html
editing/selection/empty-cell-right-click.html
fast/events/context-onmousedown-event.html
fast/events/contextmenu-scrolled-page-with-frame.html
fast/events/right-click-focus.html
svg/custom/use-events-crash.svg