Bug 132814 - [Stable] [GTK] GdiObject.h missing in WebKitGTK 2.4.1 tarball
Summary: [Stable] [GTK] GdiObject.h missing in WebKitGTK 2.4.1 tarball
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 133028
  Show dependency treegraph
 
Reported: 2014-05-12 03:02 PDT by Milan Crha
Modified: 2014-05-26 04:41 PDT (History)
5 users (show)

See Also:


Attachments
my changes to webkit (2.91 KB, patch)
2014-05-17 07:07 PDT, Milan Crha
no flags Details | Formatted Diff | Diff
Patch (3.06 KB, patch)
2014-05-25 04:22 PDT, Alberto Garcia
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Milan Crha 2014-05-12 03:02:40 PDT
Revision r155454 introduces GdiObject.h (see bug #120778), but this is missing in the sources of tarball for 2.4.1. There are still references to this file, the same as used functions (adoptGDIObject) in the sources. I reverted those references locally, but it would be good to have this fixed in the future releases.
Comment 1 Alberto Garcia 2014-05-12 14:09:58 PDT
I can see that the following files are missing from the tarball.

Source/WebCore/platform/win/GDIObjectCounter.h
Source/WebCore/platform/win/GDIObjectCounter.cpp
Source/WebCore/platform/graphics/win/SharedGDIObject.h
Source/WTF/wtf/win/GDIObject.h

Does it build fine if you add them manually?
Comment 2 Milan Crha 2014-05-13 00:24:03 PDT
I have more other changes here, which I'm trying to fine-tune before offering to upstream. I'm building with msys/mingw32.

I can try to add the file into sources, but I do not think it's enough to just get them from anywhere and hope it'll work, I expect it needs also some Makefile change thus the build system knows about those files (at least for the .cpp file). My question then is:

- where do I get those files from, please? Exact link welcome.
- which Makefile should I change and in what way?
Comment 3 Alberto Garcia 2014-05-13 00:57:16 PDT
(In reply to comment #2)
> - where do I get those files from, please? Exact link welcome.

You can browse the repository here:

   http://trac.webkit.org/browser/releases/WebKitGTK

Or you can also clone this git repository which contains the
webkit-2.4 branch with all the stable releases.

   https://github.com/bertogg/webkit

Here are the direct links to the files I mentioned earlier:

   https://raw.githubusercontent.com/bertogg/webkit/webkit-2.4/Source/WebCore/platform/win/GDIObjectCounter.h
   https://raw.githubusercontent.com/bertogg/webkit/webkit-2.4/Source/WebCore/platform/win/GDIObjectCounter.cpp
   https://raw.githubusercontent.com/bertogg/webkit/webkit-2.4/Source/WebCore/platform/graphics/win/SharedGDIObject.h
   https://raw.githubusercontent.com/bertogg/webkit/webkit-2.4/Source/WTF/wtf/win/GDIObject.h

> - which Makefile should I change and in what way?

You probably need to add those files to this Makefile:

   Source/WebCore/GNUmakefile.list.am

See the TARGET_WIN32 section. Run autogen.sh afterwards so the new
Makefiles are generated with your changes.
Comment 4 Milan Crha 2014-05-17 07:07:53 PDT
Created attachment 231626 [details]
my changes to webkit

Thanks for the pointer. i took the files from tracker's trunk branch. The path Source/WTF/wtf/win/ doesn't exist in tarball, thus I created it and added there the file. there had been some issues with GdiObject itself, mingw/msys has a clash with deleteObject, thus I renamed it to deleteObjectGDI and there was also a problem with specialization of that template,, thus the later change in the GdiObject.h. Other related changes are attached. I hope it helps.
Comment 5 WebKit Commit Bot 2014-05-17 07:09:04 PDT
Attachment 231626 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Alberto Garcia 2014-05-21 05:50:11 PDT
(In reply to comment #4)
> there had been some issues with GdiObject itself, mingw/msys has a
> clash with deleteObject, thus I renamed it to deleteObjectGDI

Doesn't the WTF namespace prevent this problem from happening?
Comment 7 Milan Crha 2014-05-21 08:50:07 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > there had been some issues with GdiObject itself, mingw/msys has a
> > clash with deleteObject, thus I renamed it to deleteObjectGDI
> 
> Doesn't the WTF namespace prevent this problem from happening?

I did not investigate it into deep, thus I do not know from where the clash comes, but if it's a define, then the namespace wouldn't help.

The thing is, without rename I get a compiler error (I can regenerate it, if it helps), then I searched the Internet and one way to fix it was to rename the symbol, which I did and it helped.
Comment 8 Alberto Garcia 2014-05-22 04:29:51 PDT
(In reply to comment #7)
> > > there had been some issues with GdiObject itself, mingw/msys has a
> > > clash with deleteObject, thus I renamed it to deleteObjectGDI
> >
> > Doesn't the WTF namespace prevent this problem from happening?

> The thing is, without rename I get a compiler error (I can
> regenerate it, if it helps), then I searched the Internet and one
> way to fix it was to rename the symbol, which I did and it helped.

It would be great if we can investigate this a bit more. If you can
regenerate the error and/or attach the relevant headers that produce
this clash then we can try to help.

Thanks!
Comment 9 Milan Crha 2014-05-24 14:10:39 PDT
(In reply to comment #8)
> It would be great if we can investigate this a bit more. If you can
> regenerate the error and/or attach the relevant headers that produce
> this clash then we can try to help.

Weird, I reverted my changes in GdiObject.h (the rename from deleteObject to deleteObjectGDI) and the compile finished without stopping on this as before. I also didn't fine the 'deleteObject' defined anywhere in mingw32 headers (and even binaries), thus it might be some other kind of a clash. I changed one thing, I configured webkitgtk with --enable-jit this time.

In any case, I propose to not use my rename in the GdiObject.h, just feel free to use the other parts of the patch.
Comment 10 Alberto Garcia 2014-05-25 04:22:59 PDT
Created attachment 232033 [details]
Patch

(In reply to comment #9)
> In any case, I propose to not use my rename in the GdiObject.h, just
> feel free to use the other parts of the patch.

Here's the updated patch. I made a couple of changes:

1) I renamed GdiObject.h to GDIObject.h in the patch. That's the
   correct name of the file.

2) I removed the if TARGET_WIN32 part around it, this is a header file
   so it's not really necessary.

We should also check if the master branch is affected by this, although
that would require a separate patch if it was the case.
Comment 11 Carlos Garcia Campos 2014-05-26 00:44:58 PDT
Comment on attachment 232033 [details]
Patch

Thanks!
Comment 12 Carlos Garcia Campos 2014-05-26 00:45:36 PDT
Pushed to the stable branch as <http://trac.webkit.org/changeset/169331>
Comment 13 Milan Crha 2014-05-26 00:51:23 PDT
(In reply to comment #10)
> 1) I renamed GdiObject.h to GDIObject.h in the patch. That's the
>    correct name of the file.

Ahh, unnoticed by me, due to Windows being case insensitive in file names. I suppose, for a sake of consistency, the #include directives around the source code should be also changed to capitals on GDI.

> 2) I removed the if TARGET_WIN32 part around it, this is a header file
>    so it's not really necessary.

Are you sure? You definitely do not want to compile windows-specific headers on Linux (or basically any other than Windows system), the same as you do not compile posix related code on windows. I would keep there the check for TARGET_WIN32, the same as it is in Source/WebCore/GNUmakefile.list.am.
Comment 14 Alberto Garcia 2014-05-26 04:41:19 PDT
(In reply to comment #13)

> > 2) I removed the if TARGET_WIN32 part around it, this is a header
> >    file so it's not really necessary.

> Are you sure? You definitely do not want to compile windows-specific
> headers on Linux

Well, headers are not compiled :)