diff options
| -rw-r--r-- | content/browser/download/save_file.h | 2 | ||||
| -rw-r--r-- | content/browser/download/save_file_manager.cc | 55 | ||||
| -rw-r--r-- | content/browser/download/save_file_manager.h | 44 | ||||
| -rw-r--r-- | content/browser/download/save_file_resource_handler.cc | 4 | ||||
| -rw-r--r-- | content/browser/download/save_file_resource_handler.h | 9 | ||||
| -rw-r--r-- | content/browser/download/save_item.cc | 6 | ||||
| -rw-r--r-- | content/browser/download/save_item.h | 4 | ||||
| -rw-r--r-- | content/browser/download/save_package.cc | 33 | ||||
| -rw-r--r-- | content/browser/download/save_package.h | 16 | ||||
| -rw-r--r-- | content/browser/download/save_types.cc | 8 | ||||
| -rw-r--r-- | content/browser/download/save_types.h | 21 | ||||
| -rw-r--r-- | content/browser/loader/resource_dispatcher_host_impl.cc | 4 | ||||
| -rw-r--r-- | content/browser/loader/resource_dispatcher_host_impl.h | 5 | ||||
| -rw-r--r-- | content/common/id_type.h | 112 | ||||
| -rw-r--r-- | content/common/id_type_unittest.cc | 205 | ||||
| -rw-r--r-- | content/content_common.gypi | 1 | ||||
| -rw-r--r-- | content/content_tests.gypi | 1 |
17 files changed, 431 insertions, 99 deletions
diff --git a/content/browser/download/save_file.h b/content/browser/download/save_file.h index 84dd3cb..1dc8217 100644 --- a/content/browser/download/save_file.h +++ b/content/browser/download/save_file.h @@ -42,7 +42,7 @@ class SaveFile { std::string DebugString() const; // Accessors. - int save_item_id() const { return info_->save_item_id; } + SaveItemId save_item_id() const { return info_->save_item_id; } int render_process_id() const { return info_->render_process_id; } int request_id() const { return info_->request_id; } SaveFileCreateInfo::SaveFileSource save_source() const { diff --git a/content/browser/download/save_file_manager.cc b/content/browser/download/save_file_manager.cc index 92ff5dc..cf2e59c 100644 --- a/content/browser/download/save_file_manager.cc +++ b/content/browser/download/save_file_manager.cc @@ -46,23 +46,23 @@ void SaveFileManager::OnShutdown() { STLDeleteValues(&save_file_map_); } -SaveFile* SaveFileManager::LookupSaveFile(int save_item_id) { +SaveFile* SaveFileManager::LookupSaveFile(SaveItemId save_item_id) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); SaveFileMap::iterator it = save_file_map_.find(save_item_id); - return it == save_file_map_.end() ? NULL : it->second; + return it == save_file_map_.end() ? nullptr : it->second; } // Look up a SavePackage according to a save id. -SavePackage* SaveFileManager::LookupPackage(int save_item_id) { +SavePackage* SaveFileManager::LookupPackage(SaveItemId save_item_id) { DCHECK_CURRENTLY_ON(BrowserThread::UI); SavePackageMap::iterator it = packages_.find(save_item_id); if (it != packages_.end()) return it->second; - return NULL; + return nullptr; } // Call from SavePackage for starting a saving job -void SaveFileManager::SaveURL(int save_item_id, +void SaveFileManager::SaveURL(SaveItemId save_item_id, const GURL& url, const Referrer& referrer, int render_process_host_id, @@ -100,7 +100,7 @@ void SaveFileManager::SaveURL(int save_item_id, // Utility function for look up table maintenance, called on the UI thread. // A manager may have multiple save page job (SavePackage) in progress, // so we just look up the save id and remove it from the tracking table. -void SaveFileManager::RemoveSaveFile(int save_item_id, +void SaveFileManager::RemoveSaveFile(SaveItemId save_item_id, SavePackage* save_package) { DCHECK(save_package); DCHECK_CURRENTLY_ON(BrowserThread::UI); @@ -138,9 +138,9 @@ void SaveFileManager::DeleteDirectoryOrFile(const base::FilePath& full_path, this, full_path, is_dir)); } -void SaveFileManager::SendCancelRequest(int save_item_id) { +void SaveFileManager::SendCancelRequest(SaveItemId save_item_id) { // Cancel the request which has specific save id. - DCHECK_GT(save_item_id, -1); + DCHECK(!save_item_id.is_null()); BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, base::Bind(&SaveFileManager::CancelSave, this, save_item_id)); @@ -173,7 +173,7 @@ void SaveFileManager::StartSave(SaveFileCreateInfo* info) { // update the UI. If the user has canceled the saving action (in the UI // thread). We may receive a few more updates before the IO thread gets the // cancel message. We just delete the data since the SaveFile has been deleted. -void SaveFileManager::UpdateSaveProgress(int save_item_id, +void SaveFileManager::UpdateSaveProgress(SaveItemId save_item_id, net::IOBuffer* data, int data_len) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); @@ -193,19 +193,17 @@ void SaveFileManager::UpdateSaveProgress(int save_item_id, // The IO thread will call this when saving is completed or it got error when // fetching data. We forward the message to OnSaveFinished in UI thread. -void SaveFileManager::SaveFinished(int save_item_id, - int save_package_id, +void SaveFileManager::SaveFinished(SaveItemId save_item_id, + SavePackageId save_package_id, bool is_success) { DVLOG(20) << " " << __FUNCTION__ << "()" << " save_item_id = " << save_item_id << " save_package_id = " << save_package_id << " is_success = " << is_success; DCHECK_CURRENTLY_ON(BrowserThread::FILE); - SaveFileMap::iterator it = save_file_map_.find(save_item_id); - if (it != save_file_map_.end()) { - SaveFile* save_file = it->second; + SaveFile* save_file = LookupSaveFile(save_item_id); + if (save_file != nullptr) { DCHECK(save_file->InProgress()); - DVLOG(20) << " " << __FUNCTION__ << "()" << " save_file = " << save_file->DebugString(); BrowserThread::PostTask( @@ -231,15 +229,14 @@ void SaveFileManager::OnStartSave(const SaveFileCreateInfo& info) { } // Insert started saving job to tracking list. - SavePackageMap::iterator sit = packages_.find(info.save_item_id); - DCHECK(sit == packages_.end()); + DCHECK(packages_.find(info.save_item_id) == packages_.end()); packages_[info.save_item_id] = save_package; // Forward this message to SavePackage. save_package->StartSave(&info); } -void SaveFileManager::OnUpdateSaveProgress(int save_item_id, +void SaveFileManager::OnUpdateSaveProgress(SaveItemId save_item_id, int64_t bytes_so_far, bool write_success) { DCHECK_CURRENTLY_ON(BrowserThread::UI); @@ -250,7 +247,7 @@ void SaveFileManager::OnUpdateSaveProgress(int save_item_id, SendCancelRequest(save_item_id); } -void SaveFileManager::OnSaveFinished(int save_item_id, +void SaveFileManager::OnSaveFinished(SaveItemId save_item_id, int64_t bytes_so_far, bool is_success) { DCHECK_CURRENTLY_ON(BrowserThread::UI); @@ -263,8 +260,8 @@ void SaveFileManager::OnSaveFinished(int save_item_id, void SaveFileManager::OnSaveURL(const GURL& url, const Referrer& referrer, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_host_id, int render_view_routing_id, int render_frame_routing_id, @@ -289,7 +286,7 @@ void SaveFileManager::ExecuteCancelSaveRequest(int render_process_id, // but we do forward the cancel to the IO thread. Since this message has been // sent from the UI thread, the saving job may have already completed and // won't exist in our map. -void SaveFileManager::CancelSave(int save_item_id) { +void SaveFileManager::CancelSave(SaveItemId save_item_id) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); SaveFileMap::iterator it = save_file_map_.find(save_item_id); if (it != save_file_map_.end()) { @@ -324,8 +321,8 @@ void SaveFileManager::CancelSave(int save_item_id) { // before this function runs. So if we can not find corresponding SaveFile by // using specified save_item_id, just return. void SaveFileManager::SaveLocalFile(const GURL& original_file_url, - int save_item_id, - int save_package_id) { + SaveItemId save_item_id, + SavePackageId save_package_id) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); SaveFile* save_file = LookupSaveFile(save_item_id); if (!save_file) @@ -366,14 +363,14 @@ void SaveFileManager::RenameAllFiles(const FinalNamesMap& final_names, const base::FilePath& resource_dir, int render_process_id, int render_frame_routing_id, - int save_package_id) { + SavePackageId save_package_id) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); if (!resource_dir.empty() && !base::PathExists(resource_dir)) base::CreateDirectory(resource_dir); for (const auto& i : final_names) { - int save_item_id = i.first; + SaveItemId save_item_id = i.first; const base::FilePath& final_name = i.second; SaveFileMap::iterator it = save_file_map_.find(save_item_id); @@ -394,7 +391,7 @@ void SaveFileManager::RenameAllFiles(const FinalNamesMap& final_names, void SaveFileManager::OnFinishSavePageJob(int render_process_id, int render_frame_routing_id, - int save_package_id) { + SavePackageId save_package_id) { DCHECK_CURRENTLY_ON(BrowserThread::UI); SavePackage* save_package = @@ -405,10 +402,10 @@ void SaveFileManager::OnFinishSavePageJob(int render_process_id, } void SaveFileManager::RemoveSavedFileFromFileMap( - const std::vector<int>& save_item_ids) { + const std::vector<SaveItemId>& save_item_ids) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); - for (const int save_item_id : save_item_ids) { + for (const SaveItemId save_item_id : save_item_ids) { SaveFileMap::iterator it = save_file_map_.find(save_item_id); if (it != save_file_map_.end()) { SaveFile* save_file = it->second; diff --git a/content/browser/download/save_file_manager.h b/content/browser/download/save_file_manager.h index 54882e7..3f36206 100644 --- a/content/browser/download/save_file_manager.h +++ b/content/browser/download/save_file_manager.h @@ -92,7 +92,7 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { // Save the specified URL. Called on the UI thread and forwarded to the // ResourceDispatcherHostImpl on the IO thread. - void SaveURL(int save_item_id, + void SaveURL(SaveItemId save_item_id, const GURL& url, const Referrer& referrer, int render_process_host_id, @@ -105,16 +105,20 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { // Notifications sent from the IO thread and run on the file thread: void StartSave(SaveFileCreateInfo* info); - void UpdateSaveProgress(int save_item_id, net::IOBuffer* data, int size); - void SaveFinished(int save_item_id, int save_package_id, bool is_success); + void UpdateSaveProgress(SaveItemId save_item_id, + net::IOBuffer* data, + int size); + void SaveFinished(SaveItemId save_item_id, + SavePackageId save_package_id, + bool is_success); // Notifications sent from the UI thread and run on the file thread. // Cancel a SaveFile instance which has specified save item id. - void CancelSave(int save_item_id); + void CancelSave(SaveItemId save_item_id); // Called on the UI thread to remove a save package from SaveFileManager's // tracking map. - void RemoveSaveFile(int save_item_id, SavePackage* package); + void RemoveSaveFile(SaveItemId save_item_id, SavePackage* package); // Helper function for deleting specified file. void DeleteDirectoryOrFile(const base::FilePath& full_path, bool is_dir); @@ -122,19 +126,19 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { // Runs on file thread to save a file by copying from file system when // original url is using file scheme. void SaveLocalFile(const GURL& original_file_url, - int save_item_id, - int save_package_id); + SaveItemId save_item_id, + SavePackageId save_package_id); // Renames all the successfully saved files. void RenameAllFiles(const FinalNamesMap& final_names, const base::FilePath& resource_dir, int render_process_id, int render_frame_routing_id, - int save_package_id); + SavePackageId save_package_id); // When the user cancels the saving, we need to remove all remaining saved // files of this page saving job from save_file_map_. - void RemoveSavedFileFromFileMap(const std::vector<int>& save_item_ids); + void RemoveSavedFileFromFileMap(const std::vector<SaveItemId>& save_item_ids); private: friend class base::RefCountedThreadSafe<SaveFileManager>; @@ -150,14 +154,14 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { int render_frame_routing_id); // Look up the SavePackage according to save item id. - SavePackage* LookupPackage(int save_item_id); + SavePackage* LookupPackage(SaveItemId save_item_id); // Called only on the file thread. // Look up one in-progress saving item according to save item id. - SaveFile* LookupSaveFile(int save_item_id); + SaveFile* LookupSaveFile(SaveItemId save_item_id); // Help function for sending notification of canceling specific request. - void SendCancelRequest(int save_item_id); + void SendCancelRequest(SaveItemId save_item_id); // Notifications sent from the file thread and run on the UI thread. @@ -166,16 +170,18 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { void OnStartSave(const SaveFileCreateInfo& info); // Update the SavePackage with the current state of a started saving job. // If the SavePackage for this saving job is gone, cancel the request. - void OnUpdateSaveProgress(int save_item_id, + void OnUpdateSaveProgress(SaveItemId save_item_id, int64_t bytes_so_far, bool write_success); // Update the SavePackage with the finish state, and remove the request // tracking entries. - void OnSaveFinished(int save_item_id, int64_t bytes_so_far, bool is_success); + void OnSaveFinished(SaveItemId save_item_id, + int64_t bytes_so_far, + bool is_success); // Notifies SavePackage that the whole page saving job is finished. void OnFinishSavePageJob(int render_process_id, int render_frame_routing_id, - int save_package_id); + SavePackageId save_package_id); // Notifications sent from the UI thread and run on the file thread. @@ -187,8 +193,8 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { // Initiates a request for URL to be saved. void OnSaveURL(const GURL& url, const Referrer& referrer, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_host_id, int render_view_routing_id, int render_frame_routing_id, @@ -198,12 +204,12 @@ class SaveFileManager : public base::RefCountedThreadSafe<SaveFileManager> { void ExecuteCancelSaveRequest(int render_process_id, int request_id); // A map from save_item_id into SaveFiles. - typedef base::hash_map<int, SaveFile*> SaveFileMap; + typedef base::hash_map<SaveItemId, SaveFile*> SaveFileMap; SaveFileMap save_file_map_; // Tracks which SavePackage to send data to, called only on UI thread. // SavePackageMap maps save item ids to their SavePackage. - typedef base::hash_map<int, SavePackage*> SavePackageMap; + typedef base::hash_map<SaveItemId, SavePackage*> SavePackageMap; SavePackageMap packages_; DISALLOW_COPY_AND_ASSIGN(SaveFileManager); diff --git a/content/browser/download/save_file_resource_handler.cc b/content/browser/download/save_file_resource_handler.cc index e20c036..faf5605 100644 --- a/content/browser/download/save_file_resource_handler.cc +++ b/content/browser/download/save_file_resource_handler.cc @@ -17,8 +17,8 @@ namespace content { SaveFileResourceHandler::SaveFileResourceHandler(net::URLRequest* request, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_host_id, int render_frame_routing_id, const GURL& url, diff --git a/content/browser/download/save_file_resource_handler.h b/content/browser/download/save_file_resource_handler.h index 2c7f4e6..e559e44 100644 --- a/content/browser/download/save_file_resource_handler.h +++ b/content/browser/download/save_file_resource_handler.h @@ -11,6 +11,7 @@ #include "base/macros.h" #include "base/memory/ref_counted.h" +#include "content/browser/download/save_types.h" #include "content/browser/loader/resource_handler.h" #include "url/gurl.h" @@ -25,8 +26,8 @@ class SaveFileManager; class SaveFileResourceHandler : public ResourceHandler { public: SaveFileResourceHandler(net::URLRequest* request, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_host_id, int render_frame_routing_id, const GURL& url, @@ -76,8 +77,8 @@ class SaveFileResourceHandler : public ResourceHandler { } private: - int save_item_id_; - int save_package_id_; + SaveItemId save_item_id_; + SavePackageId save_package_id_; int render_process_id_; int render_frame_routing_id_; scoped_refptr<net::IOBuffer> read_buffer_; diff --git a/content/browser/download/save_item.cc b/content/browser/download/save_item.cc index 604d659..edea263 100644 --- a/content/browser/download/save_item.cc +++ b/content/browser/download/save_item.cc @@ -15,10 +15,10 @@ namespace content { namespace { -int GetNextSaveItemId() { - static int g_next_save_item_id = 0; +SaveItemId GetNextSaveItemId() { + static int g_next_save_item_id = 1; DCHECK_CURRENTLY_ON(BrowserThread::UI); - return g_next_save_item_id++; + return SaveItemId::FromUnsafeValue(g_next_save_item_id++); } } // namespace diff --git a/content/browser/download/save_item.h b/content/browser/download/save_item.h index 842cd03..b0e655d 100644 --- a/content/browser/download/save_item.h +++ b/content/browser/download/save_item.h @@ -55,7 +55,7 @@ class SaveItem { void SetTotalBytes(int64_t total_bytes); // Accessors. - int id() const { return save_item_id_; } + SaveItemId id() const { return save_item_id_; } SaveState state() const { return state_; } const base::FilePath& full_path() const { return full_path_; } const base::FilePath& file_name() const { return file_name_; } @@ -75,7 +75,7 @@ class SaveItem { void UpdateSize(int64_t size); // Unique identifier for this SaveItem instance. - const int save_item_id_; + const SaveItemId save_item_id_; // Full path to the save item file. base::FilePath full_path_; diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc index c3f1c49..fe660b4 100644 --- a/content/browser/download/save_package.cc +++ b/content/browser/download/save_package.cc @@ -59,8 +59,11 @@ using base::Time; namespace content { namespace { -// A counter for uniquely identifying each save package. -int g_save_package_id = 0; +// Generates unique ids for SavePackage::unique_id_ field. +SavePackageId GetNextSavePackageId() { + static int g_save_package_id = 0; + return SavePackageId::FromUnsafeValue(g_save_package_id++); +} // Default name which will be used when we can not get proper name from // resource URL. @@ -163,7 +166,7 @@ SavePackage::SavePackage(WebContents* web_contents, all_save_items_count_(0), file_name_set_(&base::FilePath::CompareLessIgnoreCase), wait_state_(INITIALIZE), - unique_id_(g_save_package_id++), + unique_id_(GetNextSavePackageId()), wrote_to_completed_file_(false), wrote_to_failed_file_(false) { DCHECK(page_url_.is_valid()); @@ -195,7 +198,7 @@ SavePackage::SavePackage(WebContents* web_contents) all_save_items_count_(0), file_name_set_(&base::FilePath::CompareLessIgnoreCase), wait_state_(INITIALIZE), - unique_id_(g_save_package_id++), + unique_id_(GetNextSavePackageId()), wrote_to_completed_file_(false), wrote_to_failed_file_(false) { DCHECK(page_url_.is_valid()); @@ -224,7 +227,7 @@ SavePackage::SavePackage(WebContents* web_contents, all_save_items_count_(0), file_name_set_(&base::FilePath::CompareLessIgnoreCase), wait_state_(INITIALIZE), - unique_id_(g_save_package_id++), + unique_id_(GetNextSavePackageId()), wrote_to_completed_file_(false), wrote_to_failed_file_(false) {} @@ -628,7 +631,7 @@ void SavePackage::StartSave(const SaveFileCreateInfo* info) { } } -SaveItem* SavePackage::LookupSaveItemInProcess(int32_t save_item_id) { +SaveItem* SavePackage::LookupSaveItemInProcess(SaveItemId save_item_id) { auto it = in_progress_items_.find(save_item_id); if (it != in_progress_items_.end()) { SaveItem* save_item = it->second; @@ -658,7 +661,7 @@ void SavePackage::PutInProgressItemToSavedMap(SaveItem* save_item) { } // Called for updating saving state. -bool SavePackage::UpdateSaveProgress(int32_t save_item_id, +bool SavePackage::UpdateSaveProgress(SaveItemId save_item_id, int64_t size, bool write_success) { // Because we might have canceled this saving job before, @@ -688,9 +691,8 @@ void SavePackage::Stop() { // When stopping, if it still has some items in in_progress, cancel them. DCHECK(canceled()); if (in_process_count()) { - SaveItemIdMap::iterator it = in_progress_items_.begin(); - for (; it != in_progress_items_.end(); ++it) { - SaveItem* save_item = it->second; + for (const auto& it : in_progress_items_) { + SaveItem* save_item = it.second; DCHECK_EQ(SaveItem::IN_PROGRESS, save_item->state()); save_item->Cancel(); } @@ -703,7 +705,7 @@ void SavePackage::Stop() { // This vector contains the save ids of the save files which SaveFileManager // needs to remove from its save_file_map_. - std::vector<int> save_item_ids; + std::vector<SaveItemId> save_item_ids; for (const auto& it : saved_success_items_) save_item_ids.push_back(it.first); for (const auto& it : saved_failed_items_) @@ -768,7 +770,7 @@ void SavePackage::Finish() { // This vector contains the save ids of the save files which SaveFileManager // needs to remove from its save_file_map_. - std::vector<int> list_of_failed_save_item_ids; + std::vector<SaveItemId> list_of_failed_save_item_ids; for (const auto& it : saved_failed_items_) { SaveItem* save_item = it.second; DCHECK_EQ(it.first, save_item->id()); @@ -797,7 +799,7 @@ void SavePackage::Finish() { } // Called for updating end state. -void SavePackage::SaveFinished(int32_t save_item_id, +void SavePackage::SaveFinished(SaveItemId save_item_id, int64_t size, bool is_success) { // Because we might have canceled this saving job before, @@ -1044,9 +1046,8 @@ void SavePackage::OnSerializedHtmlWithLocalLinksResponse( SaveItem* save_item = it->second; DCHECK_EQ(SaveFileCreateInfo::SAVE_FILE_FROM_DOM, save_item->save_source()); if (save_item->state() != SaveItem::IN_PROGRESS) { - for (SavedItemMap::iterator saved_it = saved_success_items_.begin(); - saved_it != saved_success_items_.end(); ++saved_it) { - if (saved_it->second->url() == save_item->url()) { + for (const auto& saved_it : saved_success_items_) { + if (saved_it.second->url() == save_item->url()) { wrote_to_completed_file_ = true; break; } diff --git a/content/browser/download/save_package.h b/content/browser/download/save_package.h index 7344af4..f28cf3a 100644 --- a/content/browser/download/save_package.h +++ b/content/browser/download/save_package.h @@ -110,10 +110,10 @@ class CONTENT_EXPORT SavePackage // Notifications sent from the file thread to the UI thread. void StartSave(const SaveFileCreateInfo* info); - bool UpdateSaveProgress(int32_t save_item_id, + bool UpdateSaveProgress(SaveItemId save_item_id, int64_t size, bool write_success); - void SaveFinished(int32_t save_item_id, int64_t size, bool is_success); + void SaveFinished(SaveItemId save_item_id, int64_t size, bool is_success); void SaveCanceled(SaveItem* save_item); // Rough percent complete, -1 means we don't know (since we didn't receive a @@ -123,7 +123,8 @@ class CONTENT_EXPORT SavePackage bool canceled() const { return user_canceled_ || disk_error_occurred_; } bool finished() const { return finished_; } SavePageType save_type() const { return save_type_; } - int id() const { return unique_id_; } + + SavePackageId id() const { return unique_id_; } void GetSaveInfo(); @@ -256,7 +257,7 @@ class CONTENT_EXPORT SavePackage bool end_of_data); // Look up SaveItem by save item id from in progress map. - SaveItem* LookupSaveItemInProcess(int32_t save_item_id); + SaveItem* LookupSaveItemInProcess(SaveItemId save_item_id); // Remove SaveItem from in progress map and put it to saved map. void PutInProgressItemToSavedMap(SaveItem* save_item); @@ -277,7 +278,7 @@ class CONTENT_EXPORT SavePackage const SavePackageDownloadCreatedCallback& cb); // Map from SaveItem::id() (aka save_item_id) into a SaveItem. - typedef base::hash_map<int, SaveItem*> SaveItemIdMap; + typedef base::hash_map<SaveItemId, SaveItem*> SaveItemIdMap; // in_progress_items_ is map of all saving job in in-progress state. SaveItemIdMap in_progress_items_; // saved_failed_items_ is map of all saving job which are failed. @@ -350,9 +351,8 @@ class CONTENT_EXPORT SavePackage // Number of frames that we still need to get a response from. int number_of_frames_pending_response_; - typedef base::hash_map<int32_t, SaveItem*> SavedItemMap; // saved_success_items_ is map of all saving job which are successfully saved. - SavedItemMap saved_success_items_; + base::hash_map<SaveItemId, SaveItem*> saved_success_items_; // Non-owning pointer for handling file writing on the file thread. SaveFileManager* file_manager_; @@ -406,7 +406,7 @@ class CONTENT_EXPORT SavePackage WaitState wait_state_; // Unique ID for this SavePackage. - const int unique_id_; + const SavePackageId unique_id_; // Variables to record errors that happened so we can record them via // UMA statistics. diff --git a/content/browser/download/save_types.cc b/content/browser/download/save_types.cc index a2029ba..9c4972a 100644 --- a/content/browser/download/save_types.cc +++ b/content/browser/download/save_types.cc @@ -10,8 +10,8 @@ namespace content { SaveFileCreateInfo::SaveFileCreateInfo(const base::FilePath& path, const GURL& url, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_id, int render_frame_routing_id, SaveFileSource save_source) @@ -27,8 +27,8 @@ SaveFileCreateInfo::SaveFileCreateInfo(const base::FilePath& path, SaveFileCreateInfo::SaveFileCreateInfo(const GURL& url, const GURL& final_url, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_id, int render_frame_routing_id, int request_id, diff --git a/content/browser/download/save_types.h b/content/browser/download/save_types.h index 803196f..83d890b 100644 --- a/content/browser/download/save_types.h +++ b/content/browser/download/save_types.h @@ -13,12 +13,19 @@ #include <vector> #include "base/files/file_path.h" +#include "content/common/id_type.h" #include "url/gurl.h" namespace content { +class SavePackage; +using SavePackageId = IdType32<SavePackage>; + +class SaveItem; +using SaveItemId = IdType32<SaveItem>; + // Map from save_item_id into final file path. -typedef std::map<int, base::FilePath> FinalNamesMap; +using FinalNamesMap = std::map<SaveItemId, base::FilePath>; // This structure is used to handle and deliver some info // when processing each save item job. @@ -39,8 +46,8 @@ struct SaveFileCreateInfo { // Constructor for SAVE_FILE_FROM_DOM and/or SAVE_FILE_FROM_FILE. SaveFileCreateInfo(const base::FilePath& path, const GURL& url, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_id, int render_frame_routing_id, SaveFileSource save_source); @@ -48,8 +55,8 @@ struct SaveFileCreateInfo { // Constructor for SAVE_FILE_FROM_NET case. SaveFileCreateInfo(const GURL& url, const GURL& final_url, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int render_process_id, int render_frame_routing_id, int request_id, @@ -66,9 +73,9 @@ struct SaveFileCreateInfo { // Final URL of the saved resource since some URL might be redirected. GURL final_url; // The unique identifier of SaveItem object associated with this job. - int save_item_id; + SaveItemId save_item_id; // ID of SavePackage object. - int save_package_id; + SavePackageId save_package_id; // IDs for looking up the contents we are associated with. int render_process_id; int render_frame_routing_id; diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc index 86b1a2d..d671e2b 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.cc +++ b/content/browser/loader/resource_dispatcher_host_impl.cc @@ -1793,8 +1793,8 @@ void ResourceDispatcherHostImpl::OnAudioRenderHostStreamStateChanged( // This function is only used for saving feature. void ResourceDispatcherHostImpl::BeginSaveFile(const GURL& url, const Referrer& referrer, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int child_id, int render_view_route_id, int render_frame_route_id, diff --git a/content/browser/loader/resource_dispatcher_host_impl.h b/content/browser/loader/resource_dispatcher_host_impl.h index 1c745bd..7492264 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.h +++ b/content/browser/loader/resource_dispatcher_host_impl.h @@ -27,6 +27,7 @@ #include "base/time/time.h" #include "base/timer/timer.h" #include "content/browser/download/download_resource_handler.h" +#include "content/browser/download/save_types.h" #include "content/browser/loader/global_routing_id.h" #include "content/browser/loader/resource_loader.h" #include "content/browser/loader/resource_loader_delegate.h" @@ -131,8 +132,8 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl // request from the renderer or another child process). void BeginSaveFile(const GURL& url, const Referrer& referrer, - int save_item_id, - int save_package_id, + SaveItemId save_item_id, + SavePackageId save_package_id, int child_id, int render_view_route_id, int render_frame_route_id, diff --git a/content/common/id_type.h b/content/common/id_type.h new file mode 100644 index 0000000..cc25fa5 --- /dev/null +++ b/content/common/id_type.h @@ -0,0 +1,112 @@ +// Copyright 2016 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 CONTENT_COMMON_ID_TYPE_H_ +#define CONTENT_COMMON_ID_TYPE_H_ + +#include <stdint.h> +#include <ostream> +#include <type_traits> + +#include "base/containers/hash_tables.h" + +// IdType32<>, IdType64<>, etc. wrap an integer id in a custom, type-safe type. +// +// IdType32<Foo> is an alternative to int, for a class Foo with methods like: +// +// int GetId() { return id_; }; +// static Foo* FromId(int id) { return g_all_foos_by_id[id]; } +// +// Such methods are a standard means of safely referring to objects across +// thread and process boundaries. But if a nearby class Bar also represents +// its IDs as a bare int, horrific mixups are possible -- one example, of many, +// is http://crrev.com/365437. IdType<> offers compile-time protection against +// such mishaps, since IdType32<Foo> is incompatible with IdType32<Bar>, even +// though both just compile down to an int32_t. +// +// Templates in this file: +// IdType32<T> / IdTypeU32<T>: Signed / unsigned 32-bit IDs +// IdType64<T> / IdTypeU64<T>: Signed / unsigned 64-bit IDs +// IdType<>: For when you need a different underlying type or +// a default/invalid value other than zero. +// +// IdType32<Foo> behaves just like an int32_t in the following aspects: +// - it can be used as a key in std::map and/or base::hash_map; +// - it can be used as an argument to DCHECK_EQ or streamed to LOG(ERROR); +// - it has the same memory footprint and runtime overhead as int32_t; +// - it can be copied by memcpy. +// +// IdType32<Foo> has the following differences from a bare int32_t: +// - it forces coercions to go through GetUnsafeValue and FromUnsafeValue; +// - it restricts the set of available operations (i.e. no multiplication); +// - it ensures initialization to zero and allows checking against +// default-initialized values via is_null method. + +namespace content { + +template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue> +class IdType { + public: + IdType() : value_(kInvalidValue) {} + bool is_null() const { return value_ == kInvalidValue; } + + static IdType FromUnsafeValue(WrappedType value) { return IdType(value); } + WrappedType GetUnsafeValue() const { return value_; } + + IdType(const IdType& other) = default; + IdType& operator=(const IdType& other) = default; + + bool operator==(const IdType& other) const { return value_ == other.value_; } + bool operator!=(const IdType& other) const { return value_ != other.value_; } + bool operator<(const IdType& other) const { return value_ < other.value_; } + + protected: + explicit IdType(WrappedType val) : value_(val) {} + + private: + // In theory WrappedType could be any type that supports ==, <, <<, std::hash, + // etc., but to make things simpler (both for users and for maintainers) we + // explicitly restrict the design space to integers. This means the users + // can safely assume that IdType is relatively small and cheap to copy + // and the maintainers don't have to worry about WrappedType being a complex + // type (i.e. std::string or std::pair or a move-only type). + using IntegralWrappedType = + typename std::enable_if<std::is_integral<WrappedType>::value, + WrappedType>::type; + IntegralWrappedType value_; +}; + +// Type aliases for convenience: +template <typename TypeMarker> +using IdType32 = IdType<TypeMarker, int32_t, 0>; +template <typename TypeMarker> +using IdTypeU32 = IdType<TypeMarker, uint32_t, 0>; +template <typename TypeMarker> +using IdType64 = IdType<TypeMarker, int64_t, 0>; +template <typename TypeMarker> +using IdTypeU64 = IdType<TypeMarker, uint64_t, 0>; + +template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue> +std::ostream& operator<<( + std::ostream& stream, + const IdType<TypeMarker, WrappedType, kInvalidValue>& id) { + return stream << id.GetUnsafeValue(); +} + +} // namespace content + +namespace BASE_HASH_NAMESPACE { + +template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue> +struct hash<content::IdType<TypeMarker, WrappedType, kInvalidValue>> { + using argument_type = content::IdType<TypeMarker, WrappedType, kInvalidValue>; + using result_type = std::size_t; + result_type operator()(const argument_type& id) const { + return BASE_HASH_NAMESPACE::hash<WrappedType>()(id.GetUnsafeValue()); + } +}; + +} // namespace BASE_HASH_NAMESPACE + +#endif // CONTENT_COMMON_ID_TYPE_H_ diff --git a/content/common/id_type_unittest.cc b/content/common/id_type_unittest.cc new file mode 100644 index 0000000..aa596d4 --- /dev/null +++ b/content/common/id_type_unittest.cc @@ -0,0 +1,205 @@ +// Copyright 2016 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 <limits> +#include <map> +#include <sstream> +#include <string> +#include <type_traits> + +#include "base/containers/hash_tables.h" +#include "content/common/id_type.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace content { + +namespace { + +class Foo; +using FooId = IdType<Foo, int, 0>; + +class Bar; +using BarId = IdType<Bar, int, 0>; + +class AnotherIdMarker; +class DerivedId : public IdType<AnotherIdMarker, int, 0> { + public: + explicit DerivedId(int unsafe_value) + : IdType<AnotherIdMarker, int, 0>(unsafe_value) {} +}; + +} // namespace + +TEST(IdType, DefaultValueIsInvalid) { + FooId foo_id; + EXPECT_TRUE(foo_id.is_null()); +} + +TEST(IdType, NormalValueIsValid) { + FooId foo_id = FooId::FromUnsafeValue(123); + EXPECT_FALSE(foo_id.is_null()); +} + +TEST(IdType, OutputStreamTest) { + FooId foo_id = FooId::FromUnsafeValue(123); + + std::ostringstream ss; + ss << foo_id; + EXPECT_EQ("123", ss.str()); +} + +TEST(IdType, IdType32) { + IdType32<Foo> id; + + EXPECT_EQ(0, id.GetUnsafeValue()); + static_assert(sizeof(int32_t) == sizeof(id), ""); +} + +TEST(IdType, IdTypeU32) { + IdTypeU32<Foo> id; + + EXPECT_EQ(0u, id.GetUnsafeValue()); + static_assert(sizeof(uint32_t) == sizeof(id), ""); +} + +TEST(IdType, IdType64) { + IdType64<Foo> id; + + EXPECT_EQ(0, id.GetUnsafeValue()); + static_assert(sizeof(int64_t) == sizeof(id), ""); +} + +TEST(IdType, IdTypeU64) { + IdTypeU64<Foo> id; + + EXPECT_EQ(0u, id.GetUnsafeValue()); + static_assert(sizeof(uint64_t) == sizeof(id), ""); +} + +TEST(IdType, DerivedClasses) { + DerivedId derived_id(456); + + std::ostringstream ss; + ss << derived_id; + EXPECT_EQ("456", ss.str()); + + std::map<DerivedId, std::string> ordered_map; + ordered_map[derived_id] = "blah"; + EXPECT_EQ(ordered_map[derived_id], "blah"); + + // TODO(lukasza): Enable std::unordered_map and base::hash_map for DerivedId. + // Ideally this should be possible without having to repeat std::hash<...> + // specialization for each derived class (but then SFINAE + std::enable_if + + // std::is_base_of doesn't seem to work for std::hash because std::hash only + // has a single template parameter?). + // std::unordered_map<DerivedId, std::string> unordered_map; + // unordered_map[derived_id] = "blah2"; + // EXPECT_EQ(unordered_map[derived_id], "blah2"); +} + +TEST(IdType, StaticAsserts) { + static_assert(!std::is_constructible<FooId, int>::value, + "Should be impossible to construct FooId from a raw integer."); + static_assert(!std::is_convertible<int, FooId>::value, + "Should be impossible to convert a raw integer into FooId."); + + static_assert(!std::is_constructible<FooId, BarId>::value, + "Should be impossible to construct FooId from a BarId."); + static_assert(!std::is_convertible<BarId, FooId>::value, + "Should be impossible to convert a BarId into FooId."); + + // The presence of a custom default constructor means that FooId is not a + // "trivial" class and therefore is not a POD type (unlike an int32_t). + // At the same time FooId has almost all of the properties of a POD type: + // - is "trivially copyable" (i.e. is memcpy-able), + // - has "standard layout" (i.e. interops with things expecting C layout). + // See http://stackoverflow.com/a/7189821 for more info about these + // concepts. + static_assert(std::is_standard_layout<FooId>::value, + "FooId should have standard layout. " + "See http://stackoverflow.com/a/7189821 for more info."); + static_assert(sizeof(FooId) == sizeof(int), + "FooId should be the same size as the raw integer it wraps."); + // TODO(lukasza): Enable these once <type_traits> supports all the standard + // C++11 equivalents (i.e. std::is_trivially_copyable instead of the + // non-standard std::has_trivial_copy_assign). + // static_assert(std::has_trivial_copy_constructor<FooId>::value, + // "FooId should have a trivial copy constructor."); + // static_assert(std::has_trivial_copy_assign<FooId>::value, + // "FooId should have a trivial copy assignment operator."); + // static_assert(std::has_trivial_destructor<FooId>::value, + // "FooId should have a trivial destructor."); +} + +class IdTypeSpecificValueTest : public ::testing::TestWithParam<int> { + protected: + FooId test_id() { return FooId::FromUnsafeValue(GetParam()); } + + FooId other_id() { + if (GetParam() != std::numeric_limits<int>::max()) + return FooId::FromUnsafeValue(GetParam() + 1); + else + return FooId::FromUnsafeValue(std::numeric_limits<int>::min()); + } +}; + +TEST_P(IdTypeSpecificValueTest, ComparisonToSelf) { + EXPECT_TRUE(test_id() == test_id()); + EXPECT_FALSE(test_id() != test_id()); + EXPECT_FALSE(test_id() < test_id()); +} + +TEST_P(IdTypeSpecificValueTest, ComparisonToOther) { + EXPECT_FALSE(test_id() == other_id()); + EXPECT_TRUE(test_id() != other_id()); +} + +TEST_P(IdTypeSpecificValueTest, UnsafeValueRoundtrips) { + int original_value = GetParam(); + FooId id = FooId::FromUnsafeValue(original_value); + int final_value = id.GetUnsafeValue(); + EXPECT_EQ(original_value, final_value); +} + +TEST_P(IdTypeSpecificValueTest, Copying) { + FooId original = test_id(); + + FooId copy_via_constructor(original); + EXPECT_EQ(original, copy_via_constructor); + + FooId copy_via_assignment; + copy_via_assignment = original; + EXPECT_EQ(original, copy_via_assignment); +} + +TEST_P(IdTypeSpecificValueTest, BaseHashmap) { + base::hash_map<FooId, std::string> map; + + map[test_id()] = "test_id"; + map[other_id()] = "other_id"; + + EXPECT_EQ(map[test_id()], "test_id"); + EXPECT_EQ(map[other_id()], "other_id"); +} + +TEST_P(IdTypeSpecificValueTest, StdMap) { + std::map<FooId, std::string> map; + + map[test_id()] = "test_id"; + map[other_id()] = "other_id"; + + EXPECT_EQ(map[test_id()], "test_id"); + EXPECT_EQ(map[other_id()], "other_id"); +} + +INSTANTIATE_TEST_CASE_P(, + IdTypeSpecificValueTest, + ::testing::Values(std::numeric_limits<int>::min(), + -1, + 0, + 1, + 123, + std::numeric_limits<int>::max())); + +} // namespace content diff --git a/content/content_common.gypi b/content/content_common.gypi index 5567be5..929e235 100644 --- a/content/content_common.gypi +++ b/content/content_common.gypi @@ -369,6 +369,7 @@ 'common/host_discardable_shared_memory_manager.h', 'common/host_shared_bitmap_manager.cc', 'common/host_shared_bitmap_manager.h', + 'common/id_type.h', 'common/in_process_child_thread_params.cc', 'common/in_process_child_thread_params.h', 'common/indexed_db/indexed_db_constants.h', diff --git a/content/content_tests.gypi b/content/content_tests.gypi index 440056b..b092630 100644 --- a/content/content_tests.gypi +++ b/content/content_tests.gypi @@ -667,6 +667,7 @@ 'common/gpu/gpu_channel_unittest.cc', 'common/host_discardable_shared_memory_manager_unittest.cc', 'common/host_shared_bitmap_manager_unittest.cc', + 'common/id_type_unittest.cc', 'common/indexed_db/indexed_db_key_unittest.cc', 'common/input/gesture_event_stream_validator_unittest.cc', 'common/input/input_param_traits_unittest.cc', |