Bug 23295 - add Android platform-specific files to WebCore/page
Summary: add Android platform-specific files to WebCore/page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Feng Qian
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-13 09:46 PST by Feng Qian
Modified: 2009-04-29 14:23 PDT (History)
0 users

See Also:


Attachments
patch v1 (16.70 KB, patch)
2009-01-13 10:05 PST, Feng Qian
no flags Details | Formatted Diff | Diff
patch v2 (16.18 KB, patch)
2009-01-14 17:14 PST, Feng Qian
no flags Details | Formatted Diff | Diff
patch v3 (13.64 KB, patch)
2009-01-20 12:50 PST, Feng Qian
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Feng Qian 2009-01-13 09:46:44 PST
Part of upstreaming Android port to WebKit, create a placeholder for newly added files. Will ask for review when it is ready.
Comment 1 Feng Qian 2009-01-13 10:05:14 PST
Created attachment 26673 [details]
patch v1

Current files for reference, does not fully confirm to WebKit coding style yet.
Comment 2 Feng Qian 2009-01-14 17:14:27 PST
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.
Comment 3 Darin Adler 2009-01-16 10:34:23 PST
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
Comment 4 Feng Qian 2009-01-20 12:50:47 PST
Created attachment 26868 [details]
patch v3

A new patch for commit with fixes suggested by Darin.
Comment 5 Feng Qian 2009-01-20 12:54:04 PST
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
> 

Comment 6 Brent Fulgham 2009-02-05 11:47:26 PST
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.
Comment 7 Brent Fulgham 2009-02-13 11:43:02 PST
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.
Comment 8 Eric Seidel (no email) 2009-04-29 14:23:38 PDT
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