Bug 10467 - WebKit should have Qt platform support (Part II)
Summary: WebKit should have Qt platform support (Part II)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: Other OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-08-17 13:08 PDT by Nikolas Zimmermann
Modified: 2006-08-24 15:11 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (255.46 KB, patch)
2006-08-17 13:22 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Corrected patch (256.27 KB, patch)
2006-08-18 04:25 PDT, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Remaining bits (142.24 KB, patch)
2006-08-18 16:27 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: graphics related stuff (36.57 KB, patch)
2006-08-21 12:37 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: timer/system time related stuff (7.36 KB, patch)
2006-08-21 12:38 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: fonts related stuff (18.20 KB, patch)
2006-08-21 12:53 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: resource loader related stuff (17.57 KB, patch)
2006-08-21 12:54 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: theme/cursor related stuff (17.07 KB, patch)
2006-08-21 13:26 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: canvas/frame related stuff (32.51 KB, patch)
2006-08-21 13:47 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk: link stubs (14.47 KB, patch)
2006-08-21 13:54 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Fix build for Qt platform. See ChangeLog for details. (1.87 KB, patch)
2006-08-24 07:48 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Make Qt kcanvas device build again, after KCanvasMatrix removal. (5.32 KB, patch)
2006-08-24 07:49 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final style cleanup patch, as promised. (45.18 KB, patch)
2006-08-24 10:04 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Last things required for a working Qt build! (6.31 KB, patch)
2006-08-24 10:17 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Offer a standalone testing tool for Qt platform. (5.13 KB, patch)
2006-08-24 10:20 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2006-08-17 13:08:11 PDT
It would be nice to have Qt support in WebKit :-)
Comment 1 Nikolas Zimmermann 2006-08-17 13:22:36 PDT
Created attachment 10106 [details]
Initial patch

This contains the whole platform/qt stuff. Amazing ChangeLog diff :-)
Comment 2 Eric Seidel (no email) 2006-08-18 03:17:10 PDT
Comment on attachment 10106 [details]
Initial patch

I'm going to leave this one to darin.
Comment 3 Nikolas Zimmermann 2006-08-18 04:25:18 PDT
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 :-)
Comment 4 Eric Seidel (no email) 2006-08-18 14:26:33 PDT
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 <mueller@kde.org>
+ *

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.
Comment 5 Nikolas Zimmermann 2006-08-18 16:27:00 PDT
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... :-)
Comment 6 Darin Adler 2006-08-18 21:09:02 PDT
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.
Comment 7 Nikolas Zimmermann 2006-08-21 10:11:15 PDT
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!
Comment 8 Nikolas Zimmermann 2006-08-21 12:37:28 PDT
Created attachment 10149 [details]
Patch chunk: graphics related stuff
Comment 9 Nikolas Zimmermann 2006-08-21 12:38:21 PDT
Created attachment 10150 [details]
Patch chunk: timer/system time related stuff
Comment 10 Nikolas Zimmermann 2006-08-21 12:53:11 PDT
Created attachment 10151 [details]
Patch chunk: fonts related stuff
Comment 11 Nikolas Zimmermann 2006-08-21 12:54:31 PDT
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)
Comment 12 Nikolas Zimmermann 2006-08-21 13:26:36 PDT
Created attachment 10154 [details]
Patch chunk: theme/cursor related stuff
Comment 13 Nikolas Zimmermann 2006-08-21 13:47:17 PDT
Created attachment 10155 [details]
Patch chunk: canvas/frame related stuff

Incorporated Darin's suggestion and renamed CanvasQt to ScrollViewCanvasQt
Comment 14 Nikolas Zimmermann 2006-08-21 13:54:05 PDT
Created attachment 10156 [details]
Patch chunk: link stubs
Comment 15 Maciej Stachowiak 2006-08-21 15:10:07 PDT
Comment on attachment 10150 [details]
Patch chunk: timer/system time related stuff

r=me
Comment 16 Maciej Stachowiak 2006-08-21 15:36:26 PDT
Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

r=me
Comment 17 Eric Seidel (no email) 2006-08-21 16:07:27 PDT
Comment on attachment 10156 [details]
Patch chunk: link stubs

Not much to review here.  rs=me.
Comment 18 Sam Weinig 2006-08-21 16:11:07 PDT
Comment on attachment 10150 [details]
Patch chunk: timer/system time related stuff

Landed in r15954, removing the review flag.
Comment 19 Sam Weinig 2006-08-21 16:11:48 PDT
Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

Landed in r15955.  Removing the review flag.
Comment 20 Sam Weinig 2006-08-21 16:20:32 PDT
Comment on attachment 10156 [details]
Patch chunk: link stubs

Landed in r15956.  Removing the review flag.
Comment 21 Anders Carlsson 2006-08-23 03:33:42 PDT
Comment on attachment 10152 [details]
Patch chunk: resource loader related stuff

r=me
Comment 22 Rob Buis 2006-08-23 09:20:12 PDT
Comment on attachment 10152 [details]
Patch chunk: resource loader related stuff

Landed in r15989. Removing the review flag.
Comment 23 Adam Roben (:aroben) 2006-08-23 13:24:42 PDT
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.
Comment 24 Alexey Proskuryakov 2006-08-23 13:30:38 PDT
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.
Comment 25 Nikolas Zimmermann 2006-08-23 13:30:57 PDT
(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
Comment 26 Adam Roben (:aroben) 2006-08-23 14:25:42 PDT
Comment on attachment 10154 [details]
Patch chunk: theme/cursor related stuff

Removed review flag because patch is already landed
Comment 27 Nikolas Zimmermann 2006-08-23 14:57:52 PDT
(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
Comment 28 Rob Buis 2006-08-24 02:49:01 PDT
Comment on attachment 10149 [details]
Patch chunk: graphics related stuff

Landed in r15994, clearing review flag.
Comment 29 Anders Carlsson 2006-08-24 06:48:22 PDT
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
Comment 30 Nikolas Zimmermann 2006-08-24 07:48:44 PDT
Created attachment 10195 [details]
Fix build for Qt platform. See ChangeLog for details.
Comment 31 Nikolas Zimmermann 2006-08-24 07:49:27 PDT
Created attachment 10196 [details]
Make Qt kcanvas device build again, after KCanvasMatrix removal.
Comment 32 Alexey Proskuryakov 2006-08-24 09:04:43 PDT
Comment on attachment 10195 [details]
Fix build for Qt platform. See ChangeLog for details.

r=me
Comment 33 Alexey Proskuryakov 2006-08-24 09:12:46 PDT
Comment on attachment 10195 [details]
Fix build for Qt platform. See ChangeLog for details.

Clearing review flag for a committed patch.
Comment 34 Alexey Proskuryakov 2006-08-24 09:16:13 PDT
Comment on attachment 10196 [details]
Make Qt kcanvas device build again, after KCanvasMatrix removal.

r=me, and clearing the review flag to commit.
Comment 35 Alexey Proskuryakov 2006-08-24 09:37:58 PDT
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
Comment 36 Nikolas Zimmermann 2006-08-24 09:58:23 PDT
Comment on attachment 10155 [details]
Patch chunk: canvas/frame related stuff

Landed by Anders in r16013.
Comment 37 Nikolas Zimmermann 2006-08-24 10:04:55 PDT
Created attachment 10197 [details]
Final style cleanup patch, as promised.

Removes modelines, fixes placing of comments, unneeded braces etc...
Comment 38 Nikolas Zimmermann 2006-08-24 10:17:46 PDT
Created attachment 10198 [details]
Last things required for a working Qt build!

We are really getting there! Thanks again to all the reviewers!
Comment 39 Nikolas Zimmermann 2006-08-24 10:20:13 PDT
Created attachment 10199 [details]
Offer a standalone testing tool for Qt platform.

I hope WebKitTools/QtLauncher is fine, as there's also a GdkLauncher.
Comment 40 Adam Roben (:aroben) 2006-08-24 10:36:29 PDT
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!
Comment 41 Adam Roben (:aroben) 2006-08-24 11:30:30 PDT
Comment on attachment 10197 [details]
Final style cleanup patch, as promised.

Landed as r16014. Clearing review flag.
Comment 42 Adam Roben (:aroben) 2006-08-24 11:37:57 PDT
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!
Comment 43 Adam Roben (:aroben) 2006-08-24 11:44:31 PDT
Comment on attachment 10198 [details]
Last things required for a working Qt build!

Landed as r16016. Clearing review flag.
Comment 44 Eric Seidel (no email) 2006-08-24 12:19:49 PDT
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.
Comment 45 Rob Buis 2006-08-24 13:17:47 PDT
Comment on attachment 10199 [details]
Offer a standalone testing tool for Qt platform.

Landed in r16022, clearing review flag.