[webkit-reviews] review denied: [Bug 15682] Move wx port to TOT : [Attachment 17065] wxWebKit API files

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 8 15:05:52 PST 2007


Mark Rowe (bdash) <mrowe at apple.com> has denied Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 15682: Move wx port to TOT
http://bugs.webkit.org/show_bug.cgi?id=15682

Attachment 17065: wxWebKit API files
http://bugs.webkit.org/attachment.cgi?id=17065&action=edit

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
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.


More information about the webkit-reviews mailing list