Bug 8515 – Linux porting compile bug

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Michael Emmel]

This bug is a place holder to block on all bugs that prevent compilation on linux.
More forthcoming

[Michael Emmel]

Created attachment 8741 [details]
patch and new files for linux port (tar.gz)


Code for gdk linux port

[David Kilzer (:ddkilzer)]

Created attachment 8743 [details]
WebKit.patch (from Attachment 8741 [details])

Extracted patch from Attachment 8741 [details] for easier viewing.

[Darin Adler]

Comment on attachment 8741 [details]
patch and new files for linux port (tar.gz)

Overall looks great!

A few things need fixing before we can land all of this. Ideally I'd like to separate out the part with no comments and land it first, so we can then discuss just the parts that have issues. I reviewed only the patch this time around, not the new files.

The large list of VK_ macros in PlatformKeyboardEvent.h under the PLATFORM(GDK) guard seems strange. Why do those need to be there? Are they GDK-specific?

I'd suggest putting those in a separate header because they are as big as the rest of the file combined.

Also, I suggest using C++ constants instead of #define for such things.

The formatting and indentation of the GDK PlatformKeyboardEvent constructors doesn't match the rest of the file. Lets not have that large inline constructor at all, and please indent those to match the rest of the class definition.

I'd also prefer to not have the PlatformWheelEvent constructor inline in the header.

A number of files have whitespace changes unrelated to the GDK changes. I'd like to see the patch without those. For example, the blank lines removed from TransferJob.h.

ScrollView::setFrameGeometry is not properly indented either.

For code like this:

+      Widget(GdkDrawable *drawable);
+      virtual void setDrawable(GdkDrawable *drawable);
+      GdkDrawable *drawable() const;

our style is:

+      Widget(GdkDrawable*);
+      virtual void setDrawable(GdkDrawable*);
+      GdkDrawable* drawable() const;

The * goes next to the type name, and we don't give names to parameters unless they are needed for clarity.

This section from config.h seems wrong:

+#if PLATFORM(GDK)
+#include <math.h>
+#include <assert.h>
+#endif

If we're going to include those in the config.h file, then it should be unconditionally for all platforms. But I think this is working around missing <math.h> and <assert.h> in a few files that we could address by just adding it to those files.

From XPathValue.cpp:

+#if PLATFORM(GDK)
+#include <math.h>
+#endif

If this is needed, we should not put it inside an #f.

In maketokenizer:

+if( $flex_version = ~/2.5/ ) {

The regular expression above means 2 followed by anything followed by 5. I think instead you want /version 2\.5/ because you don't want to match versions that just happen to have "2.5" in them. But I know that's not correct, because the flex version on OS X 10.4 is 2.5.4 -- so this is not the right check. We should discuss this change on IRC or the mailing list.

+#if PLATFORM(GDK)
+#include <errno.h>
+#endif

Again, if this is needed it should not be in an ifdef.

Could you explain the change to make-generated-sources.sh? Why is it needed?

Please include a ChangeLog entry to answer questions like these.

[Darin Adler]

I looked at some of the separate files. Many have tabs for formatting. We don't use tabs for any WebKit code -- needs to be indented with spaces instead. Most editors have a setting to tell it to do that.

Non of the copyright notices in any of the files mention you -- they all say "Copyright Apple Computer". Please make sure they copyrights are accurate. Also, some of the files have LGPL licenses on them. New code should use the BSD-style license unless there's a good reason not to.

[Mark Rowe (bdash)]

I have a few random comments after trying the patch on a clean Ubuntu Dapper installation.

* JavaScriptCore fails to build as it cannot find ./grammarWrapper.cpp.  Changing the Bakefile to use DerivedSources/JavaScriptCore/grammar.cpp resolves this.

* What version of Cairo does this patch depend on?  Ubuntu Dapper ships with Cairo 1.0.4 which appears to be the most recent release.  FontDataGdk.cpp uses cairo_scaled_font_text_extents which does not appear in Cairo 1.0.4, but is present in the Cairo source within the WebKit tree.  It doesn't seem possible to build WebCore against the WebKit version of Cairo due to GTKs use of Cairo, but I'm not 100% certain of the issues around this.

* Your tarball includes gdklauncher.d and other intermediate files under WebKitTools/GdkLauncher/.  The presence of the .d file causes Make to look for some files under /home/memmel.

* WebKitTools/GdkLauncher/gdklauncher.bkl contains a reference to /home/memmel/Source/Build.  I had to remove the ldflags line containing the reference, and adjust the other ldflags line to link against libjscore.a before gdklauncher would try and link.

I'll note that I didn't successfully get this building and linking due to the Cairo issue that I mentioned above.

[Michael Emmel]

(In reply to comment #3)
> (From update of attachment 8741 [details] [edit])
> Overall looks great!
> 
> A few things need fixing before we can land all of this. Ideally I'd like to
> separate out the part with no comments and land it first, so we can then
> discuss just the parts that have issues. I reviewed only the patch this time
> around, not the new files.
> 
> The large list of VK_ macros in PlatformKeyboardEvent.h under the PLATFORM(GDK)
> guard seems strange. Why do those need to be there? Are they GDK-specific?
> 
> I'd suggest putting those in a separate header because they are as big as the
> rest of the file combined.
> 
> Also, I suggest using C++ constants instead of #define for such things.
> 
They can go in a different header and there generic I'll put them in a 
different file WindowKeyboardCodes.h ??

> The formatting and indentation of the GDK PlatformKeyboardEvent constructors
> doesn't match the rest of the file. Lets not have that large inline constructor
> at all, and please indent those to match the rest of the class definition.
> 
> I'd also prefer to not have the PlatformWheelEvent constructor inline in the
> header.
> 
> A number of files have whitespace changes unrelated to the GDK changes. I'd
> like to see the patch without those. For example, the blank lines removed from
> TransferJob.h.
> 
> ScrollView::setFrameGeometry is not properly indented either.
> 
> For code like this:
> 
> +      Widget(GdkDrawable *drawable);
> +      virtual void setDrawable(GdkDrawable *drawable);
> +      GdkDrawable *drawable() const;
> 
> our style is:
> 
> +      Widget(GdkDrawable*);
> +      virtual void setDrawable(GdkDrawable*);
> +      GdkDrawable* drawable() const;
> 
> The * goes next to the type name, and we don't give names to parameters unless
> they are needed for clarity.
> 
> This section from config.h seems wrong:
> 
> +#if PLATFORM(GDK)
> +#include <math.h>
> +#include <assert.h>
> +#endif
> 
> If we're going to include those in the config.h file, then it should be
> unconditionally for all platforms. But I think this is working around missing
> <math.h> and <assert.h> in a few files that we could address by just adding it
> to those files.
> 
I'll remove this and fix the files.

> From XPathValue.cpp:
> 
> +#if PLATFORM(GDK)
> +#include <math.h>
> +#endif
> 
> If this is needed, we should not put it inside an #f.
> 
> In maketokenizer:
> 
> +if( $flex_version = ~/2.5/ ) {
> 
> The regular expression above means 2 followed by anything followed by 5. I
> think instead you want /version 2\.5/ because you don't want to match versions
> that just happen to have "2.5" in them. But I know that's not correct, because
> the flex version on OS X 10.4 is 2.5.4 -- so this is not the right check. We
> should discuss this change on IRC or the mailing list.
> 
I'll post this patch corrected to the mailing list I'm not sure what 
the right answer is either.

> +#if PLATFORM(GDK)
> +#include <errno.h>
> +#endif
> 
> Again, if this is needed it should not be in an ifdef.
> 
> Could you explain the change to make-generated-sources.sh? Why is it needed?
> 
> Please include a ChangeLog entry to answer questions like these.
> 

I'll do that last once the the files changed is a constant.

[Michael Emmel]

(In reply to comment #5)
> I have a few random comments after trying the patch on a clean Ubuntu Dapper
> installation.
> 
> * JavaScriptCore fails to build as it cannot find ./grammarWrapper.cpp. 
> Changing the Bakefile to use DerivedSources/JavaScriptCore/grammar.cpp resolves
> this.
> 
> * What version of Cairo does this patch depend on?  Ubuntu Dapper ships with
> Cairo 1.0.4 which appears to be the most recent release.  FontDataGdk.cpp uses
> cairo_scaled_font_text_extents which does not appear in Cairo 1.0.4, but is
> present in the Cairo source within the WebKit tree.  It doesn't seem possible
> to build WebCore against the WebKit version of Cairo due to GTKs use of Cairo,
> but I'm not 100% certain of the issues around this.
> 
I've been using the CVS version for now it should work.

To keep from colliding you need to build a version of cairo 
And set up the following env var


PKG_CONFIG_PATH=/home/memmel/Source/Build/lib/pkgconfig:/usr/lib/pkgconfig

I'll also add support for using the included version of cairo.


> * Your tarball includes gdklauncher.d and other intermediate files under
> WebKitTools/GdkLauncher/.  The presence of the .d file causes Make to look for
> some files under /home/memmel.
> 
> * WebKitTools/GdkLauncher/gdklauncher.bkl contains a reference to
> /home/memmel/Source/Build.  I had to remove the ldflags line containing the
> reference, and adjust the other ldflags line to link against libjscore.a before
> gdklauncher would try and link.
> 
> I'll note that I didn't successfully get this building and linking due to the
> Cairo issue that I mentioned above.
> 
Okay I'll clean that up.

[Mark Rowe (bdash)]

I'm not entirely clear how linking WebCore against a custom Cairo build will resolve the issue.  Because of the default library search path including /usr/lib/, the system-installed incompatible libcairo.so will be found and used unless the custom libcairo.so is forced by setting LD_LIBRARY_PATH or altering ld.conf.  Even then, using a CVS version of libcairo.so may cause issues as the rest of the GTK libraries have been linked against the older stable version rather than the potentially different CVS API.

[Michael Emmel]

(In reply to comment #8)
> I'm not entirely clear how linking WebCore against a custom Cairo build will
> resolve the issue.  Because of the default library search path including
> /usr/lib/, the system-installed incompatible libcairo.so will be found and used
> unless the custom libcairo.so is forced by setting LD_LIBRARY_PATH or altering
> ld.conf.  Even then, using a CVS version of libcairo.so may cause issues as the
> rest of the GTK libraries have been linked against the older stable version
> rather than the potentially different CVS API.
> 

I actually use a complete build of gtk etc so I don't have these problems.
Cairo 1.0.4 is a stable release and should work with gtk and seems to have the calls I need can you use that as your system cairo ? I don't think 1.0.2 is usable for this. Be warned that I may need to move to the 1.2x releases at any time to get some features I need so you should really consider either doing a full build of gtk etc or carefully upgrading the system version.

Do you have other suggestions  besides downgrading to 1.0.2 that wont work?

[Mark Rowe (bdash)]

Mike, Ubuntu Dapper uses Cairo 1.0.4 and it lacks the cairo_scaled_font_text_extents call that I mentioned.  Because I'm interested in playing around with this, I built + installed the latest development snapshot of Cairo (1.1.6).  This allowed gdklauncher to link.  I definitely think it is desireable to either work with the latest stable release of Cairo or use the WebKit Cairo tree in some way that won't conflict with GTKs use.

It's impressive to see WebCore rendering so well on Linux at such an early stage.  Good work!

[Dave Hyatt]

I would recommend trying to use WebCore's Cairo, since it is a copy of Mozilla's fork of Cairo, and they have numerous bug fixes for stuff that haven't gone back upstream yet.  One issue will be that I just hardcoded the generated config stuff to be Windows-specific.

[Dave Hyatt]

You may want to look at what Mozilla did too, as I believe they build with their own version of Cairo on GTK.

[Michael Emmel]

(In reply to comment #11)
> I would recommend trying to use WebCore's Cairo, since it is a copy of
> Mozilla's fork of Cairo, and they have numerous bug fixes for stuff that
> haven't gone back upstream yet.  One issue will be that I just hardcoded the
> generated config stuff to be Windows-specific.
> 

The mozilla fixes have been rolled in for the most part in CVS/git or at least thats my understanding.  I don't really have a good answer here as I said I build all my own from cvs and considering the state of the port and the timeline before its stable I don't see that using stable versions of cairo/gdk are a requirement.

[Dave Hyatt]

Ok cool.  I don't care either way as long as it works. :)

[Michael Emmel]

Created attachment 8760 [details]
Patch file for Linux gdk build


I tried to clean up the patch based on the comments here is the new one

[Michael Emmel]

Created attachment 8761 [details]
Gdk port specific sources


Linux files main changes are the is a new PlatformKeyboardCodes.h this should prob
be moved into the generic dir.
Events reworked to only use the GdkEvent which cleanup up the code.

[Michael Emmel]

Created attachment 8762 [details]
Gdk build patch


Forgot to regerate after removing a few newlines

[Darin Adler]

Comment on attachment 8762 [details]
Gdk build patch

Still a few things that need fixing. Some minor, but some that would break Macintosh and WIndows build.

+    typedef GdkCursor * PlatformCursor;
+    void setFont(cairo_t *cr) const;
+    TransferJobInternal * getInternal() { return d;}
+      Widget(GdkDrawable *drawable);
+      virtual void setDrawable(GdkDrawable *drawable);
+      GdkDrawable *drawable() const;

Should not have a space before the *.

+    Glyph getGlyphIndex( UChar c ) const { return m_font.index (c); }

Should not have spaces around the UChar c.

Why the patch to make-charset-table.pl?

+#include "config.h"

Header files must *not* include config.h, so it's wrong to add this to headers. Instead, cpp files must include config.h.

-    
     TransferJobClient* client() const;
-
     void receivedResponse(PlatformResponse);
 
 private:
     void assembleResponseHeaders() const;
     void retrieveCharset() const;
-
     TransferJobInternal* d;

Why remove these newlines?

+#if WIN32 || PLATFORM(GDK)
         ScrollView();
         ~ScrollView();
+        virtual void setFrameGeometry(const IntRect&);

Why does WIN32 now have a setFrameGeometry virtual function in ScrollView? I think this will break the WIN32 compile.

The implementation of GraphicsContext::roundToDevicePixels is incorrect. The purpose of that function is to round a rectangle to pixel boundaries in device space and then convert it back to user space. But the function in this patch converts a rectangle to device space, which is something different.

 #include "config.h"
+#include <math.h>
 #include "XPathValue.h"

The include should go below inside the XPATH_SUPPORT ifdef. In general, the first include in every .cpp file should be the corresponding .h file, which serves as a test that the .h file includes enough to stand alone.

+$flex_version=`flex --version`;
+if( $flex_version = ~/2\.5/ ) {

As we discussed, we can't land this change as-is since it will break the Macintosh build.

+#note ENCODINGS_PREFIX needs to be a ws string so it does not turn into
+#a null value

We normally put spaces after the "#" and before the comment.

[Darin Adler]

Comment on attachment 8761 [details]
Gdk port specific sources

The .cpp files are missing includes of "config.h". Every .cpp file needs to include "config.h". Also, each .cpp file should includes its own .h file first, right after "config.h".

I don't think the file "PlatformKeyboardCodes.h" should have the name "Platform" in its name. The only reason the event files have Platform in their name is that they conflict with DOM classes. I think "KeyboardCodes.h" would be a fine name for the file.

Headers must not include "config.h". That's included by every .cpp file instead.

    return IntPoint(contentsPoint) + scrollOffset();

Why the cast to IntPoint here?

    m_isKeyUp = bool( event->type == GDK_KEY_RELEASE);
    m_shiftKey = bool(event->key.state & GDK_SHIFT_MASK != 0);
    m_ctrlKey = bool(event->key.state & GDK_CONTROL_MASK != 0);
    m_altKey = bool(event->key.state & GDK_MOD1_MASK != 0); /*alt ?*/
    m_metaKey = bool(event->key.state & GDK_MOD2_MASK != 0); /*meta ?*/

What are all those "bool" casts about? Explicit casts should not be needed.

    m_text = String(event->key.string);
    m_unmodifiedText = String(event->key.string);

And these String casts also should not be needed.

Lots of these files say Copyright 2005, 2006 in them. Did you really publish the code you are contributing in 2005 (publish roughly meaning something like "make available to others")? If not, then you shouldn't list 2005.

I see a mix of coding and formatting styles in this code. Most is not formatted according to the style we use in WebCore, for example there's a lot of 8-character indenting, spaces inside parentheses, braces around single-line if statements, return types on a separate line from function names, empty functions are {} on one line, etc. I presume you are planning to use astyle to fix some of the formatting issues. It would be good to fix the style to match other WebCore code and current gudelines as much as possible before landing all these new files, although not mandatory.

[Mark Rowe (bdash)]

Just a quick observation: there are two files included in the "GDK port specific sources" tarball that don't belong.  WebKitTools/GdkLauncher/.bakefile_gen.state and WebCore/platform/gdk/.FrameGdk.cpp.swp.  It'd be worth double-checking that you're not including files you don't intend to before submitting the final version of your patch.

[Michael Emmel]

Created attachment 8780 [details]
Latest patch for linux changes


Should address issues from reviewers

[Michael Emmel]

Created attachment 8781 [details]
Linux port files

Linux sources

[Darin Adler]

Comment on attachment 8780 [details]
Latest patch for linux changes

+    void setFont(cairo_t* cr) const;

We would leave out "cr" here.

+    Glyph getGlyphIndex(UChar c) const { return m_font.index (c); }

We would not put a space after index and before "(c)".

I think you should use round rather than nearbyint -- nearbyint is affected by the prevailing rounding mode.

 #include "config.h"
+#include <math.h>
 #include "HTMLCanvasElement.h"

The <math.h> include should go a little further down. The concept is that the .cpp file includes its own header file.

These are minor issues, so r=me.

[Michael Emmel]

Created attachment 8786 [details]
Small changes from last comments

[Darin Adler]

Comment on attachment 8786 [details]
Small changes from last comments

r=me

[Geoffrey Garen]

Committed attachment 8786 [details]: revision 14807.

Do be sure to add a ChangeLog entry and copyright info in the future, and no tabs.

What's the story with the linux port files -- they just haven't been reviewed?

[Geoffrey Garen]

Comment on attachment 8786 [details]
Small changes from last comments

Clearing review flag so this won't show up in the 'to land' queue. (I just landed it.)

[Michael Emmel]

(In reply to comment #26)
> Committed attachment 8786 [details] [edit]: revision 14807.
> 
> Do be sure to add a ChangeLog entry and copyright info in the future, and no
> tabs.
> 
My editor is set to expand tabs now so no problem.
Copyright should be right.
For this particular integration considering the number of changes a changelog
entry would have been huge or so generic it would have been meaningless.
From here on out the changes will be specific and amendable to a changelog entry.



> What's the story with the linux port files -- they just haven't been reviewed?
> 

Not to any great degree there are a number of people intrested in the linux community so I think the code will be vetted over time.
For now the most important use of the port is to help refine the porting api
by having another target platform.

[Michael Emmel]

(In reply to comment #27)
> (From update of attachment 8786 [details] [edit])
> Clearing review flag so this won't show up in the 'to land' queue. (I just
> landed it.)
> 

I just checked out it did not come down yet.
Can I get a email when its avialable so I can test a clean build.

[David Kilzer (:ddkilzer)]

Comment on attachment 8780 [details]
Latest patch for linux changes

Cleared review+ on Attachment 8780 [details] so it won't show up in review+ queries.

[Michael Emmel]

(In reply to comment #30)
> (From update of attachment 8780 [details] [edit])
> Cleared review+ on Attachment 8780 [details] [edit] so it won't show up in review+ queries.
> 

I noticed the gdk patch is in but I've not seen the new directory show up yet.

Mike

[Mark Rowe (bdash)]

They haven't been reviewed, and thus haven't been landed.

[David Kilzer (:ddkilzer)]

(In reply to comment #32)
> They haven't been reviewed, and thus haven't been landed.

Mike, it might be easier for reviewers to review the files if they were in a patch with a ChangeLog entry.

[Michael Emmel]

(In reply to comment #33)
> (In reply to comment #32)
> > They haven't been reviewed, and thus haven't been landed.
> Mike, it might be easier for reviewers to review the files if they were in a
> patch with a ChangeLog entry.

I'm on vacation at my parents and my dad just has dailup.

If its possible to work with it as it I'd appreciate it.
Once we get past this inital checkin I think things will be a lot smoother
for everyone.

[Darin Adler]

Comment on attachment 8781 [details]
Linux port files

The long delay in reviewing this is largely due to the fact that it's attached as a tar file rather than a reviewable/applyable text file patch. Newly-created files work in patches, and the best way to post them for review and check-in is as a patch. The changes should have a ChangeLog entry and ideally they would conform much more closely to the style guidelines.

Formatting in these new files does not match the guidelines in many cases. For example almost all the namespace declarations look like this:

    namespace WebCore
    {

We normally put the brace on the line with the namespace declaration.

In many cases, the * is not next the the type. For example, this:

    GdkDrawable *drawable;

GdkDrawable * Widget::drawable() const
void Widget::setDrawable(GdkDrawable *drawable)

There are a couple constants that are needlessly made class members, which means they have to go in headers. In TransferJobManager:

    static const int selectTimeoutMS=5;
    static const double pollTimeSeconds;

Putting these outside the class would allow them to be inside the .cpp file and not require recompilation of other classes if they change.

There's an unused define in TransferJobManager.cpp:

#define SIMPLE_TRANSFER 1

Strange formatting here:

TransferJobManager::TransferJobManager()
        :
        m_downloadTimer(this,&TransferJobManager::downloadTimerCallback)

We don't use that format anywhere else.

void
TransferJobManager::useSimpleTransfer( bool useSimple )

We don't put the type on a separate line or use spaces inside parentheses like this.

    //job->header((char*)ptr, realsize);

We don't check in commented-out code.

                printf("%s, select timout",__PRETTY_FUNCTION__);

This is a compiler-specific extension -- do you need to use it? And it's a printf not inside an ifdef.

#if DEBUG

We don't use DEBUG -- we use NDEBUG.

    m_position = IntPoint((int)event->scroll.x,(int)event->scroll.y);

Why are type casts needed here?

#define WIN32_COMPILE_HACK

Why is this inside a GDK-specific file?

static bool
timeout_cb(gpointer data)
{
    (void)data;

We omit parameter names to avoid unused parameter warnings, rather than using the (void) technique, in C++. And we put the static and the type on the same line as the function name.

/**
Region of the content currently visible in the viewport
in the content views coordinate system
**/

This isn't a format we use for comments.

#if WIN32
typedef void* HANDLE;
typedef struct HINSTANCE__* HINSTANCE;
typedef HINSTANCE HMODULE;
#endif

#ifdef WIN32
    HMODULE m_themeDLL;
    mutable HANDLE m_buttonTheme;
    mutable HANDLE m_textFieldTheme;
#endif

Why are these WIN32-specific code fragments still in the GDK-specific source files?

There are lots of different approaches here to errors or missing code. For example, some code has LOG_ERROR for problems, other code has printf, other code has commented-out printf, other code calls a function notImplemented(), other code has a comment with FIXME in it, and other code has a comment without FIXME. This should be handled in a more consistent way.

The hardcoded list of what to do for each key in FrameGdk::handleGdkEvent doesn't seem like a good way to do things. We should see if we can make some of that cross-platform.

                RenderObject * r = node->renderer();
                //Default to scrolling the page
                //not sure why its null
                //broke anyway when its not null
                //if (!r)
                r = renderer();

Dead code like the call to node->renderer() should not be left in like this.

                doScroll(r,wheelEvent.isHorizontal(),wheelEvent.delta());

We use spaces after commas in parameter lists.

    if (!node)
        return false;
    else {

No need for else after return.

To make this code really fit with the rest of WebKit, I'd like to see it more consistent in style with other new code we're checking in, and more self-consistent about how it handles things like incomplete sections that need work.

However, we can be flexible about this if we have to, to get the new platform started. I'd request a pass of cleanup before we land this for the first time if that's practical for you.

[Michael Emmel]

(In reply to comment #35)

Thanks for the review I can now submit a new patch that corrects some of the concerns. I can certainly work on cleaning up the code to meet spec overtime.

Right now my main concern is getting it into the build so others can help.

Mike

[Michael Emmel]

Created attachment 9064 [details]
Latest linux port as a full patch


Address several of the concerns in last review cleaned up code style but there are probably still problems here.
Added Changelog entry

Did not redo KeyCode stuff I'll save this for later

Patch works agianst svn as of yesterday.

Since the compile actually breaks often because of small issues
such as headers I'd like to see if we can get this in and let me continue
to clean up so I can track compile breakage easier. I hope we can handle remaining issues as bugs.

[Michael Emmel]

Created attachment 9069 [details]
Patch for gdk linux port


Remove generated makefile from patch.
Move change log comments for JavaScriptCore over to its changlog

[Darin Adler]

Comment on attachment 9069 [details]
Patch for gdk linux port

Looks good. The formatting of the ChangeLog is not correct, but the rest of the changes look fine.

[Michael Emmel]

(In reply to comment #39)
> (From update of attachment 9069 [details] [edit])
> Looks good. The formatting of the ChangeLog is not correct, but the rest of the
> changes look fine.
> 

Ohh I see I forgot my full name and some new lines it looks like.
Do you want me to redo the patch ?

[Mark Rowe (bdash)]

It's not obvious to me why attachment 9064 [details] has been marked as obselete when attachment 9069 [details] is a very incomplete patch that only touches five files.  Is attachment 9069 [details] dependent on 9064 being applied first?  Should attachment 9064 [details] be unobseleted and marked as review+ if it is to be landed?

[Mark Rowe (bdash)]

I've just brought my tree up to date and applied the latest patch.  Compiling inside WebCore dies with the error "make: *** No rule to make target `../../DerivedSources/WebCore/ColorData.cpp', needed by `obj-gnu/webcore_gdk_ColorData.o'. Stop."

[Michael Emmel]

(In reply to comment #42)
> I've just brought my tree up to date and applied the latest patch.  Compiling
> inside WebCore dies with the error "make: *** No rule to make target
> `../../DerivedSources/WebCore/ColorData.cpp', needed by
> `obj-gnu/webcore_gdk_ColorData.o'. Stop."
> 


Once there in I'm sure I will need to make more changes the compile break frequently.

[Michael Emmel]

(In reply to comment #42)
> I've just brought my tree up to date and applied the latest patch.  Compiling
> inside WebCore dies with the error "make: *** No rule to make target
> `../../DerivedSources/WebCore/ColorData.cpp', needed by
> `obj-gnu/webcore_gdk_ColorData.o'. Stop."
> 


Once there in I'm sure I will need to make more changes the compile break frequently.
(In reply to comment #42)

I also want to add there is no way with the delay that I can provide a patch that will build since the linux build is broken reguarly by other changes.
The reasons are generally small.

I don't expect it to build now the patch was done to long ago.

But as soon as this is in I'll checkout and generate another patch.
At that point I'd like to request that the patch applier test the build and tag
it.  If we get the change small and thus the turnaround faster there is a good chance that I'll get a working build.

Please lets get this in so were not dealing with so many changes and so hopefully someone with commit access can make the small changes needed to keep the linux port building.

[Shawn Stricker (kb1ibt)]

I failed at a different point but that is moot at this point.
Bdash told me (over irc) to make a list of the packages need to get someone w/ Dapper from fairly close to fresh install all the way to getting this to compile. The list includes: make, bake, bison, libicu34-dev, libxml2-dev, libxslt1-dev, freetype2, libcurl3-gnutls-dev, python2.4-dev, curl, autoconf, automake1.9

A few of these might no be needed or could be interchaged w/ a different version but this is what I used to get to:
make: *** No rule to make target `../../DerivedSources/WebCore/CharsetData.cpp', needed by `obj-gnu/webcore_gdk_CharsetData.o'.  Stop.

[Michael Emmel]

Plus all the packages listed in presets sqllite etc.

Anyway it will be a bit before the linux port build for most people.
This is just the first step.

Mike

(In reply to comment #45)
> I failed at a different point but that is moot at this point.
> Bdash told me (over irc) to make a list of the packages need to get someone w/
> Dapper from fairly close to fresh install all the way to getting this to
> compile. The list includes: make, bake, bison, libicu34-dev, libxml2-dev,
> libxslt1-dev, freetype2, libcurl3-gnutls-dev, python2.4-dev, curl, autoconf,
> automake1.9
> 
> A few of these might no be needed or could be interchaged w/ a different
> version but this is what I used to get to:
> make: *** No rule to make target
> `../../DerivedSources/WebCore/CharsetData.cpp', needed by
> `obj-gnu/webcore_gdk_CharsetData.o'.  Stop.
> 

[David Kilzer (:ddkilzer)]

Comment on attachment 9064 [details]
Latest linux port as a full patch

After lengthy discussions on IRC, bdash (Mark) and I aren't sure whether this patch should have been committed or not with Attachment 9069 [details].  Please render your judgement!

[Mark Rowe (bdash)]

Comment on attachment 9064 [details]
Latest linux port as a full patch

It's unclear if this has been r+'d and can be landed.  Darin, can you please mark it review+ if it's good to go or review- if it still needs work.

[Darin Adler]

Comment on attachment 9064 [details]
Latest linux port as a full patch

There is still a lot here that's not formatted right. Extra spaces in parentheses, missing spaces after if keywords, commas, and around operators, const after type in some files. It doesn't seem too challenging to fix these. If we don't fix it now, I'm not sure when we would.

I'm also wondering about file names with "Gdk" in them -- shouldn't it be all capitals GDK?

We don't normally check in #if 0'd code and commented out code, and there's a lot of both here.

Macro names need to be all capitals.

Comments like this:

+    //XXX create a new one here !

aren't really useful unless other people on the project know what "XXX" means what a "new one" is.

We try to keep all the stubs in a single file, but 
I'm mystified by other loose ends like the stubs and loadResourceIntoArray function which are defined in FrameGdk.cpp -- clearly seems the wrong place for these things.

Now that the constants in KeyboardCodes.h are plain old namespaced const, they should no longer be all capitals -- that's for macros only.

+                                      (GSourceFunc)timeout_cb,

We don't do typecasts on function types because that often leads to trouble. Instead we use function types that exactly match and do typecasting on the types inside the functions -- a lot safer -- I can go into more detail about this if you like. It's something I really learned the importance of while working on Nautilus for the Gnome project.

RenderThemeGdk.cpp has comments in it about Windows Theme API that are not correct or appropriate.

 #include "kjs_html.lut.h"
+#include <math.h>

This include is in the wrong place; should be up with the other includes, not after the "lut.h" include.

On the other hand, it's possible that I should ignore these issues and rubber-stamp this so we can get the GDK port into the tree, regardless of the many small problems in the code.

But I'm not entirely comfortable with that. I'd like to see as many of these issues as possible fixed. I'd feel a lot better about taking this into the tree if it didn't have all the minor problems I mention above.

[Darin Adler]

Comment on attachment 9064 [details]
Latest linux port as a full patch

I guess I'll review+ this even though I have lots of concerns about it. Someone please fix all those things I mentioned some day!

[Michael Emmel]

(In reply to comment #50)
> (From update of attachment 9064 [details] [edit])
> I guess I'll review+ this even though I have lots of concerns about it. Someone
> please fix all those things I mentioned some day!
> 

No Wait !!!

I'm working on it sorry for the slow reply.

I was trudging through your comment :)
Almost ready to send a new patch.
Not everything is fixed but its a lot closer.

I can't believe I'm asking you to not apply the patch :)

Mike

[Michael Emmel]

Created attachment 9111 [details]
Fixed most formatting issues


This patch fixes a lot of the formatting issues Darin had with the code.

[Michael Emmel]

(In reply to comment #49)
> (From update of attachment 9064 [details] [edit])
> There is still a lot here that's not formatted right. Extra spaces in
> parentheses, missing spaces after if keywords, commas, and around operators,
> const after type in some files. It doesn't seem too challenging to fix these.
> If we don't fix it now, I'm not sure when we would.
> 


Does xcode format correctly ?
I'm happy to hack Astyle to get it close to the format rules if
your willing to work with me on it. I prefer command line formatters.
My point is it's a lot better to find a automatic formatter thats close then document what you have to look for by hand then it is to format code by hand.
Right now I'm having to hand format this code which is not a productive use of your time nor mine. I'm willing to load the code into any ide or emacs or run any formatter program on it to get the format to match the rules.
But we should have a way to auto format the code that at least gets close.
I fixed what I could find but I'm a real believer in autoformatting and as I said I'd be happy to figure out what the process should be to remove or lessen the need for hand formatting.

In any case I agree these need to be fixed and I tried to fix as much as 
I could find.




> I'm also wondering about file names with "Gdk" in them -- shouldn't it be all
> capitals GDK?
> 

No the gdk sources use GdkWindow etc for classes. This is right.

> We don't normally check in #if 0'd code and commented out code, and there's a
> lot of both here.
> 
> Macro names need to be all capitals.
> 
> Comments like this:
> 
> +    //XXX create a new one here !
> 
Fixed
> aren't really useful unless other people on the project know what "XXX" means
> what a "new one" is.
> 
> We try to keep all the stubs in a single file, but 
> I'm mystified by other loose ends like the stubs and loadResourceIntoArray
> function which are defined in FrameGdk.cpp -- clearly seems the wrong place for
> these things.
> 
Yes and I expanded the comment the problem is the function was added
and probably should be a entry point into a resource loader but there
is no code for loading none code resources and I have no idea what the  plans
are are we going to have a platform independent resource loader ?
It placement is to invoke questions plus the main external class to override
right now is GdkFrame so this is where you could plugin your own resource loader
not that its the best place.


> Now that the constants in KeyboardCodes.h are plain old namespaced const, they
> should no longer be all capitals -- that's for macros only.

Not a problem but lets change this file one time its a lot of hand editing.
It should not be here either since its platform independent.
My suggestion is leave it alone for now lets review it later for moving up
into platform and fix it once. I'm not sure how you want it named do you 
want  VK_TAB to go to
VK_Tab
KeyCodeTab
TabKC
???
We don't have to keep the VK_
I'm fine with changing it but lets do it once and move it up into platform.



> 
> +                                      (GSourceFunc)timeout_cb,
> 
> We don't do typecasts on function types because that often leads to trouble.
> Instead we use function types that exactly match and do typecasting on the
> types inside the functions -- a lot safer -- I can go into more detail about
> this if you like. It's something I really learned the importance of while
> working on Nautilus for the Gnome project.
> 
Fixed I agree

> RenderThemeGdk.cpp has comments in it about Windows Theme API that are not
> correct or appropriate.
> 
>  #include "kjs_html.lut.h"
> +#include <math.h>
> 
> This include is in the wrong place; should be up with the other includes, not
> after the "lut.h" include.
> 
Fixed

> On the other hand, it's possible that I should ignore these issues and
> rubber-stamp this so we can get the GDK port into the tree, regardless of the
> many small problems in the code.
> 
> But I'm not entirely comfortable with that. I'd like to see as many of these
> issues as possible fixed. I'd feel a lot better about taking this into the tree
> if it didn't have all the minor problems I mention above.
> 

[Michael Emmel]

Hi I can't obsolete the last full patch its not editable.
Anyway I can create a new format fixed patch if needed depending on what goes in.

Mike

[David Kilzer (:ddkilzer)]

(In reply to comment #53)
> Does xcode format correctly ?
> I'm happy to hack Astyle to get it close to the format rules if
> your willing to work with me on it. I prefer command line formatters.

Have you seen this page?

http://webkit.opendarwin.org/coding/coding-style.html

[Michael Emmel]

(In reply to comment #55)
> (In reply to comment #53)
> > Does xcode format correctly ?
> > I'm happy to hack Astyle to get it close to the format rules if
> > your willing to work with me on it. I prefer command line formatters.
> 
> Have you seen this page?
> 
> http://webkit.opendarwin.org/coding/coding-style.html
> 
Yes of course and the format rules are not consisten with anything
I can generate automatically so far.
I certainly cannot replicate them with astyle.
If you can create a style that can be done via automatic processing then
you save a lot of time effort and energy.
If not astyle then what I'll use any tool ?
Having a set of format rules that can't be replicated with a auto formatting tool is not useful.
I'm sorry I have very strong feeling on this issue so maybe I should just keep quite.
I've spent day's dealing with formatting its a huge waste of time.
And down to style I work on a lot of different projects each with there own style rules and all different constantly flipping styles is not easy.

Sorry I'm just flaming but  style rules that don't match up with auto formatters are a big pet peeve of mine.

[Michael Emmel]

I have worked on the problem 

here are my current astyle rules

astyle \
--mode=c \
--convert-tabs \
--indent=spaces=4 \
--brackets=linux \
--indent-switches \
--max-instatement-indent=40 \
--min-conditional-indent=4 \
--one-line=keep-statements \
--one-line=keep-blocks \
--pad=oper \
$@

They don't quite work.

For example

namespace foo {

becomes 

namespace foo
{


But this is actually consistent with the style of

class
{
}

So I'd argue that astyle really does it correctly. But in any case I'm willing to modify astyle to handle this case since its easily detectable.

I can't do 
--pad=paren 
And I can't find a rule that would do if() -> if ()
But agian this should be easy to add.

I'm willing to do the work to modify astyle to support something close to the current style rules.

I think its really important to be able to automatically style code and I'll do the work I need some support.
I posted this to the list and recieved no comments.



Mike

[David Kilzer (:ddkilzer)]

(In reply to comment #57)
> I have worked on the problem 
> here are my current astyle rules

Mike, I agree with you that an automated style solution would be great!  Why don't you open another Bugzilla bug for this feature and document the changes and status there?  You could post your work-in-progress config files and others could contribute changes.

Does astyle work with C, ObjC, C++ and ObjC++?

[Michael Emmel]

(In reply to comment #58)
> (In reply to comment #57)
> > I have worked on the problem 
> > here are my current astyle rules
> 
> Mike, I agree with you that an automated style solution would be great!  Why
> don't you open another Bugzilla bug for this feature and document the changes
> and status there?  You could post your work-in-progress config files and others
> could contribute changes.
> 
> Does astyle work with C, ObjC, C++ and ObjC++?
> 

C and C++ for sure ObjC is not directly supported.
I'll open a bug.

[Darin Adler]

Comment on attachment 9111 [details]
Fixed most formatting issues

I'm working on landing this. I took care of the easy bits first. In WebCore I had to fix a lot of problems. For example, maketokenizer had incorrect Perl syntax and was always generating the Linux version, so this patch breaks the Mac OS X build.

[Darin Adler]

Comment on attachment 9111 [details]
Fixed most formatting issues

Landing a much-revised version of this. Not sure if my modified version even compiles!

[Michael Emmel]

(In reply to comment #61)
> (From update of attachment 9111 [details] [edit])
> Landing a much-revised version of this. Not sure if my modified version even
> compiles!
> 

Cool I'll update now.

Since this is closed I'll start a new bug for the patch.