Preprocessing version number for plugins to ignore leading zeros during comparisons. This eliminate…

Preprocessing version number for plugins to ignore leading zeros during comparisons. This eliminates wrong version number comparisons for versions like 1.7.0_05 vs. 1.7.0_5

BUG=137388
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151265

[Bernhard Bauer, 2012-08-13 11:25:43]

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_group.cc (right):

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group.cc:106: std::string no_leading_zeros_version =
"";
Nit: The default constructor for std::string already is an empty string.

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group.cc:115: no_leading_zeros_version += (j < n) ?
numbers[i].substr(j) : "0";
I think this slightly changes the behavior for empty version components: 1..0
with two dots would now be normalized to 1.0.0, whereas it would be rejected in
the old version. I think I'm okay with that, but we might want to ratify that
behavior via a unit test.

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_group_unittest.cc (right):

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group_unittest.cc:37: 
Could you add an ASSERT here that the parsed version is valid?

[ibraaaa, 2012-08-13 12:26:44]

Comments Addressed, PTAL.

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_group.cc (right):

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group.cc:106: std::string no_leading_zeros_version =
"";
On 2012/08/13 11:25:43, Bernhard Bauer wrote:
> Nit: The default constructor for std::string already is an empty string.

Done.

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group.cc:115: no_leading_zeros_version += (j < n) ?
numbers[i].substr(j) : "0";
On 2012/08/13 11:25:43, Bernhard Bauer wrote:
> I think this slightly changes the behavior for empty version components: 1..0
> with two dots would now be normalized to 1.0.0, whereas it would be rejected
in
> the old version. I think I'm okay with that, but we might want to ratify that
> behavior via a unit test.

Done.

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_group_unittest.cc (right):

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group_unittest.cc:37: 
On 2012/08/13 11:25:43, Bernhard Bauer wrote:
> Could you add an ASSERT here that the parsed version is valid?

Done.

[Bernhard Bauer, 2012-08-13 12:31:11]

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_group_unittest.cc (right):

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group_unittest.cc:37: 
On 2012/08/13 12:26:45, ibraaaa wrote:
> On 2012/08/13 11:25:43, Bernhard Bauer wrote:
> > Could you add an ASSERT here that the parsed version is valid?
> 
> Done.

Can you change it to an ASSERT_TRUE? (The reason I'm asking is that
Version::GetString() DCHECKs that it is valid, which would cause a crash. ASSERT
aborts the test before that.)

[ibraaaa, 2012-08-13 12:39:35]

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_group_unittest.cc (right):

http://codereview.chromium.org/10828279/diff/2001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_group_unittest.cc:37: 
On 2012/08/13 12:31:12, Bernhard Bauer wrote:
> On 2012/08/13 12:26:45, ibraaaa wrote:
> > On 2012/08/13 11:25:43, Bernhard Bauer wrote:
> > > Could you add an ASSERT here that the parsed version is valid?
> > 
> > Done.
> 
> Can you change it to an ASSERT_TRUE? (The reason I'm asking is that
> Version::GetString() DCHECKs that it is valid, which would cause a crash.
ASSERT
> aborts the test before that.)

Done.

[Bernhard Bauer, 2012-08-13 13:05:45]

LGTM! 

You should be able to check the box for the commit queue to land this CL; let me
know if you run into trouble.

[commit-bot: I haz the power, 2012-08-13 13:33:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10828279/11001

[commit-bot: I haz the power, 2012-08-13 15:07:39]

Change committed as 151265