Bug 113912 - [GTK][AC] Adding contentsLayer for image and video
Summary: [GTK][AC] Adding contentsLayer for image and video
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joone Hur
URL:
Keywords:
Depends on: 113758
Blocks: 105699 114113
  Show dependency treegraph
 
Reported: 2013-04-03 17:42 PDT by Joone Hur
Modified: 2013-04-11 06:52 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 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.
Comment 1 Joone Hur 2013-04-04 16:17:36 PDT
Created attachment 196551 [details]
Patch
Comment 2 ChangSeok Oh 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.
Comment 3 Joone Hur 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.
Comment 4 Joone Hur 2013-04-05 10:35:14 PDT
Created attachment 196648 [details]
Patch
Comment 5 Gustavo Noronha (kov) 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.
Comment 6 Joone Hur 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.
Comment 7 Joone Hur 2013-04-09 16:26:26 PDT
Created attachment 197186 [details]
Patch
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2013-04-09 17:15:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Gustavo Noronha (kov) 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