Bug 23167 – add an Android file to JavaScriptCore/wtf

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Feng Qian]

It is part of effort as bug 23118 to merging Android's webkit port to trunk.

This patch adds JavaScriptCore/wtf/android/MainThreadAndroid.cpp

[Feng Qian]

Created attachment 26498 [details]
add one file

[Feng Qian]

Created attachment 26506 [details]
patch v2

Fixed tabs in the previous patch, and includes a link to the bug in the ChangeLog file.

[Darin Adler]

Comment on attachment 26506 [details]
patch v2

It's wrong for WTF to be calling into WebCore; it needs to work the other way around. WebCore is built on top of WTF, not vice versa.

[Mark Rowe (bdash)]

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.

[Feng Qian]

Created attachment 26582 [details]
patch v3

Thanks for your comments. Android agrees to use BSD license for files upstreamed to WebKit. The license header is changed to BSD license, I think it should be the same as other files submitted by Cary Clark.

JavaSharedClient class is moved from WebCore namespace to android namespace. The change should be reflected in WebKit/android/jni/JavaSharedClient.h (which is in a different bug).

[Darin Adler]

Comment on attachment 26582 [details]
patch v3

r=me

[Darin Adler]

Comment on attachment 26582 [details]
patch v3

> +#include "jni/JavaSharedClient.h"

Normal WebKit style is to put a blank line after "MainThread.h" and before the next include.

> +using android::JavaSharedClient;

Normal WebKit style is to do using namespace on the entire namespace rather than individual using statements.

> +static void timeoutFired(void *)

Normal WebKit style is to use "void*" here instead of "void *".

> +    JavaSharedClient::EnqueueFunctionPtr(timeoutFired, NULL);

Normal WebKit style is to use "0" here instead of "NULL".

[Mark Rowe (bdash)]

Is it really appropriate for a file in WTF to import a file from *WebKit*?  That would seem to be the same layering violation that was raised in comment #3.

[Darin Adler]

(In reply to comment #8)
> Is it really appropriate for a file in WTF to import a file from *WebKit*? 
> That would seem to be the same layering violation that was raised in comment
> #3.

Mark is right! I missed that.

[Mark Rowe (bdash)]

Comment on attachment 26582 [details]
patch v3

Changing review to r- due to the layering violation.

[Feng Qian]

Thanks. I will look at a way to refactor JavaSharedClient::EnqueueFunctionPtr(timeoutFired, NULL);
 out of WebKit/WebCore namespace/directory.

From comments, my understanding is that wtf should only depend on system header files/functions, such as skia (in Android case). Is my understanding correct?


(In reply to comment #10)
> (From update of attachment 26582 [details] [review])
> Changing review to r- due to the layering violation.
> 

[Darin Adler]

(In reply to comment #11)
> From comments, my understanding is that wtf should only depend on system header
> files/functions, such as skia (in Android case). Is my understanding correct?

That's right. And note that it's not even appropriate for WTF to depend on all types of system headers; it's pretty low level. So I'd be surprised if WTF had to depend on the graphics subsystem (skia), but it's not unthinkable.

The basic layering is that WTF is a low level layer. Both the JavaScript engine and WebCore are built on top of it. And WebKit is build on top of those.

From top to bottom:

    WebKit
    WebCore
    JavaScriptCore JavaScript Engine
    WTF

Also, the "platform" directory inside WebCore should not depend on the rest of WebCore or on WebKit or on JavaScriptCore:

    WebKit
    WebCore
    WebCore platform directory
    WTF

We should probably document this somewhere on the WebKit website.

[Dimitri Glazkov (Google)]

Would it help if we moved WTF into its own library? That would certainly clear up some dependency misunderstandings.

[Darin Adler]

(In reply to comment #13)
> Would it help if we moved WTF into its own library? That would certainly clear
> up some dependency misunderstandings.

That's been proposed before, and we may indeed want to do that.

However this particular misunderstanding, where WTF is depending on something that's not even in JavaScriptCore, probably wouldn't have been affected by that. WTF already is in a separate library from WebKit!

Similarly, the WebCore platform layer could go into a separate library for similar reasons, so we don't accidentally put in inverted dependencies.

The question of how things are linked and how they find headers is one part of being a separate library. But dynamic vs. static linking and optimization is another part of it. These are topics that can have a big impact and need to be considered carefully.

For example, no matter how things were structured internally, it's critical that JavaScriptCore and WebKit remain separate frameworks on Mac OS X, and we don't want to introduce any separate dynamically linked libraries on that platform.

This discussion goes way beyond the scope of this bug.

[Feng Qian]

(In reply to comment #12)
> (In reply to comment #11)
> > From comments, my understanding is that wtf should only depend on system header
> > files/functions, such as skia (in Android case). Is my understanding correct?
> 
> That's right. And note that it's not even appropriate for WTF to depend on all
> types of system headers; it's pretty low level. So I'd be surprised if WTF had
> to depend on the graphics subsystem (skia), but it's not unthinkable.

This jni/JavaSharedClient.* file needs some refactoring, currently it uses some helper classes from skia, which I think is not necessary. I will have further discussion with Android folks on how to refactor code to following the guideline you outlined here. Thanks!

> 
> The basic layering is that WTF is a low level layer. Both the JavaScript engine
> and WebCore are built on top of it. And WebKit is build on top of those.
> 
> From top to bottom:
> 
>     WebKit
>     WebCore
>     JavaScriptCore JavaScript Engine
>     WTF
> 
> Also, the "platform" directory inside WebCore should not depend on the rest of
> WebCore or on WebKit or on JavaScriptCore:
> 
>     WebKit
>     WebCore
>     WebCore platform directory
>     WTF
> 
> We should probably document this somewhere on the WebKit website.
> 

[Eric Seidel (no email)]

Android Chromium is the new future.