Bug 32651 - [Android] MainThreadAndroid introduces a layering violation by calling JavaSharedClient methods
Summary: [Android] MainThreadAndroid introduces a layering violation by calling JavaSh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-17 02:45 PST by Steve Block
Modified: 2009-12-22 14:33 PST (History)
7 users (show)

See Also:


Attachments
Patch 1 for Bug 32651 (2.58 KB, patch)
2009-12-17 02:51 PST, Steve Block
levin: review-
Details | Formatted Diff | Diff
Patch 2 for Bug 32651 (4.51 KB, patch)
2009-12-21 09:24 PST, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-12-17 02:45:58 PST
MainThreadAndroid introduces a layering violation by calling JavaSharedClient methods in WebKit. Instead, it should use PlatformBridge.
Comment 1 Steve Block 2009-12-17 02:51:27 PST
Created attachment 45048 [details]
Patch 1 for Bug 32651
Comment 2 WebKit Review Bot 2009-12-17 02:55:33 PST
style-queue ran check-webkit-style on attachment 45048 [details] without any errors.
Comment 3 Sam Weinig 2009-12-18 09:45:33 PST
This is still a layering violation.  JavaScriptCore and WTF should not have dependencies on WebCore.
Comment 4 David Levin 2009-12-18 10:08:30 PST
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"
Comment 5 Andrei Popescu 2009-12-18 13:04:45 PST
(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
Comment 6 David Levin 2009-12-18 13:26:56 PST
(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.
Comment 7 Andrei Popescu 2009-12-18 13:55:46 PST
(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
Comment 8 Steve Block 2009-12-21 09:24:48 PST
Created attachment 45335 [details]
Patch 2 for Bug 32651

Updated patch to use AndroidThreading, as discussed.
Comment 9 WebKit Review Bot 2009-12-21 09:28:04 PST
style-queue ran check-webkit-style on attachment 45335 [details] without any errors.
Comment 10 David Levin 2009-12-22 13:31:23 PST
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 11 Steve Block 2009-12-22 14:10:19 PST
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 12 WebKit Commit Bot 2009-12-22 14:33:38 PST
Comment on attachment 45335 [details]
Patch 2 for Bug 32651

Clearing flags on attachment: 45335

Committed r52503: <http://trac.webkit.org/changeset/52503>
Comment 13 WebKit Commit Bot 2009-12-22 14:33:43 PST
All reviewed patches have been landed.  Closing bug.