Comment on attachment 390594[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=390594&action=review> Source/WebCore/ChangeLog:8
> + Add an interface to MediaPlayer to set the frame of a video in the inline mode.
"set the frame" is ambiguous when used in the context of video.
> Source/WebCore/platform/graphics/MediaPlayer.h:332
> + void setVideoInlineFrame(const FloatRect&);
Is this really a frame, or just a size? Does it ever have non-zero origin? Maybe call it setSize which would be much less confusing.
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:470
> + // Initially the size of the platformLayer will be 0 because we do not provide mediaPlayerContentBoxRect() in this class.
A size will be 0x0
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:473
> + // Since the renderer in the Web process will take care of the video layers configurations,
video layer's configuration
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:474
> + // the RemoteMediaPlayerProxy does not need to manage the layers by itself.
This is a rather long-winded comment. Maybe prune it down to the minimum that will help a future you remember why the code is this way.
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:515
> +void RemoteMediaPlayerProxy::setVideoInlineFrameFenced(const FloatRect bounds, IPC::Attachment fencePort)
const FloatRect& or FloatRect but not const FloatRect
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:521
> + m_player->setVideoInlineFrame(bounds);
"bounds" and "frame" have different meanings in AppKit/UIKit. "frame" is in the coordinate space of the superview. "bounds" establishes your local coordinate system Don't mix and match them.
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:112
> + void setVideoInlineFrameFenced(const WebCore::FloatRect bounds, IPC::Attachment);
Please use MachPort, not IPC::Attachment (existing code also needs to be migrated).
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:60
> + SetVideoInlineFrameFenced(WebCore::FloatRect bounds, IPC::Attachment fencePort)
MachPort
> Source/WebKit/WebKit.xcodeproj/project.pbxproj:2685
> + 1D32F89B23A84BA600B1EA6A /* RemoteMediaResourceProxy.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RemoteMediaResourceProxy.h; sourceTree = "<group>"; tabWidth = 4; };
> + 1D32F89D23A84C5B00B1EA6A /* RemoteMediaResourceProxy.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RemoteMediaResourceProxy.cpp; sourceTree = "<group>"; tabWidth = 4; };
Why tabWidth
> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1009
> +void MediaPlayerPrivateRemote::setVideoInlineFrameFenced(const FloatRect& rect, mach_port_name_t fencePort)
MachPort
> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:123
> + void setVideoInlineFrameFenced(const WebCore::FloatRect&, mach_port_name_t);
MachPort
> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:233
> + void setSize(const WebCore::IntSize&) final { };
No semicolon
> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:38
> +using LayerHostingContextID = uint32_t;
Pull in LayerHostingContext.h?
> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:40
> +RetainPtr<CALayer> createVideoLayerRemote(MediaPlayerPrivateRemote*, LayerHostingContextID);
This is cocoa code inside ENABLE(GPU_PROCESS). I think we should assume that other platforms will want to build with ENABLE(GPU_PROCESS), so you need #if PLATFORM(COCOA) as well.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:34
> +@interface WKVideoLayerRemote : CALayer
PLATFORM(COCOA)
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:36
> +@property (nonatomic, retain) CALayer *videoSublayer;
Layers retain their sublayers already. Does this need to be retain?
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:37
> +@property CGRect videoLayerFrame;
nonatomic?
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:100
> + [self performSelector:@selector(resolveBounds) withObject:nil afterDelay:animationDuration + 0.1];
Noooooo. Source of test flakiness for decades to come!
Also please don't use performSelector:withObject:afterDelay:. Use an explicit timer, or dispatch with a block. Then you don't need all the cancelPreviousPerformRequestsWithTarget.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:121
> + _context = nil;
Why is the context nulled out here?
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123
> + mach_port_name_t fencePort = _fenceSendRight.leakSendRight();
> + mediaPlayerPrivateRemote->setVideoInlineFrameFenced(self.videoLayerFrame, fencePort);
MachPort cleans this up.
Comment on attachment 390594[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=390594&action=review>> Source/WebCore/ChangeLog:8
>> + Add an interface to MediaPlayer to set the frame of a video in the inline mode.
>
> "set the frame" is ambiguous when used in the context of video.
Right, actually what we need to set is the size of the video container layer.
Will update the comment.
>> Source/WebCore/platform/graphics/MediaPlayer.h:332
>> + void setVideoInlineFrame(const FloatRect&);
>
> Is this really a frame, or just a size? Does it ever have non-zero origin? Maybe call it setSize which would be much less confusing.
Just a size. There is a setSize() function in MediaPlayer class. In the new patch, we use its implementation in MediaPlayerPrivate to set the video container layer size when the media in GPU process feature is on (video layers are hosted remotely). For the case that the media in GPU process feature is off, that function should do nothing. So we have to add a flag to the MediaPlayerPrivate class to decide which behavior should choose.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:470
>> + // Initially the size of the platformLayer will be 0 because we do not provide mediaPlayerContentBoxRect() in this class.
>
> A size will be 0x0
Fixed.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:473
>> + // Since the renderer in the Web process will take care of the video layers configurations,
>
> video layer's configuration
Removed.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:474
>> + // the RemoteMediaPlayerProxy does not need to manage the layers by itself.
>
> This is a rather long-winded comment. Maybe prune it down to the minimum that will help a future you remember why the code is this way.
Removed.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:515
>> +void RemoteMediaPlayerProxy::setVideoInlineFrameFenced(const FloatRect bounds, IPC::Attachment fencePort)
>
> const FloatRect& or FloatRect but not const FloatRect
Fixed.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:521
>> + m_player->setVideoInlineFrame(bounds);
>
> "bounds" and "frame" have different meanings in AppKit/UIKit. "frame" is in the coordinate space of the superview. "bounds" establishes your local coordinate system Don't mix and match them.
Right, fixed.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:112
>> + void setVideoInlineFrameFenced(const WebCore::FloatRect bounds, IPC::Attachment);
>
> Please use MachPort, not IPC::Attachment (existing code also needs to be migrated).
Fixed. Will migrate the exiting code on the video fullscreen API in webkit.org/b/207683.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:60
>> + SetVideoInlineFrameFenced(WebCore::FloatRect bounds, IPC::Attachment fencePort)
>
> MachPort
Fixed.
>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:2685
>> + 1D32F89D23A84C5B00B1EA6A /* RemoteMediaResourceProxy.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RemoteMediaResourceProxy.cpp; sourceTree = "<group>"; tabWidth = 4; };
>
> Why tabWidth
Because of some misunderstanding :-)
Fixed.
>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1009
>> +void MediaPlayerPrivateRemote::setVideoInlineFrameFenced(const FloatRect& rect, mach_port_name_t fencePort)
>
> MachPort
Fixed.
>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:123
>> + void setVideoInlineFrameFenced(const WebCore::FloatRect&, mach_port_name_t);
>
> MachPort
Fixed.
>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:233
>> + void setSize(const WebCore::IntSize&) final { };
>
> No semicolon
Fixed.
>> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:38
>> +using LayerHostingContextID = uint32_t;
>
> Pull in LayerHostingContext.h?
Right. Added the header file.
>> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:40
>> +RetainPtr<CALayer> createVideoLayerRemote(MediaPlayerPrivateRemote*, LayerHostingContextID);
>
> This is cocoa code inside ENABLE(GPU_PROCESS). I think we should assume that other platforms will want to build with ENABLE(GPU_PROCESS), so you need #if PLATFORM(COCOA) as well.
Right, fixed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:34
>> +@interface WKVideoLayerRemote : CALayer
>
> PLATFORM(COCOA)
Fixed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:36
>> +@property (nonatomic, retain) CALayer *videoSublayer;
>
> Layers retain their sublayers already. Does this need to be retain?
Not need to be retain.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:37
>> +@property CGRect videoLayerFrame;
>
> nonatomic?
Right.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:100
>> + [self performSelector:@selector(resolveBounds) withObject:nil afterDelay:animationDuration + 0.1];
>
> Noooooo. Source of test flakiness for decades to come!
>
> Also please don't use performSelector:withObject:afterDelay:. Use an explicit timer, or dispatch with a block. Then you don't need all the cancelPreviousPerformRequestsWithTarget.
Changed to dispatch_after().
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:121
>> + _context = nil;
>
> Why is the context nulled out here?
Removed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123
>> + mediaPlayerPrivateRemote->setVideoInlineFrameFenced(self.videoLayerFrame, fencePort);
>
> MachPort cleans this up.
Done.
Comment on attachment 390727[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=390727&action=review> Source/WebCore/platform/graphics/MediaPlayer.h:331
> + void setHostVideoLayerRemotely(bool);
setHostsVideoLayerRemotely here and everywhere.
> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:326
> + bool hostVideoLayerRemotely() { return m_hostVideoLayerRemotely; }
hostsVideoLayerRemotely
> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378
> + bool m_hostVideoLayerRemotely { false };
m_hostsVideoLayerRemotely
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:520
> + deallocateSendRightSafely(machPort.port());
I think you should be using MachSendRight so you don't have to call this manually. Tim? Maybe we meant to say use MachSendRight rather than MachPort.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:109
> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock);
What is this 100ms for?
Comment on attachment 390727[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=390727&action=review>> Source/WebCore/platform/graphics/MediaPlayer.h:331
>> + void setHostVideoLayerRemotely(bool);
>
> setHostsVideoLayerRemotely here and everywhere.
OK, will fix it.
>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:326
>> + bool hostVideoLayerRemotely() { return m_hostVideoLayerRemotely; }
>
> hostsVideoLayerRemotely
ditto.
>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378
>> + bool m_hostVideoLayerRemotely { false };
>
> m_hostsVideoLayerRemotely
ditto.
>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:520
>> + deallocateSendRightSafely(machPort.port());
>
> I think you should be using MachSendRight so you don't have to call this manually. Tim? Maybe we meant to say use MachSendRight rather than MachPort.
Good idea. Will update the patch to use MachSendRight.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:109
>> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock);
>
> What is this 100ms for?
To avoid excessive XPC messages when a user change the video element size continuously.
(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 390727[details]
> Patch
>
> > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:520
> > + deallocateSendRightSafely(machPort.port());
>
> I think you should be using MachSendRight so you don't have to call this
> manually. Tim? Maybe we meant to say use MachSendRight rather than MachPort.
Whoops, yes.
Comment on attachment 390854[details]
Revised patch based on review comments
View in context: https://bugs.webkit.org/attachment.cgi?id=390854&action=review> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378
> + bool m_hostsVideoLayerRemotely { false };
WebCore shouldn't care whether things are remotely hosted. This should probably be handled via an abstraction.
Comment on attachment 390854[details]
Revised patch based on review comments
View in context: https://bugs.webkit.org/attachment.cgi?id=390854&action=review>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378
>> + bool m_hostsVideoLayerRemotely { false };
>
> WebCore shouldn't care whether things are remotely hosted. This should probably be handled via an abstraction.
Agree. Your comment reminds me that maybe we can avoid this flag by modifying the RenderVideo implementation. I will upload a WIP patch for review.
Comment on attachment 391028[details]
WIP patch: RenderVideo only calls mediaPlayer->setSize() when supportsAcceleratedRendering is false
View in context: https://bugs.webkit.org/attachment.cgi?id=391028&action=review> Source/WebCore/rendering/RenderVideo.cpp:261
> + if (!mediaPlayer->supportsAcceleratedRendering()) {
Simon, what do you think about this change? I think it's correct for iOS/Mac ports, and the layout tests for other ports pass.
With this change, we do not need the hostsVideoLayerRemotely flag.
Comment on attachment 391094[details]
Rebased patch (with a simplified implementation)
View in context: https://bugs.webkit.org/attachment.cgi?id=391094&action=review> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:106
> + _pendingResolveBoundsBlock = dispatch_block_create((dispatch_block_flags_t)0, ^{
> + _pendingResolveBoundsBlock = nil;
> + [self resolveBounds];
This block is capturing |self|, so I think you have a retain cycle here?
Comment on attachment 391111[details]
Fix the retain cycle problem (with BlockPtr)
View in context: https://bugs.webkit.org/attachment.cgi?id=391111&action=review> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105
> + _pendingResolveBoundsBlock = makeBlockPtr(dispatch_block_create((dispatch_block_flags_t)0, [self] {
> + _pendingResolveBoundsBlock = nil;
> + [self resolveBounds];
> + }));
Pretty sure this is still a retain cycle. self is retained by the block.
Comment on attachment 391111[details]
Fix the retain cycle problem (with BlockPtr)
View in context: https://bugs.webkit.org/attachment.cgi?id=391111&action=review>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105
>> + }));
>
> Pretty sure this is still a retain cycle. self is retained by the block.
OK it doesn't, but we prefer weak ptrs so you can remove -dealloc.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:106
> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (animationDuration + 0.1) * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock.get());
I really want a comment that justifies this 100ms timer.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:145
> + [videoLayerRemote setName:@"WKVideoLayerRemote"];
Maybe just do this always.
(In reply to Simon Fraser (smfr) from comment #26)
> Comment on attachment 391111[details]
> Fix the retain cycle problem (with BlockPtr)
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391111&action=review
>
> >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105
> >> + }));
> >
> > Pretty sure this is still a retain cycle. self is retained by the block.
>
> OK it doesn't, but we prefer weak ptrs so you can remove -dealloc.
I suggested WeakObjCPtr<> but you can also just use __weak :D
Comment on attachment 391111[details]
Fix the retain cycle problem (with BlockPtr)
View in context: https://bugs.webkit.org/attachment.cgi?id=391111&action=review>>>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105
>>>> + }));
>>>
>>> Pretty sure this is still a retain cycle. self is retained by the block.
>>
>> OK it doesn't, but we prefer weak ptrs so you can remove -dealloc.
>
> I suggested WeakObjCPtr<> but you can also just use __weak :D
Got it. I will use WeakObjcPtr<> here and remove -dealloc.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:106
>> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (animationDuration + 0.1) * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock.get());
>
> I really want a comment that justifies this 100ms timer.
Will add some comment in the new patch.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:145
>> + [videoLayerRemote setName:@"WKVideoLayerRemote"];
>
> Maybe just do this always.
Sure.
Comment on attachment 391177[details]
Update the patch based on Wenson's comments
View in context: https://bugs.webkit.org/attachment.cgi?id=391177&action=review> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:56
> +- (WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote
Nit - * on the wrong side.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:61
> +- (void)setMediaPlayerPrivateRemote:(WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote
(Here too)
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:96
> + _pendingResolveBoundsBlock = makeBlockPtr(dispatch_block_create((dispatch_block_flags_t)0, [weakSelf = WeakObjCPtr<WKVideoLayerRemote>(self)] {
I think this would still leak, since dispatch_block_create() returns a +1 object and makeBlockPtr then `Block_copy`s it.
I think you want either just ^{} or a C++ lambda ([] {}) that will be automatically converted into an Objective-C block.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123
> + if (auto *mediaPlayerPrivateRemote = self.mediaPlayerPrivateRemote) {
Nit - * on the wrong side.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:144
> + [videoLayerRemote setFrame:CGRectMake(0, 0, 0, 0)];
Nit - I think you can remove this line, since the frame (by default) is already NSZeroRect.
Comment on attachment 391177[details]
Update the patch based on Wenson's comments
View in context: https://bugs.webkit.org/attachment.cgi?id=391177&action=review>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:56
>> +- (WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote
>
> Nit - * on the wrong side.
Should be like below?
- (WebKit::MediaPlayerPrivateRemote*)mediaPlayerPrivateRemote
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123
>> + if (auto *mediaPlayerPrivateRemote = self.mediaPlayerPrivateRemote) {
>
> Nit - * on the wrong side.
Got it, will fix it.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:144
>> + [videoLayerRemote setFrame:CGRectMake(0, 0, 0, 0)];
>
> Nit - I think you can remove this line, since the frame (by default) is already NSZeroRect.
Right. Will remove it.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:154
> + [videoSublayer setFrame:videoLayerRemote.get().bounds];
Can be removed as well.
Comment on attachment 391177[details]
Update the patch based on Wenson's comments
View in context: https://bugs.webkit.org/attachment.cgi?id=391177&action=review>>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:56
>>> +- (WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote
>>
>> Nit - * on the wrong side.
>
> Should be like below?
> - (WebKit::MediaPlayerPrivateRemote*)mediaPlayerPrivateRemote
Yep! Since WebKit::MediaPlayerPrivateRemote is a C++ class, as opposed to an Objective-C class.
Comment on attachment 391257[details]
A timer based implementation (with the fix for the style checking error)
View in context: https://bugs.webkit.org/attachment.cgi?id=391257&action=review> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:685
> - notImplemented();
> + // We always render to a layer, so this function is empty.
No need to land this in this patch.
> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:690
> - notImplemented();
> + // We always render to a layer, so this function is empty.
Ditto.
> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:28
> +#if ENABLE(GPU_PROCESS) && PLATFORM(COCOA)
I don't think PLATFORM(COCOA) is needed here, if you just replace `CALayer` with `PlatformLayer`. The call site in MediaPlayerPrivateRemote isn't guarded by a PLATFORM(COCOA) call.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:30
> +#import <pal/spi/cocoa/QuartzCoreSPI.h>
Does this need to be in the header, or can this be moved to the implementation file?
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:38
> +@property (nonatomic, assign) WebKit::MediaPlayerPrivateRemote *mediaPlayerPrivateRemote;
> +@property (nonatomic, assign) CALayer *videoSublayer;
Why are these marked as "assign"?
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:42
> + CALayer *_videoSublayer;
RetainPtr<>?
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:43
> + CAContext *_context;
Ditto.
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:65
> + if (self) {
> + self.masksToBounds = YES;
> + _resolveBoundsTimer = makeUnique<WebCore::Timer>([weakSelf = WeakObjCPtr<WKVideoLayerRemote>(self)] {
> + auto localSelf = weakSelf.get();
> + if (!localSelf)
> + return;
> +
> + [localSelf resolveBounds];
> + });
> + _shouldRestartWhenTimerFires = false;
> + }
> +
> + return self;
I'd do an early return here:
```
if (!self)
return nil;
````
If you want to be super fancy, you could do:
```
if (!(self = [super init]))
return nil;
```
> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:107
> + _delay = Seconds(animationDuration + 0.1);
This (0.1) should be declared at the top of the file as `static const Seconds PostAnimationDelay { 100_ms }`, rather than just a magic number here.
Comment on attachment 391257[details]
A timer based implementation (with the fix for the style checking error)
View in context: https://bugs.webkit.org/attachment.cgi?id=391257&action=review>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:685
>> + // We always render to a layer, so this function is empty.
>
> No need to land this in this patch.
Removed.
>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:690
>> + // We always render to a layer, so this function is empty.
>
> Ditto.
Removed.
>> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:28
>> +#if ENABLE(GPU_PROCESS) && PLATFORM(COCOA)
>
> I don't think PLATFORM(COCOA) is needed here, if you just replace `CALayer` with `PlatformLayer`. The call site in MediaPlayerPrivateRemote isn't guarded by a PLATFORM(COCOA) call.
Right. Fixed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:30
>> +#import <pal/spi/cocoa/QuartzCoreSPI.h>
>
> Does this need to be in the header, or can this be moved to the implementation file?
Moved to the implementation file.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:38
>> +@property (nonatomic, assign) CALayer *videoSublayer;
>
> Why are these marked as "assign"?
We do not need to keep a pointer to the videoSublayer because it will be the first sublayer of WKVideoLayerRemote. So it is removed in the revised patch.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:42
>> + CALayer *_videoSublayer;
>
> RetainPtr<>?
Removed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:43
>> + CAContext *_context;
>
> Ditto.
Fixed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:65
>> + return self;
>
> I'd do an early return here:
>
> ```
> if (!self)
> return nil;
> ````
>
> If you want to be super fancy, you could do:
>
> ```
> if (!(self = [super init]))
> return nil;
> ```
Fixed.
>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:107
>> + _delay = Seconds(animationDuration + 0.1);
>
> This (0.1) should be declared at the top of the file as `static const Seconds PostAnimationDelay { 100_ms }`, rather than just a magic number here.
Fixed.
2020-02-12 17:06 PST, Peng Liu
2020-02-12 17:34 PST, Peng Liu
2020-02-12 23:35 PST, Peng Liu
2020-02-13 10:04 PST, Peng Liu
2020-02-13 12:02 PST, Peng Liu
2020-02-13 18:45 PST, Peng Liu
2020-02-13 21:17 PST, Peng Liu
2020-02-14 21:42 PST, Peng Liu
2020-02-17 18:19 PST, Peng Liu
2020-02-17 21:15 PST, Peng Liu
2020-02-18 14:17 PST, Peng Liu
2020-02-18 14:43 PST, Peng Liu
2020-02-18 15:26 PST, Peng Liu
2020-02-18 15:47 PST, Peng Liu
2020-02-18 17:24 PST, Peng Liu
2020-02-18 17:31 PST, Peng Liu
2020-02-18 19:36 PST, Peng Liu
2020-02-19 10:30 PST, Peng Liu
2020-02-19 21:25 PST, Peng Liu
2020-02-19 21:32 PST, Peng Liu
2020-02-20 16:28 PST, Peng Liu
2020-02-21 10:32 PST, Peng Liu