Comparison and streaming operators for base::Version

Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/discussion

Replaces Version::Equals and Version::IsOlderThan.

Committed: https://crrev.com/dcd8b10251803c2572bb5c45d4535ff19e877a0a
Cr-Commit-Position: refs/heads/master@{#371279}

[Rob Percival, 2016-01-08 18:44:29]

Description was changed from

==========
Comparison and streaming operators for base::Version

See discussion here:
https://groups.google.com/a/chromium.org/forum/#!mydiscussions/chromium-dev/8...

Replaces Version::Equals.
==========

to

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/forum/#!mydiscussions/chromium-dev/8...

Replaces Version::Equals.
==========

[Rob Percival, 2016-01-08 18:44:29]

Description was changed from

==========
Comparison and streaming operators for base::Version

See discussion here:
https://groups.google.com/a/chromium.org/forum/#!mydiscussions/chromium-dev/8...

Replaces Version::Equals.
==========

to

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/forum/#!mydiscussions/chromium-dev/8...

Replaces Version::Equals.
==========

[Rob Percival, 2016-01-08 18:44:57]

robpercival@chromium.org changed reviewers:
+ jam@chromium.org

[Rob Percival, 2016-01-08 18:49:07]

robpercival@chromium.org changed reviewers:
+ brettw@chromium.org

[Rob Percival, 2016-01-08 18:49:24]

robpercival@chromium.org changed reviewers:
- jam@chromium.org

[Rob Percival, 2016-01-08 19:04:41]

Description was changed from

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/forum/#!mydiscussions/chromium-dev/8...

Replaces Version::Equals.
==========

to

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals.
==========

[brettw, 2016-01-08 19:31:16]

https://codereview.chromium.org/1575523002/diff/1/base/version.h
File base/version.h (right):

https://codereview.chromium.org/1575523002/diff/1/base/version.h#newcode70
base/version.h:70: BASE_EXPORT std::ostream& operator<<(std::ostream& stream,
const Version& v);
I think you need to include iosfwd so ostream is defined for this in all cases.

https://codereview.chromium.org/1575523002/diff/1/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/1575523002/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:473: if (version_x == version_y) {
There's no need to add {} here: generally we wouldn't do this for single-line
ones.

https://codereview.chromium.org/1575523002/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:476: return
version_x.IsOlderThan(y->version);
Can you update this to:
  return version_x < version_y;
As-is this will parse the Y version string again to do the comparison which is
wasteful since we already did that.

I actually think we should remove IsOlderThan entirely. This falls under the
avoidance of operators rule, and it's only used in a couple of places.

I see only 15 occurrences in the code of this call and many of those aren't
Version objects so don't need to be updated. Of the rest, some are silly and
wasteful like this one and extension_service_unittest.cc. So let's just get rid
of those while we're at it.

https://codereview.chromium.org/1575523002/diff/1/components/component_update...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1575523002/diff/1/components/component_update...
components/component_updater/default_component_installer.cc:121: if
(current_version_ == base::Version(kNullVersion)) {
Don't add {}

[Rob Percival, 2016-01-09 23:54:04]

https://codereview.chromium.org/1575523002/diff/1/base/version.cc
File base/version.cc (right):

https://codereview.chromium.org/1575523002/diff/1/base/version.cc#newcode37
base/version.cc:37: // this string conversion.
What does the above TODO mean?

https://codereview.chromium.org/1575523002/diff/1/base/version.h
File base/version.h (right):

https://codereview.chromium.org/1575523002/diff/1/base/version.h#newcode70
base/version.h:70: BASE_EXPORT std::ostream& operator<<(std::ostream& stream,
const Version& v);
On 2016/01/08 19:31:16, brettw wrote:
> I think you need to include iosfwd so ostream is defined for this in all
cases.

Done.

https://codereview.chromium.org/1575523002/diff/1/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/1575523002/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:473: if (version_x == version_y) {
On 2016/01/08 19:31:16, brettw wrote:
> There's no need to add {} here: generally we wouldn't do this for single-line
> ones.

Done. I'm in the habit of putting braces in since I saw a critical iOS TLS bug
(http://arstechnica.com/security/2014/02/extremely-critical-crypto-flaw-in-ios...)
that might have been avoided if braces had been used. Since that's not the
convention for Chromium, I'll refrain in the future.

https://codereview.chromium.org/1575523002/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:476: return
version_x.IsOlderThan(y->version);
On 2016/01/08 19:31:16, brettw wrote:
> Can you update this to:
>   return version_x < version_y;
> As-is this will parse the Y version string again to do the comparison which is
> wasteful since we already did that.
> 
> I actually think we should remove IsOlderThan entirely. This falls under the
> avoidance of operators rule, and it's only used in a couple of places.
> 
> I see only 15 occurrences in the code of this call and many of those aren't
> Version objects so don't need to be updated. Of the rest, some are silly and
> wasteful like this one and extension_service_unittest.cc. So let's just get
rid
> of those while we're at it.

Done.

https://codereview.chromium.org/1575523002/diff/1/components/component_update...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1575523002/diff/1/components/component_update...
components/component_updater/default_component_installer.cc:121: if
(current_version_ == base::Version(kNullVersion)) {
On 2016/01/08 19:31:16, brettw wrote:
> Don't add {}

Done.

[Rob Percival, 2016-01-09 23:54:59]

Description was changed from

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals.
==========

to

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals and Version::IsOlderThan.
==========

[Rob Percival, 2016-01-18 10:40:06]

PTAL

[brettw, 2016-01-19 23:33:33]

lgtm

https://codereview.chromium.org/1575523002/diff/1/base/version.cc
File base/version.cc (right):

https://codereview.chromium.org/1575523002/diff/1/base/version.cc#newcode37
base/version.cc:37: // this string conversion.
On 2016/01/09 23:54:03, Rob Percival wrote:
> What does the above TODO mean?

Look like it's obsolete and we can delete the TODO.

[Rob Percival, 2016-01-20 22:09:02]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-20 22:09:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/40001

[commit-bot: I haz the power, 2016-01-20 22:28:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 22:28:21]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Rob Percival, 2016-01-21 11:05:15]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 11:05:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/60001

[commit-bot: I haz the power, 2016-01-21 11:30:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 11:30:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Rob Percival, 2016-01-21 13:22:18]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 13:22:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/80001

[commit-bot: I haz the power, 2016-01-21 13:36:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 13:36:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Rob Percival, 2016-01-22 14:02:01]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-22 14:02:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/100001

[commit-bot: I haz the power, 2016-01-22 14:38:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-22 14:38:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rob Percival, 2016-01-22 18:20:10]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-22 18:20:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/120001

[commit-bot: I haz the power, 2016-01-22 19:05:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-22 19:05:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Rob Percival, 2016-01-25 14:19:50]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-25 14:20:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/160001

[commit-bot: I haz the power, 2016-01-25 16:16:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-25 16:16:47]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-01-25 16:35:46]

On 2016/01/25 16:16:47, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Now this is *finally* building on Windows, are you happy for me to commit it?

[brettw, 2016-01-25 19:21:43]

lgtm

[Rob Percival, 2016-01-25 19:26:10]

The CQ bit was checked by robpercival@chromium.org

[commit-bot: I haz the power, 2016-01-25 19:29:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575523002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575523002/160001

[commit-bot: I haz the power, 2016-01-25 19:39:20]

Description was changed from

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals and Version::IsOlderThan.
==========

to

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals and Version::IsOlderThan.
==========

[commit-bot: I haz the power, 2016-01-25 19:39:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-25 19:53:55]

Description was changed from

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals and Version::IsOlderThan.
==========

to

==========
Comparison and streaming operators for base::Version

Changes base::Version to comply with updated style guide.

See discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8Q6Po7y0Kc4/dis...

Replaces Version::Equals and Version::IsOlderThan.

Committed: https://crrev.com/dcd8b10251803c2572bb5c45d4535ff19e877a0a
Cr-Commit-Position: refs/heads/master@{#371279}
==========

[commit-bot: I haz the power, 2016-01-25 19:53:56]

Patchset 9 (id:??) landed as
https://crrev.com/dcd8b10251803c2572bb5c45d4535ff19e877a0a
Cr-Commit-Position: refs/heads/master@{#371279}