Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396333002:
Add loopback option and improve UX of AppRTCMobile for Mac. (Closed)
|
Created:
4 years, 2 months ago by daniela-webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd loopback option and improve UX of AppRTCMobile for Mac.
CL introduces changes in the layout of the mac demo app.
Bellow are the listed changes
* The label above the room field is removed and that text is shown in the text field's placeholder.
* Size of the window is decreased to accomodate smaller resolutions
* The local view is shown on top of remote view in 3/rd of it's size
* The log view is bellow the remote view and shows client status messages as well as some simple usage instructions.
* Loopback functionality is added
* Start call button added for improved usability
Note: This file was not following our internal ObjC style guidelines i.e
NSString *a - right
NSString* a - wrong
However I've decided to submit the changes and to follow the current style.
I'll submit a bug to update the style in separate CL
BUG=webrtc:6496
Committed: https://crrev.com/647915fb9574231e7cb3461009c8a51e0f570b5a
Cr-Commit-Position: refs/heads/master@{#14692}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Fic formatting #Patch Set 3 : Fix formatting #Messages
Total messages: 17 (7 generated)
Description was changed from ========== Add loopback option and improve UX of AppRTCMobile for Mac. CL introduces changes in the layout of the mac demo app. Bellow are the listed changes * The label above the room field is removed and that text is shown in the text field's placeholder. * Size of the window is decreased to accomodate smaller resolutions * The local view is shown on top of remote view in 3/rd of it's size * The log view is bellow the remote view and shows client status messages as well as some simple usage instructions. * Loopback functionality is added * Start call button added for improved usability Note: This file was not following our internal ObjC style guidelines i.e NSString *a - right NSString* a - wrong However I've decided to submit the changes and to follow the current style. I'll submit a bug to update the style in separate CL BUG=6496 ========== to ========== Add loopback option and improve UX of AppRTCMobile for Mac. CL introduces changes in the layout of the mac demo app. Bellow are the listed changes * The label above the room field is removed and that text is shown in the text field's placeholder. * Size of the window is decreased to accomodate smaller resolutions * The local view is shown on top of remote view in 3/rd of it's size * The log view is bellow the remote view and shows client status messages as well as some simple usage instructions. * Loopback functionality is added * Start call button added for improved usability Note: This file was not following our internal ObjC style guidelines i.e NSString *a - right NSString* a - wrong However I've decided to submit the changes and to follow the current style. I'll submit a bug to update the style in separate CL BUG=webrtc:6496 ==========
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org
Can you please take a look!
On 2016/10/17 09:25:30, denicija-webrtc wrote: > Can you please take a look! awesome, lgtm.
https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m (right): https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:149: metrics:(NSDictionary*)metrics{ nit: missing space
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
lgtm. Thanks, this will make debugging for iOS/Mac much easier! I would like Zeke to review this CL as well, until we learn the ObjC style guide. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m (right): https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:64: [NSString stringWithFormat:@"%@%@\n", _logView.string, message]; nit: This line should be indented with 4 spaces. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:167: //generate room id for loopback options nit: You should format your comment like: // Generate room id for loopback options. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:313: @"• Loopback with or without AppRTC room id"]; nit: I found this instruction slightly confusing. Can you change it to something like: • Enter AppRTC room id (not necessary for loopback) • Start call
https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m (right): https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:28: - (void)appRTCMainView:(APPRTCMainView*)mainView at some point we should change APPRTC->ARD or whatever new 3 letter prefix we think is good for AppRTCMobile. (ARD was selected for AppRTCDemo) https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:62: - (void)displayLogMessage:(NSString*)message { NSString *) https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:65: NSRange range = NSMakeRange([_logView.string length], 0); nit: dot syntax for properties (when using dot syntax, the implicit assumption is that it is a fast method. Using the message usually means that the operation could take a while) https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:69: #pragma mark - Internal we've usually called it - Private there are a bunch of legacy +Internal files still, but should really be named +Private https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:83: NSParameterAssert( consider filing a bug to replace NSParameterAssert with a custom assert macro that is more useful (e.g. that will break properly in debugger and provide useful callstack) https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:107: @"localViewHeight" : @(remoteViewSize.height/3), nit .height / 3 https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:114: NSString* verticalConstraintLeft = NSString *vertical ditto for rest of file. I know this was what it used to be in this file, but we've since figured out our style story and this never got updated https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:121: @"H:|-[_remoteVideoView(remoteViewWidth)]-|", not looking too closely at these, if they work then all good consider using anchors instead though, and programmatically getting constraints by using constraintEqualToAnchor https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:165: nit: remove blank line https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:169: roomString = [[NSUUID UUID] UUIDString]; ditto dot syntax for properties https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:310: [self.mainView displayLogMessage: might be worth just having this as a label somewhere on the UI, up to you
https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m (right): https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:62: - (void)displayLogMessage:(NSString*)message { On 2016/10/19 00:57:17, tkchin_webrtc wrote: > NSString *) Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:64: [NSString stringWithFormat:@"%@%@\n", _logView.string, message]; On 2016/10/17 15:22:41, magjed_webrtc wrote: > nit: This line should be indented with 4 spaces. Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:65: NSRange range = NSMakeRange([_logView.string length], 0); On 2016/10/19 00:57:17, tkchin_webrtc wrote: > nit: dot syntax for properties > > (when using dot syntax, the implicit assumption is that it is a fast method. > Using the message usually means that the operation could take a while) Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:69: #pragma mark - Internal On 2016/10/19 00:57:17, tkchin_webrtc wrote: > we've usually called it - Private > there are a bunch of legacy +Internal files still, but should really be named > +Private Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:83: NSParameterAssert( On 2016/10/19 00:57:17, tkchin_webrtc wrote: > consider filing a bug to replace NSParameterAssert with a custom assert macro > that is more useful (e.g. that will break properly in debugger and provide > useful callstack) https://bugs.chromium.org/p/webrtc/issues/detail?id=6561 https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:107: @"localViewHeight" : @(remoteViewSize.height/3), On 2016/10/19 00:57:17, tkchin_webrtc wrote: > nit .height / 3 Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:114: NSString* verticalConstraintLeft = On 2016/10/19 00:57:17, tkchin_webrtc wrote: > NSString *vertical > > ditto for rest of file. I know this was what it used to be in this file, but > we've since figured out our style story and this never got updated Bug filled https://bugs.chromium.org/p/webrtc/issues/detail?id=6561 https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:165: On 2016/10/19 00:57:17, tkchin_webrtc wrote: > nit: remove blank line Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:167: //generate room id for loopback options On 2016/10/17 15:22:41, magjed_webrtc wrote: > nit: You should format your comment like: > // Generate room id for loopback options. Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:169: roomString = [[NSUUID UUID] UUIDString]; On 2016/10/19 00:57:16, tkchin_webrtc wrote: > ditto dot syntax for properties Done. https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:313: @"• Loopback with or without AppRTC room id"]; On 2016/10/17 15:22:41, magjed_webrtc wrote: > nit: I found this instruction slightly confusing. Can you change it to something > like: > • Enter AppRTC room id (not necessary for loopback) > • Start call Done.
On 2016/10/19 14:45:13, denicija-webrtc wrote: > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > File webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m (right): > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:62: - > (void)displayLogMessage:(NSString*)message { > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > NSString *) > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:64: [NSString > stringWithFormat:@"%@%@\n", _logView.string, message]; > On 2016/10/17 15:22:41, magjed_webrtc wrote: > > nit: This line should be indented with 4 spaces. > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:65: NSRange range = > NSMakeRange([_logView.string length], 0); > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > nit: dot syntax for properties > > > > (when using dot syntax, the implicit assumption is that it is a fast method. > > Using the message usually means that the operation could take a while) > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:69: #pragma mark - > Internal > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > we've usually called it - Private > > there are a bunch of legacy +Internal files still, but should really be named > > +Private > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:83: > NSParameterAssert( > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > consider filing a bug to replace NSParameterAssert with a custom assert macro > > that is more useful (e.g. that will break properly in debugger and provide > > useful callstack) > > https://bugs.chromium.org/p/webrtc/issues/detail?id=6561 > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:107: > @"localViewHeight" : @(remoteViewSize.height/3), > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > nit .height / 3 > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:114: NSString* > verticalConstraintLeft = > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > NSString *vertical > > > > ditto for rest of file. I know this was what it used to be in this file, but > > we've since figured out our style story and this never got updated > > Bug filled > https://bugs.chromium.org/p/webrtc/issues/detail?id=6561 > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:165: > On 2016/10/19 00:57:17, tkchin_webrtc wrote: > > nit: remove blank line > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:167: //generate > room id for loopback options > On 2016/10/17 15:22:41, magjed_webrtc wrote: > > nit: You should format your comment like: > > // Generate room id for loopback options. > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:169: roomString = > [[NSUUID UUID] UUIDString]; > On 2016/10/19 00:57:16, tkchin_webrtc wrote: > > ditto dot syntax for properties > > Done. > > https://codereview.webrtc.org/2396333002/diff/1/webrtc/examples/objc/AppRTCMo... > webrtc/examples/objc/AppRTCMobile/mac/APPRTCViewController.m:313: @"• Loopback > with or without AppRTC room id"]; > On 2016/10/17 15:22:41, magjed_webrtc wrote: > > nit: I found this instruction slightly confusing. Can you change it to > something > > like: > > • Enter AppRTC room id (not necessary for loopback) > > • Start call > > Done. lgtm
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2396333002/#ps40001 (title: "Fix formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add loopback option and improve UX of AppRTCMobile for Mac. CL introduces changes in the layout of the mac demo app. Bellow are the listed changes * The label above the room field is removed and that text is shown in the text field's placeholder. * Size of the window is decreased to accomodate smaller resolutions * The local view is shown on top of remote view in 3/rd of it's size * The log view is bellow the remote view and shows client status messages as well as some simple usage instructions. * Loopback functionality is added * Start call button added for improved usability Note: This file was not following our internal ObjC style guidelines i.e NSString *a - right NSString* a - wrong However I've decided to submit the changes and to follow the current style. I'll submit a bug to update the style in separate CL BUG=webrtc:6496 ========== to ========== Add loopback option and improve UX of AppRTCMobile for Mac. CL introduces changes in the layout of the mac demo app. Bellow are the listed changes * The label above the room field is removed and that text is shown in the text field's placeholder. * Size of the window is decreased to accomodate smaller resolutions * The local view is shown on top of remote view in 3/rd of it's size * The log view is bellow the remote view and shows client status messages as well as some simple usage instructions. * Loopback functionality is added * Start call button added for improved usability Note: This file was not following our internal ObjC style guidelines i.e NSString *a - right NSString* a - wrong However I've decided to submit the changes and to follow the current style. I'll submit a bug to update the style in separate CL BUG=webrtc:6496 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add loopback option and improve UX of AppRTCMobile for Mac. CL introduces changes in the layout of the mac demo app. Bellow are the listed changes * The label above the room field is removed and that text is shown in the text field's placeholder. * Size of the window is decreased to accomodate smaller resolutions * The local view is shown on top of remote view in 3/rd of it's size * The log view is bellow the remote view and shows client status messages as well as some simple usage instructions. * Loopback functionality is added * Start call button added for improved usability Note: This file was not following our internal ObjC style guidelines i.e NSString *a - right NSString* a - wrong However I've decided to submit the changes and to follow the current style. I'll submit a bug to update the style in separate CL BUG=webrtc:6496 ========== to ========== Add loopback option and improve UX of AppRTCMobile for Mac. CL introduces changes in the layout of the mac demo app. Bellow are the listed changes * The label above the room field is removed and that text is shown in the text field's placeholder. * Size of the window is decreased to accomodate smaller resolutions * The local view is shown on top of remote view in 3/rd of it's size * The log view is bellow the remote view and shows client status messages as well as some simple usage instructions. * Loopback functionality is added * Start call button added for improved usability Note: This file was not following our internal ObjC style guidelines i.e NSString *a - right NSString* a - wrong However I've decided to submit the changes and to follow the current style. I'll submit a bug to update the style in separate CL BUG=webrtc:6496 Committed: https://crrev.com/647915fb9574231e7cb3461009c8a51e0f570b5a Cr-Commit-Position: refs/heads/master@{#14692} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/647915fb9574231e7cb3461009c8a51e0f570b5a Cr-Commit-Position: refs/heads/master@{#14692} |
