EnumerateModules: Address UI review comments.

Make it a simple list with columns. Sort by status (conflicts at the
top), then by location and module name. Also, convert the type of DLL
to an enum so we can dedup the list (and for non-loaded DLLs show that
they are not loaded yet, but are of type: Shell Extension / WinSock.

Make sure lower bound version specified on the blacklist is inclusive
(first version that broke) and the higher bound version is exclusive
(first version that worked).

This allows us to pointpoint exactly when the failure started and
when the fix was introduced, instead of doing 0.9999 shenanigans.

Specify an upper bound for the idmmbc.dll conflict, since Henry's
outreach produced a fix in version 6.03 of the download manager.

BUG=51105
TEST=Unit test

Review URL: http://codereview.chromium.org/5278012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67598 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 111 insertions, 95 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index 7c3a6b2..c55248e 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -4217,10 +4217,7 @@ Keep your key file in a safe place. You will need it to create new versions of y
         Learn more
       </message>
       <message name="IDS_CONFLICTS_CHECK_WARNING_PREFIX" desc="Warning label prefix on the compatibility page">
-        Warning:
-      </message>
-      <message name="IDS_CONFLICTS_CHECK_VERSION_STRING" desc="The version string to show on the compatibility page">
-        Version <ph name="VERSION">$1<ex>1.2.3.4</ex></ph>.
+        Warning
       </message>
       <message name="IDS_CONFLICTS_CHECK_INVESTIGATING" desc="A label on the compatibility page saying we are investigating.">
         We are currently investigating this issue.
@@ -4244,19 +4241,22 @@ Keep your key file in a safe place. You will need it to create new versions of y
         Unable to detect any modules loaded.
       </message>
       <message name="IDS_CONFLICTS_HEADER_SOFTWARE" desc="The 'Software:' header for each module item in the list">
-        Software:
+        Software
       </message>
       <message name="IDS_CONFLICTS_HEADER_SIGNED_BY" desc="The 'Signed by:' header for each module item in the list">
-        Signed by:
+        Signed by
       </message>
       <message name="IDS_CONFLICTS_HEADER_LOCATION" desc="The 'Location:' header for each module item in the list">
-        Location:
+        Location
       </message>
       <message name="IDS_CONFLICTS_HEADER_WARNING" desc="The 'Warning:' header for each module item in the list">
-        Warning:
+        Warning
       </message>
       <message name="IDS_CONFLICTS_HEADER_HELP_TIP" desc="The 'Help tip:' header for each module item in the list">
-        Help tip:
+        Help tip
+      </message>
+      <message name="IDS_CONFLICTS_NOT_LOADED_YET" desc="Shown next to a module that has not been loaded into memory yet">
+        Not loaded yet
       </message>
       <message name="IDS_HELP_CENTER_VIEW_CONFLICTS" desc="The url of the Help center article for the View Conflicts page">
         http://www.google.com/support/chrome/bin/answer.py?answer=666386&amp;hl=[GRITLANGCODE]&amp;n=<ph name="NAME">$1<ex>Hash</ex></ph>&amp;l=<ph name="LOCATION">$2<ex>Hash</ex></ph>&amp;d=<ph name="DESC">$3<ex>Hash</ex></ph>&amp;s=<ph name="SIGNER">$4<ex>Hash</ex></ph>
diff --git a/chrome/browser/enumerate_modules_model_unittest_win.cc b/chrome/browser/enumerate_modules_model_unittest_win.cc
index 374ec42..841c420 100644
--- a/chrome/browser/enumerate_modules_model_unittest_win.cc
+++ b/chrome/browser/enumerate_modules_model_unittest_win.cc
@@ -160,9 +160,19 @@ const struct MatchingEntryList {
     kStandardModule,
     { kMatchName, kMatchLocation, kMatchSignature,
       kEmpty, kVersionHigh, ModuleEnumerator::SEE_LINK }
+  }, {  // Matches: Name, Location, Version lower is inclusive => Confirmed.
+    ModuleEnumerator::CONFIRMED_BAD,
+    kStandardModule,
+    { kMatchName, kMatchLocation, kMatchSignature,
+      "1.0", "2.0", ModuleEnumerator::SEE_LINK }
+  }, {  // Matches: Name, Location, Version higher is exclusive => No match.
+    ModuleEnumerator::NOT_MATCHED,
+    kStandardModule,
+    { kMatchName, kMatchLocation, kEmpty,
+      "0.0", "1.0", ModuleEnumerator::SEE_LINK }
   }, {  // All empty fields doesn't produce a match.
     ModuleEnumerator::NOT_MATCHED,
-    {kType, kStatus, L"", L"", L"", L"", L""},
+    { kType, kStatus, L"", L"", L"", L"", L""},
     { "a.dll", "", "", "", "", ModuleEnumerator::SEE_LINK }
   },
 };
diff --git a/chrome/browser/enumerate_modules_model_win.cc b/chrome/browser/enumerate_modules_model_win.cc
index 26b0469..f4ed945 100644
--- a/chrome/browser/enumerate_modules_model_win.cc
+++ b/chrome/browser/enumerate_modules_model_win.cc
@@ -37,12 +37,13 @@ static const int kModuleCheckDelayMs = 60 * 1000;
 static const wchar_t kRegPath[] =
     L"Software\\Microsoft\\Windows\\CurrentVersion\\Shell Extensions\\Approved";
 
-// A sort method that sorts by ModuleType ordinal (loaded module at the top),
-// then by full name (including path).
+// A sort method that sorts by bad modules first, then by full name (including
+// path).
 static bool ModuleSort(const ModuleEnumerator::Module& a,
                        const ModuleEnumerator::Module& b) {
-  if (a.type != b.type)
-    return a.type < b.type;
+  if (a.status != b.status)
+    return a.status > b.status;
+
   if (a.location == b.location)
     return a.name < b.name;
 
@@ -61,10 +62,8 @@ struct FindModule {
  public:
   explicit FindModule(const ModuleEnumerator::Module& x)
     : module(x) {}
-  // Coverity issue 13896.
   bool operator()(const ModuleEnumerator::Module& module_in) const {
-    return (module.type == module_in.type) &&
-           (module.location == module_in.location) &&
+    return (module.location == module_in.location) &&
            (module.name == module_in.name);
   }
 
@@ -91,7 +90,7 @@ const ModuleEnumerator::BlacklistEntry ModuleEnumerator::kModuleBlacklist[] = {
 
   // idmmbc.dll, "%programfiles%\\internet download manager\\", "Tonec Inc.".
   // See: http://crbug.com/26892/.
-  {"b8dce5c3", "94541bf5", "d33ad640", "", "", NONE},
+  {"b8dce5c3", "94541bf5", "d33ad640", "", "6.03", UPDATE},
 
   // imon.dll.  See: http://crbug.com/21715.
   {"8f42f22e", "", "", "", "", NONE},
@@ -194,7 +193,7 @@ ModuleEnumerator::ModuleStatus ModuleEnumerator::Match(
           module_version->CompareTo(*version_min.get()) < 0);
       bool too_high = version_max.get() &&
           (!module_version.get() ||
-          module_version->CompareTo(*version_max.get()) > 0);
+          module_version->CompareTo(*version_max.get()) >= 0);
       version_ok = !too_low && !too_high;
     }
 
@@ -251,6 +250,9 @@ void ModuleEnumerator::ScanOnFileThread() {
   PreparePathMappings();
 
   base::TimeTicks checkpoint = base::TimeTicks::Now();
+
+  // Enumerating loaded modules must happen first since the other types of
+  // modules check for duplication against the loaded modules.
   EnumerateLoadedModules();
   HISTOGRAM_TIMES("Conflicts.EnumerateLoadedModules",
                   base::TimeTicks::Now() - checkpoint);
@@ -403,10 +405,12 @@ void ModuleEnumerator::AddToListWithoutDuplicating(const Module& module) {
   iter = std::find_if(enumerated_modules_->begin(),
                       enumerated_modules_->end(),
                       FindModule(module));
-  if (iter != enumerated_modules_->end())
+  if (iter != enumerated_modules_->end()) {
     iter->duplicate_count++;
-  else
+    iter->type = static_cast<ModuleType>(iter->type | module.type);
+  } else {
     enumerated_modules_->push_back(module);
+  }
 }
 
 void ModuleEnumerator::PreparePathMappings() {
@@ -613,25 +617,28 @@ ListValue* EnumerateModulesModel::GetModuleList() {
        module != enumerated_modules_.end(); ++module) {
     DictionaryValue* data = new DictionaryValue();
     data->SetInteger("type", module->type);
-    switch (module->type) {
-      case ModuleEnumerator::SHELL_EXTENSION:
-        data->SetString("type_description", ASCIIToWide("Shell Extension"));
-        break;
-      case ModuleEnumerator::WINSOCK_MODULE_REGISTRATION:
-        data->SetString("type_description", ASCIIToWide("Winsock"));
-        break;
-      default:
-        data->SetString("type_description", ASCIIToWide(""));
-        break;
+    string16 type_string;
+    if ((module->type & ModuleEnumerator::LOADED_MODULE) == 0) {
+      // Module is not loaded, denote type of module.
+      if (module->type & ModuleEnumerator::SHELL_EXTENSION)
+        type_string = ASCIIToWide("Shell Extension");
+      if (module->type & ModuleEnumerator::WINSOCK_MODULE_REGISTRATION) {
+        if (!type_string.empty())
+          type_string += ASCIIToWide(", ");
+        type_string += ASCIIToWide("Winsock");
+      }
+      // Must be one of the above type.
+      DCHECK(!type_string.empty());
+      type_string += ASCIIToWide(" -- ");
+      type_string += l10n_util::GetStringUTF16(IDS_CONFLICTS_NOT_LOADED_YET);
     }
+    data->SetString("type_description", type_string);
     data->SetInteger("status", module->status);
     data->SetString("location", module->location);
     data->SetString("name", module->name);
     data->SetString("product_name", module->product_name);
     data->SetString("description", module->description);
-    data->SetString("version", module->version.empty() ? ASCIIToWide("") :
-        l10n_util::GetStringF(IDS_CONFLICTS_CHECK_VERSION_STRING,
-                   module->version));
+    data->SetString("version", module->version);
     data->SetString("digital_signer", module->digital_signer);
 
     // Figure out the possible resolution help string.
diff --git a/chrome/browser/enumerate_modules_model_win.h b/chrome/browser/enumerate_modules_model_win.h
index cf65eaf..9000a6c 100644
--- a/chrome/browser/enumerate_modules_model_win.h
+++ b/chrome/browser/enumerate_modules_model_win.h
@@ -29,9 +29,9 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   // modules of interest and may or may not be loaded in the process at the
   // time of scan.
   enum ModuleType {
-    LOADED_MODULE,
-    SHELL_EXTENSION,
-    WINSOCK_MODULE_REGISTRATION,
+    LOADED_MODULE               = 1 << 0,
+    SHELL_EXTENSION             = 1 << 1,
+    WINSOCK_MODULE_REGISTRATION = 1 << 2,
   };
 
   // The blacklist status of the module. Suspected Bad modules have been
@@ -96,8 +96,8 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
     const char* filename;
     const char* location;
     const char* desc_or_signer;
-    const char* version_from;
-    const char* version_to;
+    const char* version_from;  // Version where conflict started.
+    const char* version_to;    // First version that works.
     RecommendedAction help_tip;
   };
 
diff --git a/chrome/browser/resources/about_conflicts.html b/chrome/browser/resources/about_conflicts.html
index c4365ff..14274f49 100644
--- a/chrome/browser/resources/about_conflicts.html
+++ b/chrome/browser/resources/about_conflicts.html
@@ -128,12 +128,21 @@ html[dir=rtl] #top {
 }
 
 .suspected-bad {
-  color: orange;
+  color: #DD7700;
 }
 
 .confirmed-bad {
   color: red;
 }
+
+.nowrap {
+  white-space: nowrap;
+}
+
+.extra-info-text {
+  margin-top: -1em;
+  margin-bottom: 1em;
+}
 </style>
 <script>

 

@@ -230,62 +239,52 @@ html[dir=rtl] #top {
         <div i18n-content="modulesNoneLoaded">NO_MODULES_ARE_AVAILABLE</div>
       </div>
 
-      <div jsdisplay="moduleList.length > 0">
-      <div class="module" jsselect="moduleList">
-        <table width="100%" cellpadding="0" cellspacing="0">
-        <tr class="module-loaded">
-        <td valign="top">
-          <table cellpadding="2" cellspacing="0">
-            <tr>
-              <td colspan="2"><span class="module-name" dir="ltr"
-                                   jscontent="name">NAME</span>
-                              <span dir="ltr"
-                                    jsdisplay="type_description.length > 0"
-                              >(<span dir="ltr"
-                                      jscontent="type_description">MODULE_TYPE</span
-                              >)</span>
-              </td>
-            </tr>
-            <tr>
-              <td width="75"><span i18n-content="headerSoftware" /></td>
-              <td><span dir="ltr" jsvalues=".innerHTML:description"></span>
-                  <span dir="ltr" jsdisplay="version.length > 0"> - </span>
-                  <span dir="ltr" jscontent="version">VERSION</span></td>
-            </tr>
-            <tr jsdisplay="digital_signer.length > 0">
-              <td><span i18n-content="headerSignedBy" /></td>
-              <td><span dir="ltr" jscontent="digital_signer">SIGNER</span></td>
-            </tr>
-            <tr>
-              <td><span i18n-content="headerLocation" /></td>
-              <td><span dir="ltr"
-                        jscontent="location">LOCATION</span><span
-                            dir="ltr" jscontent="name">NAME</span></td>
-            </tr>
-            <tr jsdisplay="status == 2 || status == 3">
-              <td><span i18n-content="headerWarning" /></td>
-              <td><span jsdisplay="status == 2"
-                        i18n-content="moduleSuspectedBad"
-                        class="suspected-bad">SUSPECTED_BAD</span>
-                  <span jsdisplay="status == 3"
-                        i18n-content="moduleConfirmedBad"
-                        class="confirmed-bad">CONFIRMED_BAD</span>
-                  <a jsdisplay="help_url.length > 0"
-                     jsvalues=".href:help_url"><span
-                         i18n-content="helpCenterLink">HELP_CENTER</span></a>
-              </td>
-            </tr>
-            <tr jsdisplay="possibleResolution.length > 0">
-              <td><span i18n-content="headerHelpTip" /></td>
-              <td><span
-                  jscontent="possibleResolution">POSSIBLE_RESOLUTION</span></td>
-            </tr>
-          </table>
-
-        </td>
-        </tr>
+      <div jsdisplay="moduleList.length &gt; 0">
+      <table width="100%" cellpadding="0" cellspacing="0">
+      <tr class="module-loaded">
+      <td valign="top">
+        <table cellpadding="2" cellspacing="0" border="0">
+          <tr jsselect="moduleList">
+            <td valign="top">
+              <span dir="ltr"
+                    jsvalues=".innerHTML:description" class="nowrap"></span>
+              <div jsdisplay="status == 2 || status == 3"
+                   class="extra-info-text"><br>
+                <span jsdisplay="status == 2"
+                      i18n-content="moduleSuspectedBad"
+                      class="suspected-bad">SUSPECTED_BAD</span>
+                <span jsdisplay="status == 3"
+                      i18n-content="moduleConfirmedBad"
+                      class="confirmed-bad">CONFIRMED_BAD</span>
+                <a jsdisplay="help_url.length &gt; 0"
+                   jsvalues=".href:help_url"><span
+                      i18n-content="helpCenterLink">HELP_CENTER</span></a>
+                <span jsdisplay="possibleResolution.length &gt; 0"><br>
+                  <span jscontent="possibleResolution"
+                      >POSSIBLE_RESOLUTION</span>
+                </span>
+              </div>
+            </td>
+            <td valign="top"><span dir="ltr" jscontent="digital_signer"
+                                   class="nowrap">SIGNER</span></td>
+            <td valign="top"><span dir="ltr" jscontent="version"
+                                   class="nowrap">VERSION</span></td>
+            <td valign="top">
+              <span class="nowrap">
+                <span dir="ltr" jscontent="location">LOCATION</span><strong
+                    ><span dir="ltr" jscontent="name">NAME</span></strong>
+                <span dir="ltr"
+                      jsdisplay="type_description.length &gt; 0"
+                    >(<span dir="ltr"
+                            jscontent="type_description">MODULE_TYPE</span
+                    >)</span>
+              </span>
+            </td>
+          </tr>
         </table>
-      </div>
+      </td>
+      </tr>
+      </table>
       </div>
     </div>
   </div>