[Webkit-unassigned] [Bug 11617] Compile and work on Qt/KDE platforms

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=11617





------- Comment #6 from zack at kde.org  2006-11-17 01:46 PDT -------
(In reply to comment #4)
> Please avoid trailing spaces:
> 
> -   ${CMAKE_CURRENT_SOURCE_DIR}/page
> +   ${CMAKE_CURRENT_SOURCE_DIR}/page   

hmm, we have a script that removes trailing whitespace on saving of a file,
unfortunately the code in WebKit is filled with trailing whitespace due to
which we had to disable removing of trailing whitespace. So i'm not sure why
this is an issue. I fixed it, of course, though.

> Don't use tabs in ChangeLog:
> 
> +       http://bugs.webkit.org/show_bug.cgi?id=11617
> 
> +       Adjusting to the new api.

Fixed.

> I don't think this comment makes sense for Qt:
> 
> +    // Double-click events don't exist in Cocoa. Since
> passWidgetMouseDownEventToWidget will
> +    // just pass currentEvent down to the widget, we don't want to call it for
> events that
> +    // don't correspond to Cocoa events.  The mousedown/ups will have already
> been passed on as
> +    // part of the pressed/released handling.

It actually makes a lot of sense. Currently methods are being frequently moved
and renamed due to which I have no idea what's going on with them. If I'm not
sure what a method is doing, I put the comment from the mac implementation in
the stub - this way when methods are moved/renamed I can grep for it and it
actually  works :)

> What's the point of this comment?
> 
> +    // FIXME: this method always returns true
> +    notImplemented();
> +    return false;
> +}

The point of it is that the mac implementation returns always true for reasons
I don't fully understand and once I'll be implementing this it's gonna be worth
to go through platform code and either remove it if it's meant to always return
true or do something with it.

> This should be '#include "Shared.h"'
> 
> +#include <Shared.h>

Fixed.

> Should use braces for one-liners:
> 
> +    if (!m_menu) {
> +        m_menu = new QMenu();
> +    }

Not in this case. In this case I could add something like:
> +    if (!m_menu) {
> +        //FIXME: in this spot right here a code to detect
> +        //       who should own the menu should go. unfortunately
> +        //       i'm not sure what exactly should own it which makes
> +        //       me think that using qlist with a structure representing
> +        //       this class might make more sense
> +        m_menu = new QMenu();
> +    }

Or you could just trust that I actually know what I'm doing ;)

> 
> FIXME style is "// FIXME: Sentence-capitalized comment". I think these comments
> could be phrased better (the second one might not make sense once the first one
> is changed or removed):
> 
> +    //FIXME this method is silly
> +    //FIXME another silly method

This is a reminder to myself that those two methods are, well, silly and
instead of trying to implement them I need to go through platform code and fix
it properly there rather than hack around in platform/qt trying to implement it
(it would be impossible with qmenu).

> Include <WTF/Forward.h> and drop the WTF:: from these:
> 
> +    virtual void
> registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand>);
> +    virtual void
> registerCommandForRedo(WTF::PassRefPtr<WebCore::EditCommand>);

Fixed. 

All in all, I think we'll have to work out a different solution for the compile
fixes by which they're not r-'ed because of trailing whitespace and comments
you don't understand. We need to get compile fixes very quickly, maybe we could
come to some understanding by which either i'm being trusted that i know what
i'm doing with my code or maybe you could take and remove/change whatever you
feel is necessary to follow any kind of style you feel that code should follow.
I just want head compiling so that we can be merging it to the tree in which we
do actual work without getting compilation errors all the time. Ideally the
code that's being committed nightly wouldn't be breaking compilation in Qt but
that's never going to happen so lets try to fix our current way of doing
things. 


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