[webkit-reviews] review granted: [Bug 60826] Video is blank, controller is misplaced on trailers.apple.com movie in fullscreen (with two screens) : [Attachment 93544] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 60826: Video is blank, controller is misplaced on trailers.apple.com movie
in fullscreen (with two screens)
https://bugs.webkit.org/show_bug.cgi?id=60826

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=93544&action=review

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1238
> +    PlatformLayer* layer = m_qtVideoLayer.get();
> +    do {
> +	   if (rootLayer != layer)
> +	       continue;
> +
> +	   // We own a child layer of a layer which has switched contexts.  
> +	   // Tear down our layer, and set m_visible to false, so that the 
> +	   // next time setVisible(true) is called, the layer will be re-
> +	   // created in the correct context.
> +	   tearDownVideoRendering();
> +	   m_visible = false;
> +	   break;
> +    } while((layer = [layer superlayer]));

This code is confusing. I think you want to have a loop that walks up the tree,
and to pull the tearDownVideoRendering() code out of the loop.

> Source/WebKit2/WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:145
> +	   [[NSNotificationCenter defaultCenter]
postNotificationName:@"CAContextChanged" object:nil userInfo:[NSDictionary
dictionaryWithObject:m_fullScreenRootLayer.get() forKey:@"RootLayer"]];
> +	   m_fullScreenRootLayer = 0;

You should add a comment to say who listens for this notification. I also think
you should use a more unique name than "CAContextChanged", since it would be
easy to imagine CA adding a notification with the same name in future.