Bug 23295 – add Android platform-specific files to WebCore/page

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Feng Qian]

Part of upstreaming Android port to WebKit, create a placeholder for newly added files. Will ask for review when it is ready.

[Feng Qian]

Created attachment 26673 [details]
patch v1

Current files for reference, does not fully confirm to WebKit coding style yet.

[Feng Qian]

Created attachment 26738 [details]
patch v2

Adding Android-specific files under WebCore/page/android. InspectorControllerAndroid.cpp is an empty stub of InspectorController. I feel there can be an empty InspectorController in WebCore for builds that does not support InspectorController.

[Darin Adler]

Comment on attachment 26738 [details]
patch v2

> +        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

[Feng Qian]

Created attachment 26868 [details]
patch v3

A new patch for commit with fixes suggested by Darin.

[Feng Qian]

Thanks. See reply below.

(In reply to comment #3)
> (From update of attachment 26738 [details] [review])
> > +        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.
> 

Fixed

> > +
> > +        * 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.
> 

Fixed.

> > +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".

Changed to notImplemented(); and fixed space between // and 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?

LOG_TAG is Android-specific flag. I leave it here for now, it is a common pattern used in Android files. Cary may know better if we can move it below "config.h"

> 
> > +// 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.

That's what I was wondering too. If there is a common solution for builds without inspector, Android should take it rather than creating this empty link stub.

> 
> r=me, but please consider these comments
> 

[Brent Fulgham]

Submitter updated the patch but didn't mark it for review.  Adding review request so this issue doesn't show up in the commit queue.

[Brent Fulgham]

Comment on attachment 26738 [details]
patch v2

Although this was r+, the later revision has not been reviewed and is causing this to show up in the list of pending commits.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/page/android/DragControllerAndroid.cpp
	A	WebCore/page/android/EventHandlerAndroid.cpp
	A	WebCore/page/android/InspectorControllerAndroid.cpp
Committed r43011