Created attachment 91845 [details] Patch

Ah, I clearly read the patches in the wrong order.

Comment on attachment 91845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91845&action=review Cool. Does this actually build? > Source/WebCore/CMakeListsAndroid.txt:2 > + "${JAVASCRIPTCORE_DIR}/wtf/text" WebCore shouldn't be including directly from this directory. Even if it did, it shouldn't be in an Android-specific include block. > Source/cmake/OptionsAndroid.cmake:34 > +WEBKIT_FEATURE(ENABLE_AS_IMAGE "Enable SVG as image" DEFAULT ON SVG) > +WEBKIT_FEATURE(ENABLE_BLOB "Enable blob slice" DEFAULT OFF) > +WEBKIT_FEATURE(ENABLE_CHANNEL_MESSAGING "Enable channel messaging" DEFAULT ON) > +WEBKIT_FEATURE(ENABLE_DATABASE "Enable database" DEFAULT ON) > +WEBKIT_FEATURE(ENABLE_DATAGRID "Enable datagrid" DEFAULT OFF) > +WEBKIT_FEATURE(ENABLE_DATALIST "Enable datalist" DEFAULT ON HTML) > +WEBKIT_FEATURE(ENABLE_DATA_TRANSFER_ITEMS "Enable data transfer items" DEFAULT OFF) > +WEBKIT_FEATURE(ENABLE_DOM_STORAGE "Enable DOM storage" DEFAULT ON) > +WEBKIT_FEATURE(ENABLE_EVENTSOURCE "Enable event source" DEFAULT ON) > +WEBKIT_FEATURE(ENABLE_FAST_MALLOC "Enable TCmalloc instead of system's allocator" DEFAULT ON) > +WEBKIT_FEATURE(ENABLE_FAST_MOBILE_SCROLLING "Enable fast mobile scrolling" DEFAULT ON) > +WEBKIT_FEATURE(ENABLE_FILTERS "Enable SVG filters" DEFAULT ON SVG) Does all this stuff have to be duplicated in all these files?

+steveblock

Comment on attachment 91845 [details] Patch R- for the errant wtf/text include path.

Comment on attachment 91845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91845&action=review I'm a little confused about your overall plan. As I'm sure you're aware, the Android port is not yet fully upstreamed, and the upstreamed portion alone won't currently build. It certainly won't build with features enabled other than those currently in Platform.h and config.h. Do you intend to fix this? It would help if there was a meta bug to capture your larger intent. > Source/cmake/OptionsAndroid.cmake:4 > +ADD_DEFINITIONS(-DWEBCORE_NAVIGATOR_VENDOR="moiji-mobile.com") !! I guess we shouldn't be specifying a vendor at all. What do other ports do? > Source/cmake/OptionsAndroid.cmake:59 > +WEBKIT_FEATURE(ENABLE_XSLT "Enable XSLT" DEFAULT ON) I'm not sure exactly what your intent is here. Are you trying to replicate the set of feature enables that you removed from Platform.h and config.h in Bug 59889 ? It might be easier to follow if these were combined in a single patch to move these flags from code to the build system.

(In reply to comment #3) > (From update of attachment 91845 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91845&action=review > > Cool. Does this actually build? > > > Source/WebCore/CMakeListsAndroid.txt:2 > > + "${JAVASCRIPTCORE_DIR}/wtf/text" > > WebCore shouldn't be including directly from this directory. Even if it did, it shouldn't be in an Android-specific include block. Okay, both CE and Efl did it in their file. I agree... if it is necessary it should be shared and if it is not necessary I will remove it. > > Source/cmake/OptionsAndroid.cmake:34 ... > > +WEBKIT_FEATURE(ENABLE_FAST_MOBILE_SCROLLING "Enable fast mobile scrolling" DEFAULT ON) > > +WEBKIT_FEATURE(ENABLE_FILTERS "Enable SVG filters" DEFAULT ON SVG) It appears to be. The reasoning could be to have different defaults on different platforms (e.g. SVG being mostly off Foo and XSLT always on for Bar). I will attempt to consult with one of the authors of the CMake build support.

> It appears to be. The reasoning could be to have different defaults on different platforms (e.g. SVG being mostly off Foo and XSLT always on for Bar). I will attempt to consult with one of the authors of the CMake build support. Thanks. It's not a super big deal either way, and it certainly doesn't need to block your work.

(In reply to comment #6) > (From update of attachment 91845 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91845&action=review > > I'm a little confused about your overall plan. As I'm sure you're aware, the Android port is not yet fully upstreamed, and the upstreamed portion alone won't currently build. It certainly won't build with features enabled other than those currently in Platform.h and config.h. Do you intend to fix this? It would help if there was a meta bug to capture your larger intent. Has my email to webkit-dev helped? The short answer is. - I want build-webkit --android to work with a NDK (and external libs for Skia) - I will provide a build-bot to keep it working - I will start with DRT and will be unlikely to make this a core builder > > > Source/cmake/OptionsAndroid.cmake:4 > > +ADD_DEFINITIONS(-DWEBCORE_NAVIGATOR_VENDOR="moiji-mobile.com") It will default to Apple Inc.

Yes, your email did help, though you didn't explicitly mention your plans regarding all of the components of the Android port that are not yet upstreamed. Please CC me on all Android changes so we can make sure to avoid conflicts between your work and the existing Android port. I still think that a meta-but would be useful to track your work.

(In reply to comment #10) > Yes, your email did help, though you didn't explicitly mention your plans regarding all of the components of the Android port that are not yet upstreamed. Please CC me on all Android changes so we can make sure to avoid conflicts between your work and the existing Android port. CC: Sure, I can do that. We could also ask for an android component if it does not exist yet. conflict: My current plan is to get everything out of tree into the tree (including WebKit/android) but then again my Agenda is different to the "Google Android". Then I'm also not aware of the current merge status but I try to pick up as much as possible. In general I think an upstream project should not show much sympathy toward out of tree forks. irc: It would be nice if we could chat a bit on IRC. > > I still think that a meta-but would be useful to track your work. So, I would add each of my android bugs to this bug?

Android Chromium is the new future.