Bug 143674 – [Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a HTMLVideoElement.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

[Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a HTMLVideoElement.

[Jer Noble]

Created attachment 250665 [details]
Patch

[Sam Weinig]

Comment on attachment 250665 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:336
> +    virtual void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*) { }

Is it ever null? If not, can we pass it by reference?

[Jer Noble]

(In reply to comment #2)
> Comment on attachment 250665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250665&action=review
> 
> > Source/WebCore/page/ChromeClient.h:336
> > +    virtual void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*) { }
> 
> Is it ever null? If not, can we pass it by reference?


Sure can!

[Darin Adler]

Comment on attachment 250665 [details]
Patch

iOS error: WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:222:9: error: use of undeclared identifier 'exitVideoFullscreen'
Windows error: win\WebCoreSupport\WebChromeClient.cpp(776): error C2275: 'WebCore::HTMLVideoElement' : illegal use of this type as an expression

Regressions: Unexpected crashes (1)
  compositing/video/video-clip-change-src.html [ Crash ]

Regressions: Unexpected missing results (1)
  compositing/geometry/fixed-transformed.html [ Missing ]

[Jer Noble]

Created attachment 250740 [details]
Patch

[Darin Adler]

Comment on attachment 250740 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:336
> +    virtual void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*) { }

Should be a reference, not a pointer. Didn’t you and Sam already agree to that?

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:183
> +    virtual void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*) override;

Ditto.

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:774
> +void WebChromeClient::exitVideoFullscreenForVideoElement(HTMLVideoElement* videoElement)

Ditto.

> Source/WebKit/win/WebCoreSupport/WebChromeClient.h:150
> +    virtual void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*);

Ditto.

> Source/WebKit/win/WebView.h:948
> +    void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*);

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:920
> +void WebChromeClient::exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement* videoElement)

Ditto.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:64
> +    void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*);

Ditto.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:110
> +void WebVideoFullscreenManager::exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*)

Ditto.

[Jer Noble]

(In reply to comment #6)
> Comment on attachment 250740 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250740&action=review
> 
> > Source/WebCore/page/ChromeClient.h:336
> > +    virtual void exitVideoFullscreenForVideoElement(WebCore::HTMLVideoElement*) { }
> 
> Should be a reference, not a pointer. Didn’t you and Sam already agree to
> that?

I did! I was midway through updating all the various ports, but my machine panic'd. I'll definitely do that before ii land.

[Jer Noble]

Created attachment 250830 [details]
Patch for landing

Made the matching change (* -> &) to enterVideoFullscreenForElement()

[Jer Noble]

Committed r182860: <http://trac.webkit.org/changeset/182860>