Bug 15682 – Move wx port to TOT

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kevin Ollivier]

I think it's time to finally move wxWebKit over to TOT. I'm submitting the port as one big patch to make it easy to apply and to ensure it doesn't negatively affect other ports, but of course I can rework the patch if someone has a better suggestion. I've made some architectural changes to the wx port also, considering some past discussions and issues and developments on TOT. The primary ones are as follows: 

- I removed Bakefiles directory. Those pieces of the wx build process are now in the WebKit/wx directory.

- The webcore-wx.bkl has moved up to WebCore, removing the need for WebCore/Projects dirs. 

- I've created WebKitTools/wx directory. Currently it only has a sample app, but I prefer to have everything wx-related in a wx subdirectory.

Other than that, a vast majority of this patch is comprised of the necessary wx platform interfaces, a wxWebView class that is the wxWidgets frontend for WebKit, along with the necessarily #if PLATFORM(WX) changes to headers and occasionally sources needed throughout the tree. There are also a few MSVC7 and PLATFORM(WIN_OS) && USE(CURL) build fixes.  

I've tested Windows and Mac builds and both compile and run with this patch. The wx port requires compiling wx and setting up dependencies manually though, which we intend to simplify in the near future, but for the moment we wanted to get this patch in and reviewed so that we can get on trunk and get some tests running while we make the necessary fixes. :-)

Also, thanks to everyone in the project for helping to make all this possible!

[Kevin Ollivier]

Created attachment 16849 [details]
wx port patch, made against r27004

[Kevin Ollivier]

Created attachment 16850 [details]
wx port patch take 2, made against r27004

Sorry, I knew there would be something I overlooked. Bakefile generates JavaScriptCore/makefile.vc, which already exists, so it overwrites it and that made it show up in the diff. Fixed. (Note that I'm not sure JavaScriptCore/makefile.vc has been updated in years or even works for building JavaScriptCore these days... Should it be removed?)

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> I think it's time to finally move wxWebKit over to TOT. I'm submitting the port
> as one big patch to make it easy to apply and to ensure it doesn't negatively
> affect other ports, but of course I can rework the patch if someone has a
> better suggestion. [...]

I think it would be easier to review this patch in smaller "logical" chunks.  No need to create separate Bugzilla bugs for this, just attach them here and clear the review flags as they're landed.

Some random thoughts:

- All the casting changes to cross-platform files required to make the Wx port build could be put in their own patch.
- Any code that otherwise touches cross-platform files should be put in their own patches (grouped by what they fix).
- All changes to Wx-specific files (including new files) could be bundled in their own patch since the only thing being reviewed there is style and (perhaps) architecture.
- The Python bindings code (especially WebKit/wx/bindings/python/webview.cpp) could be it's own patch (it's HUGE--about 1/3 to 1/2 of the current patch :)!
- Should WebKit/wx/.bakefile_gen.state be included in the patch (or be checked into the repository)?  That seems like it may be a build-time Bakefile artifact.

[Kevin Ollivier]

Created attachment 16862 [details]
MSVC7 fixes for wxWebKit port

Okay, thanks, I'm breaking things down and am attaching the MSVC fixes first.

[Kevin Ollivier]

(In reply to comment #3)
> - The Python bindings code (especially WebKit/wx/bindings/python/webview.cpp)
> could be it's own patch (it's HUGE--about 1/3 to 1/2 of the current patch :)!
> - Should WebKit/wx/.bakefile_gen.state be included in the patch (or be checked
> into the repository)?  That seems like it may be a build-time Bakefile
> artifact.

BTW, sorry, both WebKit/wx/bindings/python/webview.cpp and WebKit/wx/.bakefile_gen.state were generated files that I overlooked when making the patch. I've removed them and the overall patch has actually decreased in size by 1/2. :-)

[Eric Seidel (no email)]

Comment on attachment 16862 [details]
MSVC7 fixes for wxWebKit port

please always use static_cast instead of c-style casts.  static_cast is much safer.

I don't understand why your compiler would need you to downcast next.get() and other calls to Node*.  Those changes look totally bogus.

The rest of the change looks OK.  I wonder if there isn't a better place to add the pthread include.

[Kevin Ollivier]

Created attachment 16908 [details]
Definition of types and options for wx platform

As discussed on #WebKit, we are putting aside the MSVC7 fixes until we can figure out a correct fix for the Vector<Node*> issue that avoids using explicit casts. So I'm canceling that patch and moving on to the rest of the changes needed to get wx port on trunk.

This patch comprises all the remaining modifications to files in WebKit TOT. Once this is applied, the remainder will be all new added files for the wx port.

[Adam Roben (:aroben)]

Comment on attachment 16908 [details]
Definition of types and options for wx platform

+class wxDataObject;
+typedef wxDataObject* DragDataRef;

I think you can just say

typedef class wxDataObject* DragDataRef;

+        PlatformMouseEvent(wxMouseEvent&, wxPoint& globalPoint);

I think the parameters should both be const.

r=me

[Kevin Ollivier]

Created attachment 16917 [details]
wx platform defines for graphics and network layers

Thanks, applied with your suggestions. :-) Looks like I overlooked header changes to the network and graphics when creating the patch, so I'm submitting those next.

[Darin Adler]

Comment on attachment 16917 [details]
wx platform defines for graphics and network layers

Looks fine. r=me

[Kevin Ollivier]

(In reply to comment #10)
> (From update of attachment 16917 [details] [edit])
> Looks fine. r=me
> 

Note that it was brought up that __APPLE__ is not the right define for the GraphicsContext.h fix for the wxGCDC typedef (though it will work for the common case). (Also that it would be nice to be consistent about #if and #elif in IntRect/IntPoint.h) I'll resolve these in a future patch and I'm leaving this comment in as a reminder to fix this before closing out this bug. 

[Kevin Ollivier]

Created attachment 16921 [details]
wx impl. for editing interfaces 

Note that WebCore/platform/wx/ClipboardWx.h/cpp are already in Apple trunk, so refer to those if you are wondering about ClipboardWx.

[Kevin Ollivier]

Created attachment 16922 [details]
wx impl. for dragging and event handler interfaces.

[Kevin Ollivier]

Created attachment 16923 [details]
wx impl. of platform graphics interfaces

[Kevin Ollivier]

Created attachment 16925 [details]
wx. impl and updates for platform interfaces

[Kevin Ollivier]

Created attachment 16927 [details]
wx WebKit API and frontend

Sorry, I realize this is a big one but it's really all one piece.

[Adam Roben (:aroben)]

Comment on attachment 16921 [details]
wx impl. for editing interfaces 

r=me

[Adam Roben (:aroben)]

Comment on attachment 16922 [details]
wx impl. for dragging and event handler interfaces. 

+#include "Frame.h"
+#include "Page.h"
+#include "FocusController.h"
+#include "FrameView.h"
+#include "KeyboardEvent.h"
+#include "MouseEventWithHitTestResults.h"
+#include "PlatformScrollBar.h"
+#include "RenderWidget.h"
+#include "ClipboardWx.h"

Please alphabetize the #includes.

+namespace WebCore
+{

The brace should go on the same line as "namespace".

+bool DragController::isCopyKeyDown()
+{
+    return false;
+}

Perhaps you want to call notImplemented() here?

r=me, but please fix the above before checking in.

[Kevin Ollivier]

(In reply to comment #18)
> (From update of attachment 16922 [details] [edit])
> +#include "Frame.h"
> +#include "Page.h"
> +#include "FocusController.h"
> +#include "FrameView.h"
> +#include "KeyboardEvent.h"
> +#include "MouseEventWithHitTestResults.h"
> +#include "PlatformScrollBar.h"
> +#include "RenderWidget.h"
> +#include "ClipboardWx.h"
> 
> Please alphabetize the #includes.
> 
> +namespace WebCore
> +{
> 
> The brace should go on the same line as "namespace".
> 
> +bool DragController::isCopyKeyDown()
> +{
> +    return false;
> +}
> 
> Perhaps you want to call notImplemented() here?
> 
> r=me, but please fix the above before checking in.
> 

Applied with all suggested changes made, thanks! :-) 

[Kevin Ollivier]

Also, a quick note to say two things:

1) I am going to go through and review the remainder of the patches for header include order and consistency with coding guidelines (e.g. the namespace WebCore issue). I won't upload new patches in order to not interrupt any started reviews, but if you just make a note about what coding style issues need to be addressed I'll make sure they're taken care of before commit.

2) I mentioned on #WebKit that there is at least one more dev working on the wx port. A majority of the code that we've touched since this patch was submitted is code in the "wx. impl and updates for platform interfaces" patch, so while I know I didn't post that one first, I'd appreciate if anyone starting a new review could look at that patch first so that we can send diffs back and forth, etc.

Thanks again for all your help!

[Kevin Ollivier]

Created attachment 17007 [details]
wx platform impl. patch part #1, localizations and filesystem.

Updated to adhere to WebKit coding style and in a smaller size for easier review. :-)

[Kevin Ollivier]

Created attachment 17008 [details]
wx platform impl. patch part #2, font-related files

Note for the platform/wx stuff that a number of files are already in trunk, so these are primarily build updates and coding style tweaks.

[Kevin Ollivier]

Created attachment 17009 [details]
wx platform impl. patch part #3, coding style only fixes

[Kevin Ollivier]

Created attachment 17010 [details]
wx platform impl. patch part #4, trunk build fixes

Of course, coding style adjustments made as needed as well. (Applies to all patches.)

[Kevin Ollivier]

Created attachment 17011 [details]
wx platform impl. patch part #5, wx-related fixes

[Kevin Ollivier]

Created attachment 17013 [details]
wx platform impl. patch part #6, temporary link stubs update

[Kevin Ollivier]

Created attachment 17016 [details]
wx graphics impl. #1 - color and pen

[Kevin Ollivier]

Created attachment 17017 [details]
wx graphics impl. #2 - point and rect

[Kevin Ollivier]

Created attachment 17018 [details]
wx graphics impl. #3 - images

[Kevin Ollivier]

Created attachment 17019 [details]
wx graphics impl. #4 - graphics context

[Kevin Ollivier]

Created attachment 17020 [details]
wx graphics impl. #5 - affine transform and path

[Darin Adler]

Comment on attachment 17016 [details]
wx graphics impl. #1 - color and pen

Better to not do an else after each return. r=me

[Mark Rowe (bdash)]

Comment on attachment 17009 [details]
wx platform impl. patch part #3, coding style only fixes

r=me, except for the change in ScreenWx.cpp that moves the include of "Screen.h".  Our coding style guidelines state:

All files must #include the primary header second, just after "config.h". So for example, Node.cpp should include Node.h first, before other files

[Mark Rowe (bdash)]

Comment on attachment 17010 [details]
wx platform impl. patch part #4, trunk build fixes

r=me, with some minor style fixes.

In DragDataWx.cpp the include of DragData.h should be below config.h.  Similarly, the ordering should be fixed in DragImageWx.cpp and PasteboardWx.cpp.

 in Widget::setCursor, the opening brace should be on the same line as the if statement.  The inner if statement should probably be combined with the outer if, and the explicit comparison to zero removed per the style guidelines.

[Mark Rowe (bdash)]

Comment on attachment 17017 [details]
wx graphics impl. #2 - point and rect

r=me.  I think the initialization list for IntRect::IntRect should probably be split across multiple lines like with the other classes.

[Kevin Ollivier]

Comment on attachment 17011 [details]
wx platform impl. patch part #5, wx-related fixes

Accidently marked the wrong patch as applied.

[Kevin Ollivier]

Comment on attachment 16927 [details]
wx WebKit API and frontend

Removing the 'big' patch now, will break up later.

[Mark Rowe (bdash)]

Comment on attachment 17013 [details]
wx platform impl. patch part #6, temporary link stubs update

r=me

[Kevin Ollivier]

Created attachment 17028 [details]
wx platform impl. patch part #5, wx-related fixes (updated)

With style fixes and a PlatformKeyboardEvent update to get it building on TOT.

[Kevin Ollivier]

Created attachment 17055 [details]
wx platform impl. patch part #1, new files (updated)

New version with style issues addressed.

[Maciej Stachowiak]

Comment on attachment 17055 [details]
wx platform impl. patch part #1, new files (updated)

r=me

[Maciej Stachowiak]

Comment on attachment 17008 [details]
wx platform impl. patch part #2, font-related files

r=me

[Maciej Stachowiak]

Comment on attachment 17018 [details]
wx graphics impl. #3 - images

rs=me

[Maciej Stachowiak]

Comment on attachment 17019 [details]
wx graphics impl. #4 - graphics context

rs=me

[Maciej Stachowiak]

Comment on attachment 17020 [details]
wx graphics impl. #5 - affine transform and path

rs=me

[Maciej Stachowiak]

Comment on attachment 17028 [details]
wx platform impl. patch part #5, wx-related fixes (updated)

rs=me

[Kevin Ollivier]

Created attachment 17065 [details]
wxWebKit API files

[Kevin Ollivier]

Created attachment 17067 [details]
wxWebKit impls. of WebKit client APIs

[Kevin Ollivier]

Created attachment 17069 [details]
wxWebKit Python bindings and sample wxPy app.

[Kevin Ollivier]

Created attachment 17071 [details]
sample C++ app for wxWebKit

[Kevin Ollivier]

Created attachment 17073 [details]
JSCore Bakefiles

[Kevin Ollivier]

Created attachment 17074 [details]
WebCore Bakefiles for wx

[Kevin Ollivier]

Created attachment 17075 [details]
WebKit Bakefiles for wx

[Kevin Ollivier]

Created attachment 17076 [details]
Sample app Bakefile and wxWebKit build script

[Mark Rowe (bdash)]

Comment on attachment 17076 [details]
Sample app Bakefile and wxWebKit build script

I think build-wxwebkit needs to be part of build-webkit rather than a separate script.  This would allow us to maintain a common build interface across all the ports.  It will also prevent the tab completion pain that having two scripts prefixed with build-w would bring ;-)

[Mark Rowe (bdash)]

Comment on attachment 17075 [details]
WebKit Bakefiles for wx

r=me

Several of the files have the copyright assigned to Apple Computer, Inc. which does not look correct.  Other also claim to have "No newline at end of file" -- it would be nice to fix that for sake of cleanliness.

[Mark Rowe (bdash)]

Comment on attachment 17074 [details]
WebCore Bakefiles for wx

r=me with the same comment about copyright notices as with the previous patch.

[Mark Rowe (bdash)]

Comment on attachment 17073 [details]
JSCore Bakefiles

r=me, though I think the section of the bakefile related to USE_CONSERVATIVE_GC is obsolete.  That constant does not appear to be used at all in JSCore.  Same comment about copyright notices apply here too.

[Mark Rowe (bdash)]

Comment on attachment 17071 [details]
sample C++ app for wxWebKit

r=me.  The coding style jumps out at me as being "wrong", but as this is example code for wx developers I guess it makes sense to match the wx style here.

[Mark Rowe (bdash)]

Comment on attachment 17069 [details]
wxWebKit Python bindings and sample wxPy app.

r=me.

One slightly confusing point here from the view of this as example code is that the TestPanel constructor calls LoadURL, but wkFrame's then immediately loads a different URL.

[Mark Rowe (bdash)]

Comment on attachment 17067 [details]
wxWebKit impls. of WebKit client APIs

This all looks fine to me.  I threw a bunch of coding style issues out on IRC -- it'd be great if you could fix them when landing.

[Mark Rowe (bdash)]

Comment on attachment 17065 [details]
wxWebKit API files

Several of the classes (wxWebView, wxWebViewDOMElementInfo, and others) have getter methods that should be declared as const.

wxWebView's m_impl member should not be public.

The wxWEBVIEW_STATE_FAILED enum declaration looks to have some whacky indentation.

We typically declare members as private rather than protected.

Several of your macros starting with EVT_WEBVIEW_STATE_CHANGED use C-style casts rather than C++-style.

The wxPageSourceViewFrame constructor assigns to a variable named control which is never used.  The variable declaration seems unnecessary.

Right above the declaration of ID_LOADFILE the { should be on the same line as "enum".

The wxWebFrame constructor should initialize m_checkBeforeLoad in it's initialization list.

I'm not sure that the API should create a status bar with the text "wxWebKit works" in it :)

Many of the wxWebFrame methods have {} placement issues -- too many, and on the wrong lines.

wxWebFrame::OnAbout has some extra whitespace inside function calls.

wxWebFrame::OnStop has weird indentation.

The message boxes in wxWebFrame::OnMakeTextLarger don't seem desirable for an API.

wxWebViewDOMElementInfo and wxWebView should also use the initializer list instead of assignments.

wxWebView::~wxWebView has a printf which should be removed.

wxWebView::m_impl may be better suited as an OwnPtr to avoid the need to explicitly delete it in the destructor.

wxWebView::SetPageSource has some C-style casts.

The code in wxWebView::LoadURL checks whether a file exists on disk then uses "ftp" for the protocol in that instance.  That looks incorrect.

wxWebView::OnKeyEvents has some more C-style casts.

Because of the issues above I'm going to say r- for now.  Many were coding style issues, but it'd be good for an updated version of the patch to get another look from someone else too.

[Kevin Ollivier]

Created attachment 17136 [details]
wxWebKit API files (updated)

Addresses all the issues raised by Mark except for the switch to OwnPtr, as we've been trying to avoid using WebKit specific elements in public header API. This may change in the future, but the current code does delete the object fine and so I think we can revisit this later. Many thanks to Mark for the detailed review, he caught a lot of things I overlooked, particularly that strange 'ftp' reference in there. 

Though, while I saw the reasoning for removing it, I kinda liked that the browser reported that wxWebKit works... :)

[Mark Rowe (bdash)]

Comment on attachment 17136 [details]
wxWebKit API files (updated)

r=me.  

Your ChangeLog entry appears to have lost it's descriptive text, be sure to add something meaningful back in there.  I just noticed that the ChangeLog is not in the wx subdirectory.  It probably makes sense to create a new ChangeLog inside WebKit/wx and have the wxWebKit layer info in there.

I mentioned few more changes to be made on IRC before landing, and we'll do some more coding style cleanup after this hits Subversion.

[Kevin Ollivier]

Created attachment 17218 [details]
wxWebKit build script, sample app, and impl of build-webkit --clean

[Darin Adler]

Comment on attachment 17218 [details]
wxWebKit build script, sample app, and impl of build-webkit --clean

+if ($clean) {
+    exit 0;
+}

Things like this read better in perl as:

    exit if $clean;

I'm really disappointed to see build-wxwebkit as an entirely separate script with so much code in it!

r=me

[Kevin Ollivier]

(In reply to comment #66)
> (From update of attachment 17218 [details] [edit])
> +if ($clean) {
> +    exit 0;
> +}
> 
> Things like this read better in perl as:
> 
>     exit if $clean;
> 
> I'm really disappointed to see build-wxwebkit as an entirely separate script
> with so much code in it!
> 
> r=me
> 

Well, we'll try to integrate it as we move forward. At the time most of it was written, there weren't many other ports and those that did were not integrated with build-webkit yet, plus the wxWebKit build system does things differently from other ports by performing pre-build steps before calling make. So what I'd like to do is start looking at these issues as I have the spare time to address them.  

[Kevin Ollivier]

Created attachment 17371 [details]
Get wx building with latest tree, and hopefully on the buildbot

[Kevin Ollivier]

Created attachment 17374 [details]
Updated wx build fix patch, now build-webkit returns proper error code

[Adam Roben (:aroben)]

Comment on attachment 17374 [details]
Updated wx build fix patch, now build-webkit returns proper error code

+    exit $? >> 8;

There's an exitStatus() function in webkitdirs that you could use here.

r=me

[Mark Rowe (bdash)]

I went ahead and landed this last patch to get the wx buildbot up and running.  Sooner it's up and running, the sooner you can have assistance in keeping it that way.  r27899.

I think we should go ahead and close this bug now.  Any further issues that come up can be addressed by new specific bug reports.