Allowing multiple SaveItems to have same URLs.

content/browser/download/save_package.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 "content/browser/download/save_package.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
@@ skipping 533 matching lines @@
   generated_name->assign(file_name);
 
   return true;
 }
 
 // We have received a message from SaveFileManager about a new saving job. We
 // create a SaveItem and store it in our in_progress list.
 void SavePackage::StartSave(const SaveFileCreateInfo* info) {
   DCHECK(info && !info->url.is_empty());
 
-  SaveUrlItemMap::iterator it = in_progress_items_.find(info->url.spec());
+  SaveItemIdMap::iterator it = in_progress_items_.find(info->save_item_id);
   if (it == in_progress_items_.end()) {
     // If not found, we must have cancel action.
     DCHECK(canceled());
     return;
   }
   SaveItem* save_item = it->second;
 
   DCHECK(!saved_main_file_path_.empty());
 
-  save_item->SetSaveId(info->save_id);
   save_item->SetTotalBytes(info->total_bytes);
 
   // Determine the proper path for a saving job, by choosing either the default
   // save directory, or prompting the user.
   DCHECK(!save_item->has_final_name());
   if (info->url != page_url_) {
     base::FilePath::StringType generated_name;
     // For HTML resource file, make sure it will have .htm as extension name,
     // otherwise, when you open the saved page in Chrome again, download
     // file manager will treat it as downloadable resource, and download it
     // instead of opening it as HTML.
     bool need_html_ext =
         info->save_source == SaveFileCreateInfo::SAVE_FILE_FROM_DOM;
     if (!GenerateFileName(info->content_disposition,
                           GURL(info->url),
                           need_html_ext,
                           &generated_name)) {
       // We can not generate file name for this SaveItem, so we cancel the
       // saving page job if the save source is from serialized DOM data.
       // Otherwise, it means this SaveItem is sub-resource type, we treat it
       // as an error happened on saving. We can ignore this type error for
       // sub-resource links which will be resolved as absolute links instead
       // of local links in final saved contents.
       if (info->save_source == SaveFileCreateInfo::SAVE_FILE_FROM_DOM)
         Cancel(true);
       else
-        SaveFinished(save_item->save_id(), 0, false);
+        SaveFinished(save_item->id(), 0, false);
       return;
     }
 
     // When saving page as only-HTML, we only have a SaveItem whose url
     // must be page_url_.
     DCHECK_EQ(SAVE_PAGE_TYPE_AS_COMPLETE_HTML, save_type_);
     DCHECK(!saved_main_directory_path_.empty());
 
     // Now we get final name retrieved from GenerateFileName, we will use it
     // rename the SaveItem.
     base::FilePath final_name =
         saved_main_directory_path_.Append(generated_name);
     save_item->Rename(final_name);
   } else {
     // It is the main HTML file, use the name chosen by the user.
     save_item->Rename(saved_main_file_path_);
   }
 
   // If the save source is from file system, inform SaveFileManager to copy
   // corresponding file to the file path which this SaveItem specifies.
   if (info->save_source == SaveFileCreateInfo::SAVE_FILE_FROM_FILE) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&SaveFileManager::SaveLocalFile, file_manager_,
-                   save_item->url(), save_item->save_id(), id()));
+                   save_item->url(), save_item->id(), id()));
     return;
   }
 
   // Check whether we begin to require serialized HTML data.
   if (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML &&
       wait_state_ == HTML_DATA) {
     // Inform backend to serialize the all frames' DOM and send serialized
     // HTML data back.
     GetSerializedHtmlWithLocalLinks();
   }
 }
 
-SaveItem* SavePackage::LookupItemInProcessBySaveId(int32 save_id) {
-  if (in_process_count()) {
-    for (SaveUrlItemMap::iterator it = in_progress_items_.begin();
-        it != in_progress_items_.end(); ++it) {
-      SaveItem* save_item = it->second;
-      DCHECK_EQ(SaveItem::IN_PROGRESS, save_item->state());
-      if (save_item->save_id() == save_id)
-        return save_item;
-    }
+SaveItem* SavePackage::LookupSaveItemInProcess(int32 save_item_id) {
+  auto it = in_progress_items_.find(save_item_id);
+  if (it != in_progress_items_.end()) {
+    SaveItem* save_item = it->second;
+    DCHECK_EQ(SaveItem::IN_PROGRESS, save_item->state());
+    return save_item;
   }
-  return NULL;
+  return nullptr;
 }
 
 void SavePackage::PutInProgressItemToSavedMap(SaveItem* save_item) {
-  SaveUrlItemMap::iterator it = in_progress_items_.find(
-      save_item->url().spec());
+  SaveItemIdMap::iterator it = in_progress_items_.find(save_item->id());
   DCHECK(it != in_progress_items_.end());
   DCHECK(save_item == it->second);
   in_progress_items_.erase(it);
 
   if (save_item->success()) {
     // Add it to saved_success_items_.
-    DCHECK(saved_success_items_.find(save_item->save_id()) ==
+    DCHECK(saved_success_items_.find(save_item->id()) ==
            saved_success_items_.end());
-    saved_success_items_[save_item->save_id()] = save_item;
+    saved_success_items_[save_item->id()] = save_item;
   } else {
     // Add it to saved_failed_items_.
-    DCHECK(saved_failed_items_.find(save_item->url().spec()) ==
+    DCHECK(saved_failed_items_.find(save_item->id()) ==
            saved_failed_items_.end());
-    saved_failed_items_[save_item->url().spec()] = save_item;
+    saved_failed_items_[save_item->id()] = save_item;
   }
 }
 
 // Called for updating saving state.
-bool SavePackage::UpdateSaveProgress(int32 save_id,
+bool SavePackage::UpdateSaveProgress(int32 save_item_id,
                                      int64 size,
                                      bool write_success) {
   // Because we might have canceled this saving job before,
   // so we might not find corresponding SaveItem.
-  SaveItem* save_item = LookupItemInProcessBySaveId(save_id);
+  SaveItem* save_item = LookupSaveItemInProcess(save_item_id);
   if (!save_item)
     return false;
 
   save_item->Update(size);
 
   // If we got disk error, cancel whole save page job.
   if (!write_success) {
     // Cancel job with reason of disk error.
     Cancel(false);
   }
   return true;
 }
 
 // Stop all page saving jobs that are in progress and instruct the file thread
 // to delete all saved  files.
 void SavePackage::Stop() {
   // If we haven't moved out of the initial state, there's nothing to cancel and
   // there won't be valid pointers for file_manager_ or download_.
   if (wait_state_ == INITIALIZE)
     return;
 
   // When stopping, if it still has some items in in_progress, cancel them.
   DCHECK(canceled());
   if (in_process_count()) {
-    SaveUrlItemMap::iterator it = in_progress_items_.begin();
+    SaveItemIdMap::iterator it = in_progress_items_.begin();
     for (; it != in_progress_items_.end(); ++it) {
       SaveItem* save_item = it->second;
       DCHECK_EQ(SaveItem::IN_PROGRESS, save_item->state());
       save_item->Cancel();
     }
     // Remove all in progress item to saved map. For failed items, they will
     // be put into saved_failed_items_, for successful item, they will be put
     // into saved_success_items_.
     while (in_process_count())
       PutInProgressItemToSavedMap(in_progress_items_.begin()->second);
   }
 
   // This vector contains the save ids of the save files which SaveFileManager
   // needs to remove from its save_file_map_.
-  SaveIDList save_ids;
-  for (SavedItemMap::iterator it = saved_success_items_.begin();
-      it != saved_success_items_.end(); ++it)
-    save_ids.push_back(it->first);
-  for (SaveUrlItemMap::iterator it = saved_failed_items_.begin();
-      it != saved_failed_items_.end(); ++it)
-    save_ids.push_back(it->second->save_id());
+  std::vector<int> save_item_ids;
+  for (const auto& it : saved_success_items_)
+    save_item_ids.push_back(it.first);
+  for (const auto& it : saved_failed_items_)
+    save_item_ids.push_back(it.first);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SaveFileManager::RemoveSavedFileFromFileMap,
-                 file_manager_,
-                 save_ids));
+      base::Bind(&SaveFileManager::RemoveSavedFileFromFileMap, file_manager_,
+                 save_item_ids));
 
   finished_ = true;
   wait_state_ = FAILED;
 
   // Inform the DownloadItem we have canceled whole save page job.
   if (download_) {
     download_->Cancel(false);
     FinalizeDownloadEntry();
   }
 }
 
 void SavePackage::CheckFinish() {
   if (in_process_count() || finished_)
     return;
 
   base::FilePath dir = (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML &&
                         saved_success_items_.size() > 1) ?
                         saved_main_directory_path_ : base::FilePath();
 
-  // This vector contains the final names of all the successfully saved files
-  // along with their save ids. It will be passed to SaveFileManager to do the
-  // renaming job.
-  FinalNameList final_names;
-  for (SavedItemMap::iterator it = saved_success_items_.begin();
-      it != saved_success_items_.end(); ++it)
-    final_names.push_back(std::make_pair(it->first,
-                                         it->second->full_path()));
+  FinalNamesMap final_names;
+  for (const auto& it : saved_success_items_)
+    final_names.insert(std::make_pair(it.first, it.second->full_path()));
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&SaveFileManager::RenameAllFiles,
                  file_manager_,
                  final_names,
                  dir,
                  web_contents()->GetRenderProcessHost()->GetID(),
                  web_contents()->GetMainFrame()->GetRoutingID(),
                  id()));
 }
 
 // Successfully finished all items of this SavePackage.
 void SavePackage::Finish() {
   // User may cancel the job when we're moving files to the final directory.
   if (canceled())
     return;
 
   wait_state_ = SUCCESSFUL;
   finished_ = true;
 
   // Record finish.
   RecordSavePackageEvent(SAVE_PACKAGE_FINISHED);
 
   // Record any errors that occurred.
-  if (wrote_to_completed_file_) {
+  if (wrote_to_completed_file_)
     RecordSavePackageEvent(SAVE_PACKAGE_WRITE_TO_COMPLETED);
-  }
 
-  if (wrote_to_failed_file_) {
+  if (wrote_to_failed_file_)
     RecordSavePackageEvent(SAVE_PACKAGE_WRITE_TO_FAILED);
-  }
 
   // This vector contains the save ids of the save files which SaveFileManager
   // needs to remove from its save_file_map_.
-  SaveIDList save_ids;
-  for (SaveUrlItemMap::iterator it = saved_failed_items_.begin();
-       it != saved_failed_items_.end(); ++it)
-    save_ids.push_back(it->second->save_id());
+  std::vector<int> save_item_ids;

[Randy Smith (Not in Mondays), 2015/12/03 20:58:28]

nit, suggestion, not a from this CL: Use a name that indicates that it's a list
of failed save items?

[Łukasz Anforowicz, 2015/12/04 21:16:45]

Good idea.  Done.  The only thing I dislike is having to include some indication
in the name that these ints are really save_item_ids.  

BTW: I also more explicitly spelled out how this list is populated (i.e.
spelling out save_item->id() instead of asking the reader to guess/infer/know
that it.first is the same value).

[Randy Smith (Not in Mondays), 2015/12/07 19:57:14]

Acknowledged.

+  for (const auto& it : saved_failed_items_)
+    save_item_ids.push_back(it.first);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SaveFileManager::RemoveSavedFileFromFileMap,
-                 file_manager_,
-                 save_ids));
+      base::Bind(&SaveFileManager::RemoveSavedFileFromFileMap, file_manager_,
+                 save_item_ids));
 
   if (download_) {
     // Hack to avoid touching download_ after user cancel.
     // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
     // with SavePackage flow.
     if (download_->GetState() == DownloadItem::IN_PROGRESS) {
       if (save_type_ != SAVE_PAGE_TYPE_AS_MHTML) {
         download_->DestinationUpdate(
             all_save_items_count_, CurrentSpeed(), std::string());
         download_->OnAllDataSaved(DownloadItem::kEmptyFileHash);
       }
       download_->MarkAsComplete();
     }
     FinalizeDownloadEntry();
   }
 }
 
 // Called for updating end state.
-void SavePackage::SaveFinished(int32 save_id, int64 size, bool is_success) {
+void SavePackage::SaveFinished(int32 save_item_id,
+                               int64 size,
+                               bool is_success) {
   // Because we might have canceled this saving job before,
   // so we might not find corresponding SaveItem. Just ignore it.
-  SaveItem* save_item = LookupItemInProcessBySaveId(save_id);
+  SaveItem* save_item = LookupSaveItemInProcess(save_item_id);
   if (!save_item)
     return;
 
   // Let SaveItem set end state.
   save_item->Finish(size, is_success);
   // Remove the associated save id and SavePackage.
-  file_manager_->RemoveSaveFile(save_id, save_item->url(), this);
+  file_manager_->RemoveSaveFile(save_item->id(), this);
 
   PutInProgressItemToSavedMap(save_item);
 
   // Inform the DownloadItem to update UI.
   // We use the received bytes as number of saved files.
   // Hack to avoid touching download_ after user cancel.
   // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
   // with SavePackage flow.
   if (download_ && (download_->GetState() == DownloadItem::IN_PROGRESS)) {
     download_->DestinationUpdate(
@@ skipping 12 matching lines @@
     return;
   }
 
   // Continue processing the save page job.
   DoSavingProcess();
 
   // Check whether we can successfully finish whole job.
   CheckFinish();
 }
 
-// Sometimes, the net io will only call SaveFileManager::SaveFinished with
-// save id -1 when it encounters error. Since in this case, save id will be
-// -1, so we can only use URL to find which SaveItem is associated with
-// this error.
-// Saving an item failed. If it's a sub-resource, ignore it. If the error comes
-// from serializing HTML data, then cancel saving page.
-void SavePackage::SaveFailed(const GURL& save_url) {
-  SaveUrlItemMap::iterator it = in_progress_items_.find(save_url.spec());
-  if (it == in_progress_items_.end()) {
-    NOTREACHED();  // Should not exist!
-    return;
-  }
-  SaveItem* save_item = it->second;
-
-  save_item->Finish(0, false);
-
-  PutInProgressItemToSavedMap(save_item);
-
-  // Inform the DownloadItem to update UI.
-  // We use the received bytes as number of saved files.
-  // Hack to avoid touching download_ after user cancel.
-  // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
-  // with SavePackage flow.
-  if (download_ && (download_->GetState() == DownloadItem::IN_PROGRESS)) {
-    download_->DestinationUpdate(
-        completed_count(), CurrentSpeed(), std::string());
-  }
-
-  if ((save_type_ == SAVE_PAGE_TYPE_AS_ONLY_HTML) ||
-      (save_type_ == SAVE_PAGE_TYPE_AS_MHTML) ||
-      (save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM)) {
-    // We got error when saving page. Treat it as disk error.
-    Cancel(true);
-  }
-
-  if (canceled()) {
-    DCHECK(finished_);
-    return;
-  }
-
-  // Continue processing the save page job.
-  DoSavingProcess();
-
-  CheckFinish();
-}
-
 void SavePackage::SaveCanceled(SaveItem* save_item) {
   // Call the RemoveSaveFile in UI thread.
-  file_manager_->RemoveSaveFile(save_item->save_id(),
-                                save_item->url(),
-                                this);
-  if (save_item->save_id() != -1)
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&SaveFileManager::CancelSave,
-                   file_manager_,
-                   save_item->save_id()));
+  file_manager_->RemoveSaveFile(save_item->id(), this);
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&SaveFileManager::CancelSave, file_manager_, save_item->id()));
 }
 
 // Initiate a saving job of a specific URL. We send the request to
 // SaveFileManager, which will dispatch it to different approach according to
 // the save source. Parameter process_all_remaining_items indicates whether
 // we need to save all remaining items.
 void SavePackage::SaveNextFile(bool process_all_remaining_items) {
   DCHECK(web_contents());
   DCHECK(waiting_item_queue_.size());
 
   do {
     // Pop SaveItem from waiting list.
     SaveItem* save_item = waiting_item_queue_.front();
     waiting_item_queue_.pop_front();
 
     // Add the item to in_progress_items_.
-    SaveUrlItemMap::iterator it = in_progress_items_.find(
-        save_item->url().spec());
+    SaveItemIdMap::iterator it = in_progress_items_.find(save_item->id());
     DCHECK(it == in_progress_items_.end());
-    in_progress_items_[save_item->url().spec()] = save_item;
+    in_progress_items_[save_item->id()] = save_item;
     save_item->Start();
-    file_manager_->SaveURL(save_item->url(),
-                           save_item->referrer(),
-                           web_contents()->GetRenderProcessHost()->GetID(),
-                           routing_id(),
-                           web_contents()->GetMainFrame()->GetRoutingID(),
-                           save_item->save_source(),
-                           save_item->full_path(),
-                           web_contents()->
-                               GetBrowserContext()->GetResourceContext(),
-                           this);
+    file_manager_->SaveURL(
+        save_item->id(), save_item->url(), save_item->referrer(),
+        web_contents()->GetRenderProcessHost()->GetID(), routing_id(),
+        web_contents()->GetMainFrame()->GetRoutingID(),
+        save_item->save_source(), save_item->full_path(),
+        web_contents()->GetBrowserContext()->GetResourceContext(), this);
   } while (process_all_remaining_items && waiting_item_queue_.size());
 }
 
 // Calculate the percentage of whole save page job.
 int SavePackage::PercentComplete() {
   if (!all_save_items_count_)
     return 0;
   else if (!in_process_count())
     return 100;
   else
@@ skipping 159 matching lines @@
   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()) {
         wrote_to_completed_file_ = true;
         break;
       }
     }
 
-    auto it2 = saved_failed_items_.find(save_item->url().spec());
+    auto it2 = saved_failed_items_.find(save_item->id());
     if (it2 != saved_failed_items_.end())
       wrote_to_failed_file_ = true;
 
     return;
   }
 
   if (!data.empty()) {
     // Prepare buffer for saving HTML data.
     scoped_refptr<net::IOBuffer> new_data(new net::IOBuffer(data.size()));
     memcpy(new_data->data(), data.data(), data.size());
 
     // Call write file functionality in file thread.
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&SaveFileManager::UpdateSaveProgress,
-                   file_manager_,
-                   save_item->save_id(),
-                   new_data,
-                   static_cast<int>(data.size())));
+        base::Bind(&SaveFileManager::UpdateSaveProgress, file_manager_,
+                   save_item->id(), new_data, static_cast<int>(data.size())));
   }
 
   // Current frame is completed saving, call finish in file thread.
   if (end_of_data) {
     DVLOG(20) << " " << __FUNCTION__ << "()"
-              << " save_id = " << save_item->save_id()
-              << " url = \"" << save_item->url().spec() << "\"";
+              << " save_item_id = " << save_item->id() << " url = \""
+              << save_item->url().spec() << "\"";
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&SaveFileManager::SaveFinished, file_manager_,
-                   save_item->save_id(), save_item->url(), id(), true));
+                   save_item->id(), id(), true));
     number_of_frames_pending_response_--;
     DCHECK_LE(0, number_of_frames_pending_response_);
   }
-
-  // If all frames are finished saving, we need to close the remaining
-  // SaveItems.

[Randy Smith (Not in Mondays), 2015/12/03 20:58:28]

So I'm not understanding why this code was here in the first place, which makes
me worried that there's something about the overall code flow that I don't
understand.  Do you understand why this used to be needed and isn't any more in
your new scheme?

[Łukasz Anforowicz, 2015/12/04 21:16:45]

I also don't understand why this code was needed in the first place.  I've been
mindlessly ( :-( ) copy&pasting equivalents of the old code for a few iterations
already, starting from
https://chromium.googlesource.com/chromium/src/+/6e60c08c059cca5d0ff7750c3bc5...

Even back then (then = 6e60c08c059) - this code shouldn't have been needed (i.e.
even back then SaveFinished should have been called after
WebPageSerializerClient::CurrentFrameIsFinished, so I don't see a point for
calling it again after WebPageSerializerClient::AllFramesAreFinished).

The comment below
  // If the all frames are finished saving, we need to close the
  // remaining SaveItems.
dates all the way back to the "initial.commit", and digging more takes us to:
  commit c23f96fb89520df7ef0b09e4f246a111e2023230
  Author: jnd <jnd@9f7223c8-0a38-11dc-a625-d7732372afcb>
  Date:   Fri Apr 18 08:24:51 2008 +0000
  git-svn-id: https://svn/chrome/trunk@33628
9f7223c8-0a38-11dc-a625-d7732372afcb
but doesn't really offer more context...  We can ping jnd@ and/or rocking@ to
ask for feedback, but they probably forgot about this 7 year old commit...

(interestingly the |save_id| also got born in the pre-initial-commit above)

[Randy Smith (Not in Mondays), 2015/12/07 19:57:14]

Reasonable.  I certainly don't believe in the pre-SPOOPIF effort purity of this
section of the code base :-}.  I'll assume tests would catch any problems.

-  if (number_of_frames_pending_response_ == 0) {
-    for (SaveUrlItemMap::iterator it = in_progress_items_.begin();
-         it != in_progress_items_.end(); ++it) {
-      DVLOG(20) << " " << __FUNCTION__ << "()"
-                << " save_id = " << it->second->save_id() << " url = \""
-                << it->second->url().spec() << "\"";
-      BrowserThread::PostTask(
-          BrowserThread::FILE, FROM_HERE,
-          base::Bind(&SaveFileManager::SaveFinished, file_manager_,
-                     it->second->save_id(), it->second->url(), id(), true));
-    }
-  }
 }
 
 // Ask for all savable resource links from backend, include main frame and
 // sub-frame.
 void SavePackage::GetSavableResourceLinks() {
   if (wait_state_ != START_PROCESS)
     return;
 
   wait_state_ = RESOURCES_LIST;
 
@@ skipping 45 matching lines @@
     }
 
     EnqueueFrame(container_frame_tree_node_id,
                  subframe_tree_node->frame_tree_node_id(),
                  subframe.original_url);
   }
 
   CompleteSavableResourceLinksResponse();
 }
 
-SaveItem* SavePackage::FindOrCreatePendingSaveItem(
+SaveItem* SavePackage::CreatePendingSaveItem(
     int container_frame_tree_node_id,
     const GURL& url,
     const Referrer& referrer,
+    SaveFileCreateInfo::SaveFileSource save_source) {
+  DCHECK(url.is_valid());  // |url| should be validated by the callers.
+
+  SaveItem* save_item;
+  Referrer sanitized_referrer = Referrer::SanitizeForRequest(url, referrer);
+  save_item = new SaveItem(url, sanitized_referrer, this, save_source);
+  waiting_item_queue_.push_back(save_item);
+  url_to_save_item_[url] = save_item;  // Ok to clobber existing entries
+                                       // (in case of duplicated URIs).

[Randy Smith (Not in Mondays), 2015/12/03 20:58:28]

Hmmm.  I find myself wondering about assertion opportunities, since the times
when we're clobbering are I think exactly the times when that entry in
url_to_save_item_ won't ever be used.  But that relies a bit on sane behavior in
web pages (not having the same URL for an iframe and a subresource), which seems
a mistake, so mostly I'm just thinking out loud in case you see something we can
rely on in the web page behavior.

[Łukasz Anforowicz, 2015/12/04 21:16:45]

Thanks for mentioning that - I should have thought about it more.

I think that it is indeed possible that a given URL will be reported by
savable_resources.cc once as a resource and once as a frame.  I think this will
happen when an URL is used in a non-frame-owning element (weird, but possible). 
I think this might also happen in one specific combination of object-elements
and local/remote-frames.

I think that in such scenario, it is important to keep 2 save items (the one
associated with a URL [i.e. saved from the network as-is] and the one associated
with a FrameTreeNodeId [i.e. saved by serializing html dom]).  Therefore I think
I need to avoid setting url_to_save_item_in CreatePendingSaveItem, because it is
also used for frame-related SaveItems (and I said above I don't want
frame-related SaveItems to stop creation of the single resource).

With that being said, please note that the "clobbering" would still happen for
now in GetSerializedHtmlWithLocalLinksForFrame when populating the
url_to_local_path local variable.  I'll take a look at a follow-up CL (not sent
out to Rietveld yet) that splits url_to_local_path into url_to_local_path +
frame_routing_id_to_local_path and make sure that both SaveItems would appear
(one in one map, and the other int the other map).

[Łukasz Anforowicz, 2015/12/04 21:21:06]

I should have explained why I think this is important.  Keeping 2 save items is
important,
because otherwise the follow-up CL (i.e. crrev.com/1502563004) might end up not
putting the save item into urls_to_local_paths (i.e. it might put the save item
only into [frame_]routing_id_to_local_path).  We don't want that, because that
would mean the URL gets rewritten for frames (based on
[frame_]routing_id_to_local_path), but not for other elements.

[Randy Smith (Not in Mondays), 2015/12/07 19:57:14]

Thanks for the detailed exploration.  I agree keeping two SaveItems is
important, just because we're doing two very different things with the SaveItems
(ones managing a DOM save with some complexity, one a net resource save without
that complexity).  

I wonder a bit whether we might want to rename these functions, given that we're
now having differing implementations based on DOM vs. NET sources, and by name
(and save_source argument) they're sorta pretending that they're more general
than that.  I'll go with your instinct on that, but I'll at least raise the
possibility of making their names more specific and not passing in save_source
(or passing it in and DCHECKing, since the de-dup function probably needs to
handle FILE save sources as well).

[Łukasz Anforowicz, 2015/12/07 20:32:31]

The main reason why I didn't rename is the fact that CreatePendingSaveItem ends
up being called by both EnqueueFrame and by
CreatePendingSaveItemDeduplicatingByUrl (therefore we can't say that
CreatePendingSaveItem is frame-specific and
CreatePendingSaveItemDeduplicatingByUrl is savable-resources-specific).

OTOH, it is probably a good idea to verify / explicitly state via a DCHECK that
deduping should not be done for frames / DOM docs.  I've added a DCHECK.

+
+  frame_tree_node_id_to_contained_save_items_[container_frame_tree_node_id]
+      .push_back(save_item);
+  return save_item;
+}
+
+SaveItem* SavePackage::CreatePendingSaveItemDeduplicatingByUrl(
+    int container_frame_tree_node_id,
+    const GURL& url,
+    const Referrer& referrer,
     SaveFileCreateInfo::SaveFileSource save_source) {
   DCHECK(url.is_valid());  // |url| should be validated by the callers.
 
   SaveItem* save_item;
   auto it = url_to_save_item_.find(url);
   if (it != url_to_save_item_.end()) {
     save_item = it->second;
+    frame_tree_node_id_to_contained_save_items_[container_frame_tree_node_id]
+        .push_back(save_item);
   } else {
-    Referrer sanitized_referrer = Referrer::SanitizeForRequest(url, referrer);
-    save_item = new SaveItem(url, sanitized_referrer, this, save_source);
-    waiting_item_queue_.push_back(save_item);
-    url_to_save_item_[url] = save_item;
+    save_item = CreatePendingSaveItem(container_frame_tree_node_id, url,
+                                      referrer, save_source);
   }
 
-  frame_tree_node_id_to_contained_save_items_[container_frame_tree_node_id]
-      .push_back(save_item);
   return save_item;
 }
 
 void SavePackage::EnqueueSavableResource(int container_frame_tree_node_id,
                                          const GURL& url,
                                          const Referrer& referrer) {
   if (!url.is_valid())
     return;
 
   SaveFileCreateInfo::SaveFileSource save_source =
       url.SchemeIsFile() ? SaveFileCreateInfo::SAVE_FILE_FROM_FILE
                          : SaveFileCreateInfo::SAVE_FILE_FROM_NET;
-  FindOrCreatePendingSaveItem(container_frame_tree_node_id, url, referrer,
-                              save_source);
+  CreatePendingSaveItemDeduplicatingByUrl(container_frame_tree_node_id, url,
+                                          referrer, save_source);
 }
 
 void SavePackage::EnqueueFrame(int container_frame_tree_node_id,
                                int frame_tree_node_id,
                                const GURL& frame_original_url) {
   if (!frame_original_url.is_valid())
     return;
 
-  SaveItem* save_item = FindOrCreatePendingSaveItem(
-      container_frame_tree_node_id, frame_original_url, Referrer(),
-      SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
+  SaveItem* save_item =
+      CreatePendingSaveItem(container_frame_tree_node_id, frame_original_url,
+                            Referrer(), SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
   DCHECK(save_item);
   frame_tree_node_id_to_save_item_[frame_tree_node_id] = save_item;
 }
 
 void SavePackage::OnSavableResourceLinksError(RenderFrameHostImpl* sender) {
   CompleteSavableResourceLinksResponse();
 }
 
 void SavePackage::CompleteSavableResourceLinksResponse() {
   --number_of_frames_pending_response_;
@@ skipping 269 matching lines @@
 
 void SavePackage::FinalizeDownloadEntry() {
   DCHECK(download_);
   DCHECK(download_manager_);
 
   download_manager_->OnSavePackageSuccessfullyFinished(download_);
   StopObservation();
 }
 
 }  // namespace content