Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1232713002:
Removed unused variables and the need to include the d3dx9.h file. (Closed)
|
Created:
5 years, 5 months ago by Dani Kirov Modified:
5 years, 5 months ago CC:
mflodman, perkj_webrtc, tterriberry_mozilla.com, webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemoved unused variables and the need to include the d3dx9.h file.
BUG=webrtc:3667
Committed: https://crrev.com/4988ca50dfe6ed77255cda8da3832390afaf6a5a
Cr-Commit-Position: refs/heads/master@{#9576}
Patch Set 1 : Removed unused variables and the need to include the d3dx9.h file. #
Total comments: 1
Patch Set 2 : Removed conditional inclusion of d3dx9.h #Patch Set 3 : Added BroadSoft to the AUTHORS file. #
Messages
Total messages: 20 (5 generated)
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... File webrtc/modules/video_render/windows/video_render_direct3d9.h (right): https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... webrtc/modules/video_render/windows/video_render_direct3d9.h:17: #if _MSC_VER < 1800 Why does the MSVC version matter? If what you're concerned about is whether the user has an older Windows/DirectX SDK installed, that's unrelated to the MSVC version.
On 2015/07/10 23:37:27, Peter Kasting wrote: > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > File webrtc/modules/video_render/windows/video_render_direct3d9.h (right): > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > webrtc/modules/video_render/windows/video_render_direct3d9.h:17: #if _MSC_VER < > 1800 > Why does the MSVC version matter? If what you're concerned about is whether the > user has an older Windows/DirectX SDK installed, that's unrelated to the MSVC > version. Yes, the proper check should have been against the version of DirectX instead of the version of MSC. I am not aware how to check this and have to research this further? Yet I am assuming if somebody is using older MSC, chances are high they may be using older DirectX SDK as well. I don't know if this file is needed for older DirectX SDK or not, and I am trying to be cautious about removing it completely. If you guys think it is safe to remove this check and the file completely, and not worry about older versions - I am fine with it.
On 2015/07/11 at 00:08:13, dkirovbroadsoft wrote: > On 2015/07/10 23:37:27, Peter Kasting wrote: > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > File webrtc/modules/video_render/windows/video_render_direct3d9.h (right): > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > webrtc/modules/video_render/windows/video_render_direct3d9.h:17: #if _MSC_VER < > > 1800 > > Why does the MSVC version matter? If what you're concerned about is whether the > > user has an older Windows/DirectX SDK installed, that's unrelated to the MSVC > > version. > > Yes, the proper check should have been against the version of DirectX instead of > the version of MSC. I am not aware how to check this and have to research this > further? > Yet I am assuming if somebody is using older MSC, > chances are high they may be using older DirectX SDK as well. > I don't know if this file is needed for older DirectX SDK or not, and I am trying > to be cautious about removing it completely. > If you guys think it is safe to remove this check and the file completely, > and not worry about older versions - I am fine with it. If you don't know that this is needed by something, just remove it. We don't support building with out-of-date SDKs anyway, but it might make some sense to keep this if you knew for sure that it was needed, and how to do the correct check. Since you're not certain of either of those things let's go the simple route; that avoids leaving clutter and confusion in the codebase.
On 2015/07/11 07:00:05, Peter Kasting wrote: > On 2015/07/11 at 00:08:13, dkirovbroadsoft wrote: > > On 2015/07/10 23:37:27, Peter Kasting wrote: > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > > File webrtc/modules/video_render/windows/video_render_direct3d9.h (right): > > > > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > > webrtc/modules/video_render/windows/video_render_direct3d9.h:17: #if > _MSC_VER < > > > 1800 > > > Why does the MSVC version matter? If what you're concerned about is whether > the > > > user has an older Windows/DirectX SDK installed, that's unrelated to the > MSVC > > > version. > > > > Yes, the proper check should have been against the version of DirectX instead > of > > the version of MSC. I am not aware how to check this and have to research this > > further? > > Yet I am assuming if somebody is using older MSC, > > chances are high they may be using older DirectX SDK as well. > > I don't know if this file is needed for older DirectX SDK or not, and I am > trying > > to be cautious about removing it completely. > > If you guys think it is safe to remove this check and the file completely, > > and not worry about older versions - I am fine with it. > > If you don't know that this is needed by something, just remove it. We don't > support building with out-of-date SDKs anyway, but it might make some sense to > keep this if you knew for sure that it was needed, and how to do the correct > check. Since you're not certain of either of those things let's go the simple > route; that avoids leaving clutter and confusion in the codebase. +1 on what Peter said. Thanks for fixing and let me know if you need help with landing.
On 2015/07/11 08:53:16, tommi (webrtc) wrote: > On 2015/07/11 07:00:05, Peter Kasting wrote: > > On 2015/07/11 at 00:08:13, dkirovbroadsoft wrote: > > > On 2015/07/10 23:37:27, Peter Kasting wrote: > > > > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > > > File webrtc/modules/video_render/windows/video_render_direct3d9.h (right): > > > > > > > > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > > > webrtc/modules/video_render/windows/video_render_direct3d9.h:17: #if > > _MSC_VER < > > > > 1800 > > > > Why does the MSVC version matter? If what you're concerned about is > whether > > the > > > > user has an older Windows/DirectX SDK installed, that's unrelated to the > > MSVC > > > > version. > > > > > > Yes, the proper check should have been against the version of DirectX > instead > > of > > > the version of MSC. I am not aware how to check this and have to research > this > > > further? > > > Yet I am assuming if somebody is using older MSC, > > > chances are high they may be using older DirectX SDK as well. > > > I don't know if this file is needed for older DirectX SDK or not, and I am > > trying > > > to be cautious about removing it completely. > > > If you guys think it is safe to remove this check and the file completely, > > > and not worry about older versions - I am fine with it. > > > > If you don't know that this is needed by something, just remove it. We don't > > support building with out-of-date SDKs anyway, but it might make some sense to > > keep this if you knew for sure that it was needed, and how to do the correct > > check. Since you're not certain of either of those things let's go the simple > > route; that avoids leaving clutter and confusion in the codebase. > > +1 on what Peter said. Thanks for fixing and let me know if you need help with > landing. Yes, please take care of the landing once this looks good Tommi. :)
On 2015/07/12 15:17:20, pbos-webrtc wrote: > On 2015/07/11 08:53:16, tommi (webrtc) wrote: > > On 2015/07/11 07:00:05, Peter Kasting wrote: > > > On 2015/07/11 at 00:08:13, dkirovbroadsoft wrote: > > > > On 2015/07/10 23:37:27, Peter Kasting wrote: > > > > > > > > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > > > > File webrtc/modules/video_render/windows/video_render_direct3d9.h > (right): > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1232713002/diff/1/webrtc/modules/video_render/w... > > > > > webrtc/modules/video_render/windows/video_render_direct3d9.h:17: #if > > > _MSC_VER < > > > > > 1800 > > > > > Why does the MSVC version matter? If what you're concerned about is > > whether > > > the > > > > > user has an older Windows/DirectX SDK installed, that's unrelated to the > > > MSVC > > > > > version. > > > > > > > > Yes, the proper check should have been against the version of DirectX > > instead > > > of > > > > the version of MSC. I am not aware how to check this and have to research > > this > > > > further? > > > > Yet I am assuming if somebody is using older MSC, > > > > chances are high they may be using older DirectX SDK as well. > > > > I don't know if this file is needed for older DirectX SDK or not, and I am > > > trying > > > > to be cautious about removing it completely. > > > > If you guys think it is safe to remove this check and the file completely, > > > > > and not worry about older versions - I am fine with it. > > > > > > If you don't know that this is needed by something, just remove it. We > don't > > > support building with out-of-date SDKs anyway, but it might make some sense > to > > > keep this if you knew for sure that it was needed, and how to do the correct > > > check. Since you're not certain of either of those things let's go the > simple > > > route; that avoids leaving clutter and confusion in the codebase. > > > > +1 on what Peter said. Thanks for fixing and let me know if you need help with > > landing. > > Yes, please take care of the landing once this looks good Tommi. :) Patch 2 removes the d3dx9.h inclusion completely. Since this is my first fix and not quite sure about the process, what should I do next for this issue?
On 2015/07/13 at 18:26:01, dkirovbroadsoft wrote: > Since this is my first fix and not quite sure about the process, > what should I do next for this issue? You need to follow the steps on http://www.webrtc.org/contributing , including signing the committer agreement and touching AUTHORS.
Files updated as per the review comments and AUTHORS updated with the Broadsoft Inc organization name, since this is my first fix using the BroadSoft corporate agreement.
Everything looks correct to me. I'm not an OWNER here so I can't do the final signoff + commit of your patch but I think you've done everything you need to. We'll just wait for tommi to approve and land.
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1232713002/40001
The CQ bit was unchecked by commit-bot@chromium.org
lgtm
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by tommi@webrtc.org
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1232713002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4988ca50dfe6ed77255cda8da3832390afaf6a5a Cr-Commit-Position: refs/heads/master@{#9576} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
