RESOLVED FIXED 165139
[MediaStream] Sync video preview layer and parent layer sizes
https://bugs.webkit.org/show_bug.cgi?id=165139
Summary [MediaStream] Sync video preview layer and parent layer sizes
Eric Carlson
Reported 2016-11-29 06:54:53 PST
Resize video preview layer when its parent layer size changes.
Attachments
Proposed patch. (6.91 KB, patch)
2016-11-29 07:30 PST, Eric Carlson
no flags
Updated patch. (8.03 KB, patch)
2016-11-29 20:14 PST, Eric Carlson
no flags
YAUP. (8.02 KB, patch)
2016-11-30 04:10 PST, Eric Carlson
no flags
Eric Carlson
Comment 1 2016-11-29 07:30:14 PST
Created attachment 295593 [details] Proposed patch.
WebKit Commit Bot
Comment 2 2016-11-29 07:31:07 PST
Attachment 295593 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:656: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:660: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Daniel Bates
Comment 3 2016-11-29 14:31:01 PST
Comment on attachment 295593 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295593&action=review > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:100 > + AVVideoSourcePreview* _parent; Nit: * is on the wrong side. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:101 > + BOOL _haveObserver; This is OK as-is. Would using the present tense, _hasObserver, read better? > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:117 > + PlatformLayer* platformLayer() const final { return m_previewBackgroundLayer.get(); } It is an unwritten convention to that const functions should return pointers to const objects. When returning a non-const pointer the function should not be const. The motivation behind this idiom is that the object pointed by the non-const pointer can be mutated, which could have implications to the behavior of the class. Humans not well versed in the C++ concept of const typically consider const functions to convey that an instance of a class will not be modified by the call. Returning a non-const pointer circumvents this notion. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:640 > + [_parent->platformLayer() addObserver:self forKeyPath:@"bounds" options:0 context:0]; We should pass nullptr instead of 0 for context. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:644 > +- (void)observeValueForKeyPath:keyPath ofObject:(id)object change:(NSDictionary *)change context:(void*)context Nit: Missing a space after "void". > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:656 > + if ([keyPath isEqual:@"bounds"] && object == _parent->platformLayer()) We repeatedly hardcode the string literal "bounds" throughout this patch. I suggest that we extract it into a constant and reference this constant throughout the patch.
Eric Carlson
Comment 4 2016-11-29 19:12:54 PST
Comment on attachment 295593 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295593&action=review >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:100 >> + AVVideoSourcePreview* _parent; > > Nit: * is on the wrong side. OK >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:101 >> + BOOL _haveObserver; > > This is OK as-is. Would using the present tense, _hasObserver, read better? Changed. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:117 >> + PlatformLayer* platformLayer() const final { return m_previewBackgroundLayer.get(); } > > It is an unwritten convention to that const functions should return pointers to const objects. When returning a non-const pointer the function should not be const. The motivation behind this idiom is that the object pointed by the non-const pointer can be mutated, which could have implications to the behavior of the class. Humans not well versed in the C++ concept of const typically consider const functions to convey that an instance of a class will not be modified by the call. Returning a non-const pointer circumvents this notion. Sounds like this should be codified in our style guidelines. This isn't new to this patch, and changing it will requires a change a number of other classes so I will address this in another patch. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:640 >> + [_parent->platformLayer() addObserver:self forKeyPath:@"bounds" options:0 context:0]; > > We should pass nullptr instead of 0 for context. OK >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:644 >> +- (void)observeValueForKeyPath:keyPath ofObject:(id)object change:(NSDictionary *)change context:(void*)context > > Nit: Missing a space after "void". Fixed. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:656 >> + if ([keyPath isEqual:@"bounds"] && object == _parent->platformLayer()) > > We repeatedly hardcode the string literal "bounds" throughout this patch. I suggest that we extract it into a constant and reference this constant throughout the patch. Fixed.
Eric Carlson
Comment 5 2016-11-29 20:14:06 PST
Created attachment 295698 [details] Updated patch.
WebKit Commit Bot
Comment 6 2016-11-29 20:15:12 PST
Attachment 295698 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:647: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:651: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Eric Carlson
Comment 7 2016-11-30 04:10:35 PST
WebKit Commit Bot
Comment 8 2016-11-30 04:13:18 PST
Attachment 295714 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:647: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:651: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 9 2016-11-30 11:20:52 PST
Comment on attachment 295714 [details] YAUP. Clearing flags on attachment: 295714 Committed r209141: <http://trac.webkit.org/changeset/209141>
WebKit Commit Bot
Comment 10 2016-11-30 11:20:56 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.