Comparison and streaming operators for base::Version

chrome/common/chrome_content_client.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/chrome_content_client.h"
 
 #include <stdint.h>
 
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
@@ skipping 452 matching lines @@
 #if defined(ENABLE_PLUGINS)
 // static
 content::PepperPluginInfo* ChromeContentClient::FindMostRecentPlugin(
     const std::vector<content::PepperPluginInfo*>& plugins) {
   auto it = std::max_element(
       plugins.begin(), plugins.end(),
       [](content::PepperPluginInfo* x, content::PepperPluginInfo* y) {
         Version version_x(x->version);
         Version version_y(y->version);
         DCHECK(version_x.IsValid() && version_y.IsValid());
-        if (version_x.Equals(version_y))
+        if (version_x == version_y) {

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

There's no need to add {} here: generally we wouldn't do this for single-line
ones.

[Rob Percival, 2016/01/09 23:54:03]

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.

           return !x->is_debug && y->is_debug;
+        }
         return version_x.IsOlderThan(y->version);

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

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.

[Rob Percival, 2016/01/09 23:54:03]

Done.

       });
   return it != plugins.end() ? *it : nullptr;
 }
 #endif  // defined(ENABLE_PLUGINS)
 
 void ChromeContentClient::AddPepperPlugins(
     std::vector<content::PepperPluginInfo>* plugins) {
 #if defined(ENABLE_PLUGINS)
   ComputeBuiltInPlugins(plugins);
   AddPepperFlashFromCommandLine(plugins);
@@ skipping 144 matching lines @@
 #endif
 }
 
 bool ChromeContentClient::IsSupplementarySiteIsolationModeEnabled() {
 #if defined(ENABLE_EXTENSIONS)
   return extensions::IsIsolateExtensionsEnabled();
 #else
   return false;
 #endif
 }