Bug 143674

Summary: [Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a HTMLVideoElement.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 143680    
Attachments:
Description Flags
Patch
none
Patch
darin: review+
Patch for landing none

Jer Noble
Reported 2015-04-13 12:32:01 PDT
[Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a HTMLVideoElement.
Attachments
Patch (12.53 KB, patch)
2015-04-13 12:39 PDT, Jer Noble
no flags
Patch (12.87 KB, patch)
2015-04-14 14:26 PDT, Jer Noble
darin: review+
Patch for landing (18.80 KB, patch)
2015-04-15 12:09 PDT, Jer Noble
no flags
Jer Noble
Comment 1 2015-04-13 12:39:28 PDT
Sam Weinig
Comment 2 2015-04-14 08:51:38 PDT
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
Comment 3 2015-04-14 08:57:03 PDT
(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 4 2015-04-14 09:58:03 PDT
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
Comment 5 2015-04-14 14:26:50 PDT
Darin Adler
Comment 6 2015-04-14 16:39:27 PDT
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
Comment 7 2015-04-14 16:46:33 PDT
(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
Comment 8 2015-04-15 12:09:34 PDT
Created attachment 250830 [details] Patch for landing Made the matching change (* -> &) to enterVideoFullscreenForElement()
Jer Noble
Comment 9 2015-04-15 14:00:48 PDT
Note You need to log in before you can comment on or make changes to this bug.