Bug 32651 – [Android] MainThreadAndroid introduces a layering violation by calling JavaSharedClient methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

MainThreadAndroid introduces a layering violation by calling JavaSharedClient methods in WebKit. Instead, it should use PlatformBridge.

[Steve Block]

Created attachment 45048 [details]
Patch 1 for Bug 32651

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45048 [details] without any errors.

[Sam Weinig]

This is still a layering violation.  JavaScriptCore and WTF should not have dependencies on WebCore.

[David Levin]

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"

[Andrei Popescu]

(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

[David Levin]

(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.

[Andrei Popescu]

(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

[Steve Block]

Created attachment 45335 [details]
Patch 2 for Bug 32651

Updated patch to use AndroidThreading, as discussed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45335 [details] without any errors.

[David Levin]

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

[Steve Block]

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.

[WebKit Commit Bot]

Comment on attachment 45335 [details]
Patch 2 for Bug 32651

Clearing flags on attachment: 45335

Committed r52503: <http://trac.webkit.org/changeset/52503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.