Bug 11617 – Compile and work on Qt/KDE platforms

• Component: Platform
• Status: RESOLVED FIXED

[Zack Rusin]

Making Qt/KDE codepaths work after recent api changes.

[Zack Rusin]

Created attachment 11538 [details]
patch to fix Qt/KDE code

[Sam Weinig]

Comment on attachment 11538 [details]
patch to fix Qt/KDE code

r- for a few missing things.

- Add the URL for this bug to the Changelog.
- #include "config.h" to ContextMenuClientQt.cpp.
- In ContextMenuClientQt.h, please but the opening curly-brace for the namespace and the class on the same line as the declaration. Same goes for the namespace in ContextMenuQt.cpp.
- No need for the braces around one line if statement in void ContextMenu::appendItem(ContextMenuItem item)

- These last two are really nitpicking, but, in EditorClientQt.cpp, please separate the #include "EditCommand.h" from the first 2 #includes as is our style.  And, at the end of ContextMenuClientQt.cpp you have 2 newlines when only one called for.

[Zack Rusin]

Created attachment 11539 [details]
fixes the style issues

Comment on attachment 11539 [details]
fixes the style issues

r- since there are still a few coding style and comment issues, but other than that the patch looks good.

Please avoid trailing spaces:

-   ${CMAKE_CURRENT_SOURCE_DIR}/page
+   ${CMAKE_CURRENT_SOURCE_DIR}/page   

Don't use tabs in ChangeLog:

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

+	Adjusting to the new api.

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.

What's the point of this comment?

+    // FIXME: this method always returns true
+    notImplemented();
+    return false;
+}

This should be '#include "Shared.h"'

+#include <Shared.h>

Should use braces for one-liners:

+    if (!m_menu) {
+        m_menu = new QMenu();
+    }

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

Include <WTF/Forward.h> and drop the WTF:: from these:

+    virtual void registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand>);
+    virtual void registerCommandForRedo(WTF::PassRefPtr<WebCore::EditCommand>);

(In reply to comment #4)

> Should use braces for one-liners:

Bah. I meant to write "Shouldn't".

[Zack Rusin]

(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. 

[Zack Rusin]

Created attachment 11546 [details]
adds some more compilation fixes

Comment on attachment 11546 [details]
adds some more compilation fixes

r=me

[Nikolas Zimmermann]

Landed in r17827.

(In reply to comment #6)
> 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();
> > +    }

I thought so too, but it turns out that the "no braces around one-liners" rule also applies to a single line of code preceded by a comment (there is such an example in the guidelines).

> 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.

That is already the case. I also have no problem with not r-ing patches that don't conform to the style guidelines.

> We need to get compile fixes very quickly

Conforming to the style guidelines doesn't conflict with getting 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 can't do that.