Trim version and extraneus parts from AntiVirus product names.

Trim version and extraneus parts from AntiVirus product names.

Some AV products include their version in the product name in WMI which makes
maintaining the list of hashes of product names onerous.

Therefore, trim anything that looks like a version, along with trailing spaces.

BUG=615154
TEST=unit_tests --gtest_filter=AntiVirusMetricsProvider*

Review-Url: https://codereview.chromium.org/2631583002
Cr-Commit-Position: refs/heads/master@{#443594}
Committed: https://chromium.googlesource.com/chromium/src/+/da07d58a5d041395d3d6317ab3aef31c0087285e

[Will Harris, 2017-01-12 23:07:26]

Description was changed from

==========
Trim version and extraneus parts from AntiVirus product names.

Some AV products include their version in the product name in WMI which makes
maintaining the list of hashes of product names onerous.

Therefore, trim anything that looks like a version, along with trailing spaces.

BUG=615154
TEST=unit_tests --gtest_filter=AntiVirusMetricsProvider*
==========

to

==========
Trim version and extraneus parts from AntiVirus product names.

Some AV products include their version in the product name in WMI which makes
maintaining the list of hashes of product names onerous.

Therefore, trim anything that looks like a version, along with trailing spaces.

BUG=615154
TEST=unit_tests --gtest_filter=AntiVirusMetricsProvider*
==========

[Will Harris, 2017-01-12 23:07:26]

wfh@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Will Harris, 2017-01-12 23:10:34]

PTAL

Companion code in g3 is cl/144374105

[rkaplow, 2017-01-12 23:18:57]

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.cc:72: return true;
shouldn't this not return right away? i.e. this would mark "abc." as a version
string.
Could we verify that all elements of the string are digits/periods/etc?

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.h (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.h:88: static std::string
TrimVersionOfAvProductName(const std::string& av_product);
hm, I'd add a small comment on some idea of what kind of algorithm it is going
to try to identify what the version is

[Will Harris, 2017-01-12 23:23:59]

WDYT?

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.cc:72: return true;
On 2017/01/12 23:18:56, rkaplow wrote:
> shouldn't this not return right away? i.e. this would mark "abc." as a version
> string.
> Could we verify that all elements of the string are digits/periods/etc?

this was specifically to handle stuff like

"nProtect Anti-Virus/Spyware V4.0"
"ESET NOD32 Antivirus 9.0.349.15P"

to be honest it doesn't actually matter too much if we're overzealous here as
long as some semblance of the name is retained.

I could add 'V' and 'P' to the blacklist but then it starts to get sort of
crazy... Happy to do what you think here...

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.h (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.h:88: static std::string
TrimVersionOfAvProductName(const std::string& av_product);
On 2017/01/12 23:18:56, rkaplow wrote:
> hm, I'd add a small comment on some idea of what kind of algorithm it is going
> to try to identify what the version is

Acknowledged. Once we agree what the algorithm is :)

[Will Harris, 2017-01-12 23:27:05]

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.cc:72: return true;
On 2017/01/12 23:23:59, Will Harris wrote:
> On 2017/01/12 23:18:56, rkaplow wrote:
> > shouldn't this not return right away? i.e. this would mark "abc." as a
version
> > string.
> > Could we verify that all elements of the string are digits/periods/etc?
> 
> this was specifically to handle stuff like
> 
> "nProtect Anti-Virus/Spyware V4.0"
> "ESET NOD32 Antivirus 9.0.349.15P"
> 
> to be honest it doesn't actually matter too much if we're overzealous here as
> long as some semblance of the name is retained.
> 
> I could add 'V' and 'P' to the blacklist but then it starts to get sort of
> crazy... Happy to do what you think here...

hmm turns out this algorithm doesn't handle "nProtect Anti-Virus/Spyware V4.0"
anyway... working on it...

[rkaplow, 2017-01-12 23:29:13]

On 2017/01/12 23:23:59, Will Harris wrote:
> WDYT?
> 
>
https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
> File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):
> 
>
https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
> chrome/browser/metrics/antivirus_metrics_provider_win.cc:72: return true;
> On 2017/01/12 23:18:56, rkaplow wrote:
> > shouldn't this not return right away? i.e. this would mark "abc." as a
version
> > string.
> > Could we verify that all elements of the string are digits/periods/etc?
> 
> this was specifically to handle stuff like
> 
> "nProtect Anti-Virus/Spyware V4.0"
> "ESET NOD32 Antivirus 9.0.349.15P"
> 
> to be honest it doesn't actually matter too much if we're overzealous here as
> long as some semblance of the name is retained.
> 
> I could add 'V' and 'P' to the blacklist but then it starts to get sort of
> crazy... Happy to do what you think here...
> 
>
https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
> File chrome/browser/metrics/antivirus_metrics_provider_win.h (right):
> 
>
https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
> chrome/browser/metrics/antivirus_metrics_provider_win.h:88: static std::string
> TrimVersionOfAvProductName(const std::string& av_product);
> On 2017/01/12 23:18:56, rkaplow wrote:
> > hm, I'd add a small comment on some idea of what kind of algorithm it is
going
> > to try to identify what the version is
> 
> Acknowledged. Once we agree what the algorithm is :)

Yeah i hear you. I was just trying to make sure we we're cutting off too much.
My only other thought is maybe to see the the size of the {digits, .} set is
larger than the non-digits,. characters.
But honestly since this is free form there's no way we can handle all possible
cases, so if this works for all the ones we have now that seems fine to me.

[Will Harris, 2017-01-12 23:32:05]

I am thinking of simplifying it to just

bool ShouldFilterPart(const std::string& str) {
  if (str.empty())
    return false;
  // Special case for "360" (used by Norton) and "365" (used by Kaspersky).
  if (str == "365" || str == "360")
    return false;
  for (const auto ch : str) {
    if (isdigit(ch))
      return true;
  }
  return false;
}

basically any word after the first word that contains a number except 360 and
265 will be stripped. It's more aggressive but I don't think we lose any
fidelity.

[rkaplow, 2017-01-12 23:33:55]

Seems reasonable.

On Thu, Jan 12, 2017 at 6:32 PM, <wfh@chromium.org> wrote:

> I am thinking of simplifying it to just
>
> bool ShouldFilterPart(const std::string& str) {
> if (str.empty())
> return false;
> // Special case for "360" (used by Norton) and "365" (used by Kaspersky).
> if (str == "365" || str == "360")
> return false;
> for (const auto ch : str) {
> if (isdigit(ch))
> return true;
> }
> return false;
> }
>
> basically any word after the first word that contains a number except 360
> and
> 265 will be stripped. It's more aggressive but I don't think we lose any
> fidelity.
>
> https://codereview.chromium.org/2631583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Will Harris, 2017-01-12 23:39:06]

PTAL I think I'm happy with this. It doesn't change the g3 CL at all.

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.cc:72: return true;
On 2017/01/12 23:27:05, Will Harris wrote:
> On 2017/01/12 23:23:59, Will Harris wrote:
> > On 2017/01/12 23:18:56, rkaplow wrote:
> > > shouldn't this not return right away? i.e. this would mark "abc." as a
> version
> > > string.
> > > Could we verify that all elements of the string are digits/periods/etc?
> > 
> > this was specifically to handle stuff like
> > 
> > "nProtect Anti-Virus/Spyware V4.0"
> > "ESET NOD32 Antivirus 9.0.349.15P"
> > 
> > to be honest it doesn't actually matter too much if we're overzealous here
as
> > long as some semblance of the name is retained.
> > 
> > I could add 'V' and 'P' to the blacklist but then it starts to get sort of
> > crazy... Happy to do what you think here...
> 
> hmm turns out this algorithm doesn't handle "nProtect Anti-Virus/Spyware V4.0"
> anyway... working on it...

Done.

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
File chrome/browser/metrics/antivirus_metrics_provider_win.h (right):

https://codereview.chromium.org/2631583002/diff/1/chrome/browser/metrics/anti...
chrome/browser/metrics/antivirus_metrics_provider_win.h:88: static std::string
TrimVersionOfAvProductName(const std::string& av_product);
On 2017/01/12 23:23:59, Will Harris wrote:
> On 2017/01/12 23:18:56, rkaplow wrote:
> > hm, I'd add a small comment on some idea of what kind of algorithm it is
going
> > to try to identify what the version is
> 
> Acknowledged. Once we agree what the algorithm is :)

Done.

[rkaplow, 2017-01-12 23:44:27]

lgtm

https://codereview.chromium.org/2631583002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):

https://codereview.chromium.org/2631583002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/antivirus_metrics_provider_win.cc:65: if (str.empty())
don't think this is needed

[Will Harris, 2017-01-12 23:48:13]

Thanks for review

I'd wait to commit this until the g3 side with the new map goes in, so canary
data gets processed correctly.

https://codereview.chromium.org/2631583002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right):

https://codereview.chromium.org/2631583002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/antivirus_metrics_provider_win.cc:65: if (str.empty())
On 2017/01/12 23:44:27, rkaplow wrote:
> don't think this is needed

Done.

[Will Harris, 2017-01-13 16:41:01]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2017-01-13 16:41:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2631583002/#ps40001
(title: "remove unneeded code")

[commit-bot: I haz the power, 2017-01-13 16:41:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 17:27:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484325661372590,
"parent_rev": "5fa6332176e79da76fd8c513eeadd5379bba98d3", "commit_rev":
"da07d58a5d041395d3d6317ab3aef31c0087285e"}

[commit-bot: I haz the power, 2017-01-13 17:27:58]

Description was changed from

==========
Trim version and extraneus parts from AntiVirus product names.

Some AV products include their version in the product name in WMI which makes
maintaining the list of hashes of product names onerous.

Therefore, trim anything that looks like a version, along with trailing spaces.

BUG=615154
TEST=unit_tests --gtest_filter=AntiVirusMetricsProvider*
==========

to

==========
Trim version and extraneus parts from AntiVirus product names.

Some AV products include their version in the product name in WMI which makes
maintaining the list of hashes of product names onerous.

Therefore, trim anything that looks like a version, along with trailing spaces.

BUG=615154
TEST=unit_tests --gtest_filter=AntiVirusMetricsProvider*

Review-Url: https://codereview.chromium.org/2631583002
Cr-Commit-Position: refs/heads/master@{#443594}
Committed:
https://chromium.googlesource.com/chromium/src/+/da07d58a5d041395d3d6317ab3ae...
==========

[commit-bot: I haz the power, 2017-01-13 17:27:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/da07d58a5d041395d3d6317ab3ae...