Description
Kevin Ollivier
2007-10-24 20:40:00 PDT
Created attachment 16849 [details] wx port patch, made against r27004 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?) (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. Created attachment 16862 [details]
MSVC7 fixes for wxWebKit port
Okay, thanks, I'm breaking things down and am attaching the MSVC fixes first.
(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 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.
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 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
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 on attachment 16917 [details]
wx platform defines for graphics and network layers
Looks fine. r=me
(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. 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.
Created attachment 16922 [details]
wx impl. for dragging and event handler interfaces.
Created attachment 16923 [details]
wx impl. of platform graphics interfaces
Created attachment 16925 [details]
wx. impl and updates for platform interfaces
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 on attachment 16921 [details]
wx impl. for editing interfaces
r=me
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.
(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! :-) 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! 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. :-)
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.
Created attachment 17009 [details]
wx platform impl. patch part #3, coding style only fixes
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.)
Created attachment 17011 [details]
wx platform impl. patch part #5, wx-related fixes
Created attachment 17013 [details]
wx platform impl. patch part #6, temporary link stubs update
Created attachment 17016 [details]
wx graphics impl. #1 - color and pen
Created attachment 17017 [details]
wx graphics impl. #2 - point and rect
Created attachment 17018 [details]
wx graphics impl. #3 - images
Created attachment 17019 [details]
wx graphics impl. #4 - graphics context
Created attachment 17020 [details]
wx graphics impl. #5 - affine transform and path
Comment on attachment 17016 [details]
wx graphics impl. #1 - color and pen
Better to not do an else after each return. r=me
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 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 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 on attachment 17011 [details]
wx platform impl. patch part #5, wx-related fixes
Accidently marked the wrong patch as applied.
Comment on attachment 16927 [details]
wx WebKit API and frontend
Removing the 'big' patch now, will break up later.
Comment on attachment 17013 [details]
wx platform impl. patch part #6, temporary link stubs update
r=me
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.
Created attachment 17055 [details]
wx platform impl. patch part #1, new files (updated)
New version with style issues addressed.
Comment on attachment 17055 [details]
wx platform impl. patch part #1, new files (updated)
r=me
Comment on attachment 17008 [details]
wx platform impl. patch part #2, font-related files
r=me
Comment on attachment 17018 [details]
wx graphics impl. #3 - images
rs=me
Comment on attachment 17019 [details]
wx graphics impl. #4 - graphics context
rs=me
Comment on attachment 17020 [details]
wx graphics impl. #5 - affine transform and path
rs=me
Comment on attachment 17028 [details]
wx platform impl. patch part #5, wx-related fixes (updated)
rs=me
Created attachment 17065 [details]
wxWebKit API files
Created attachment 17067 [details]
wxWebKit impls. of WebKit client APIs
Created attachment 17069 [details]
wxWebKit Python bindings and sample wxPy app.
Created attachment 17071 [details]
sample C++ app for wxWebKit
Created attachment 17073 [details]
JSCore Bakefiles
Created attachment 17074 [details]
WebCore Bakefiles for wx
Created attachment 17075 [details]
WebKit Bakefiles for wx
Created attachment 17076 [details]
Sample app Bakefile and wxWebKit build script
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 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 on attachment 17074 [details]
WebCore Bakefiles for wx
r=me with the same comment about copyright notices as with the previous patch.
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 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 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 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 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.
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 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.
Created attachment 17218 [details]
wxWebKit build script, sample app, and impl of build-webkit --clean
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
(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. Created attachment 17371 [details]
Get wx building with latest tree, and hopefully on the buildbot
Created attachment 17374 [details]
Updated wx build fix patch, now build-webkit returns proper error code
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
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. |