Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1326663002:
Fixed not to append the model id when fake video device is enabled. (Closed)
DescriptionFixed not to append the model id when fake video device is enabled.
BUG=526633
Committed: https://crrev.com/1835d414dbd8ae8a8e9aa2c70d612b09f4c07785
Cr-Commit-Position: refs/heads/master@{#349146}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Messages
Total messages: 21 (5 generated)
msu.koo@samsung.com changed reviewers: + a1.gomes@sisa.samsung.com
PTAL :)
msu.koo@samsung.com changed reviewers: + mcasas@chromium.org
On 2015/09/02 05:33:27, esnada wrote: informal l g t m. I will defer to others to decide if such simple change requires any sort of unit test.
https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.cc (right): https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.cc:22: // Ignore |model_id| if |kUseFakeDeviceForMediaStream| flag is present. Hmm it's not that simple :) Command line flags, specially long standing ones like this, should be contained in its spread, otherwise we create two what essentially amounts to two code paths. The problem here is that GetModel() is linked on compile time to whatever platform we're compiling, whereas we should use more a run time call. And that will prove hard since we'd like to collect the Model, to paste to the Name, /before/ actually creating the VideoCaptureDevice proper.
https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.cc (right): https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.cc:22: // Ignore |model_id| if |kUseFakeDeviceForMediaStream| flag is present. Thank you for your comment :) A little bit hard to understand what "should be contained in its spread" means. Can you explain a little bit more? On 2015/09/04 02:24:50, mcasas wrote: > Hmm it's not that simple :) > > Command line flags, specially long standing ones like this, > should be contained in its spread, otherwise we create two > what essentially amounts to two code paths. > > The problem here is that GetModel() is linked on compile > time to whatever platform we're compiling, whereas we > should use more a run time call. And that will prove hard > since we'd like to collect the Model, to paste to the Name, > /before/ actually creating the VideoCaptureDevice proper.
https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.cc (right): https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.cc:22: // Ignore |model_id| if |kUseFakeDeviceForMediaStream| flag is present. On 2015/09/05 07:58:11, esnada wrote: > Thank you for your comment :) > A little bit hard to understand what "should be contained in its spread" means. > Can you explain a little bit more? > Yeah, so command line flags are supposed to be limited in its scope and to be used at minimal points and for limited functionality. If a command line flag is used in several points, it'd break this implicit limitation, and it could potentially be used for several things. A flag should be used only once, in one place, and, if possible, with a view on removing it altogether long term. Hmmm... makes sense? > On 2015/09/04 02:24:50, mcasas wrote: > > Hmm it's not that simple :) > > > > Command line flags, specially long standing ones like this, > > should be contained in its spread, otherwise we create two > > what essentially amounts to two code paths. > > > > The problem here is that GetModel() is linked on compile > > time to whatever platform we're compiling, whereas we > > should use more a run time call. And that will prove hard > > since we'd like to collect the Model, to paste to the Name, > > /before/ actually creating the VideoCaptureDevice proper.
https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.cc (right): https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.cc:22: // Ignore |model_id| if |kUseFakeDeviceForMediaStream| flag is present. I fully agree with you that should minimize using command-line branch statements. So not to use the command-line checking in here, I think it's required to change the root approach(select platform at build time) because fake device is selected at runtime. I'm not sure that it's a better way to change the root approach for fixing this bug instead of using command-line branching. If you are not hardly opposite on this way, I would take the current way(using commandline switch). How do you think? On 2015/09/08 17:50:15, mcasas wrote: > On 2015/09/05 07:58:11, esnada wrote: > > Thank you for your comment :) > > A little bit hard to understand what "should be contained in its spread" > means. > > Can you explain a little bit more? > > > > Yeah, so command line flags are supposed to be > limited in its scope and to be used at > minimal points and for limited functionality. > If a command line flag is used in several > points, it'd break this implicit limitation, > and it could potentially be used for several > things. A flag should be used only once, in > one place, and, if possible, with a view on > removing it altogether long term. > Hmmm... makes sense? > > > On 2015/09/04 02:24:50, mcasas wrote: > > > Hmm it's not that simple :) > > > > > > Command line flags, specially long standing ones like this, > > > should be contained in its spread, otherwise we create two > > > what essentially amounts to two code paths. > > > > > > The problem here is that GetModel() is linked on compile > > > time to whatever platform we're compiling, whereas we > > > should use more a run time call. And that will prove hard > > > since we'd like to collect the Model, to paste to the Name, > > > /before/ actually creating the VideoCaptureDevice proper. >
https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.cc (right): https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.cc:22: // Ignore |model_id| if |kUseFakeDeviceForMediaStream| flag is present. On 2015/09/11 07:08:50, esnada wrote: > I fully agree with you that should minimize using command-line branch > statements. > So not to use the command-line checking in here, I think it's required to change > the root approach(select platform at build time) because fake device is selected > at runtime. > I'm not sure that it's a better way to change the root approach for fixing this > bug instead of using command-line branching. > If you are not hardly opposite on this way, I would take the current way(using > commandline switch). > How do you think? > > On 2015/09/08 17:50:15, mcasas wrote: > > On 2015/09/05 07:58:11, esnada wrote: > > > Thank you for your comment :) > > > A little bit hard to understand what "should be contained in its spread" > > means. > > > Can you explain a little bit more? > > > > > > > Yeah, so command line flags are supposed to be > > limited in its scope and to be used at > > minimal points and for limited functionality. > > If a command line flag is used in several > > points, it'd break this implicit limitation, > > and it could potentially be used for several > > things. A flag should be used only once, in > > one place, and, if possible, with a view on > > removing it altogether long term. > > Hmmm... makes sense? > > > > > On 2015/09/04 02:24:50, mcasas wrote: > > > > Hmm it's not that simple :) > > > > > > > > Command line flags, specially long standing ones like this, > > > > should be contained in its spread, otherwise we create two > > > > what essentially amounts to two code paths. > > > > > > > > The problem here is that GetModel() is linked on compile > > > > time to whatever platform we're compiling, whereas we > > > > should use more a run time call. And that will prove hard > > > > since we'd like to collect the Model, to paste to the Name, > > > > /before/ actually creating the VideoCaptureDevice proper. > > > Hmm is true that changing the whole infra might prove daunting at this point. Proposal: leave and land this CL as is after adding and creating another bug, adding a line here: // TODO(esnada): http://crbug.com/ABCDEF, remove checking the switch in favour // of deferring GetModel() call to the actual VideoCaptureDevice object. Or any other wording of you choice :) SGTY?
On 2015/09/15 17:11:50, mcasas wrote: > https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... > File media/capture/video/video_capture_device.cc (right): > > https://codereview.chromium.org/1326663002/diff/1/media/capture/video/video_c... > media/capture/video/video_capture_device.cc:22: // Ignore |model_id| if > |kUseFakeDeviceForMediaStream| flag is present. > On 2015/09/11 07:08:50, esnada wrote: > > I fully agree with you that should minimize using command-line branch > > statements. > > So not to use the command-line checking in here, I think it's required to > change > > the root approach(select platform at build time) because fake device is > selected > > at runtime. > > I'm not sure that it's a better way to change the root approach for fixing > this > > bug instead of using command-line branching. > > If you are not hardly opposite on this way, I would take the current way(using > > commandline switch). > > How do you think? > > > > On 2015/09/08 17:50:15, mcasas wrote: > > > On 2015/09/05 07:58:11, esnada wrote: > > > > Thank you for your comment :) > > > > A little bit hard to understand what "should be contained in its spread" > > > means. > > > > Can you explain a little bit more? > > > > > > > > > > Yeah, so command line flags are supposed to be > > > limited in its scope and to be used at > > > minimal points and for limited functionality. > > > If a command line flag is used in several > > > points, it'd break this implicit limitation, > > > and it could potentially be used for several > > > things. A flag should be used only once, in > > > one place, and, if possible, with a view on > > > removing it altogether long term. > > > Hmmm... makes sense? > > > > > > > On 2015/09/04 02:24:50, mcasas wrote: > > > > > Hmm it's not that simple :) > > > > > > > > > > Command line flags, specially long standing ones like this, > > > > > should be contained in its spread, otherwise we create two > > > > > what essentially amounts to two code paths. > > > > > > > > > > The problem here is that GetModel() is linked on compile > > > > > time to whatever platform we're compiling, whereas we > > > > > should use more a run time call. And that will prove hard > > > > > since we'd like to collect the Model, to paste to the Name, > > > > > /before/ actually creating the VideoCaptureDevice proper. > > > > > > > Hmm is true that changing the whole infra might prove daunting > at this point. Proposal: leave and land this CL as is after adding > and creating another bug, adding a line here: > > // TODO(esnada): http://crbug.com/ABCDEF, remove checking the switch in favour > // of deferring GetModel() call to the actual VideoCaptureDevice object. > > Or any other wording of you choice :) > SGTY? Thank you for your proposal :) Addressed as you commented.
LGTM, thanks!
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326663002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326663002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msu.koo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326663002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326663002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1835d414dbd8ae8a8e9aa2c70d612b09f4c07785 Cr-Commit-Position: refs/heads/master@{#349146}
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1835d414dbd8ae8a8e9aa2c70d612b09f4c07785 Cr-Commit-Position: refs/heads/master@{#349146} |
