[webkit-reviews] review denied: [Bug 8515] Linux porting compile bug : [Attachment 8781] Linux port files

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 8515: Linux porting compile bug
http://bugzilla.opendarwin.org/show_bug.cgi?id=8515

Attachment 8781: Linux port files
http://bugzilla.opendarwin.org/attachment.cgi?id=8781&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The long delay in reviewing this is largely due to the fact that it's attached
as a tar file rather than a reviewable/applyable text file patch. Newly-created
files work in patches, and the best way to post them for review and check-in is
as a patch. The changes should have a ChangeLog entry and ideally they would
conform much more closely to the style guidelines.

Formatting in these new files does not match the guidelines in many cases. For
example almost all the namespace declarations look like this:

    namespace WebCore
    {

We normally put the brace on the line with the namespace declaration.

In many cases, the * is not next the the type. For example, this:

    GdkDrawable *drawable;

GdkDrawable * Widget::drawable() const
void Widget::setDrawable(GdkDrawable *drawable)

There are a couple constants that are needlessly made class members, which
means they have to go in headers. In TransferJobManager:

    static const int selectTimeoutMS=5;
    static const double pollTimeSeconds;

Putting these outside the class would allow them to be inside the .cpp file and
not require recompilation of other classes if they change.

There's an unused define in TransferJobManager.cpp:

#define SIMPLE_TRANSFER 1

Strange formatting here:

TransferJobManager::TransferJobManager()
	:
	m_downloadTimer(this,&TransferJobManager::downloadTimerCallback)

We don't use that format anywhere else.

void
TransferJobManager::useSimpleTransfer( bool useSimple )

We don't put the type on a separate line or use spaces inside parentheses like
this.

    //job->header((char*)ptr, realsize);

We don't check in commented-out code.

		printf("%s, select timout",__PRETTY_FUNCTION__);

This is a compiler-specific extension -- do you need to use it? And it's a
printf not inside an ifdef.

#if DEBUG

We don't use DEBUG -- we use NDEBUG.

    m_position = IntPoint((int)event->scroll.x,(int)event->scroll.y);

Why are type casts needed here?

#define WIN32_COMPILE_HACK

Why is this inside a GDK-specific file?

static bool
timeout_cb(gpointer data)
{
    (void)data;

We omit parameter names to avoid unused parameter warnings, rather than using
the (void) technique, in C++. And we put the static and the type on the same
line as the function name.

/**
Region of the content currently visible in the viewport
in the content views coordinate system
**/

This isn't a format we use for comments.

#if WIN32
typedef void* HANDLE;
typedef struct HINSTANCE__* HINSTANCE;
typedef HINSTANCE HMODULE;
#endif

#ifdef WIN32
    HMODULE m_themeDLL;
    mutable HANDLE m_buttonTheme;
    mutable HANDLE m_textFieldTheme;
#endif

Why are these WIN32-specific code fragments still in the GDK-specific source
files?

There are lots of different approaches here to errors or missing code. For
example, some code has LOG_ERROR for problems, other code has printf, other
code has commented-out printf, other code calls a function notImplemented(),
other code has a comment with FIXME in it, and other code has a comment without
FIXME. This should be handled in a more consistent way.

The hardcoded list of what to do for each key in FrameGdk::handleGdkEvent
doesn't seem like a good way to do things. We should see if we can make some of
that cross-platform.

		RenderObject * r = node->renderer();
		//Default to scrolling the page
		//not sure why its null
		//broke anyway when its not null
		//if (!r)
		r = renderer();

Dead code like the call to node->renderer() should not be left in like this.

		doScroll(r,wheelEvent.isHorizontal(),wheelEvent.delta());

We use spaces after commas in parameter lists.

    if (!node)
	return false;
    else {

No need for else after return.

To make this code really fit with the rest of WebKit, I'd like to see it more
consistent in style with other new code we're checking in, and more
self-consistent about how it handles things like incomplete sections that need
work.

However, we can be flexible about this if we have to, to get the new platform
started. I'd request a pass of cleanup before we land this for the first time
if that's practical for you.