Bug 174437 - Remove Web prefix from WebVideoFullscreen and WebPlaybackSession classes.
Summary: Remove Web prefix from WebVideoFullscreen and WebPlaybackSession classes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-12 15:44 PDT by Jeremy Jones
Modified: 2017-11-02 11:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (712.62 KB, patch)
2017-07-12 16:59 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Better diff. No filename change. (410.93 KB, patch)
2017-07-12 17:17 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (712.61 KB, patch)
2017-07-13 11:17 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Better diff. No filename change. (410.93 KB, patch)
2017-07-13 11:21 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (712.06 KB, patch)
2017-07-13 12:20 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Better diff. No filename change. (405.74 KB, patch)
2017-07-13 12:23 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Part1: File name changes. (625.37 KB, patch)
2017-07-13 18:19 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Part1: File name changes. (625.16 KB, patch)
2017-07-13 19:15 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (713.05 KB, patch)
2017-07-21 16:15 PDT, Jeremy Jones
darin: review+
Details | Formatted Diff | Diff
Patch that matches renamed files for easier viewing. (408.15 KB, patch)
2017-07-21 16:17 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (677.75 KB, patch)
2017-07-24 15:11 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (677.71 KB, patch)
2017-07-27 20:51 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (677.89 KB, patch)
2017-07-27 22:44 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2017-07-12 15:44:26 PDT
Remove Web prefix from WebVideoFullscreen and WebPlaybackSession classes.
Comment 1 Jeremy Jones 2017-07-12 15:45:49 PDT
rdar://problem/33227420
Comment 2 Jeremy Jones 2017-07-12 16:59:15 PDT
Created attachment 315309 [details]
Patch
Comment 3 Build Bot 2017-07-12 17:01:52 PDT
Attachment 315309 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450:  Using +alloc with a soft-linked class. Use allocWebAVPictureInPicturePlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619:  Using +alloc with a soft-linked class. Use allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jeremy Jones 2017-07-12 17:17:37 PDT
Created attachment 315313 [details]
Better diff. No filename change.
Comment 5 Jeremy Jones 2017-07-13 11:17:17 PDT
Created attachment 315359 [details]
Patch
Comment 6 Build Bot 2017-07-13 11:20:26 PDT
Attachment 315359 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450:  Using +alloc with a soft-linked class. Use allocWebAVPictureInPicturePlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619:  Using +alloc with a soft-linked class. Use allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Jeremy Jones 2017-07-13 11:21:16 PDT
Created attachment 315360 [details]
Better diff. No filename change.
Comment 8 Jeremy Jones 2017-07-13 12:20:27 PDT
Created attachment 315367 [details]
Patch
Comment 9 Jeremy Jones 2017-07-13 12:23:20 PDT
Created attachment 315368 [details]
Better diff. No filename change.
Comment 10 Build Bot 2017-07-13 12:23:57 PDT
Attachment 315367 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450:  Using +alloc with a soft-linked class. Use allocWebAVPictureInPicturePlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619:  Using +alloc with a soft-linked class. Use allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Darin Adler 2017-07-13 13:43:09 PDT
Sometimes you can do renames like this with do-webcore-rename, as long as the same name is not used for something else.
Comment 12 Darin Adler 2017-07-13 13:43:37 PDT
(In reply to Build Bot from comment #10)
> ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450: 
> Using +alloc with a soft-linked class. Use
> allocWebAVPictureInPicturePlayerLayerViewInstance() instead. 
> [runtime/soft-linked-alloc] [4]
> ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619: 
> Using +alloc with a soft-linked class. Use
> allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]

Maybe these are good warnings?
Comment 13 Jeremy Jones 2017-07-13 13:51:13 PDT
(In reply to Darin Adler from comment #12)
> (In reply to Build Bot from comment #10)
> > ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450: 
> > Using +alloc with a soft-linked class. Use
> > allocWebAVPictureInPicturePlayerLayerViewInstance() instead. 
> > [runtime/soft-linked-alloc] [4]
> > ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619: 
> > Using +alloc with a soft-linked class. Use
> > allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
> 
> Maybe these are good warnings?

These aren't actually soft-linked. This is actually an obj-c class that is dynamically generated. The function name happens to match the naming scheme used for soft-linking.

I could create and use an alloc version, but the implementation of that would probably just get this style warning.

The correct fix is probably to find a way to not dynamically generate the class. That might involve moving the class into a different framework.
Comment 14 Jeremy Jones 2017-07-13 13:55:44 PDT
(In reply to Jeremy Jones from comment #13)
> (In reply to Darin Adler from comment #12)
> > (In reply to Build Bot from comment #10)
> > > ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450: 
> > > Using +alloc with a soft-linked class. Use
> > > allocWebAVPictureInPicturePlayerLayerViewInstance() instead. 
> > > [runtime/soft-linked-alloc] [4]
> > > ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619: 
> > > Using +alloc with a soft-linked class. Use
> > > allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
> > 
> > Maybe these are good warnings?
> 
> These aren't actually soft-linked. This is actually an obj-c class that is
> dynamically generated. The function name happens to match the naming scheme
> used for soft-linking.
> 
> I could create and use an alloc version, but the implementation of that
> would probably just get this style warning.
> 
> The correct fix is probably to find a way to not dynamically generate the
> class. That might involve moving the class into a different framework.

Also, my patch doesn't really touch those lines. It is just that the diff doesn't realize the file has been renamed, and thinks I've re-added the whole file. Notice that the other patch without file name changes passes style.
Comment 15 Darin Adler 2017-07-13 14:03:10 PDT
(In reply to Jeremy Jones from comment #13)
> These aren't actually soft-linked. This is actually an obj-c class that is
> dynamically generated. The function name happens to match the naming scheme
> used for soft-linking.

OK. Should we change the naming scheme?

(In reply to Jeremy Jones from comment #14)
> Also, my patch doesn't really touch those lines.

Yes, I was aware of that.
Comment 16 Jeremy Jones 2017-07-13 15:38:07 PDT
(In reply to Darin Adler from comment #15)
> (In reply to Jeremy Jones from comment #13)
> > These aren't actually soft-linked. This is actually an obj-c class that is
> > dynamically generated. The function name happens to match the naming scheme
> > used for soft-linking.
> 
> OK. Should we change the naming scheme?

I'll handle this in a separate bug. And I'll break up this change into #1 rename files and #includes, #2 change class names.
Comment 17 Jeremy Jones 2017-07-13 15:50:25 PDT
(In reply to Jeremy Jones from comment #16)
> (In reply to Darin Adler from comment #15)
> > (In reply to Jeremy Jones from comment #13)
> > > These aren't actually soft-linked. This is actually an obj-c class that is
> > > dynamically generated. The function name happens to match the naming scheme
> > > used for soft-linking.
> > 
> > OK. Should we change the naming scheme?
> 
> I'll handle this in a separate bug. And I'll break up this change into #1
> rename files and #includes, #2 change class names.

I've renamed the function here and made it do an alloc.
https://bugs.webkit.org/show_bug.cgi?id=174476
Comment 18 Jeremy Jones 2017-07-13 18:19:31 PDT
Created attachment 315390 [details]
Part1: File name changes.
Comment 19 Build Bot 2017-07-13 18:29:35 PDT
Attachment 315390 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:337:  The parameter name "playbackSessionModel" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:450:  Using +alloc with a soft-linked class. Use allocWebAVPictureInPicturePlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:619:  Using +alloc with a soft-linked class. Use allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
Total errors found: 5 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Jeremy Jones 2017-07-13 19:15:49 PDT
Created attachment 315395 [details]
Part1: File name changes.
Comment 21 Jeremy Jones 2017-07-21 16:15:33 PDT
Created attachment 316134 [details]
Patch
Comment 22 Jeremy Jones 2017-07-21 16:17:53 PDT
Created attachment 316136 [details]
Patch that matches renamed files for easier viewing.
Comment 23 Build Bot 2017-07-21 16:19:13 PDT
Attachment 316134 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:337:  The parameter name "playbackSessionModel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Darin Adler 2017-07-21 21:03:15 PDT
Comment on attachment 316134 [details]
Patch

No real reason to leave the giant change log listing all the function names.

Not sure why the iOS and iOS Simulator EWS bots say the build failed.
Comment 25 Jeremy Jones 2017-07-24 15:11:47 PDT
(In reply to Darin Adler from comment #24)
> Comment on attachment 316134 [details]
> Patch
> 
> No real reason to leave the giant change log listing all the function names.

Removed.

> 
> Not sure why the iOS and iOS Simulator EWS bots say the build failed.

Fixed 1-character typo.
Comment 26 Jeremy Jones 2017-07-24 15:11:53 PDT
Created attachment 316322 [details]
Patch for landing.
Comment 27 Jeremy Jones 2017-07-24 15:12:34 PDT
(In reply to Jeremy Jones from comment #25)
> (In reply to Darin Adler from comment #24)
> > Comment on attachment 316134 [details]
> > Patch
> > 
> > No real reason to leave the giant change log listing all the function names.
> 
> Removed.
> 
> > 
> > Not sure why the iOS and iOS Simulator EWS bots say the build failed.
> 
> Fixed 1-character typo.

I'll confirm the patch passes EWS before landing.
Comment 28 Jeremy Jones 2017-07-27 20:51:47 PDT
Created attachment 316609 [details]
Patch for landing.
Comment 29 Build Bot 2017-07-27 21:22:08 PDT
Attachment 316609 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:337:  The parameter name "playbackSessionModel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Jeremy Jones 2017-07-27 22:44:24 PDT
Created attachment 316620 [details]
Patch for landing.
Comment 31 Build Bot 2017-07-27 22:49:01 PDT
Attachment 316620 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:256:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:283:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:337:  The parameter name "playbackSessionModel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 WebKit Commit Bot 2017-07-28 00:25:46 PDT
Comment on attachment 316620 [details]
Patch for landing.

Clearing flags on attachment: 316620

Committed r219996: <http://trac.webkit.org/changeset/219996>
Comment 33 Csaba Osztrogonác 2017-08-16 06:24:59 PDT
(In reply to WebKit Commit Bot from comment #32)
> Comment on attachment 316620 [details]
> Patch for landing.
> 
> Clearing flags on attachment: 316620
> 
> Committed r219996: <http://trac.webkit.org/changeset/219996>

It broke the Apple Mac cmake build 3 weeks ago:


CMake Error in Source/WebKit/CMakeLists.txt:
  Cannot find source file:

    UIProcess/Cocoa/WebVideoFullscreenManagerProxy.messages.in


Is Apple Mac make cmake build still maintained or should we remove it 
from the trunk if it is completely dead project?