Bug 113912

Summary:

[GTK][AC] Adding contentsLayer for image and video

Product:

WebKit

Reporter:

Joone Hur <joone>

Component:

WebKitGTK

Assignee:

Joone Hur <joone>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, gustavo, kevin.cs.oh

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

113758

Bug Blocks:

105699, 114113

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Joone Hur

Reported 2013-04-03 17:42:24 PDT

We need to add m_contentsLayer to GraphicsLayerClutter in order to render an image or video on a different ClutterActor. The patch is coming soon.

Attachments
Patch (10.71 KB, patch) 2013-04-04 16:17 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (10.72 KB, patch) 2013-04-05 10:35 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (10.65 KB, patch) 2013-04-09 16:26 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Joone Hur

Comment 1 2013-04-04 16:17:36 PDT

Created attachment 196551 [details] Patch

ChangSeok Oh

Comment 2 2013-04-05 02:18:20 PDT

I leave some notes as an informal review. Please just refer to those. :) > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:452 > + graphicsLayerActorInvalidateRectangle(m_contentsLayer.get(), FloatRect(FloatPoint(0, 0), size())); I believe we don't need to call this here. Because we set ContentsNeedsDisplay below so above lines will be called in updateContentsNeedsDisplay(). > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:701 > + updateContentsRect(); We would be better to follow macport's order for calling above lines to avoid conflicts that might occur in the future. > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:716 > + updateContentsNeedsDisplay(); ditto.

Joone Hur

Comment 3 2013-04-05 10:06:39 PDT

(In reply to comment #2) > I leave some notes as an informal review. Please just refer to those. :) > > > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:452 > > + graphicsLayerActorInvalidateRectangle(m_contentsLayer.get(), FloatRect(FloatPoint(0, 0), size())); > > I believe we don't need to call this here. Because we set ContentsNeedsDisplay below so above lines will be called in updateContentsNeedsDisplay(). Yes, right. graphicsLayerActorInvalidateRectangle is called twice. I will fix it. > > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:701 > > + updateContentsRect(); > > We would be better to follow macport's order for calling above lines to avoid conflicts that might occur in the future. > > > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:716 > > + updateContentsNeedsDisplay(); > > ditto. Yes, it seems more safe to follow the Mac port's layer change order.

Joone Hur

Comment 4 2013-04-05 10:35:14 PDT

Created attachment 196648 [details] Patch

Gustavo Noronha (kov)

Comment 5 2013-04-08 14:57:07 PDT

Comment on attachment 196648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196648&action=review > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:743 > +#ifndef NDEBUG > + clutter_actor_set_name(CLUTTER_ACTOR(m_contentsLayer.get()), "Image Layer"); > +#endif I think we should set the name regardless of debug mode, it's not like this adds any overhead, right? > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:220 > + // FIXME: Need to use RetainPtr This FIXME makes no sense.

Joone Hur

Comment 6 2013-04-09 10:42:26 PDT

Comment on attachment 196648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196648&action=review >> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:743 >> +#endif > > I think we should set the name regardless of debug mode, it's not like this adds any overhead, right? But, ClutterActor name is only useful to print out a PlatformLayer tree for debugging. Is there any case where this is useful in release build? >> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:220 >> + // FIXME: Need to use RetainPtr > > This FIXME makes no sense. Yes, RetainPtr is a specific smart pointer class of the Mac port. I will remove this comment.

Joone Hur

Comment 7 2013-04-09 16:26:26 PDT

Created attachment 197186 [details] Patch

WebKit Commit Bot

Comment 8 2013-04-09 17:15:37 PDT

Comment on attachment 197186 [details] Patch Clearing flags on attachment: 197186 Committed r148069: <http://trac.webkit.org/changeset/148069>

WebKit Commit Bot

Comment 9 2013-04-09 17:15:39 PDT

All reviewed patches have been landed. Closing bug.

Gustavo Noronha (kov)

Comment 10 2013-04-11 06:52:48 PDT

(In reply to comment #6) > But, ClutterActor name is only useful to print out a PlatformLayer tree for debugging. > Is there any case where this is useful in release build? Yeah, it's useful for running with CLUTTER_PAINT=paint-volumes

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