Bug 97651 – [EFL] Make PageClient accessible to both WebKit and WebKit2

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit EFL

[Regina Chung]

WebKit needs access to PageClientEfl during initialization of GraphicsContext3DEfl, while WebKit2 doesn't.
But to build WebCore for both WebKit and WebKit2, we need a dummy PageClientEfl class that WebKit2 can also access.

[Regina Chung]

Created attachment 168381 [details]
Patch

[Ryuan Choi]

Comment on attachment 168381 [details]
Patch

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

> Source/WebCore/platform/efl/WebPageClientEfl.h:23
> +#include <Evas.h>

Can we just declare some types istead of include Evas.h in header file.

> Source/WebCore/platform/efl/WebPageClientEfl.h:30
> +class WebPageClientEfl {

What do you think about adding this in WebCore namespae?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:52
> +    WebPageClientEfl* pageClient = 0;

Can we move this below 54 line.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:63
> +                sharedContext = static_cast<Evas_GL_Context*>(context->platformGraphicsContext3D());

Can I know the meaning of this?

I could not find usage of this value.

[Kenneth Rohde Christiansen]

Comment on attachment 168381 [details]
Patch

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

>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:63
>> +                sharedContext = static_cast<Evas_GL_Context*>(context->platformGraphicsContext3D());
> 
> Can I know the meaning of this?
> 
> I could not find usage of this value.

Better question (answer in changelog) why is this different code path needed for webkit1? Could it use the same one as webkit2 (fake evas) or is there any advantages to this new approach?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:119
> +bool GraphicsContext3DPrivate::createSurface(WebPageClientEfl* pageClient, bool renderDirectlyToHostWindow)

Why not adding the actual Evas_Object* instead? There is like almost nothing shared in WebPageClientEfl

[Regina Chung]

(In reply to comment #2)
> (From update of attachment 168381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168381&action=review
> > Source/WebCore/platform/efl/WebPageClientEfl.h:23
> > +#include <Evas.h>
> Can we just declare some types istead of include Evas.h in header file.
Sure, removed the include and moved the type declarations in PageClientEfl.h to WebPageClientEfl.h

> > Source/WebCore/platform/efl/WebPageClientEfl.h:30
> > +class WebPageClientEfl {
> What do you think about adding this in WebCore namespae?
WebPageClientEfl is typedefed as PlatformPageClient in Widget.h
I'm not sure if it is a good idea to add it into the WebCore namespace...

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:52
> > +    WebPageClientEfl* pageClient = 0;
> Can we move this below 54 line.
pageClient is needed outside of the if statement in line 54. (see line 98)

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:63
> > +                sharedContext = static_cast<Evas_GL_Context*>(context->platformGraphicsContext3D());
> Can I know the meaning of this?
> I could not find usage of this value.
Sorry, sharedContext was supposed to be passed as the 2nd argument for evas_gl_context_create() (line 93). Will fix this in new patch.

[Regina Chung]

(In reply to comment #3)
> (From update of attachment 168381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168381&action=review
> >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:63
> >> +                sharedContext = static_cast<Evas_GL_Context*>(context->platformGraphicsContext3D());
> > 
> > Can I know the meaning of this?
> > 
> > I could not find usage of this value.
> Better question (answer in changelog) why is this different code path needed for webkit1? Could it use the same one as webkit2 (fake evas) or is there any advantages to this new approach?

In WebKit2 a GraphicsContext3D is created for Accelerated Compositing(AC), and another for WebGL (if needed). 
When creating the evas gl context for WebGL we create it as a shared context with ACs evas gl context, so we can use the evas created for webview for both of them.
Thus no need for a fake evas.

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:119
> > +bool GraphicsContext3DPrivate::createSurface(WebPageClientEfl* pageClient, bool renderDirectlyToHostWindow)
> Why not adding the actual Evas_Object* instead? There is like almost nothing shared in WebPageClientEfl

PageClientEfl (which is only used for webkit1) also provides a way to access ewk_view_*** calls from GraphicsContext3D.

[Regina Chung]

Created attachment 168637 [details]
Patch

[Kenneth Rohde Christiansen]

> > > I could not find usage of this value.
> > Better question (answer in changelog) why is this different code path needed for webkit1? Could it use the same one as webkit2 (fake evas) or is there any advantages to this new approach?
> 
> In WebKit2 a GraphicsContext3D is created for Accelerated Compositing(AC), and another for WebGL (if needed). 
> When creating the evas gl context for WebGL we create it as a shared context with ACs evas gl context, so we can use the evas created for webview for both of them.
> Thus no need for a fake evas.

You answered the opposite question. I know you dont need the fake evas, but would it make any difference if you did use it?

[Kenneth Rohde Christiansen]

Comment on attachment 168637 [details]
Patch

This seems like a terrible abuse of the page client. The page client is not supposed to be reused across ports. It is something that is very specific to the client of the page (aka view/viewport!) and something that is specific to each port. This moved in the wrong direction of what the page client was created for.

[Regina Chung]

(In reply to comment #7)
> > > > I could not find usage of this value.
> > > Better question (answer in changelog) why is this different code path needed for webkit1? Could it use the same one as webkit2 (fake evas) or is there any advantages to this new approach?
> > 
> > In WebKit2 a GraphicsContext3D is created for Accelerated Compositing(AC), and another for WebGL (if needed). 
> > When creating the evas gl context for WebGL we create it as a shared context with ACs evas gl context, so we can use the evas created for webview for both of them.
> > Thus no need for a fake evas.
> You answered the opposite question. I know you dont need the fake evas, but would it make any difference if you did use it?

Actually that was a typo :( 
I ment WebKit doesn't need the fake evas (But Webkit2 does). Sorry about that.
WebKit HAS to use the evas created for webview when creating a GC3D for AC, otherwise the composited webpage won't get rendered into the webview.

[Kenneth Rohde Christiansen]

Can we do this in another way instead of passing a specialized PageClient around.

[Regina Chung]

(In reply to comment #10)
> Can we do this in another way instead of passing a specialized PageClient around.

I'm not sure there's a way to access the webview's evas from GC3D without the help of PageClient..

Maybe I should move all the WebKit1 related code into a new function in PageClientEfl?
At least that's what QT is doing.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.