Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809913002:
return comparevideos stdout and fix missing device case (Closed)
|
Created:
3 years, 8 months ago by janssonWebRTC Modified:
3 years, 8 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
Descriptionreturn comparevideos stdout and fix missing device case
BUG=webrtc:7203
NOTRY=True
Review-Url: https://codereview.webrtc.org/2809913002
Cr-Commit-Position: refs/heads/master@{#17665}
Committed: https://chromium.googlesource.com/external/webrtc/+/07e20db42dbf866e76ae52f8f7dfd6274e1f1045
Patch Set 1 #Patch Set 2 : Merge branch 'master' into returnStatsToCaller #
Total comments: 4
Patch Set 3 : address comments #
Messages
Total messages: 18 (6 generated)
jansson@webrtc.org changed reviewers: + kjellander@webrtc.org
This is a continuatation from https://codereview.webrtc.org/2789533002/. We can skip that one entirely and just look at this one instead.
Description was changed from ========== return comparevideos stdout and fix missing device case BUG= ========== to ========== return comparevideos stdout and fix missing device case BUG=webrtc:7203 ==========
On 2017/04/11 08:56:26, janssonWebRTC wrote: > This is a continuatation from https://codereview.webrtc.org/2789533002/. We can > skip that one entirely and just look at this one instead. Why is that? Then it's really hard to tell what's different between this CL and that one without doing annoying side-by-side comparisons. Can't you just add another patch set on that one?
On 2017/04/11 08:56:26, janssonWebRTC wrote: > This is a continuatation from https://codereview.webrtc.org/2789533002/. We can > skip that one entirely and just look at this one instead. Why is that? Then it's really hard to tell what's different between this CL and that one without doing annoying side-by-side comparisons. Can't you just add another patch set on that one?
On 2017/04/11 08:56:26, janssonWebRTC wrote: > This is a continuatation from https://codereview.webrtc.org/2789533002/. We can > skip that one entirely and just look at this one instead. Why is that? Then it's really hard to tell what's different between this CL and that one without doing annoying side-by-side comparisons. Can't you just add another patch set on that one?
On 2017/04/11 08:56:26, janssonWebRTC wrote: > This is a continuatation from https://codereview.webrtc.org/2789533002/. We can > skip that one entirely and just look at this one instead. Why is that? Then it's really hard to tell what's different between this CL and that one without doing annoying side-by-side comparisons. Can't you just add another patch set on that one?
On 2017/04/11 14:18:39, kjellander_webrtc wrote: > On 2017/04/11 08:56:26, janssonWebRTC wrote: > > This is a continuatation from https://codereview.webrtc.org/2789533002/. We > can > > skip that one entirely and just look at this one instead. > > Why is that? Then it's really hard to tell what's different between this CL and > that one without doing annoying side-by-side comparisons. Can't you just add > another patch set on that one? I added the other one to the CQ. Will rebase this once it lands thus making it easier to see what's changed.
Description was changed from ========== return comparevideos stdout and fix missing device case BUG=webrtc:7203 ========== to ========== return comparevideos stdout and fix missing device case BUG=webrtc:7203 NOTRY=True ==========
Rebased
https://codereview.webrtc.org/2809913002/diff/20001/webrtc/tools/video_analys... File webrtc/tools/video_analysis.py (right): https://codereview.webrtc.org/2809913002/diff/20001/webrtc/tools/video_analys... webrtc/tools/video_analysis.py:191: # One of the devices specified cannot be found, return empty list. If possible, it's easier to follow the flow of execution if the amount of places where return is called is minimized. In this case I also think it makes sense to print something, like: logging.info('One of the devices specified cannot be found, return empty list.') or in this case, you're nog using logging so a regular print would do. I think it's cleaner to remove line 192 here and change line 204 to be outside the else-clause, i.e. -2 spaces indent (preferably with a blank line before as well to make it more obvious it's not a part of that part. https://codereview.webrtc.org/2809913002/diff/20001/webrtc/tools/video_analys... webrtc/tools/video_analysis.py:462: print 'Result recorded to: ' + os.path.abspath(result_file_name) nit: use string formatting instead of concatenation, i.e.: print 'Result recorded to: %s' % os.path.abspath(result_file_name)
https://codereview.webrtc.org/2809913002/diff/20001/webrtc/tools/video_analys... File webrtc/tools/video_analysis.py (right): https://codereview.webrtc.org/2809913002/diff/20001/webrtc/tools/video_analys... webrtc/tools/video_analysis.py:191: # One of the devices specified cannot be found, return empty list. On 2017/04/12 06:18:52, kjellander_webrtc wrote: > If possible, it's easier to follow the flow of execution if the amount of places > where return is called is minimized. In this case I also think it makes sense to > print something, like: > logging.info('One of the devices specified cannot be found, return empty list.') > or in this case, you're nog using logging so a regular print would do. > > I think it's cleaner to remove line 192 here and change line 204 to be outside > the else-clause, i.e. -2 spaces indent (preferably with a blank line before as > well to make it more obvious it's not a part of that part. Done. https://codereview.webrtc.org/2809913002/diff/20001/webrtc/tools/video_analys... webrtc/tools/video_analysis.py:462: print 'Result recorded to: ' + os.path.abspath(result_file_name) On 2017/04/12 06:18:52, kjellander_webrtc wrote: > nit: use string formatting instead of concatenation, i.e.: > print 'Result recorded to: %s' % os.path.abspath(result_file_name) Done.
lgtm
The CQ bit was checked by jansson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491986001246630,
"parent_rev": "844d2b9670857b5bc2be88d9199d54ca870dc4b2", "commit_rev":
"07e20db42dbf866e76ae52f8f7dfd6274e1f1045"}
Message was sent while issue was closed.
Description was changed from ========== return comparevideos stdout and fix missing device case BUG=webrtc:7203 NOTRY=True ========== to ========== return comparevideos stdout and fix missing device case BUG=webrtc:7203 NOTRY=True Review-Url: https://codereview.webrtc.org/2809913002 Cr-Commit-Position: refs/heads/master@{#17665} Committed: https://chromium.googlesource.com/external/webrtc/+/07e20db42dbf866e76ae52f8f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/07e20db42dbf866e76ae52f8f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
