Bug 182615 - Move WebVideoFullscreenController from WebCore to WebKitLegacy.
Summary: Move WebVideoFullscreenController from WebCore to WebKitLegacy.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-08 12:38 PST by Per Arne Vollan
Modified: 2018-03-01 15:53 PST (History)
12 users (show)

See Also:


Attachments
Patch (147.08 KB, patch)
2018-02-08 13:14 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (151.42 KB, patch)
2018-02-08 14:31 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (152.20 KB, patch)
2018-02-08 14:37 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (152.24 KB, patch)
2018-02-08 14:48 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-02-08 12:38:18 PST
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.
Comment 1 Per Arne Vollan 2018-02-08 13:14:55 PST
Created attachment 333416 [details]
Patch
Comment 2 Build Bot 2018-02-08 13:17:18 PST
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.
Comment 3 Per Arne Vollan 2018-02-08 14:31:48 PST
Created attachment 333422 [details]
Patch
Comment 4 Build Bot 2018-02-08 14:34:01 PST
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.
Comment 5 Per Arne Vollan 2018-02-08 14:37:29 PST
Created attachment 333425 [details]
Patch
Comment 6 Build Bot 2018-02-08 14:39:52 PST
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.
Comment 7 Per Arne Vollan 2018-02-08 14:48:58 PST
Created attachment 333426 [details]
Patch
Comment 8 Build Bot 2018-02-08 14:51:20 PST
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.
Comment 9 Per Arne Vollan 2018-02-08 19:05:54 PST
Comment on attachment 333426 [details]
Patch

Thanks for reviewing!
Comment 10 Simon Fraser (smfr) 2018-02-08 19:26:43 PST
Did you test that this works?
Comment 11 WebKit Commit Bot 2018-02-08 19:30:51 PST
Comment on attachment 333426 [details]
Patch

Clearing flags on attachment: 333426

Committed r228308: <https://trac.webkit.org/changeset/228308>
Comment 12 WebKit Commit Bot 2018-02-08 19:30:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-02-08 19:31:37 PST
<rdar://problem/37376454>
Comment 14 mitz 2018-02-08 19:48:15 PST
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.
Comment 15 Per Arne Vollan 2018-02-09 08:08:59 PST
(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.
Comment 16 Per Arne Vollan 2018-02-09 08:47:25 PST
(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.
Comment 17 Ryan Haddad 2018-02-09 09:13:11 PST
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
Comment 18 Per Arne Vollan 2018-02-09 09:28:24 PST
(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>
Comment 19 Per Arne Vollan 2018-02-09 11:14:42 PST
(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.
Comment 20 Simon Fraser (smfr) 2018-02-09 12:20:43 PST
(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.
Comment 21 Per Arne Vollan 2018-02-09 12:27:50 PST
(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 :)
Comment 22 Darin Adler 2018-03-01 15:53:56 PST
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.