Bug 44767 – [EFL] WebCore depends on symbols from WebKit

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

[Lucas De Marchi]

Current ContextMenu implementation adds a dependency in WebCore from WebKit. This should not happen since it's WebKit that depends on WebCore.

[Lucas De Marchi]

Created attachment 65733 [details]
Patch

[WebKit Review Bot]

Attachment 65733 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lucas De Marchi]

Created attachment 65737 [details]
Patch

[Antonio Gomes]

(In reply to comment #3)
> Created an attachment (id=65737) [details]
> Patch

Looks good.

@luiz, could you please informally review that patch? (he is our lord on it currentl :)

[Adam Barth]

Comment on attachment 65737 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=65737&action=prettypatch

> WebCore/page/ContextMenuClient.h:60
> +#elif PLATFORM(EFL)
> +        virtual PlatformMenuDescription createPlatformDescription(ContextMenu*) = 0;
> +        virtual void freePlatformDescription(PlatformMenuDescription) = 0;
> +        virtual void appendItem(PlatformMenuDescription, ContextMenuItem&) = 0;
> +        virtual void show(PlatformMenuDescription menu) = 0;
I'm not sure we should have platform specific methods here.  I see that Mac does it, but that doesn't mean it's a good pattern.

[Kenneth Rohde Christiansen]

Darin, can you give your comments on this?

[Luiz Agostini]

If ContextMenu was an abstract class, EFL could have its own ContextMenu inherited class implemented in WebKit layer. The ContextMenu instance to be used would need to be provided by one of the clients (maybe ContextMenuClient). Then there would be no layering violations. It would demand a lot of changes in all platforms. Those changes would not be complex but they would be extensive.

An other possibility would be to have a new pure abstract class that inherits from ContextMenuClient in WebCore/platform/efl. This new class would have the EFL specific methods (createPlatformDescription, freePlatformDescription, appendItem and show) and could be used by ContextMenu with no layering violations (am I right?). ContextMenuClientEfl would then inherit from this new class instead of ContextMenuClient.

[Lucas De Marchi]

After the refactoring on context menu in WebCore, this is not needed anymore. Instead, EFL port must fix its implementation to adhere to the new API.