[Webkit-unassigned] [Bug 37369] [GTK] Enable building whatever already exists of WebKit2

Thu Oct 28 06:59:33 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=37369


--- Comment #37 from Amruth Raj <amruthraj at motorola.com>  2010-10-28 06:59:30 PST ---
(In reply to comment #30)
> (From update of attachment 71887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71887&action=review
> 
> Phwew! This is a big patch. Thanks for contributing it. I've reviewed the build changes so far. Hopefully I'll find some time later to look over the rest of the patch.
> 
> > GNUmakefile.am:163
> > +# WebKit2
> > +if ENABLE_WEBKIT2
> > +
> > +libJavaScriptCore_ladir = $(prefix)/include/webkit2- at WEBKITGTK_API_VERSION@/JavaScriptCore
> > +libJavaScriptCore_la_HEADERS = $(javascriptcore_h_api)
> > +
> > +lib_LTLIBRARIES += \
> > +	libwebkit2gtk- at WEBKITGTK_API_MAJOR_VERSION@. at WEBKITGTK_API_MINOR_VERSION@.la
> > +
> > +nodist_EXTRA_libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \
> > +	$(webcore_built_nosources)
> > +
> > +nodist_libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \
> > +	DerivedSources/WebKit2/WebKit2Prefix.h \
> > +	$(webcore_built_sources) \
> > +	$(webkit2_built_sources)
> > +
> > +libwebkit2gtk_ at WEBKITGTK_API_MAJOR_VERSION@_ at WEBKITGTK_API_MINOR_VERSION@_ladir = $(prefix)/include/webkit2- at WEBKITGTK_API_VERSION@/webkit2
> 
> Instead of repeating all of these source and flag lists, is it possible to create a variable in configure.ac and use it much like WEBKITGTK_API_MAJOR_VERSION?
Addressed in the newly attached patch
> 
> > JavaScriptCore/GNUmakefile.am:-54
> > -	JavaScriptCore/API/APICast.h \
> 
> Why have you removed these headers from the source list?
Originally done to remove the multiple copy error while creating the forward headers. With the suggestion by Balazs Keleman, these are no longer required. Reverted the changes now.
> 
> > JavaScriptCore/GNUmakefile.am:-71
> > -	JavaScriptCore/API/JSRetainPtr.h \
> 
> Same question.
Reverted the changes.
> 
> > JavaScriptCore/GNUmakefile.am:-76
> > -	JavaScriptCore/API/OpaqueJSString.h \
> 
> And here.
Reverted the changes.
> 
> > WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> This line should be replaced with an explaination of what sort of testing is possible, if at all.
> 
> > WebCore/ChangeLog:14
> > +        * GNUmakefile.am: Changes to enable disable building of WebCore/bindings/gobject when building WebKit2.
> > +        This is required since WebCore/bindings/gobject access WebKit/gtk/webkit files, which are skipped for WebKit2.
> > +        * platform/network/soup/cache/webkit/soup-cache.h: Changes to stop including <webkit/webkitdefines.h>
> > +        when building WebKit2. Instead the macro WEBKIT_API is defined directly in soup-cache.h. <webkit/webkitdefines.h>
> > +        is including only when building WebKit1. This is done as entire dir WebKit/gtk/webkit is skipped when building
> 
> Talking to Xan today, it seems that the bindings/gobject files only include webkitdefines.h for the WEBKIT_API visibility markers. Can you try to simply eliminate the webkitdefines.h include and reproduce WEBKIT_API in bindings/gobject. That would eliminate a lot of the conditional stuff below.
GObject separation is still required for WebKit2 build.
> 
> > WebCore/GNUmakefile.am:-2916
> > -	WebCore/storage/IDBIndexBackendInterface.h \
> 
> Why remove this?
Reverted
> 
> > WebCore/GNUmakefile.am:-2927
> > -	WebCore/storage/IDBObjectStoreBackendInterface.h \
> 
> And this?
Reverted
> 
> > WebCore/platform/network/soup/cache/webkit/soup-cache.h:6
> > + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  Except as provided below, all rights reserved.
> 
> It's fine for your copyright message to omit "Portions" and "Except as provided below,". If I understand correctly, these are implied.
We will get back to you after discussing this with our Legal Department.
> 
> > WebKit2/GNUmakefile.am:481
> > +# Copy header files to DerivedSources
> 
> I guess I feel that 'include' makes sense as the destination instead of DerivedSources/WebKit2/include. These files aren't generated, they are just copied.
Now that we are generating the forward headers, I hope the location should be fine :)
> 
> > WebKit2/GNUmakefile.am:483
> > +copy_wc_headers := $(filter %.h, $(webcore_sources))
> > +webcore-headers: $(copy_wc_headers)
> 
> This is sort of the reverse of what I would expect. I think the rule should have the verb. Something like this:
Removed this way of copying the headers now.
> 
> webcore_headers := $(filter %.h, $(webcore_sources))
> copy-webcore-headers: $(webcore_headers)
> 
> I have this preference for the rest of these copying rules too.
Same as above.
> 
> > WebKit2/GNUmakefile.am:538
> > +	$(WebKit2)/UIProcess/Plugins \
> > +    $(WebKit2)/WebProcess/Plugins \
> > +	$(WebKit2)/WebProcess/WebPage \
> 
> The indentation is a little funky here.
Done.
> 
> > WebKit2/GNUmakefile.am:541
> > +vpath %.messages.in = $(MESSAGE_INPUT_PATH)
> 
> Is it possible to list these directly in the vpath directive instead of spinning them off into a separate variable?
Done.
> 
> > WebKit2/GNUmakefile.am:543
> > +MESSAGE_GEN_SCRIPT = \
> 
> I'd rather this be in lower-case since it's not global. Something like message_gen_scripts.
Done.
> 
> > WebKit2/GNUmakefile.am:592
> > +# .PHONY: include-headers
> 
> I think you need to add all of the header copying targets to the .PHONY rule.
Removed PHONY rule.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.