Bug 82315 – [Texmap][EFL] Implementation of AC related functions in ChromeClientEfl and ewkView.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

Add missing implementation of functions for AC in ChromeClientEfl and ewkView.

[Hyowon Kim]

Created attachment 134008 [details]
Patch

[Noam Rosenthal]

Comment on attachment 134008 [details]
Patch

If another EFL developer wants to sign off on this, LGTM

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 134008 [details]
Patch

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

Looks fine overall with one minor comment.

> Source/WebKit/efl/ewk/ewk_view.cpp:154
> +    Evas_Object* compositingObject;

You could wrap an OwnPtr around this Evas_Object to avoid having to delete it manually below.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> (From update of attachment 134008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134008&action=review
> 
> Looks fine overall with one minor comment.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:154
> > +    Evas_Object* compositingObject;
> 
> You could wrap an OwnPtr around this Evas_Object to avoid having to delete it manually below.

(If you don't do this you should probably check if the pointer is valid before calling evas_object_del()).

[Hyowon Kim]

Created attachment 134478 [details]
Modified patch

add null-check for evas_object(compositingObject) in _ewk_view_priv_del().

[Hyowon Kim]

> > Looks fine overall with one minor comment.
> > 
> > > Source/WebKit/efl/ewk/ewk_view.cpp:154
> > > +    Evas_Object* compositingObject;
> > 
> > You could wrap an OwnPtr around this Evas_Object to avoid having to delete it manually below.
> 
> (If you don't do this you should probably check if the pointer is valid before calling evas_object_del()).

Thanks Raphael. :) I've added some code to check the evas_object pointer.

[Hyowon Kim]

Created attachment 145720 [details]
updated patch

[Noam Rosenthal]

Peer review first please :)

[WebKit Review Bot]

Comment on attachment 145720 [details]
updated patch

Attachment 145720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12896614

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145834 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hyowon Kim]

Created attachment 146477 [details]
rebased patch

[Igor Trindade Oliveira]

Hyowon Kim could you add the necessary files in EFL build system? So other people can test.

[Hyowon Kim]

(In reply to comment #12)
> Hyowon Kim could you add the necessary files in EFL build system? So other people can test.

Sure. I need feedback.
Bug 88630 is needed to build, and bug 88634 makes AC enable.

[Gyuyoung Kim]

Comment on attachment 146477 [details]
rebased patch

Cleared review? from attachment 146477 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach new patch to this bug or new bug again.

[Hyowon Kim]

Created attachment 170795 [details]
rebased patch

I've attached the rebased patch again.
I kindly ask for review and comments.

[Ryuan Choi]

Comment on attachment 170795 [details]
rebased patch

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:611
> +    return AllTriggers;

As a curiosity, Doesn't we need options to choose features ?

> Source/WebKit/efl/ewk/ewk_view.cpp:4514
> +static void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object* object)

(void* data, Evas_Object*) looks enough.

> Source/WebKit/efl/ewk/ewk_view.cpp:4519
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv);

If this use only priv, why don't you just send priv as data.

> Source/WebKit/efl/ewk/ewk_view.cpp:4527
> +void _ewk_view_accelerated_compositing_context_create_if_needed(Evas_Object* ewkView)

static?

> Source/WebKit/efl/ewk/ewk_view.cpp:4566
> +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* ewkView)

This function returns GraphicsContext3D.

> Source/WebKit/efl/ewk/ewk_view.cpp:4592
> +        _ewk_view_accelerated_compositing_context_create_if_needed(ewkView);
> +        evas_object_show(priv->compositingObject);
> +    } else
> +        evas_object_hide(priv->compositingObject);
> +
> +    priv->acceleratedCompositingContext->attachRootGraphicsLayer(rootLayer);

acceleratedCompositingContext are used, irrespective of compositingActive

[Viatcheslav Ostapenko]

Comment on attachment 170795 [details]
rebased patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:255
> +    bool compositingActive;

Do you really need this "compositingActive" . IMHO, it is enough to check that acceleratedCompositingContext exists.

[Kenneth Rohde Christiansen]

Comment on attachment 170795 [details]
rebased patch

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:596
> -    notImplemented();
> +    ewk_view_root_graphics_layer_set(m_view, rootLayer);

Werent we moving to use C++ methods?

> Source/WebKit/efl/ewk/ewk_view.cpp:4538
> +
> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, const WebCore::IntRect& rect)
> +{
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);

Same...

[Hyowon Kim]

Created attachment 170846 [details]
patch from comment #16

[Kenneth Rohde Christiansen]

Comment on attachment 170846 [details]
patch from comment #16

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

> Source/WebKit/efl/ewk/ewk_view.cpp:255
> +    bool compositingActive;

isCompositingActive

> Source/WebKit/efl/ewk/ewk_view.cpp:960
> +    if (priv->compositingObject)
> +        evas_object_del(priv->compositingObject);

use RefPtr they support evas objects

> Source/WebKit/efl/ewk/ewk_view.cpp:4572
> +void ewk_view_root_graphics_layer_set(Evas_Object* ewkView, WebCore::GraphicsLayer* rootLayer)

Why cant these be in the EwkViewImpl ?

> Source/WebKit/efl/ewk/ewk_view.cpp:4577
> +    bool active = rootLayer ? true : false;

= !!rootLayer

[Hyowon Kim]

(In reply to comment #16)
> (From update of attachment 170795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:611
> > +    return AllTriggers;
> As a curiosity, Doesn't we need options to choose features ?

To check options from settings would be needed.
I think it would be better to make a new bug to deal with it.

> > Source/WebKit/efl/ewk/ewk_view.cpp:4514
> > +static void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object* object)
> (void* data, Evas_Object*) looks enough.

Done.

> > Source/WebKit/efl/ewk/ewk_view.cpp:4519
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv);
> If this use only priv, why don't you just send priv as data.

Done.

> > Source/WebKit/efl/ewk/ewk_view.cpp:4527
> > +void _ewk_view_accelerated_compositing_context_create_if_needed(Evas_Object* ewkView)
> static?

My C++ habit. I've removed it.

> > Source/WebKit/efl/ewk/ewk_view.cpp:4566
> > +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* ewkView)
> This function returns GraphicsContext3D.

Any problem?

> > Source/WebKit/efl/ewk/ewk_view.cpp:4592
> > +        _ewk_view_accelerated_compositing_context_create_if_needed(ewkView);
> > +        evas_object_show(priv->compositingObject);
> > +    } else
> > +        evas_object_hide(priv->compositingObject);
> > +
> > +    priv->acceleratedCompositingContext->attachRootGraphicsLayer(rootLayer);
> acceleratedCompositingContext are used, irrespective of compositingActive

The acceleratedCompositingContext should always know the current root layer.
If compositingActive is false, because the root layer is 0, we should also pass 0 to acceleratedCompositingContext.

[Hyowon Kim]

(In reply to comment #17)
> (From update of attachment 170795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170795&action=review
> > Source/WebKit/efl/ewk/ewk_view.cpp:255
> > +    bool compositingActive;
> Do you really need this "compositingActive" . IMHO, it is enough to check that acceleratedCompositingContext exists.

The instance of ACContextEfl is created, the first time the root layer is attached.
If the root layer is 0, compositingActive is false but ACContextEfl still exists.
The compositingActive can be changed regardless of ACContextEfl.

[Hyowon Kim]

> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:596
> > -    notImplemented();
> > +    ewk_view_root_graphics_layer_set(m_view, rootLayer);
> Werent we moving to use C++ methods?
> > Source/WebKit/efl/ewk/ewk_view.cpp:4538
> > +
> > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* ewkView, Evas_Native_Surface* nativeSurface, const WebCore::IntRect& rect)
> > +{
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);
> Same...

Should we also apply that to WK1?
In ChromeClientEfl.cpp, many ewk_view_*() functions are called now.

[Kenneth Rohde Christiansen]

No, let's leave webkit1 for now, unless for new code I suppose

[Hyowon Kim]

Created attachment 170859 [details]
patch from comment #20

[Hyowon Kim]

> > Source/WebKit/efl/ewk/ewk_view.cpp:255
> > +    bool compositingActive;
> isCompositingActive

Done.

> > Source/WebKit/efl/ewk/ewk_view.cpp:960
> > +    if (priv->compositingObject)
> > +        evas_object_del(priv->compositingObject);
> use RefPtr they support evas objects

Done.

> > Source/WebKit/efl/ewk/ewk_view.cpp:4572
> > +void ewk_view_root_graphics_layer_set(Evas_Object* ewkView, WebCore::GraphicsLayer* rootLayer)
> Why cant these be in the EwkViewImpl ?

This patch is for WK1.

> > Source/WebKit/efl/ewk/ewk_view.cpp:4577
> > +    bool active = rootLayer ? true : false;
> = !!rootLayer

Done.

Thanks you for your review.

[WebKit Review Bot]

Comment on attachment 170859 [details]
patch from comment #20

Clearing flags on attachment: 170859

Committed r132615: <http://trac.webkit.org/changeset/132615>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.