Bug 10467 – WebKit should have Qt platform support (Part II)

• Component: Platform
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

It would be nice to have Qt support in WebKit :-)

[Nikolas Zimmermann]

Created attachment 10106 [details]
Initial patch

This contains the whole platform/qt stuff. Amazing ChangeLog diff :-)

[Eric Seidel (no email)]

Comment on attachment 10106 [details]
Initial patch

I'm going to leave this one to darin.

[Nikolas Zimmermann]

Created attachment 10122 [details]
Corrected patch

Eric advised me to change the coding style to confirm to the WebKit style guide. This time it should be correct :-)

[Eric Seidel (no email)]

Comment on attachment 10122 [details]
Corrected patch

FontCache probably needs your copyright as well:

--- qt/FontCacheQt.cpp	(Revision 0)
+++ qt/FontCacheQt.cpp	(Revision 0)
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2006 Dirk Mueller <[e-mail]>
+ *

likewise for PlatformTextEdit
and ImageSourceQt

normally webkit doesn't have {} around one line ifs
+    if (length < 4) {
+        return 0;
+    }

Um, these really should be part of the individual decoders:

+    if (strncmp(contents, "GIF8", 4) == 0) {
+        return new GIFImageDecoder();
+    }
+

GiffImageDecoder::isGif(char*)

Seems silly for all the platforms to have to write the same length if checks.

Even better would be an ImageDecoder class method which just new'd the right one. (even though that would be a layering violation)

This seems to be indented "wrong":

+    : m_x(p.x())
+      , m_y(p.y())

the , and : should line up.

No breaks are need here:

+        return QPainter::CompositionMode_Clear;
+        break;

likewise:

+    case ButtCap:
+        return Qt::FlatCap;
+        break;

They just double the length of the source files for no good reason.

This should probably use the normal constructor initializers:

+    TransparencyLayer(const QPainter& p, int width, int height)
+    {
+        pixmap = QPixmap(width, height);
+
+        painter = new QPainter(&pixmap);

: pixmap(width, height), etc

Short one-liners can be written on one line if you choose:

+    bool isNull()
+    {
+        return !x && !y && !blur;
+    }

it's your call there.

I'm not sure you always want antialiasing on:

+    // Good looking SVGs please...
+    painter->setRenderHint(QPainter::Antialiasing);

I feel like I remember webkit turning it on/off at times, but I'm not sure.

return should have it's own line:

+    if (m_data->shadow.isNull()) return;

I've seen this type of naming avoided in WebKit, probably because it can be confused with obj-c private member variables:

+        Pen _pen = pen();

Hum... I can't remember seeing alloca ever used anywhere else in WebKit:

+        float *bmap = (float*)alloca(sizeof(float)*(md+1));

That also seems like a bad idea, as thickness is defined by the user and could cause that to crash.

Generally we use spaces between operators:

n=-thickness
int d = n*n+m*m;
etc.

Personally I find multi-line blocks less confusing if they have an extra {} here:

+        for (int n=-thickness; n<=thickness; n++)
+            for (int m=-thickness; m<=thickness; m++) {
+                int d = n*n+m*m;
+                if (d<=md)
+                    factor += bmap[d];
+            }

even though for () for() doesn't really need the extra outside block for the compiler's sake.

Looks like another crash possibility:
+        float* amap = (float*)alloca(sizeof(float)*(h*w));
+        memset(amap, 0, h*w*(sizeof(float)));

Hum, where is the corresponding save() to this end()?

+void GraphicsContext::endTransparencyLayer()

+    TransparencyLayer layer = m_data->layers.pop();
+    layer.painter->end();

There *has* to be a cleaner way (like using well named local variables?) to simplify this:

+    path.addRect(QRectF(rect.x() + qMax(topLeft.width(), bottomLeft.width()),
+                        rect.y() + qMax(topLeft.height(), topRight.height()),
+                        rect.width() - qMax(topLeft.width(), bottomLeft.width()) - qMax(topRight.width(), bottomRight.width()),
+                        rect.height() - qMax(topLeft.height(), topRight.height()) - qMax(bottomLeft.height(),
+                                                                                         bottomRight.height())));

More missing copyright:
qt/FontPlatformDataQt.cpp

With a const QString&() operator, I wouldn't think this would be necessary:

font.setFamily(toQString(familyName));

These should use the constructor form:

+FontPlatformData::FontPlatformData(const FontPlatformData& other)
+{
+    m_font = other.m_font;
+}

m_font(other.m_font)

more copyright:

qt/FontPlatformData.h

* in the "wrong" place:

+    static Cursors *self();
+    static Cursors *s_self;

spacing:
+    for(unsigned i = 0; i < bufferLength; i++) {

{ takes a separate line in function declarations:
+void setSharedTimerFiredFunction(void (*f)()) {

Hum, generally this either shouldn't be commited, or should be in WebKitTools:

+++ qt/test/testunity.cpp	(Revision 0)

Eventually this will all need to be wired into the KPart/khtml API:

+    Settings* settings = new Settings;
+    settings->setAutoLoadImages(true);
+    settings->setMinFontSize(5);


More copyright:
 qt/PathQt.cpp


Should use constructor initializers:

+void FontData::platformInit()
+{
+    QFontMetrics metrics(m_font.font());
+    m_ascent = metrics.ascent();
+    m_descent = metrics.descent();


Ok, I was only able to stomach about 75% if the patch ;)

My #1 suggestion: Break this up in to pieces.  Even if that means landing "non-functional" pieces of Qt.  An example would be to get all of the fundamental classes like FloatPoint, Size, etc. landed, then come back to more complicated ones like FrameQt.

There are lots of style nits, not all of which need to be resolved to land, but the more like the rest of webkit this code is, the easier it will be for someone to read.

There are also some alloca uses which could result in crashes, which should be fixed.

[Nikolas Zimmermann]

Created attachment 10136 [details]
Remaining bits

Eric already went through a lot of this code. These are the remaining
bits we didn't manage to go through. If anyone already wants to review it... :-)

[Darin Adler]

Comment on attachment 10136 [details]
Remaining bits

I have some questions and comments:

1) Does CanvasQt really below in the platform directory?

If Canvas is specific to Qt, then I don't think it should be in the platform directory. The equivalent to this on Macintosh is in WebKit, not WebCore. And canvas already has a meaning in WebCore (the canvas element).

If we do want to have CanvasQt in WebCore, I think it belongs in bridge rather than platform. The platform directory is for things "below" WebCore, not "above" WebCore.

2) I'd prefer not to have a file named "helper". We went out of our way and worked hard to get rid of "misc" files in WebCore. The functions for converting strings would be fine to put into PlatformString.h itself, I think, and similarly it's fine to have the conversion to QFont in Font.h.

Does Qt really need its own notImplemented macro? Given our experience with bring-up on the original Macintosh platform, we found that having a central "link stubs" location (used for more recent porting efforts) worked way better for us than notImplemented() calls sprinkled all around the code (which is what we did for the original port to OS X). Can I convince you to take this approach?

3) Do we really want a header SharedTimerLinux.h as well as the implementation file? Does more than one file include this header? If not, then maybe this should all be in SharedTimerLinux.cpp. Also, this is named "Linux" but it has <QTimer> in it, so I think it's SharedTimerQt.

[Nikolas Zimmermann]

Hi Darin,

thanks for the reply :-)
 
> 1) Does CanvasQt really below in the platform directory?
> 
> If Canvas is specific to Qt, then I don't think it should be in the platform
> directory. The equivalent to this on Macintosh is in WebKit, not WebCore. And
> canvas already has a meaning in WebCore (the canvas element).
> 
> If we do want to have CanvasQt in WebCore, I think it belongs in bridge rather
> than platform. The platform directory is for things "below" WebCore, not
> "above" WebCore.

I am not sure about that, maybe it's just the name which is misleading.
It's only used from ScrollViewQt, to seperate the Qt specific widget
drawing and event handling. But it's definately not meant for external
usage - so I actually think this belongs into WebCore/platform/qt, but
maybe with a better (non-misleading) name. Suggestions?
 
> 2) I'd prefer not to have a file named "helper". We went out of our way and
> worked hard to get rid of "misc" files in WebCore. The functions for converting
> strings would be fine to put into PlatformString.h itself, I think, and
> similarly it's fine to have the conversion to QFont in Font.h.

Definately a much better approach - already prepared a patch yesterday, and
it got reviewed by Maciej & landed by Rob. Thanks for the suggestion!
> 
> Does Qt really need its own notImplemented macro? Given our experience with
> bring-up on the original Macintosh platform, we found that having a central
> "link stubs" location (used for more recent porting efforts) worked way better
> for us than notImplemented() calls sprinkled all around the code (which is what
> we did for the original port to OS X). Can I convince you to take this
> approach?

I could be convinced to use that for sure, but I actually like it a lot:
* I think if there's an unimplemented function - for instance in WidgetQt -
  I think it's a good idea to keep it in WidgetQt.cpp, for stuff we don't
  have a seperated file TemporaryLinkStubs.cpp is good

* pragmatic reason:
  As our notImplemented() macro prints file & line to stdout it's very
  easy to see in which are this missing function lives, otherwhise I have
  to open TemporaryLinkStubs.cpp everytime to check what it was...

These are my reasons for this behaviour. Do you think it would be okay?
(I moved the notImplemented() macro in the respective files and removed
HelperQt.h completly...)
 
> 3) Do we really want a header SharedTimerLinux.h as well as the implementation
> file? Does more than one file include this header? If not, then maybe this
> should all be in SharedTimerLinux.cpp. Also, this is named "Linux" but it has
> <QTimer> in it, so I think it's SharedTimerQt.

Good one, changed. I think it was based on ideas from the gdk code, they
named it like this IIRC.

Thanks for the good suggestions!

[Nikolas Zimmermann]

Created attachment 10149 [details]
Patch chunk: graphics related stuff

[Nikolas Zimmermann]

Created attachment 10150 [details]
Patch chunk: timer/system time related stuff

[Nikolas Zimmermann]

Created attachment 10151 [details]
Patch chunk: fonts related stuff

[Nikolas Zimmermann]

Created attachment 10152 [details]
Patch chunk: resource loader related stuff

Beware: The libcurl usage is meant temporarily. It should use KIO soon.
(the libcurl part is directly taken from gdk platform code)

[Nikolas Zimmermann]

Created attachment 10154 [details]
Patch chunk: theme/cursor related stuff

[Nikolas Zimmermann]

Created attachment 10155 [details]
Patch chunk: canvas/frame related stuff

Incorporated Darin's suggestion and renamed CanvasQt to ScrollViewCanvasQt

[Nikolas Zimmermann]

Created attachment 10156 [details]
Patch chunk: link stubs

[Maciej Stachowiak]

Comment on attachment 10150 [details]
Patch chunk: timer/system time related stuff

r=me

[Maciej Stachowiak]

Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

r=me

[Eric Seidel (no email)]

Comment on attachment 10156 [details]
Patch chunk: link stubs

Not much to review here.  rs=me.

[Sam Weinig]

Comment on attachment 10150 [details]
Patch chunk: timer/system time related stuff

Landed in r15954, removing the review flag.

[Sam Weinig]

Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

Landed in r15955.  Removing the review flag.

[Sam Weinig]

Comment on attachment 10156 [details]
Patch chunk: link stubs

Landed in r15956.  Removing the review flag.

[Anders Carlsson]

Comment on attachment 10152 [details]
Patch chunk: resource loader related stuff

r=me

[Rob Buis]

Comment on attachment 10152 [details]
Patch chunk: resource loader related stuff

Landed in r15989. Removing the review flag.

[Adam Roben (:aroben)]

Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

Comments:

+class Cursors {

Classes should be declared in .h files, not .cpp files

+public:
+    static Cursors *self();
+    static Cursors *s_self;

Stars should be next to the type, not the identifier (Cursors* self())

+    if (!s_self) {
+        s_self = new Cursors;
+    }

Shouldn't use braces here

+
+// vim: ts=4 sw=4 et

We don't have modelines in our files

+#include <QStyle>
+#include <QWidget>
+#include <QPainter>
+#include <QStyleOptionButton>
+
+#include "config.h"
+
+#include "RenderTheme.h"
+#include "GraphicsContext.h"

Is there a reason config.h is not the first #include? The convention for #includes is to first have config.h, then the .h file specific to this implementation, then any other includes in alphabetical order.

+class RenderThemeQt : public RenderTheme

Again, classes should be declared in .h files (in this case, RenderThemeQt.h)

+    bool stylePainterAndWidgetForPaintInfo(const RenderObject::PaintInfo&, QStyle*&, QPainter*&, QWidget*&) const;

The name of this method is a bit unclear. Perhaps "getStylePainterAndWidgetFromPaintInfo"?

+        option.state |= QStyle::State_ReadOnly; // Readonly is supported on textfields.

Put this comment above the line instead of next to it.

+    // TODO: this is not enough for sure! (use 'option'...)

Might want to use a FIXME instead of TODO, since that's the convention used through most of WebKit

+// vim: ts=4 sw=4 et

Again, no modelines, please.


   Overall, this looks pretty solid. As you can see, the comments above are mostly style-related, so it should be pretty easy to clean it up. The biggest thing is to move those class declarations into .h files.

[Alexey Proskuryakov]

Comment on attachment 10149 [details]
Patch chunk: graphics related stuff

r=me. There are a number of minor formatting/coding style nitpicks still left (discussed on IRC). The only more or less substantial pick is that TransparencyLayer should have a proper destructor, rather than a dangerous cleanup() method.

This all can go in a follow-up patch, of course.

[Nikolas Zimmermann]

(In reply to comment #23) 
>    Overall, this looks pretty solid. As you can see, the comments above are
> mostly style-related, so it should be pretty easy to clean it up. The biggest
> thing is to move those class declarations into .h files.
> 

Thanks a lot, will upload a clean-the-mess-up patch soon!

Niko

[Adam Roben (:aroben)]

Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

Removed review flag because patch is already landed

[Nikolas Zimmermann]

(In reply to comment #25)
> 
> Thanks a lot, will upload a clean-the-mess-up patch soon!

Here lives an initial cleanup patch (only for already commited files):
http://ktown.kde.org/~wildfox/ktown/WebKit-PatchFlood/platform-qt-cleanup.diff

Once the fonts/frame/canvas stuff is in, I'll attach a completly patch
fixing the style, modelines, comments etc in all files in platform/qt.

Hope that pleases everyone,
Niko

[Rob Buis]

Comment on attachment 10149 [details]
Patch chunk: graphics related stuff

Landed in r15994, clearing review flag.

[Anders Carlsson]

Comment on attachment 10155 [details]
Patch chunk: canvas/frame related stuff

Just two comments:

1) There should not be a page object for every frame, all frames on a web page should share the page object.

2) In FrameQt.h, there are two functions at the bottom called "Win". These are used to cast easily between Frame and the Frame subclass (which is FrameQt in this case). This function should be renamed to Qt (or something like that). Win() is used for the windows port and we have Mac() for the mac port.

I've discussed #1 with Nikolas Zimmermann and he will fix this when the Unity KPart is written. 

Regarding #2. I see that the Win() casting function isn't used anywhere (it could be in some places, such as ScrollViewCanvasQt::handleKeyEvent) so you could either remove it or rename it and start using it.

Either way, r=me

[Nikolas Zimmermann]

Created attachment 10195 [details]
Fix build for Qt platform. See ChangeLog for details.

[Nikolas Zimmermann]

Created attachment 10196 [details]
Make Qt kcanvas device build again, after KCanvasMatrix removal.

[Alexey Proskuryakov]

Comment on attachment 10195 [details]
Fix build for Qt platform. See ChangeLog for details.

r=me

[Alexey Proskuryakov]

Comment on attachment 10195 [details]
Fix build for Qt platform. See ChangeLog for details.

Clearing review flag for a committed patch.

[Alexey Proskuryakov]

Comment on attachment 10196 [details]
Make Qt kcanvas device build again, after KCanvasMatrix removal.

r=me, and clearing the review flag to commit.

[Alexey Proskuryakov]

Comment on attachment 10151 [details]
Patch chunk: fonts related stuff

+FontPlatformData* FontCache::createFontPlatformData(const FontDescription& fontDescription, const AtomicString& family)
+{
+    return new FontPlatformData(fontDescription, family);
+}

This leaks; will change to "return 0;" when landing.

r=me

[Nikolas Zimmermann]

Comment on attachment 10155 [details]
Patch chunk: canvas/frame related stuff

Landed by Anders in r16013.

[Nikolas Zimmermann]

Created attachment 10197 [details]
Final style cleanup patch, as promised.

Removes modelines, fixes placing of comments, unneeded braces etc...

[Nikolas Zimmermann]

Created attachment 10198 [details]
Last things required for a working Qt build!

We are really getting there! Thanks again to all the reviewers!

[Nikolas Zimmermann]

Created attachment 10199 [details]
Offer a standalone testing tool for Qt platform.

I hope WebKitTools/QtLauncher is fine, as there's also a GdkLauncher.

[Adam Roben (:aroben)]

Comment on attachment 10197 [details]
Final style cleanup patch, as promised.

This all looks great! Since discussion on modelines is ongoing, I will leave out the modeline deletions when I land this, as they are easier to take out later than to put back in. Thanks for doing this!

[Adam Roben (:aroben)]

Comment on attachment 10197 [details]
Final style cleanup patch, as promised.

Landed as r16014. Clearing review flag.

[Adam Roben (:aroben)]

Comment on attachment 10198 [details]
Last things required for a working Qt build!

This looks good, although I'm going to reformat the ChangeLog entry just slightly. Thanks!

[Adam Roben (:aroben)]

Comment on attachment 10198 [details]
Last things required for a working Qt build!

Landed as r16016. Clearing review flag.

[Eric Seidel (no email)]

Comment on attachment 10199 [details]
Offer a standalone testing tool for Qt platform.

Looks fine.  r=me.

You could come up with a more creative name though. ;)  Although QtLauncher does make it clear that it's for Qt and not any other platform/toolkit.

[Rob Buis]

Comment on attachment 10199 [details]
Offer a standalone testing tool for Qt platform.

Landed in r16022, clearing review flag.