Bug 56220 - Add full screen animation code to WebFullScreenManager.
Summary: Add full screen animation code to WebFullScreenManager.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on: 55993
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-11 13:47 PST by Jer Noble
Modified: 2011-03-11 21:43 PST (History)
4 users (show)

See Also:


Attachments
Patch (25.85 KB, patch)
2011-03-11 14:08 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (25.27 KB, patch)
2011-03-11 14:46 PST, Jer Noble
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-03-11 13:47:18 PST
Animation of the full screen element's CALayer must be done in the WebProcess, so add that code to WebFullScreenManager.
Comment 1 Jer Noble 2011-03-11 14:08:38 PST
Created attachment 85530 [details]
Patch
Comment 2 Jer Noble 2011-03-11 14:46:00 PST
Created attachment 85539 [details]
Patch

Modified the linker settings for new files in the .xcodeproj.
Comment 3 Anders Carlsson 2011-03-11 17:05:05 PST
Comment on attachment 85539 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        * WebProcess/FullScreen/WebFullScreenManager.cpp: Move functions into virtual base class.

I don't see a virtual base class here. Maybe this should say "Move virtual functions into base class"?

> Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:51
> +    

Extra newline.

> Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.h:38
> +typedef struct objc_object *id;

I don't think forward declaring id like this works with newer compilers. Could you forward declare WebFullScreenManagerAnimationListener instead and use it for the RetainPtr?

We have an OBJC_CLASS macro that is useful for forward declaring Objective-C classes.

> Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:88
> +    _manager = NULL;

= 0;

> Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:132
> +        m_rootLayer->setName("LayerTreeHost root layer");

This should say something else, maybe "Full screen root layer".
Comment 4 Jer Noble 2011-03-11 18:03:46 PST
(In reply to comment #3)
> (From update of attachment 85539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85539&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        * WebProcess/FullScreen/WebFullScreenManager.cpp: Move functions into virtual base class.
> 
> I don't see a virtual base class here. Maybe this should say "Move virtual functions into base class"?

Yes, this was incredibly poorly written.  How about, "Make functions pure virtual to be implemented by a concrete subclass."?

> > Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:51
> > +    
> 
> Extra newline.

Deleted.

> > Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.h:38
> > +typedef struct objc_object *id;
> 
> I don't think forward declaring id like this works with newer compilers. Could you forward declare WebFullScreenManagerAnimationListener instead and use it for the RetainPtr?
> 
> We have an OBJC_CLASS macro that is useful for forward declaring Objective-C classes.

OBJC_CLASS seems to work fine here.  Changed.

> > Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:88
> > +    _manager = NULL;
> 
> = 0;

Changed.

> > Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:132
> > +        m_rootLayer->setName("LayerTreeHost root layer");
> 
> This should say something else, maybe "Full screen root layer".

Suggestion taken.  Changed.

Thanks!
Comment 5 Jer Noble 2011-03-11 20:20:22 PST
Committed r80923: <http://trac.webkit.org/changeset/80923>
Comment 6 Jer Noble 2011-03-11 20:53:23 PST
Committed r80924: <http://trac.webkit.org/changeset/80924>
Comment 7 Jer Noble 2011-03-11 21:17:09 PST
Committed r80925: <http://trac.webkit.org/changeset/80925>
Comment 8 Jer Noble 2011-03-11 21:32:53 PST
webkit-patch added the above commit messages incorrectly, probably due to #56241.
Comment 9 Jer Noble 2011-03-11 21:43:18 PST
The correct message is the last one:

Committed r80925: <http://trac.webkit.org/changeset/80925>