Bug 23118 – add Android platform-specific files

• Component: Platform
• Status: RESOLVED INVALID

[Cary Clark]

This is the first-cut at Android's WebKit port. The files are not complete -- for instance, the makefile is missing. A complete port is forthcoming -- but this permits seeing how other submitted changes to WebCore are used by Android.

[Cary Clark]

Created attachment 26430 [details]
WebKit platform files for Android

[Oliver Hunt]

Comment on attachment 26430 [details]
WebKit platform files for Android

First pass is just formatting issues, 
  * the opening '{' for a method should go the the line following the prototype, 
  * license comments at the beginning of the files should have a single column of '*' not '**' (I'd prefer these be lgpl v2 or bsd like the rest of webkit but i am not sure if there's a rule about that)
  * Plenty of commented code like "//  virtual void didChangeTypingStyle:(NSNotification *)notification;" exists which is less than ideal, they should be converted to the appropriate platform specific model, and have fixme's (or whatever) inserted.
  * Inside WebCore and WebKit rather than continually repeating WTF::Vector/RefPtr etc you should put "using namespace WTF" at the top of the implementation file and ddrop the naespace prefix
  * You should not be including a copy of the stl at all,  WTF provides everything that should be necessary, and including stl for the hell of it is folly

[Darin Adler]

Comment on attachment 26430 [details]
WebKit platform files for Android

It's great to get this code in the tree. We can make code style changes and other improvements once that's done.

r=me

To make this truly useful we'll need to get build instructions that work and a buildbot. I hope that comes soon! Cary and I have been talking about that a little.

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 26430 [details] [review])
> It's great to get this code in the tree. We can make code style changes and
> other improvements once that's done.
> 
> r=me

Maybe Chrome should take the same approach.  Currently we've been cleaning up as we go.  But that's been rather painful.  There are lots of style violations which could easily be cleaned up in this patch with a single regexp.

[Oliver Hunt]

> 
> Maybe Chrome should take the same approach.  Currently we've been cleaning up
> as we go.  But that's been rather painful.  There are lots of style violations
> which could easily be cleaned up in this patch with a single regexp.
> 

I do not believe so -- the vast majority of this code has correct style (per webkit.org guidelines) and there are only a few places where it's wrong, and it's basically only the one error ('{' not going on a new line), i had passed on the bulk of the style violations months ago, and they appear to have been corrected.

[Oliver Hunt]

I was just reminded, and therefore felt i should comment -- it still *really* irks me that this code has a different license, there's a very clear noticed when you submit a patch that states that all patches should be bsd or lgpl.  I'm not sure why there is a necessity for this code to be apache licensed, vs. the *entire* rest of webkit

[Mark Rowe (bdash)]

Comment on attachment 26430 [details]
WebKit platform files for Android

Per <https://lists.webkit.org/pipermail/webkit-dev/2009-January/006302.html>, current WebKit policy is that code should be
licensed under either a BSD license or GNU Lesser General Public License
v.2.1.  Accepting code that uses a different license would require changing this policy, which would require a larger discussion outside of the context of a code review.  Please consider resubmitting the patch with the changes using either a BSD license or GNU Lesser General Public License v.2.1.

[David Levin]

I glanced quickly out of curiosity.  Here's a few things I noticed along the way.

* This one is frequently violated "In a header, code inside a namespace should be indented."

* "Lower-case the first letter, including all letters in an acronym, in a variable or function name." (violations: MIMEType, URLScheme)


In WebKit/android/FrameLoaderClientAndroid.cpp

* "occurances" is misspelled (a few times).

* The indents look wrong in serveral places.

* || and && happen at the end of lines instead of the beginning of the next line in several places (e.g "if ((error.errorCode() == ErrorFile ||").

* Tests against 0 are to be done without ==/!=.  (e.g. "if (m_frame->ownerElement() != 0)" should be "if (m_frame->ownerElement())" (and imo, would be even clearer if it used ?: in this case).

* A few single line if's have {}

[Cary Clark]

Created attachment 26564 [details]
WebKit platform files for Android

[Mark Rowe (bdash)]

Comment on attachment 26564 [details]
WebKit platform files for Android

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!

[Cary Clark]

Created attachment 26681 [details]
WebKit platform files for Android 

This patch addresses these reviewers' concerns:
- remove android's stl
- move *ClientAndroid.cpp to namespace android
- move out some plugin files (to be separately submitted)
- remove methods on WebCore::Frame

[Eric Seidel (no email)]

Comment on attachment 26681 [details]
WebKit platform files for Android 

too large to review, unless you're looking for a rubber stamp.  Marking this r- since I can't take action on this patch and it's been in the queue for over 4 months.

[Feng Qian]

(In reply to comment #12)
> (From update of attachment 26681 [details] [review])
> too large to review, unless you're looking for a rubber stamp.  Marking this r-
> since I can't take action on this patch and it's been in the queue for over 4
> months.
> 

Cary, you might want to split the patch into smaller patches, and each small patch has a small number of files, with its own change log. It makes review and commit easier.
I used this method in bug 23296.

[Maciej Stachowiak]

This would be much easier to review if split by file, or related group of files.

[Cary Clark]

This is no longer an active port.