Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064313004:
Add support for obtaining AV products on Win7. (Closed)
|
Created:
4 years, 6 months ago by Will Harris Modified:
4 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for obtaining AV products on Win7.
Previously WSC API was used which is only available on Windows 8.
This CL adds support via WMI, which is available from Vista onwards.
BUG=615154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167
Committed: https://crrev.com/6813e4fdd732d11c532c02fd07e16f6c76df78ab
Cr-Original-Commit-Position: refs/heads/master@{#400496}
Cr-Commit-Position: refs/heads/master@{#403544}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #
Total comments: 4
Patch Set 3 : std::move #
Total comments: 8
Patch Set 4 : code review changes #
Total comments: 2
Patch Set 5 : WSC is not available on Server products. #
Messages
Total messages: 33 (17 generated)
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 ==========
wfh@chromium.org changed reviewers: + grt@chromium.org
grt - can you take an initial look at this before I inflict it on metrics OWNERS? Thanks! https://codereview.chromium.org/2064313004/diff/1/chrome/browser/metrics/anti... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/1/chrome/browser/metrics/anti... chrome/browser/metrics/antivirus_metrics_provider_win.cc:43: struct PRODUCT_STATE { Source: http://neophob.com/2010/03/wmi-query-windows-securitycenter2/ Also, my own testing with COMODO, avast!, Windows Defender, Microsoft Security Essentials, and Kaspersky. https://codereview.chromium.org/2064313004/diff/1/chrome/browser/metrics/anti... chrome/browser/metrics/antivirus_metrics_provider_win.cc:161: result = FillAntiVirusProductsFromWMI(&av_products); Note: The WMI interface is not officially documented, so I prefer to use the WSC interface on Win8 and above...
jschuh@chromium.org changed reviewers: + jschuh@chromium.org
Lgtm from the Windows and COM side, modulo some nits. https://codereview.chromium.org/2064313004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:275: products->insert(products->end(), result_list.begin(), result_list.end()); Why the change from std::move? https://codereview.chromium.org/2064313004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:402: products->insert(products->end(), result_list.begin(), result_list.end()); Why not std::move?
wfh@chromium.org changed reviewers: + rkaplow@chromium.org - grt@chromium.org
rkaplow, can you take a look now? Thanks. https://codereview.chromium.org/2064313004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:275: products->insert(products->end(), result_list.begin(), result_list.end()); On 2016/06/16 20:55:57, jschuh (very slow) wrote: > Why the change from std::move? because at one point I thought I would call both and append to the product list, but changed my mind. I've changed it back. https://codereview.chromium.org/2064313004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:402: products->insert(products->end(), result_list.begin(), result_list.end()); On 2016/06/16 20:55:57, jschuh (very slow) wrote: > Why not std::move? Done.
grt@chromium.org changed reviewers: + grt@chromium.org
drive-by lgtm w/ a nit and request https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:292: wmi_locator.CreateInstance(CLSID_WbemLocator, NULL, CLSCTX_INPROC_SERVER); nit: nullptr throughout https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:344: case 0: nit: is it possible to include a doc link for these values in a comment?
lgtm https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:320: while (true) { can you add some more comments? What is this iterating through and looking for https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:376: if (ShouldReportFullNames()) maybe add one liner comment about what this is about
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Patchset #4 (id:60001) has been deleted
Thanks for reviews. Fixed NULLs, and added more comments. https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:292: wmi_locator.CreateInstance(CLSID_WbemLocator, NULL, CLSCTX_INPROC_SERVER); On 2016/06/17 13:19:33, grt (slow) wrote: > nit: nullptr throughout Done. https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:320: while (true) { On 2016/06/17 14:45:14, rkaplow wrote: > can you add some more comments? What is this iterating through and looking for Done. https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:344: case 0: On 2016/06/17 13:19:33, grt (slow) wrote: > nit: is it possible to include a doc link for these values in a comment? Done. https://codereview.chromium.org/2064313004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:376: if (ShouldReportFullNames()) On 2016/06/17 14:45:14, rkaplow wrote: > maybe add one liner comment about what this is about this is already quite heavily commented at the function above.
lgtm w/ a language nit which you are free to disregard. https://codereview.chromium.org/2064313004/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:164: // will fall-back to the undocumented WMI interface on Windows 7 and below. nit: when i read "fall-back to" i think that it'll try one and then try the other. this looks more like "use" to me.
https://codereview.chromium.org/2064313004/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2064313004/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/antivirus_metrics_provider_win.cc:164: // will fall-back to the undocumented WMI interface on Windows 7 and below. On 2016/06/17 19:50:32, grt (slow) wrote: > nit: when i read "fall-back to" i think that it'll try one and then try the > other. this looks more like "use" to me. i'm okay with this as per off-line discussion.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2064313004/#ps80001 (title: "code review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064313004/80001
Message was sent while issue was closed.
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496}
Message was sent while issue was closed.
revert UI is not working, but I am reverting this CL in https://codereview.chromium.org/2078093002 reason is: it broke the official builder, please see issue 621235
Message was sent while issue was closed.
Patchset #5 (id:100001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496} ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496} ==========
WSC is not available on Windows Server products so this is why it broke on the official builder. I have changed the code to not attempt to enumerate AV on SUITE_SERVER and tested on Windows Server 2008 R2 Please let me know if you want me to hold off committing this minor addition otherwise I'll land later today. Thanks.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org, rkaplow@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2064313004/#ps120001 (title: "WSC is not available on Server products.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496} ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Cr-Commit-Position: refs/heads/master@{#400496} ========== to ========== Add support for obtaining AV products on Win7. Previously WSC API was used which is only available on Windows 8. This CL adds support via WMI, which is available from Vista onwards. BUG=615154 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/1e8be206b2e8a96af660720bef1450f8a06af167 Committed: https://crrev.com/6813e4fdd732d11c532c02fd07e16f6c76df78ab Cr-Original-Commit-Position: refs/heads/master@{#400496} Cr-Commit-Position: refs/heads/master@{#403544} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6813e4fdd732d11c532c02fd07e16f6c76df78ab Cr-Commit-Position: refs/heads/master@{#403544} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
