[Webkit-unassigned] [Bug 23118] add Android platform-specific files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23118


mrowe at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26564|                            |review-
               Flag|                            |




------- Comment #10 from mrowe at apple.com  2009-01-09 14:33 PDT -------
(From update of attachment 26564)
It's great to see Android code making its way in to the WebKit tree!  It'll be
exciting when enough of it is in that we're able to add an Android build bot to
ensure that it all keeps working.

I've started looking over the patch, but due to the sheer size of it its hard
to get through it in a single sitting.  As this is entirely platform-specific
code, our policy is to not be too picky about minor style issues when getting
the code landed and to encourage that they be addressed going forwards.  There
are a number of these sorts of issues, many of which Oliver pointed out, which
should be kept in mind for future changes.

A few things jump out at me:

All of this code is inside the WebKit directory, yet a lot of the classes are
declared and implemented in the WebCore namespace. In some cases this appears
to be due to the implementation living in the wrong place:  
PluginPackageAndroid, PluginDataAndroid and PluginViewAndroid should all live
in WebCore/plugins alongside the implementations for other platforms.  In other
cases, such as the various FooClient implementations, they should not be in the
WebCore namespace at all, but probably in the android namespace.  Finally,
there are a few cases where methods on WebCore::Frame appear to be implemented
in WebKit.  They should be in WebCore.

The sample Android plugin may make more sense under the top level
WebKitExamplePlugins directory rather than inside the WebKit implementation.

I'm not comfortable with including a copy of the STL in the WebKit source in
this fashion.  My understanding is that WebKit is the only project on Android
that uses the STL, so I can sort of see why it made sense for Android to put
the STL inside WebKit, but it doesn't make sense for these to be landed in the
upstream repository.  I would suggest that these files be moved out of the
WebKit tree and in to a separate Android project that Android WebKit depends
on.

It's not clear to me what "WDS" is.  WebKit Debug Server perhaps?  Though I
don't understand what it does, from looking over the code it is not clear that
the WebKit directory is the right location for it to live.


Again, due to the size of the patch I didn't spend much time focusing on
details.  These are just some of the bigger-picture issues that I noticed while
reading over the patch.  When submitting an updated patch to address these
issues, it may be worth considering splitting the patch in order to make it
easier to review, and so that the obviously correct parts can be landed while
other parts of the patch are being refined.  A few chunks come to mind as parts
that could be submitted independently: the plugin code that is part of WebCore,
the implementation of the plugin API that Android exposes to plugins, the
FooClient implementations, "WDS", and so on.

I'm going to mark this as r- for now, due to the WebCore namespace and STL
issues.  I look forward to seeing further patches!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.