Bug 143674 - [Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a HTMLVideoElement.
Summary: [Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a H...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks: 143680
  Show dependency tree / graph
 
Reported: 2015-04-13 12:32 PDT by Jer Noble
Modified: 2015-04-15 14:00 PDT (History)
1 user (show)

See Also:

Attachments
Patch (12.53 KB, patch)
2015-04-13 12:39 PDT, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2015-04-14 14:26 PDT, Jer Noble 		darin: review+
Details | Formatted Diff | Diff
Patch for landing (18.80 KB, patch)
2015-04-15 12:09 PDT, Jer Noble 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2015-04-13 12:32:01 PDT 
[Fullscreen] ChromeClient::exitVideoFullscreen() should take a pointer to a HTMLVideoElement.
Comment 1 Jer Noble 2015-04-13 12:39:28 PDT 
Created attachment 250665 [details]
Patch
Comment 2 Sam Weinig 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?
Comment 3 Jer Noble 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!
Comment 4 Darin Adler 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 ]
Comment 5 Jer Noble 2015-04-14 14:26:50 PDT 
Created attachment 250740 [details]
Patch
Comment 6 Darin Adler 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.
Comment 7 Jer Noble 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.
Comment 8 Jer Noble 2015-04-15 12:09:34 PDT 
Created attachment 250830 [details]
Patch for landing

Made the matching change (* -> &) to enterVideoFullscreenForElement()
Comment 9 Jer Noble 2015-04-15 14:00:48 PDT 
Committed r182860: <http://trac.webkit.org/changeset/182860>