Bug 135819

Summary: progress towards building WebCore with CMake on Mac
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: WebCore Misc.Assignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, cgarcia, commit-queue, eric.carlson, glenn, gyuyoung.kim, jer.noble, laszlo.gombos, mrobinson, philipj, rakuco, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch laszlo.gombos: review+

Description Alex Christensen 2014-08-11 16:25:24 PDT
The ObjC bindings have a few things that are different than other bindings.  There are also a few quirks I've found.
Comment 1 Alex Christensen 2014-08-12 13:45:09 PDT
Created attachment 236464 [details]
Patch
Comment 2 Brent Fulgham 2014-08-12 14:39:01 PDT
Comment on attachment 236464 [details]
Patch

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

I don't think this is quite right, since it breaks an existing port that relies on CMake for its builds. Otherwise I think the general approach is good.

> Source/WebCore/CMakeLists.txt:-178
> -

Can these just be commented out with a FIXME(Bug ###) comment?

> Source/WebCore/PlatformMac.cmake:-146
> -    platform/mac/WebCoreView.m

Where did this go? Do we not have this file anymore?

> Source/WebCore/PlatformMac.cmake:-166
> -    platform/network/cf/ProtectionSpaceCFNet.cpp

Ditto.

> Source/WebCore/PlatformMac.cmake:-204
> -    plugins/mac/PluginViewMac.mm

Were these removed?

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:384
> +    die if $fatalError && $className ne "DOMAbstractView"; # ObjCCustomImplementation doesn't work with CMake right now.

Can you make this a FIXME(Bug XXX) comment, please?

> Source/cmake/OptionsMac.cmake:212
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_IOS_AIRPLAY_iphonesimulator OFF)

You should have a FIXME(Bug XXX) here to remind you to turn these back on.

> Source/cmake/WebKitMacros.cmake:37
> +macro(GENERATE_BINDINGS _output_source _input_files _base_dir _idl_includes _features _destination _prefix _generator _extension _idl_attributes_file)

This is causing the EFL build to fail. You need to add the 'cpp' target to those CMake files.
Comment 3 Alex Christensen 2014-08-12 14:44:19 PDT
Created attachment 236469 [details]
Patch
Comment 4 Alex Christensen 2014-08-12 14:50:29 PDT
(In reply to comment #2)
> (From update of attachment 236464 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236464&action=review
> 
> I don't think this is quite right, since it breaks an existing port that relies on CMake for its builds. Otherwise I think the general approach is good.
> 
> > Source/WebCore/CMakeLists.txt:-178
> > -
> 
> Can these just be commented out with a FIXME(Bug ###) comment?
Will do, once I see the EWS response to this patch.
> 
> > Source/WebCore/PlatformMac.cmake:-146
> > -    platform/mac/WebCoreView.m
> 
> Where did this go? Do we not have this file anymore?
It doesn't know what to do with .m files right now.  I was removing it to get other things to compile.  I'll get rid of this change.
> 
> > Source/WebCore/PlatformMac.cmake:-166
> > -    platform/network/cf/ProtectionSpaceCFNet.cpp
> 
> Ditto.
> 
> > Source/WebCore/PlatformMac.cmake:-204
> > -    plugins/mac/PluginViewMac.mm
> 
> Were these removed?
They aren't in the mac build any more.  Maybe. I don't know what they are.
Comment 5 Alex Christensen 2014-08-12 15:32:02 PDT
Created attachment 236475 [details]
Patch
Comment 6 Alex Christensen 2014-08-12 15:47:23 PDT
Created attachment 236476 [details]
Patch
Comment 7 Alex Christensen 2014-08-12 15:52:42 PDT
I think plugins/mac is dead code.  I'll remove it in a separate patch.
ProtectionSpaceCFNet.cpp is only used in the AppleWin port.  I had added it to the Mac build accidentally with the other CF files.
Comment 8 Carlos Garcia Campos 2014-08-12 23:31:30 PDT
Comment on attachment 236476 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:253
>      my $fileName = "WebCore/bindings/objc/PublicDOMInterfaces.h";
> +    if (!-e $fileName) {
> +        # CMake looks in the WebCore directory.
> +        $fileName = "bindings/objc/PublicDOMInterfaces.h"
> +    }

You could use the same code in both cases by finding the bindings dir using:

use FindBin;

my $bindingsDir = dirname($FindBin::Bin);
my $fileName = "$bindingsDir/objc/PublicDOMInterfaces.h";

That's what we do in CodeGeneratorGObject.pm
Comment 9 Alex Christensen 2014-08-13 13:15:55 PDT
Created attachment 236545 [details]
Patch
Comment 10 Alex Christensen 2014-08-13 14:48:35 PDT
(In reply to comment #8)
> (From update of attachment 236476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236476&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:253
> >      my $fileName = "WebCore/bindings/objc/PublicDOMInterfaces.h";
> > +    if (!-e $fileName) {
> > +        # CMake looks in the WebCore directory.
> > +        $fileName = "bindings/objc/PublicDOMInterfaces.h"
> > +    }
> 
> You could use the same code in both cases by finding the bindings dir using:
> 
> use FindBin;
> 
> my $bindingsDir = dirname($FindBin::Bin);
> my $fileName = "$bindingsDir/objc/PublicDOMInterfaces.h";
> 
> That's what we do in CodeGeneratorGObject.pm
Done.

I think the Mac WK2 test failures have nothing to do with this.
Comment 11 Laszlo Gombos 2014-08-13 15:49:07 PDT
Comment on attachment 236545 [details]
Patch

LGTM - as it seems the Gamepad code is not yet enable in any existing cmake-based ports anyways, so this will not cause a regression.
Comment 12 Alex Christensen 2014-08-13 15:53:48 PDT
http://trac.webkit.org/changeset/172540