Summary: | Move WebVideoFullscreenController from WebCore to WebKitLegacy. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, darin, eric.carlson, ews-watchlist, jeremyj-wk, jer.noble, mitz, ryanhaddad, simon.fraser, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2018-02-08 12:38:18 PST
Created attachment 333416 [details]
Patch
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.
Created attachment 333422 [details]
Patch
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.
Created attachment 333425 [details]
Patch
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.
Created attachment 333426 [details]
Patch
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 on attachment 333426 [details]
Patch
Thanks for reviewing!
Did you test that this works? Comment on attachment 333426 [details] Patch Clearing flags on attachment: 333426 Committed r228308: <https://trac.webkit.org/changeset/228308> All reviewed patches have been landed. Closing bug. 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. (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. (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. 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 (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> (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. (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. (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 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. |