From fca4da9db9bde9992c6a4ba0c49946f8fc8a8eba Mon Sep 17 00:00:00 2001 From: "vandebo@chromium.org" Date: Thu, 4 Apr 2013 08:32:06 +0000 Subject: [PATCH] Change the Media Galleries file system name back to a generic string. Now that we have getMediaFileSystemMetadata, we can remove the JSON string we had stuffed into the name field. BUG=170138 Review URL: https://chromiumcodereview.appspot.com/11946025 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@192238 0039d316-1c4b-4281-b951-d872f2087c98 --- .../media_galleries/media_galleries_api.cc | 19 +++---- .../media_file_system_registry.cc | 49 +----------------- .../media_file_system_registry.h | 9 +--- .../media_file_system_registry_unittest.cc | 51 ++++++------------- .../media_galleries_custom_bindings.cc | 17 ++----- .../media_galleries_custom_bindings.js | 5 +- 6 files changed, 32 insertions(+), 118 deletions(-) diff --git a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc index f7d672fca27f8b..34c045a365a300 100644 --- a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc +++ b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc @@ -54,8 +54,11 @@ const char kDisallowedByPolicy[] = "Media Galleries API is disallowed by policy: "; const char kInvalidInteractive[] = "Unknown value for interactive."; -const char kIsRemovableKey[] = "isRemovable"; +const char kDeviceIdKey[] = "deviceId"; +const char kGalleryIdKey[] = "galleryId"; const char kIsMediaDeviceKey[] = "isMediaDevice"; +const char kIsRemovableKey[] = "isRemovable"; +const char kNameKey[] = "name"; // Checks whether the MediaGalleries API is currently accessible (it may be // disallowed even if an extension has the requisite permission). @@ -146,7 +149,6 @@ void MediaGalleriesGetMediaFileSystemsFunction::ReturnGalleries( APIPermission::kMediaGalleries, &read_param); const int child_id = rvh->GetProcess()->GetID(); - std::set file_system_names; base::ListValue* list = new base::ListValue(); for (size_t i = 0; i < filesystems.size(); i++) { scoped_ptr file_system_dict_value( @@ -157,21 +159,14 @@ void MediaGalleriesGetMediaFileSystemsFunction::ReturnGalleries( file_system_dict_value->SetStringWithoutPathExpansion( "fsid", filesystems[i].fsid); - // The name must be unique according to the HTML5 File System API spec. - if (ContainsKey(file_system_names, filesystems[i].name)) { - NOTREACHED() << "Duplicate file system name: " << filesystems[i].name; - continue; - } - file_system_names.insert(filesystems[i].name); file_system_dict_value->SetStringWithoutPathExpansion( - MediaFileSystemRegistry::kNameKey, filesystems[i].name); + kNameKey, filesystems[i].name); file_system_dict_value->SetStringWithoutPathExpansion( - MediaFileSystemRegistry::kGalleryIdKey, + kGalleryIdKey, base::Uint64ToString(filesystems[i].pref_id)); if (!filesystems[i].transient_device_id.empty()) { file_system_dict_value->SetStringWithoutPathExpansion( - MediaFileSystemRegistry::kDeviceIdKey, - filesystems[i].transient_device_id); + kDeviceIdKey, filesystems[i].transient_device_id); } file_system_dict_value->SetBooleanWithoutPathExpansion( kIsRemovableKey, filesystems[i].removable); diff --git a/chrome/browser/media_galleries/media_file_system_registry.cc b/chrome/browser/media_galleries/media_file_system_registry.cc index 64a64cebb0c08a..242fdf71b985a2 100644 --- a/chrome/browser/media_galleries/media_file_system_registry.cc +++ b/chrome/browser/media_galleries/media_file_system_registry.cc @@ -12,7 +12,6 @@ #include "base/bind.h" #include "base/callback.h" #include "base/files/file_path.h" -#include "base/json/json_writer.h" #include "base/path_service.h" #include "base/prefs/pref_service.h" #include "base/stl_util.h" @@ -63,7 +62,7 @@ struct InvalidatedGalleriesInfo { } // namespace -MediaFileSystemInfo::MediaFileSystemInfo(const std::string& fs_name, +MediaFileSystemInfo::MediaFileSystemInfo(const string16& fs_name, const base::FilePath& fs_path, const std::string& filesystem_id, MediaGalleryPrefId pref_id, @@ -357,7 +356,7 @@ class ExtensionGalleriesHost DCHECK(!fsid.empty()); MediaFileSystemInfo new_entry( - MakeJSONFileSystemName(gallery_info.display_name, pref_id, device_id), + gallery_info.display_name, path, fsid, pref_id, @@ -391,46 +390,6 @@ class ExtensionGalleriesHost return monitor->GetTransientIdForDeviceId(device_id); } - // This code is deprecated and should be removed. See http://crbug.com/170138 - // Make a JSON string out of |name|, |pref_id| and |device_id|. The IDs makes - // the combined name unique. The JSON string should not contain any slashes. - std::string MakeJSONFileSystemName(const string16& name, - const MediaGalleryPrefId& pref_id, - const std::string& device_id) { - string16 sanitized_name; - string16 separators = -#if defined(FILE_PATH_USES_WIN_SEPARATORS) - base::FilePath::kSeparators -#else - ASCIIToUTF16(base::FilePath::kSeparators) -#endif - ; // NOLINT - ReplaceChars(name, separators.c_str(), ASCIIToUTF16("_"), &sanitized_name); - - base::DictionaryValue dict_value; - dict_value.SetStringWithoutPathExpansion( - MediaFileSystemRegistry::kNameKey, sanitized_name); - - // This should have been a StringValue, but it's a bit late to change it. - dict_value.SetIntegerWithoutPathExpansion( - MediaFileSystemRegistry::kGalleryIdKey, pref_id); - - // |device_id| can be empty, in which case, just omit it. - std::string transient_device_id = - GetTransientIdForRemovableDeviceId(device_id); - if (!transient_device_id.empty()) { - dict_value.SetStringWithoutPathExpansion( - MediaFileSystemRegistry::kDeviceIdKey, transient_device_id); - } - dict_value.SetStringWithoutPathExpansion( - "DEPRECATED", - "This JSON string is deprecated, use getMediaFileSystemMetadata."); - - std::string json_string; - base::JSONWriter::Write(&dict_value, &json_string); - return json_string; - } - void CleanUp() { DCHECK(rph_refs_.empty()); for (PrefIdFsInfoMap::const_iterator it = pref_id_map_.begin(); @@ -474,10 +433,6 @@ class ExtensionGalleriesHost * Public methods ******************/ -const char MediaFileSystemRegistry::kDeviceIdKey[] = "deviceId"; -const char MediaFileSystemRegistry::kGalleryIdKey[] = "galleryId"; -const char MediaFileSystemRegistry::kNameKey[] = "name"; - void MediaFileSystemRegistry::GetMediaFileSystemsForExtension( const content::RenderViewHost* rvh, const extensions::Extension* extension, diff --git a/chrome/browser/media_galleries/media_file_system_registry.h b/chrome/browser/media_galleries/media_file_system_registry.h index c9ff14b935a301..7ce269a0a252db 100644 --- a/chrome/browser/media_galleries/media_file_system_registry.h +++ b/chrome/browser/media_galleries/media_file_system_registry.h @@ -47,7 +47,7 @@ class MediaGalleriesPreferences; class ScopedMTPDeviceMapEntry; struct MediaFileSystemInfo { - MediaFileSystemInfo(const std::string& fs_name, + MediaFileSystemInfo(const string16& fs_name, const base::FilePath& fs_path, const std::string& filesystem_id, MediaGalleryPrefId pref_id, @@ -57,7 +57,7 @@ struct MediaFileSystemInfo { MediaFileSystemInfo(); ~MediaFileSystemInfo(); - std::string name; // JSON string, must not contain slashes. + string16 name; base::FilePath path; std::string fsid; MediaGalleryPrefId pref_id; @@ -96,11 +96,6 @@ class MediaFileSystemRegistry : public RemovableStorageObserver { // See TransientDeviceIds::GetTransientIdForDeviceId(). uint64 GetTransientIdForDeviceId(const std::string& device_id); - // Keys for metadata about a media gallery file system. - static const char kDeviceIdKey[]; - static const char kGalleryIdKey[]; - static const char kNameKey[]; - private: friend class TestMediaFileSystemContext; class MediaFileSystemContextImpl; diff --git a/chrome/browser/media_galleries/media_file_system_registry_unittest.cc b/chrome/browser/media_galleries/media_file_system_registry_unittest.cc index 374f3a9672e295..3ea9db936aa5bd 100644 --- a/chrome/browser/media_galleries/media_file_system_registry_unittest.cc +++ b/chrome/browser/media_galleries/media_file_system_registry_unittest.cc @@ -10,7 +10,6 @@ #include "base/command_line.h" #include "base/file_util.h" #include "base/files/scoped_temp_dir.h" -#include "base/json/json_reader.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" @@ -188,39 +187,17 @@ void GetGalleryInfoCallback( } } -void CheckGalleryJSONName(const std::string& name, bool removable) { - scoped_ptr dict(static_cast( - base::JSONReader::Read(name))); - ASSERT_TRUE(dict); - - // Check deviceId. - EXPECT_EQ(removable, - dict->HasKey(MediaFileSystemRegistry::kDeviceIdKey)) << name; - if (removable) { - std::string device_id; - EXPECT_TRUE(dict->GetString(MediaFileSystemRegistry::kDeviceIdKey, - &device_id)) << name; - EXPECT_FALSE(device_id.empty()) << name; - } - - // Check galleryId. - EXPECT_TRUE(dict->HasKey(MediaFileSystemRegistry::kGalleryIdKey)) << name; - - // Check name. - EXPECT_TRUE(dict->HasKey(MediaFileSystemRegistry::kNameKey)) << name; - std::string gallery_name; - EXPECT_TRUE(dict->GetString(MediaFileSystemRegistry::kNameKey, - &gallery_name)) << name; - EXPECT_FALSE(gallery_name.empty()) << name; -} - void CheckGalleryInfo(const MediaFileSystemInfo& info, TestMediaFileSystemContext* fs_context, + const string16* name, const base::FilePath& path, bool removable, bool media_device) { - // TODO(vandebo) check the name (from path.LossyDisplayName) after JSON - // removal. + if (name) { + EXPECT_EQ(*name, info.name); + } else { + EXPECT_EQ(path.LossyDisplayName(), info.name); + } EXPECT_EQ(path, info.path); EXPECT_EQ(removable, info.removable); EXPECT_EQ(media_device, info.media_device); @@ -322,6 +299,10 @@ class MediaFileSystemRegistryTest : public ChromeRenderViewHostTestHarness { return dcim_dir_; } + TestMediaFileSystemContext* test_file_system_context() { + return test_file_system_context_; + } + // Create a user added gallery based on the information passed and add it to // |profiles|. Returns the device id. std::string AddUserGallery(MediaStorageUtil::Type type, @@ -711,9 +692,8 @@ void MediaFileSystemRegistryTest::CheckNewGalleryInfo( continue; ASSERT_FALSE(found_new); - CheckGalleryJSONName(it->second.name, removable); - CheckGalleryInfo(it->second, test_file_system_context_, location, removable, - media_device); + CheckGalleryInfo(it->second, test_file_system_context_, NULL, location, + removable, media_device); found_new = true; } ASSERT_TRUE(found_new); @@ -730,7 +710,7 @@ MediaFileSystemRegistryTest::GetAutoAddedGalleries( ++it) { if (it->second.type == MediaGalleryPrefInfo::kAutoDetected) { base::FilePath path = it->second.AbsolutePath(); - MediaFileSystemInfo info(path.AsUTF8Unsafe(), path, std::string(), + MediaFileSystemInfo info(path.LossyDisplayName(), path, std::string(), 0, std::string(), false, false); result.push_back(info); } @@ -811,7 +791,7 @@ TEST_F(MediaFileSystemRegistryTest, UserAddedGallery) { profile_state->regular_permission_extension(), device_id, true /*has access*/); - MediaFileSystemInfo added_info(empty_dir().AsUTF8Unsafe(), empty_dir(), + MediaFileSystemInfo added_info(empty_dir().LossyDisplayName(), empty_dir(), std::string(), 0, std::string(), false, false); added_galleries.push_back(added_info); profile_state->CheckGalleries("user added regular", added_galleries, @@ -891,7 +871,8 @@ TEST_F(MediaFileSystemRegistryTest, GalleryNameDefault) { for (FSInfoMap::const_iterator it = galleries_info.begin(); it != galleries_info.end(); ++it) { - CheckGalleryJSONName(it->second.name, false /*not removable*/); + CheckGalleryInfo(it->second, test_file_system_context(), &it->second.name, + it->second.path, false, false); } } diff --git a/chrome/renderer/extensions/media_galleries_custom_bindings.cc b/chrome/renderer/extensions/media_galleries_custom_bindings.cc index e0adeb81931ce2..c403d24ad19826 100644 --- a/chrome/renderer/extensions/media_galleries_custom_bindings.cc +++ b/chrome/renderer/extensions/media_galleries_custom_bindings.cc @@ -31,7 +31,7 @@ MediaGalleriesCustomBindings::MediaGalleriesCustomBindings( v8::Handle MediaGalleriesCustomBindings::GetMediaFileSystemObject( const v8::Arguments& args) { - if (args.Length() != 2) { + if (args.Length() != 1) { NOTREACHED(); return v8::Undefined(); } @@ -39,30 +39,21 @@ v8::Handle MediaGalleriesCustomBindings::GetMediaFileSystemObject( NOTREACHED(); return v8::Undefined(); } - if (!args[1]->IsString()) { - NOTREACHED(); - return v8::Undefined(); - } std::string fsid(*v8::String::Utf8Value(args[0])); if (fsid.empty()) { NOTREACHED(); return v8::Undefined(); } - std::string name(*v8::String::Utf8Value(args[1])); - if (name.empty()) { - NOTREACHED(); - return v8::Undefined(); - } WebKit::WebFrame* webframe = WebKit::WebFrame::frameForCurrentContext(); const GURL origin = GURL(webframe->document().securityOrigin().toString()); + const std::string fs_name = fileapi::GetIsolatedFileSystemName(origin, fsid); const std::string root_url = fileapi::GetIsolatedFileSystemRootURIString( origin, fsid, extension_misc::kMediaFileSystemPathPart); - return webframe->createFileSystem( - WebKit::WebFileSystemTypeIsolated, - WebKit::WebString::fromUTF8(name), + return webframe->createFileSystem(WebKit::WebFileSystemTypeIsolated, + WebKit::WebString::fromUTF8(fs_name), WebKit::WebString::fromUTF8(root_url)); } diff --git a/chrome/renderer/resources/extensions/media_galleries_custom_bindings.js b/chrome/renderer/resources/extensions/media_galleries_custom_bindings.js index 60ac567e760cf0..93e0a34c986f06 100644 --- a/chrome/renderer/resources/extensions/media_galleries_custom_bindings.js +++ b/chrome/renderer/resources/extensions/media_galleries_custom_bindings.js @@ -23,13 +23,10 @@ binding.registerCustomHook(function(bindingsAPI, extensionId) { result = []; for (var i = 0; i < response.length; i++) { var filesystem = mediaGalleriesNatives.GetMediaFileSystemObject( - response[i].fsid, response[i].name); + response[i].fsid); result.push(filesystem); var metadata = response[i]; delete metadata.fsid; - // TODO(vandebo) Until we remove the JSON name string (one release), we - // need to pull out the real name part for metadata. - metadata.name = JSON.parse(metadata.name).name; mediaGalleriesMetadata[filesystem.name] = metadata; } }