Bug 23167

Summary: add an Android file to JavaScriptCore/wtf
Product: WebKit Reporter: Feng Qian <feng>
Component: JavaScriptCoreAssignee: Feng Qian <feng>
Status: RESOLVED WONTFIX    
Severity: Normal CC: caryclark, darin, eric, laszlo.gombos, mrowe
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
add one file
none
patch v2
darin: review-
patch v3 mrowe: review-

Description Feng Qian 2009-01-07 11:21:24 PST
It is part of effort as bug 23118 to merging Android's webkit port to trunk.

This patch adds JavaScriptCore/wtf/android/MainThreadAndroid.cpp
Comment 1 Feng Qian 2009-01-07 11:24:19 PST
Created attachment 26498 [details]
add one file
Comment 2 Feng Qian 2009-01-07 13:20:09 PST
Created attachment 26506 [details]
patch v2

Fixed tabs in the previous patch, and includes a link to the bug in the ChangeLog file.
Comment 3 Darin Adler 2009-01-07 13:21:45 PST
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.
Comment 4 Mark Rowe (bdash) 2009-01-07 14:24:27 PST
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.
Comment 5 Feng Qian 2009-01-09 15:23:00 PST
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).
Comment 6 Darin Adler 2009-01-10 14:25:13 PST
Comment on attachment 26582 [details]
patch v3

r=me
Comment 7 Darin Adler 2009-01-10 14:26:47 PST
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".
Comment 8 Mark Rowe (bdash) 2009-01-10 22:42:10 PST
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.
Comment 9 Darin Adler 2009-01-11 08:55:14 PST
(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.
Comment 10 Mark Rowe (bdash) 2009-01-11 13:59:18 PST
Comment on attachment 26582 [details]
patch v3

Changing review to r- due to the layering violation.
Comment 11 Feng Qian 2009-01-13 08:47:44 PST
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.
> 
Comment 12 Darin Adler 2009-01-13 09:14:02 PST
(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.
Comment 13 Dimitri Glazkov (Google) 2009-01-13 09:20:22 PST
Would it help if we moved WTF into its own library? That would certainly clear up some dependency misunderstandings.
Comment 14 Darin Adler 2009-01-13 09:25:04 PST
(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.
Comment 15 Feng Qian 2009-01-13 11:14:21 PST
(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.
> 

Comment 16 Eric Seidel (no email) 2012-02-13 14:43:55 PST
Android Chromium is the new future.