Normalize extension action icon paths before adding them to icon set

chrome/common/extensions/extension.cc

Index: chrome/common/extensions/extension.cc
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 1b3e947a1b50a4fadc104b8175c0f88978ceaca4..f6df45d0d04ed52f71d3f23c8c147b654c426951 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -150,14 +150,15 @@ bool LoadIconsFromDictionary(const DictionaryValue* icons_value,
         return false;
       }
 
-      if (!icon_path.empty() && icon_path[0] == '/')
-        icon_path = icon_path.substr(1);
-
       if (icon_path.empty()) {
         *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
             errors::kInvalidIconPath, key);
         return false;
       }
+
+      if (icon_path[0] == '/')

[not at google - send to devlin, 2012/09/20 06:00:55]

should be "while" not "if"?

also seems like this should go before the icon_path.empty check since we would
want "/" and "//" etc to trigger the invalid icon path error.

[tbarzic, 2012/09/20 06:45:06]

Done.

+        icon_path = icon_path.substr(1);
+
       icons->Add(icon_sizes[i], icon_path);
     }
   }
@@ -857,6 +858,9 @@ scoped_ptr<ExtensionAction> Extension::LoadExtensionActionHelper(
           return scoped_ptr<ExtensionAction>();
         }
 
+        if (path[0] == '/')

[not at google - send to devlin, 2012/09/20 06:00:55]

ditto while?

actually it seems like this (and same below) isn't compatible with the check
above:

if (!(*iter)->GetAsString(&path) || path.empty()) {
  // error
}

i.e. the path.empty() check. As I said above, trimming the absolute paths out of
the icon might still leave it empty and we'd want to show a warning.

how about adding a method like NormalizeAndValidatePath method which takes a
pointer, so this would be

if (!(*iter)->GetAsString(&path) || !NormalizeAndValidatePath(path)) {
  // error
}

and use elsewhere.

or something like that.

[tbarzic, 2012/09/20 06:45:06]

Yeah, good point.. Done.

+          path = path.substr(1);
+
         scoped_ptr<ExtensionIconSet> icon_set(new ExtensionIconSet);
         icon_set->Add(extension_misc::EXTENSION_ICON_ACTION, path);
         result->set_default_icon(icon_set.Pass());
@@ -895,6 +899,9 @@ scoped_ptr<ExtensionAction> Extension::LoadExtensionActionHelper(
     } else if (extension_action->GetString(keys::kPageActionDefaultIcon,
                                            &default_icon) &&
                !default_icon.empty()) {
+      if (default_icon[0] == '/')
+        default_icon = default_icon.substr(1);
+
       scoped_ptr<ExtensionIconSet> icon_set(new ExtensionIconSet);
       icon_set->Add(extension_misc::EXTENSION_ICON_ACTION, default_icon);
       result->set_default_icon(icon_set.Pass());
@@ -2417,9 +2424,8 @@ bool Extension::LoadScriptBadge(string16* error) {
   for (size_t i = 0; i < extension_misc::kNumScriptBadgeIconSizes; i++) {
     std::string path = icons().Get(extension_misc::kScriptBadgeIconSizes[i],
                                    ExtensionIconSet::MATCH_EXACTLY);
-    if (!path.empty()) {
+    if (!path.empty())
       icon_set->Add(extension_misc::kScriptBadgeIconSizes[i], path);
-    }
   }
 
   if (!icon_set->map().empty()) {