Bug 174437 – Remove Web prefix from WebVideoFullscreen and WebPlaybackSession classes.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jeremy Jones]

Remove Web prefix from WebVideoFullscreen and WebPlaybackSession classes.

[Jeremy Jones]

rdar://problem/33227420

[Jeremy Jones]

Created attachment 315309 [details]
Patch

[Build Bot]

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.

[Jeremy Jones]

Created attachment 315313 [details]
Better diff. No filename change.

[Jeremy Jones]

Created attachment 315359 [details]
Patch

[Build Bot]

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.

[Jeremy Jones]

Created attachment 315360 [details]
Better diff. No filename change.

[Jeremy Jones]

Created attachment 315367 [details]
Patch

[Jeremy Jones]

Created attachment 315368 [details]
Better diff. No filename change.

[Build Bot]

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.

[Darin Adler]

Sometimes you can do renames like this with do-webcore-rename, as long as the same name is not used for something else.

[Darin Adler]

(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?

[Jeremy Jones]

(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.

[Jeremy Jones]

(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.

[Darin Adler]

(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.

[Jeremy Jones]

(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.

[Jeremy Jones]

(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

[Jeremy Jones]

Created attachment 315390 [details]
Part1: File name changes.

[Build Bot]

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.

[Jeremy Jones]

Created attachment 315395 [details]
Part1: File name changes.

[Jeremy Jones]

Created attachment 316134 [details]
Patch

[Jeremy Jones]

Created attachment 316136 [details]
Patch that matches renamed files for easier viewing.

[Build Bot]

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.

[Darin Adler]

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.

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 316322 [details]
Patch for landing.

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 316609 [details]
Patch for landing.

[Build Bot]

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.

[Jeremy Jones]

Created attachment 316620 [details]
Patch for landing.

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 316620 [details]
Patch for landing.

Clearing flags on attachment: 316620

Committed r219996: <http://trac.webkit.org/changeset/219996>

[Csaba Osztrogonác]

(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?