Bug 82315

Summary:

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

Product:

WebKit

Reporter:

Hyowon Kim <hw1008.kim>

Component:

WebKit EFL

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dglazkov, gyuyoung.kim, gyuyoung.kim, igor.oliveira, kenneth, laszlo.gombos, lucas.de.marchi, noam, ostap73, rakuco, webkit.review.bot, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

79766, 88634

Attachments:

Description	Flags
Patch	none
Modified patch	none
updated patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02	none
rebased patch	none
rebased patch	none
patch from comment #16	none
patch from comment #20	none

Hyowon Kim

Reported 2012-03-27 02:29:56 PDT

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

Attachments
Patch (8.87 KB, patch) 2012-03-27 02:36 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Modified patch (8.84 KB, patch) 2012-03-28 19:08 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
updated patch (8.98 KB, patch) 2012-06-05 01:16 PDT, Hyowon Kim	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (741.48 KB, application/zip) 2012-06-05 10:38 PDT, WebKit Review Bot	no flags	Details
rebased patch (8.98 KB, patch) 2012-06-07 22:08 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
rebased patch (9.28 KB, patch) 2012-10-25 20:13 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
patch from comment #16 (9.17 KB, patch) 2012-10-26 01:32 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
patch from comment #20 (9.14 KB, patch) 2012-10-26 02:54 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Hyowon Kim

Comment 1 2012-03-27 02:36:05 PDT

Created attachment 134008 [details] Patch

Noam Rosenthal

Comment 2 2012-03-27 06:00:55 PDT

Comment on attachment 134008 [details] Patch If another EFL developer wants to sign off on this, LGTM

Raphael Kubo da Costa (:rakuco)

Comment 3 2012-03-27 07:57:33 PDT

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)

Comment 4 2012-03-27 07:58:14 PDT

(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

Comment 5 2012-03-28 19:08:53 PDT

Created attachment 134478 [details] Modified patch add null-check for evas_object(compositingObject) in _ewk_view_priv_del().

Hyowon Kim

Comment 6 2012-03-28 19:27:09 PDT

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

Comment 7 2012-06-05 01:16:02 PDT

Created attachment 145720 [details] updated patch

Noam Rosenthal

Comment 8 2012-06-05 06:36:31 PDT

Peer review first please :)

WebKit Review Bot

Comment 9 2012-06-05 10:38:26 PDT

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

Comment 10 2012-06-05 10:38:31 PDT

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

Comment 11 2012-06-07 22:08:53 PDT

Created attachment 146477 [details] rebased patch

Igor Trindade Oliveira

Comment 12 2012-06-07 22:15:55 PDT

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

Hyowon Kim

Comment 13 2012-06-08 02:23:17 PDT

(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 14 2012-09-18 04:03:09 PDT

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

Comment 15 2012-10-25 20:13:10 PDT

Created attachment 170795 [details] rebased patch I've attached the rebased patch again. I kindly ask for review and comments.

Ryuan Choi

Comment 16 2012-10-25 21:16:56 PDT

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 17 2012-10-25 23:16:16 PDT

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 18 2012-10-26 01:08:01 PDT

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

Comment 19 2012-10-26 01:32:18 PDT

Created attachment 170846 [details] patch from comment #16

Kenneth Rohde Christiansen

Comment 20 2012-10-26 01:48:38 PDT

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

Comment 21 2012-10-26 01:53:00 PDT

(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

Comment 22 2012-10-26 01:54:25 PDT

(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

Comment 23 2012-10-26 02:14:00 PDT

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

Comment 24 2012-10-26 02:22:33 PDT

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

Hyowon Kim

Comment 25 2012-10-26 02:54:52 PDT

Created attachment 170859 [details] patch from comment #20

Hyowon Kim

Comment 26 2012-10-26 02:57:46 PDT

> > 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 27 2012-10-26 04:21:43 PDT

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

Comment 28 2012-10-26 04:21:49 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.