[webkit-reviews] review granted: [Bug 23295] add Android platform-specific files to WebCore/page : [Attachment 26738] patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Feng Qian <feng at chromium.org>'s
request for review:
Bug 23295: add Android platform-specific files to WebCore/page
https://bugs.webkit.org/show_bug.cgi?id=23295

Attachment 26738: patch v2
https://bugs.webkit.org/attachment.cgi?id=26738&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   Add Android-specific files to the WebCore/page directory.
> +	https://bugs.webkit.org/show_bug.cgi?id=23295

There's a tab in the ChangeLog here. Makes it slightly harder for the committer
to land it.

> +
> +	   * page/android/DragControllerAndroid.cpp: Added.
> +	   (WebCore::DragController::isCopyKeyDown):
> +	   (WebCore::DragController::dragOperation):
> +	   (WebCore::DragController::maxDragImageSize):

Even though prepare-ChangeLog adds all the function names for new files, I
don't think they're all that helpful, especially if you don't comment on them
individually. I suggest removing them from the ChangeLog before putting the
patch up for review.

> +DragOperation DragController::dragOperation(DragData* dragData)
> +{
> +    //FIXME: This logic is incomplete  
> +    ASSERT(0);      

This is a bit weak. If it's not implemented, we normally call notImplemented().
An assertion is not typically how we indicate that code isn't done. We normally
put spaces between the "//" and the "FIXME".

> +#define LOG_TAG "WebCore"
> +
> +#include "config.h"
> +#include "EventHandler.h"

It's highly unusual that we'd define anything before including "config.h". Is
this LOG_TAG thing something Android-specific?

> +// This stub file was created to avoid building and linking in all the
> +// Inspector codebase.

Ideally I'd like someone more familiar with the inspector to comment on this.
Why is this approach specific to Android? Aren't there other platforms that
don't have an inspector? This doesn't seem like something that should be
platform-specific.

r=me, but please consider these comments