[webkit-reviews] review granted: [Bug 56220] Add full screen animation code to WebFullScreenManager. : [Attachment 85539] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 56220: Add full screen animation code to WebFullScreenManager.
https://bugs.webkit.org/show_bug.cgi?id=56220

Attachment 85539: Patch
https://bugs.webkit.org/attachment.cgi?id=85539&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
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".