Improve extension packaging by filtering more kinds of files.

When packaging extensions, ignore files with Windows hidden-file attribute, as well as the special OS X file __MACOSX. These are in addition to the existing dotfile exclusion rule.

BUG=27840
TEST=added chrome/browser/extensions/extension_creator_filter_unittest.cc


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

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

9 files changed, 208 insertions, 16 deletions

diff --git a/chrome/browser/extensions/extension_creator.cc b/chrome/browser/extensions/extension_creator.cc
index 6cf0f4f..1b7af2aef 100644
--- a/chrome/browser/extensions/extension_creator.cc
+++ b/chrome/browser/extensions/extension_creator.cc
@@ -7,10 +7,13 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/file_util.h"
 #include "base/memory/scoped_handle.h"
 #include "base/scoped_temp_dir.h"
 #include "base/string_util.h"
+#include "chrome/browser/extensions/extension_creator_filter.h"
 #include "chrome/browser/extensions/sandboxed_extension_unpacker.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_file_util.h"
@@ -153,7 +156,10 @@ bool ExtensionCreator::CreateZip(const FilePath& extension_dir,
                                  FilePath* zip_path) {
   *zip_path = temp_path.Append(FILE_PATH_LITERAL("extension.zip"));
 
-  if (!Zip(extension_dir, *zip_path, false)) {  // no hidden files
+  scoped_refptr<ExtensionCreatorFilter> filter = new ExtensionCreatorFilter();
+  const base::Callback<bool(const FilePath&)>& filter_cb =
+    base::Bind(&ExtensionCreatorFilter::ShouldPackageFile, filter.get());
+  if (!Zip(extension_dir, *zip_path, filter_cb)) {
     error_message_ =
         l10n_util::GetStringUTF8(IDS_EXTENSION_FAILED_DURING_PACKAGING);
     return false;
diff --git a/chrome/browser/extensions/extension_creator_filter.cc b/chrome/browser/extensions/extension_creator_filter.cc
new file mode 100644
index 0000000..07881fa
--- /dev/null
+++ b/chrome/browser/extensions/extension_creator_filter.cc
@@ -0,0 +1,36 @@
+// Copyright (c) 2011 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/browser/extensions/extension_creator_filter.h"
+
+#include "base/file_path.h"
+
+#if defined(OS_WIN)
+#include <windows.h>
+#endif
+
+bool ExtensionCreatorFilter::ShouldPackageFile(const FilePath& file_path) {
+  const FilePath& base_name = file_path.BaseName();
+
+  if (base_name.BaseName().value()[0] == '.') {
+    return false;
+  }
+  if (base_name.value() == FILE_PATH_LITERAL("__MACOSX")) {
+    return false;
+  }
+
+#if defined(OS_WIN)
+  // It's correct that we use file_path, not base_name, here, because we
+  // are working with the actual file.
+  DWORD file_attributes = ::GetFileAttributes(file_path.value().c_str());
+  if (file_attributes == INVALID_FILE_ATTRIBUTES) {
+    return false;
+  }
+  if ((file_attributes & FILE_ATTRIBUTE_HIDDEN) != 0) {
+    return false;
+  }
+#endif
+
+  return true;
+}
diff --git a/chrome/browser/extensions/extension_creator_filter.h b/chrome/browser/extensions/extension_creator_filter.h
new file mode 100644
index 0000000..21bf50b
--- /dev/null
+++ b/chrome/browser/extensions/extension_creator_filter.h
@@ -0,0 +1,30 @@
+// Copyright (c) 2011 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_CREATOR_FILTER_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_CREATOR_FILTER_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+
+class FilePath;
+
+// Determines which files should be included in a packaged extension.
+// Designed specifically to operate with the callback in chrome/common/zip.
+class ExtensionCreatorFilter
+      : public base::RefCounted<ExtensionCreatorFilter> {
+ public:
+  ExtensionCreatorFilter() {}
+
+  // Returns true if the given FilePath should be included in a
+  // packed extension.
+  bool ShouldPackageFile(const FilePath& file_path);
+
+ private:
+  friend class base::RefCounted<ExtensionCreatorFilter>;
+  ~ExtensionCreatorFilter() {}
+  DISALLOW_COPY_AND_ASSIGN(ExtensionCreatorFilter);
+};
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_CREATOR_FILTER_H_
diff --git a/chrome/browser/extensions/extension_creator_filter_unittest.cc b/chrome/browser/extensions/extension_creator_filter_unittest.cc
new file mode 100644
index 0000000..3b287e7
--- /dev/null
+++ b/chrome/browser/extensions/extension_creator_filter_unittest.cc
@@ -0,0 +1,97 @@
+// Copyright (c) 2011 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 "base/file_util.h"
+#include "base/scoped_ptr.h"
+#include "base/scoped_temp_dir.h"
+#include "chrome/browser/extensions/extension_creator_filter.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace {
+
+class ExtensionCreatorFilterTest : public PlatformTest {
+ protected:
+  virtual void SetUp() {
+    PlatformTest::SetUp();
+
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    test_dir_ = temp_dir_.path();
+
+    filter_ = new ExtensionCreatorFilter();
+  }
+
+  FilePath CreateTestFile(const FilePath& file_path) {
+    FilePath test_file(test_dir_.Append(file_path));
+    FilePath temp_file;
+    EXPECT_TRUE(file_util::CreateTemporaryFileInDir(test_dir_, &temp_file));
+    EXPECT_TRUE(file_util::Move(temp_file, test_file));
+    return test_file;
+  }
+
+  scoped_refptr<ExtensionCreatorFilter> filter_;
+
+  ScopedTempDir temp_dir_;
+
+  FilePath test_dir_;
+};
+
+struct UnaryBooleanTestData {
+  const FilePath::CharType* input;
+  bool expected;
+};
+
+TEST_F(ExtensionCreatorFilterTest, NormalCases) {
+  const struct UnaryBooleanTestData cases[] = {
+    { FILE_PATH_LITERAL("foo"), true },
+    { FILE_PATH_LITERAL(".foo"), false },
+    { FILE_PATH_LITERAL(".svn"), false },
+    { FILE_PATH_LITERAL("__MACOSX"), false },
+  };
+
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    FilePath input(cases[i].input);
+    FilePath test_file(CreateTestFile(input));
+    bool observed = filter_->ShouldPackageFile(test_file);
+
+    EXPECT_EQ(cases[i].expected, observed) <<
+      "i: " << i << ", input: " << test_file.value();
+  }
+}
+
+#if defined(OS_WIN)
+struct StringBooleanWithBooleanTestData {
+  const FilePath::CharType* input_char;
+  bool input_bool;
+  bool expected;
+};
+
+TEST_F(ExtensionCreatorFilterTest, WindowsHiddenFiles) {
+  const struct StringBooleanWithBooleanTestData cases[] = {
+    { FILE_PATH_LITERAL("a-normal-file"), false, true },
+    { FILE_PATH_LITERAL(".a-dot-file"), false, false },
+    { FILE_PATH_LITERAL(".a-dot-file-that-we-have-set-to-hidden"),
+      true, false },
+    { FILE_PATH_LITERAL("a-file-that-we-have-set-to-hidden"), true, false },
+    { FILE_PATH_LITERAL("a-file-that-we-have-not-set-to-hidden"),
+      false, true },
+  };
+
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    FilePath input(cases[i].input_char);
+    bool should_hide = cases[i].input_bool;
+    FilePath test_file(CreateTestFile(input));
+
+    if (should_hide) {
+      SetFileAttributes(test_file.value().c_str(), FILE_ATTRIBUTE_HIDDEN);
+    }
+    bool observed = filter_->ShouldPackageFile(test_file);
+    EXPECT_EQ(cases[i].expected, observed) <<
+      "i: " << i << ", input: " << test_file.value();
+  }
+}
+#endif
+
+}  // namespace
+
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 410d72c..29f494f 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -940,6 +940,8 @@
         'browser/extensions/extension_cookies_helpers.h',
         'browser/extensions/extension_creator.cc',
         'browser/extensions/extension_creator.h',
+        'browser/extensions/extension_creator_filter.cc',
+        'browser/extensions/extension_creator_filter.h',
         'browser/extensions/extension_data_deleter.cc',
         'browser/extensions/extension_data_deleter.h',
         'browser/extensions/extension_debugger_api.cc',
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 4ab5fd5..a49496f 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1420,6 +1420,7 @@
         'browser/extensions/apps_promo_unittest.cc',
         'browser/extensions/convert_user_script_unittest.cc',
         'browser/extensions/convert_web_app_unittest.cc',
+        'browser/extensions/extension_creator_filter_unittest.cc',
         'browser/extensions/extension_event_router_forwarder_unittest.cc',
         'browser/extensions/extension_icon_manager_unittest.cc',
         'browser/extensions/extension_info_map_unittest.cc',
diff --git a/chrome/common/zip.cc b/chrome/common/zip.cc
index eb6aa40..b834b9a 100644
--- a/chrome/common/zip.cc
+++ b/chrome/common/zip.cc
@@ -5,7 +5,6 @@
 #include "chrome/common/zip.h"
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_split.h"
@@ -264,8 +263,8 @@ static bool AddEntryToZip(zipFile zip_file, const FilePath& path,
   return success;
 }
 
-static bool Zip(const FilePath& src_dir, const FilePath& dest_file,
-                const base::Callback<bool(const FilePath&)>& filter_cb) {
+bool Zip(const FilePath& src_dir, const FilePath& dest_file,
+         const base::Callback<bool(const FilePath&)>& filter_cb) {
   DCHECK(file_util::DirectoryExists(src_dir));
 
 #if defined(OS_WIN)
@@ -316,21 +315,19 @@ static bool Zip(const FilePath& src_dir, const FilePath& dest_file,
   return success;
 }
 
-static bool FilterNoFiles(const FilePath& file_path) {
+static bool ExcludeNoFilesFilter(const FilePath& file_path) {
   return true;
 }
 
-bool Zip(const FilePath& src_dir, const FilePath& dest_file) {
-  return Zip(src_dir, dest_file, base::Bind(&FilterNoFiles));
-}
-
-static bool FilterHiddenFiles(bool include_hidden_files,
-                              const FilePath& file_path) {
-  return !(!include_hidden_files && file_path.BaseName().value()[0] == '.');
+static bool ExcludeHiddenFilesFilter(const FilePath& file_path) {
+  return file_path.BaseName().value()[0] != '.';
 }
 
 bool Zip(const FilePath& src_dir, const FilePath& dest_file,
          bool include_hidden_files) {
-  return Zip(src_dir, dest_file,
-             base::Bind(&FilterHiddenFiles, include_hidden_files));
+  if (include_hidden_files) {
+    return Zip(src_dir, dest_file, base::Bind(&ExcludeNoFilesFilter));
+  } else {
+    return Zip(src_dir, dest_file, base::Bind(&ExcludeHiddenFilesFilter));
+  }
 }
diff --git a/chrome/common/zip.h b/chrome/common/zip.h
index 069d150..7056055 100644
--- a/chrome/common/zip.h
+++ b/chrome/common/zip.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -6,11 +6,19 @@
 #define CHROME_COMMON_ZIP_H_
 #pragma once
 
+#include "base/callback.h"
+
 class FilePath;
 
 // Zip the contents of src_dir into dest_file. src_path must be a directory.
 // An entry will *not* be created in the zip for the root folder -- children
-// of src_dir will be at the root level of the created zip.
+// of src_dir will be at the root level of the created zip. For each file in
+// src_dir, include it only if the callback |filter_cb| returns true. Otherwise
+// omit it.
+bool Zip(const FilePath& src_dir, const FilePath& dest_file,
+         const base::Callback<bool(const FilePath&)>& filter_cb);
+
+// Convenience method for callers who don't need to set up the filter callback.
 // If |include_hidden_files| is true, files starting with "." are included.
 // Otherwise they are omitted.
 bool Zip(const FilePath& src_dir, const FilePath& dest_file,
diff --git a/chrome/test/data/zip/create_test_zip.sh b/chrome/test/data/zip/create_test_zip.sh
new file mode 100755
index 0000000..e19d675
--- /dev/null
+++ b/chrome/test/data/zip/create_test_zip.sh
@@ -0,0 +1,15 @@
+#!/bin/bash
+#
+# Copyright (c) 2011 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.
+#
+# Run this script in its directory to recreate test.zip
+# and test_nocompress.zip.
+
+rm test.zip
+rm test_nocompress.zip
+pushd test
+zip -r ../test.zip .
+zip -r -0 ../test_nocompress.zip .
+popd