Bug 182615 – Move WebVideoFullscreenController from WebCore to WebKitLegacy.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

It is only used by WK1. Also there are a few references to NSApp in WebVideoFullscreenController.mm. Ideally, we should not reference NSApp in WebCore.

[Per Arne Vollan]

Created attachment 333416 [details]
Patch

[EWS Watchlist]

Attachment 333416 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:52:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:213:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:214:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:333:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:205:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:274:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:500:  The parameter name "videoElement" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:566:  Use std::max() or std::max<type>() instead of the MAX() macro.  [runtime/max_min_macros] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:211:  Use std::min() or std::min<type>() instead of the MIN() macro.  [runtime/max_min_macros] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:263:  Use std::max() or std::max<type>() instead of the MAX() macro.  [runtime/max_min_macros] [4]
ERROR: Source/WebCore/ChangeLog:11:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 16 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 333422 [details]
Patch

[EWS Watchlist]

Attachment 333422 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:500:  The parameter name "videoElement" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 333425 [details]
Patch

[EWS Watchlist]

Attachment 333425 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:499:  The parameter name "videoElement" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 333426 [details]
Patch

[EWS Watchlist]

Attachment 333426 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:499:  The parameter name "videoElement" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Comment on attachment 333426 [details]
Patch

Thanks for reviewing!

[Simon Fraser (smfr)]

Did you test that this works?

[WebKit Commit Bot]

Comment on attachment 333426 [details]
Patch

Clearing flags on attachment: 333426

Committed r228308: <https://trac.webkit.org/changeset/228308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37376454>

Comment on attachment 333426 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.h:47
> +WEBCORE_EXPORT @interface WebVideoFullscreenController : NSWindowController {

Why do we need WEBCORE_EXPORT here?

> Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:49
> +namespace WebCore {

Can we avoid defining things in the WebCore namespace in WebKitLegacy?

> Source/WebKitLegacy/mac/WebView/WebWindowAnimation.h:28
> +WEBCORE_EXPORT @interface WebWindowScaleAnimation : NSAnimation {

Why do we need WEBCORE_EXPORT here?

> Source/WebKitLegacy/mac/WebView/WebWindowAnimation.h:46
> +WEBCORE_EXPORT @interface WebWindowFadeAnimation : NSAnimation {

Ditto.

> Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:35
> +namespace WebCore {

Ditto.

[Per Arne Vollan]

(In reply to mitz from comment #14)
> Comment on attachment 333426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333426&action=review
> 
> > Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.h:47
> > +WEBCORE_EXPORT @interface WebVideoFullscreenController : NSWindowController {
> 
> Why do we need WEBCORE_EXPORT here?
> 
> > Source/WebKitLegacy/mac/WebView/WebVideoFullscreenHUDWindowController.mm:49
> > +namespace WebCore {
> 
> Can we avoid defining things in the WebCore namespace in WebKitLegacy?
> 
> > Source/WebKitLegacy/mac/WebView/WebWindowAnimation.h:28
> > +WEBCORE_EXPORT @interface WebWindowScaleAnimation : NSAnimation {
> 
> Why do we need WEBCORE_EXPORT here?
> 
> > Source/WebKitLegacy/mac/WebView/WebWindowAnimation.h:46
> > +WEBCORE_EXPORT @interface WebWindowFadeAnimation : NSAnimation {
> 
> Ditto.
> 
> > Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:35
> > +namespace WebCore {
> 
> Ditto.

Thanks! I'll follow up with an additional patch.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #10)
> Did you test that this works?

I thought this was covered by layout tests, but perhaps it is not? If not, I will test this manually.

[Ryan Haddad]

This change broke the Sierra 32-bit build:
/Volumes/Data/slave/sierra-32bit-release/build/Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:213:12: error: no matching function for call to 'min'

https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/8890

[Per Arne Vollan]

(In reply to Ryan Haddad from comment #17)
> This change broke the Sierra 32-bit build:
> /Volumes/Data/slave/sierra-32bit-release/build/Source/WebKitLegacy/mac/
> WebView/WebWindowAnimation.mm:213:12: error: no matching function for call
> to 'min'
> 
> https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-
> bit%20Build%29/builds/8890

Landed 32-bit build fix in <https://trac.webkit.org/changeset/228322/webkit>

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #16)
> (In reply to Simon Fraser (smfr) from comment #10)
> > Did you test that this works?
> 
> I thought this was covered by layout tests, but perhaps it is not? If not, I
> will test this manually.

Basic testing of WK1 video fullscreen in MiniBrowser, indicates that the behavior is the same as before.

[Simon Fraser (smfr)]

(In reply to Per Arne Vollan from comment #19)
> (In reply to Per Arne Vollan from comment #16)
> > (In reply to Simon Fraser (smfr) from comment #10)
> > > Did you test that this works?
> > 
> > I thought this was covered by layout tests, but perhaps it is not? If not, I
> > will test this manually.

No, there are no layout tests for this.

> 
> Basic testing of WK1 video fullscreen in MiniBrowser, indicates that the
> behavior is the same as before.

Does that actually hit this code? There are multiple variants of fullscreen; check with Jer.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #20)
> (In reply to Per Arne Vollan from comment #19)
> > (In reply to Per Arne Vollan from comment #16)
> > > (In reply to Simon Fraser (smfr) from comment #10)
> > > > Did you test that this works?
> > > 
> > > I thought this was covered by layout tests, but perhaps it is not? If not, I
> > > will test this manually.
> 
> No, there are no layout tests for this.
> 
> > 
> > Basic testing of WK1 video fullscreen in MiniBrowser, indicates that the
> > behavior is the same as before.
> 
> Does that actually hit this code? There are multiple variants of fullscreen;
> check with Jer.

Yes, hitting that code when going fullscreen in WK1, I'll check with Jer :)

[Darin Adler]

Comment on attachment 333426 [details]
Patch

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

q

> Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:213
> +    return std::min(sqrt(maxDist) * speedFactor, maxAdditionalDuration);

Using std::sqrt here would be better because it will do the right thing for both float and double; float -> float or double.-> double. I think the reason we needed to add a cast to CGFloat to fix the 32-bit build is that sqrt (the one without the std:: prefix) converts a float maxDist to a double.