[Webkit-unassigned] [Bug 8515] Linux porting compile bug

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=8515





------- Comment #6 from mike.emmel at gmail.com  2006-06-06 20:26 PDT -------
(In reply to comment #3)
> (From update of attachment 8741 [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.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.