Summary: | [Android] MainThreadAndroid introduces a layering violation by calling JavaSharedClient methods | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Steve Block <steveblock> | ||||||
Component: | WebCore Misc. | Assignee: | Steve Block <steveblock> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andreip, android-webkit-unforking, commit-queue, dimich, levin, sam, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Android | ||||||||
OS: | Android | ||||||||
Attachments: |
|
Description
Steve Block
2009-12-17 02:45:58 PST
Created attachment 45048 [details] Patch 1 for Bug 32651 style-queue ran check-webkit-style on attachment 45048 [details] without any errors.
This is still a layering violation. JavaScriptCore and WTF should not have dependencies on WebCore. Comment on attachment 45048 [details] Patch 1 for Bug 32651 r- per Sam's comment. I think the right thing to do here is to move the implementation from WebCore to wtf. Then have the WebCore version (if it is still needed) call the wtf version. > Index: WebCore/platform/android/PlatformBridge.h > + static void EnqueueOnMainthread(void (*function)(void*), void* payload); fwiw, bad casing "Mainthread" (In reply to comment #4) > (From update of attachment 45048 [details]) > r- per Sam's comment. > > I think the right thing to do here is to move the implementation from WebCore > to wtf. Then have the WebCore version (if it is still needed) call the wtf > version. > Sadly, the implementation is pretty complex and I am not sure it's feasible to move it all inside wtf. Instead, we could just do what Chromium does. In fact, we could even reuse ChromiumThreading if the naming wouldn't introduce some confusion. So how about we just copy ChromiumThreading and call it AndroidThreading? Thanks, Andrei (In reply to comment #5) > So how about we just copy ChromiumThreading and call it AndroidThreading? The chromium version basically is calling out to the "chromium platform" (but not WebCore/platform). If you did something similar here, that seems like it would be fine. (In reply to comment #6) > (In reply to comment #5) > > So how about we just copy ChromiumThreading and call it AndroidThreading? > > The chromium version basically is calling out to the "chromium platform" (but > not WebCore/platform). If you did something similar here, that seems like it > would be fine. Cool, thanks. Steve's now on vacation so I'll update the patch to reflect this discussion. Andrei Created attachment 45335 [details] Patch 2 for Bug 32651 Updated patch to use AndroidThreading, as discussed. style-queue ran check-webkit-style on attachment 45335 [details] without any errors.
Comment on attachment 45335 [details] Patch 2 for Bug 32651 > Index: JavaScriptCore/wtf/android/AndroidThreading.h > + * Copyright 2009, The Android Open Source Project Please consider this format (which is more typical of WK headers): * Copyright (C) 2009 The Android Open Source Project Comment on attachment 45335 [details] Patch 2 for Bug 32651 > Please consider this format (which is more typical of WK headers): > * Copyright (C) 2009 The Android Open Source Project This style is used throughout all Android WebKit files, so I thinks it's best to leave it as it is. Comment on attachment 45335 [details] Patch 2 for Bug 32651 Clearing flags on attachment: 45335 Committed r52503: <http://trac.webkit.org/changeset/52503> All reviewed patches have been landed. Closing bug. |