WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
113912
[GTK][AC] Adding contentsLayer for image and video
https://bugs.webkit.org/show_bug.cgi?id=113912
Summary
[GTK][AC] Adding contentsLayer for image and video
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.
Top of Page
Format For Printing
XML
Clone This Bug