Normalize extension action icon paths before adding them to icon set

chrome/common/extensions/extension.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/extensions/extension.h"
 
 #include <ostream>
 
 #include "base/base64.h"
 #include "base/basictypes.h"
@@ skipping 132 matching lines @@
   for (size_t i = 0; i < num_icon_sizes; ++i) {
     std::string key = base::IntToString(icon_sizes[i]);
     if (icons_value->HasKey(key)) {
       std::string icon_path;
       if (!icons_value->GetString(key, &icon_path)) {
         *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
             errors::kInvalidIconPath, key);
         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);
     }
   }
   return true;
 }
 
 // A singleton object containing global data needed by the extension objects.
 class ExtensionConfig {
  public:
   static ExtensionConfig* GetInstance() {
@@ skipping 679 matching lines @@
     if (extension_action->HasKey(keys::kPageActionIcons) &&
         extension_action->GetList(keys::kPageActionIcons, &icons)) {
       for (ListValue::const_iterator iter = icons->begin();
            iter != icons->end(); ++iter) {
         std::string path;
         if (!(*iter)->GetAsString(&path) || path.empty()) {
           *error = ASCIIToUTF16(errors::kInvalidPageActionIconPath);
           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());
         break;
       }
     }
 
     std::string id;
     if (extension_action->HasKey(keys::kPageActionId)) {
       if (!extension_action->GetString(keys::kPageActionId, &id)) {
@@ skipping 18 matching lines @@
                                    extension_misc::kNumExtensionActionIconSizes,
                                    default_icons.get(),
                                    error)) {
         return scoped_ptr<ExtensionAction>();
       }
 
       result->set_default_icon(default_icons.Pass());
     } 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());
     } else {
       *error = ASCIIToUTF16(errors::kInvalidPageActionIconPath);
       return scoped_ptr<ExtensionAction>();
     }
   }
 
   // Read the page action title from |default_title| if present, |name| if not
@@ skipping 1502 matching lines @@
     install_warnings_.push_back(
         InstallWarning(InstallWarning::FORMAT_TEXT,
                        errors::kScriptBadgeIconIgnored));
   }
 
   scoped_ptr<ExtensionIconSet> icon_set(new ExtensionIconSet);
 
   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()) {
     script_badge_->set_default_icon(icon_set.Pass());
   } else {
     script_badge_->set_default_icon(scoped_ptr<ExtensionIconSet>());
   }
 
   return true;
 }
@@ skipping 1588 matching lines @@
 
 UpdatedExtensionPermissionsInfo::UpdatedExtensionPermissionsInfo(
     const Extension* extension,
     const PermissionSet* permissions,
     Reason reason)
     : reason(reason),
       extension(extension),
       permissions(permissions) {}
 
 }   // namespace extensions