Bug 15682 - Move wx port to TOT
Summary: Move wx port to TOT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 15729
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-24 20:40 PDT by Kevin Ollivier
Modified: 2007-12-07 09:21 PST (History)
3 users (show)

See Also:


Attachments
wx port patch, made against r27004 (699.64 KB, patch)
2007-10-24 20:42 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx port patch take 2, made against r27004 (678.84 KB, patch)
2007-10-24 20:48 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
MSVC7 fixes for wxWebKit port (12.58 KB, patch)
2007-10-25 12:21 PDT, Kevin Ollivier
eric: review-
Details | Formatted Diff | Diff
Definition of types and options for wx platform (16.40 KB, patch)
2007-10-27 23:32 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform defines for graphics and network layers (8.94 KB, patch)
2007-10-28 13:16 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx impl. for editing interfaces (2.45 KB, patch)
2007-10-28 15:51 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx impl. for dragging and event handler interfaces. (7.54 KB, patch)
2007-10-28 15:58 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx impl. of platform graphics interfaces (48.77 KB, patch)
2007-10-28 16:32 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx. impl and updates for platform interfaces (39.72 KB, patch)
2007-10-28 16:52 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx WebKit API and frontend (118.27 KB, patch)
2007-10-28 17:54 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #1, localizations and filesystem. (9.59 KB, patch)
2007-11-02 21:25 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #2, font-related files (3.81 KB, patch)
2007-11-02 21:40 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #3, coding style only fixes (1.96 KB, patch)
2007-11-02 22:06 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #4, trunk build fixes (5.59 KB, patch)
2007-11-02 22:10 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #5, wx-related fixes (9.23 KB, patch)
2007-11-02 22:21 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #6, temporary link stubs update (13.81 KB, patch)
2007-11-02 23:58 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx graphics impl. #1 - color and pen (5.27 KB, patch)
2007-11-03 00:50 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx graphics impl. #2 - point and rect (6.65 KB, patch)
2007-11-03 00:51 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx graphics impl. #3 - images (15.79 KB, patch)
2007-11-03 00:52 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx graphics impl. #4 - graphics context (13.79 KB, patch)
2007-11-03 00:53 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx graphics impl. #5 - affine transform and path (9.14 KB, patch)
2007-11-03 00:53 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #5, wx-related fixes (updated) (11.40 KB, patch)
2007-11-03 22:50 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wx platform impl. patch part #1, new files (updated) (9.59 KB, patch)
2007-11-05 21:11 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wxWebKit API files (44.16 KB, patch)
2007-11-06 16:07 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wxWebKit impls. of WebKit client APIs (74.13 KB, patch)
2007-11-06 16:33 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wxWebKit Python bindings and sample wxPy app. (9.26 KB, patch)
2007-11-06 16:44 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
sample C++ app for wxWebKit (2.69 KB, patch)
2007-11-06 17:04 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
JSCore Bakefiles (9.71 KB, patch)
2007-11-06 17:55 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
WebCore Bakefiles for wx (39.75 KB, patch)
2007-11-06 17:56 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
WebKit Bakefiles for wx (24.23 KB, patch)
2007-11-06 17:56 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Sample app Bakefile and wxWebKit build script (15.67 KB, patch)
2007-11-06 17:57 PST, Kevin Ollivier
mrowe: review-
Details | Formatted Diff | Diff
wxWebKit API files (updated) (44.01 KB, patch)
2007-11-08 17:59 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
wxWebKit build script, sample app, and impl of build-webkit --clean (23.44 KB, patch)
2007-11-12 18:52 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Get wx building with latest tree, and hopefully on the buildbot (13.88 KB, patch)
2007-11-18 15:46 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Updated wx build fix patch, now build-webkit returns proper error code (14.02 KB, patch)
2007-11-18 21:49 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ollivier 2007-10-24 20:40:00 PDT
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!
Comment 1 Kevin Ollivier 2007-10-24 20:42:09 PDT
Created attachment 16849 [details]
wx port patch, made against r27004
Comment 2 Kevin Ollivier 2007-10-24 20:48:54 PDT
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?)
Comment 3 David Kilzer (:ddkilzer) 2007-10-25 08:42:51 PDT
(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.

Comment 4 Kevin Ollivier 2007-10-25 12:21:42 PDT
Created attachment 16862 [details]
MSVC7 fixes for wxWebKit port

Okay, thanks, I'm breaking things down and am attaching the MSVC fixes first.
Comment 5 Kevin Ollivier 2007-10-25 12:31:21 PDT
(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. :-)
Comment 6 Eric Seidel (no email) 2007-10-27 12:39:05 PDT
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.
Comment 7 Kevin Ollivier 2007-10-27 23:32:57 PDT
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.
Comment 8 Adam Roben (:aroben) 2007-10-27 23:51:29 PDT
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
Comment 9 Kevin Ollivier 2007-10-28 13:16:29 PDT
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.
Comment 10 Darin Adler 2007-10-28 14:19:25 PDT
Comment on attachment 16917 [details]
wx platform defines for graphics and network layers

Looks fine. r=me
Comment 11 Kevin Ollivier 2007-10-28 15:49:06 PDT
(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. 
Comment 12 Kevin Ollivier 2007-10-28 15:51:07 PDT
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.
Comment 13 Kevin Ollivier 2007-10-28 15:58:22 PDT
Created attachment 16922 [details]
wx impl. for dragging and event handler interfaces.
Comment 14 Kevin Ollivier 2007-10-28 16:32:46 PDT
Created attachment 16923 [details]
wx impl. of platform graphics interfaces
Comment 15 Kevin Ollivier 2007-10-28 16:52:59 PDT
Created attachment 16925 [details]
wx. impl and updates for platform interfaces
Comment 16 Kevin Ollivier 2007-10-28 17:54:19 PDT
Created attachment 16927 [details]
wx WebKit API and frontend

Sorry, I realize this is a big one but it's really all one piece.
Comment 17 Adam Roben (:aroben) 2007-11-01 13:44:18 PDT
Comment on attachment 16921 [details]
wx impl. for editing interfaces 

r=me
Comment 18 Adam Roben (:aroben) 2007-11-01 13:46:35 PDT
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.
Comment 19 Kevin Ollivier 2007-11-01 14:56:27 PDT
(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! :-) 
Comment 20 Kevin Ollivier 2007-11-02 14:05:27 PDT
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!
Comment 21 Kevin Ollivier 2007-11-02 21:25:11 PDT
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. :-)
Comment 22 Kevin Ollivier 2007-11-02 21:40:42 PDT
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.
Comment 23 Kevin Ollivier 2007-11-02 22:06:01 PDT
Created attachment 17009 [details]
wx platform impl. patch part #3, coding style only fixes
Comment 24 Kevin Ollivier 2007-11-02 22:10:57 PDT
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.)
Comment 25 Kevin Ollivier 2007-11-02 22:21:01 PDT
Created attachment 17011 [details]
wx platform impl. patch part #5, wx-related fixes
Comment 26 Kevin Ollivier 2007-11-02 23:58:07 PDT
Created attachment 17013 [details]
wx platform impl. patch part #6, temporary link stubs update
Comment 27 Kevin Ollivier 2007-11-03 00:50:30 PDT
Created attachment 17016 [details]
wx graphics impl. #1 - color and pen
Comment 28 Kevin Ollivier 2007-11-03 00:51:40 PDT
Created attachment 17017 [details]
wx graphics impl. #2 - point and rect
Comment 29 Kevin Ollivier 2007-11-03 00:52:20 PDT
Created attachment 17018 [details]
wx graphics impl. #3 - images
Comment 30 Kevin Ollivier 2007-11-03 00:53:00 PDT
Created attachment 17019 [details]
wx graphics impl. #4 - graphics context
Comment 31 Kevin Ollivier 2007-11-03 00:53:47 PDT
Created attachment 17020 [details]
wx graphics impl. #5 - affine transform and path
Comment 32 Darin Adler 2007-11-03 01:35:12 PDT
Comment on attachment 17016 [details]
wx graphics impl. #1 - color and pen

Better to not do an else after each return. r=me
Comment 33 Mark Rowe (bdash) 2007-11-03 03:28:21 PDT
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
Comment 34 Mark Rowe (bdash) 2007-11-03 08:15:18 PDT
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.
Comment 35 Mark Rowe (bdash) 2007-11-03 08:19:58 PDT
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.
Comment 36 Kevin Ollivier 2007-11-03 08:48:27 PDT
Comment on attachment 17011 [details]
wx platform impl. patch part #5, wx-related fixes

Accidently marked the wrong patch as applied.
Comment 37 Kevin Ollivier 2007-11-03 19:20:16 PDT
Comment on attachment 16927 [details]
wx WebKit API and frontend

Removing the 'big' patch now, will break up later.
Comment 38 Mark Rowe (bdash) 2007-11-03 21:43:54 PDT
Comment on attachment 17013 [details]
wx platform impl. patch part #6, temporary link stubs update

r=me
Comment 39 Kevin Ollivier 2007-11-03 22:50:54 PDT
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.
Comment 40 Kevin Ollivier 2007-11-05 21:11:47 PST
Created attachment 17055 [details]
wx platform impl. patch part #1, new files (updated)

New version with style issues addressed.
Comment 41 Maciej Stachowiak 2007-11-05 21:20:51 PST
Comment on attachment 17055 [details]
wx platform impl. patch part #1, new files (updated)

r=me
Comment 42 Maciej Stachowiak 2007-11-05 21:22:15 PST
Comment on attachment 17008 [details]
wx platform impl. patch part #2, font-related files

r=me
Comment 43 Maciej Stachowiak 2007-11-05 21:23:01 PST
Comment on attachment 17018 [details]
wx graphics impl. #3 - images

rs=me
Comment 44 Maciej Stachowiak 2007-11-05 21:23:47 PST
Comment on attachment 17019 [details]
wx graphics impl. #4 - graphics context

rs=me
Comment 45 Maciej Stachowiak 2007-11-05 21:24:15 PST
Comment on attachment 17020 [details]
wx graphics impl. #5 - affine transform and path

rs=me
Comment 46 Maciej Stachowiak 2007-11-05 21:24:34 PST
Comment on attachment 17028 [details]
wx platform impl. patch part #5, wx-related fixes (updated)

rs=me
Comment 47 Kevin Ollivier 2007-11-06 16:07:48 PST
Created attachment 17065 [details]
wxWebKit API files
Comment 48 Kevin Ollivier 2007-11-06 16:33:56 PST
Created attachment 17067 [details]
wxWebKit impls. of WebKit client APIs
Comment 49 Kevin Ollivier 2007-11-06 16:44:16 PST
Created attachment 17069 [details]
wxWebKit Python bindings and sample wxPy app.
Comment 50 Kevin Ollivier 2007-11-06 17:04:46 PST
Created attachment 17071 [details]
sample C++ app for wxWebKit
Comment 51 Kevin Ollivier 2007-11-06 17:55:47 PST
Created attachment 17073 [details]
JSCore Bakefiles
Comment 52 Kevin Ollivier 2007-11-06 17:56:14 PST
Created attachment 17074 [details]
WebCore Bakefiles for wx
Comment 53 Kevin Ollivier 2007-11-06 17:56:38 PST
Created attachment 17075 [details]
WebKit Bakefiles for wx
Comment 54 Kevin Ollivier 2007-11-06 17:57:12 PST
Created attachment 17076 [details]
Sample app Bakefile and wxWebKit build script
Comment 55 Mark Rowe (bdash) 2007-11-08 13:41:07 PST
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 ;-)
Comment 56 Mark Rowe (bdash) 2007-11-08 13:42:52 PST
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.
Comment 57 Mark Rowe (bdash) 2007-11-08 13:44:13 PST
Comment on attachment 17074 [details]
WebCore Bakefiles for wx

r=me with the same comment about copyright notices as with the previous patch.
Comment 58 Mark Rowe (bdash) 2007-11-08 13:46:54 PST
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.
Comment 59 Mark Rowe (bdash) 2007-11-08 13:54:51 PST
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.
Comment 60 Mark Rowe (bdash) 2007-11-08 14:02:29 PST
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.
Comment 61 Mark Rowe (bdash) 2007-11-08 14:21:59 PST
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.
Comment 62 Mark Rowe (bdash) 2007-11-08 15:05:52 PST
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.
Comment 63 Kevin Ollivier 2007-11-08 17:59:55 PST
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... :)
Comment 64 Mark Rowe (bdash) 2007-11-08 20:11:38 PST
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.
Comment 65 Kevin Ollivier 2007-11-12 18:52:40 PST
Created attachment 17218 [details]
wxWebKit build script, sample app, and impl of build-webkit --clean
Comment 66 Darin Adler 2007-11-16 20:20:45 PST
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
Comment 67 Kevin Ollivier 2007-11-18 14:40:40 PST
(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.  
Comment 68 Kevin Ollivier 2007-11-18 15:46:00 PST
Created attachment 17371 [details]
Get wx building with latest tree, and hopefully on the buildbot
Comment 69 Kevin Ollivier 2007-11-18 21:49:01 PST
Created attachment 17374 [details]
Updated wx build fix patch, now build-webkit returns proper error code
Comment 70 Adam Roben (:aroben) 2007-11-19 00:32:52 PST
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
Comment 71 Mark Rowe (bdash) 2007-11-19 04:37:57 PST
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.