WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Updated patch.
(8.03 KB, patch)
2016-11-29 20:14 PST
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
YAUP.
(8.02 KB, patch)
2016-11-30 04:10 PST
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 295714
[details]
YAUP.
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.
Top of Page
Format For Printing
XML
Clone This Bug