OOPIFs: Transitioning Get/Send...SavableResourceLinks away from RenderViewHost.

content/browser/download/save_package.h

 // 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.
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_SAVE_PACKAGE_H_
 #define CONTENT_BROWSER_DOWNLOAD_SAVE_PACKAGE_H_
 
 #include <queue>
 #include <set>
 #include <string>
@@ skipping 128 matching lines @@
 
   // Notes from Init() above applies here as well.
   void InternalInit();
 
   void Stop();
   void CheckFinish();
   void SaveNextFile(bool process_all_remainder_items);
   void DoSavingProcess();
 
   // WebContentsObserver implementation.
-  bool OnMessageReceived(const IPC::Message& message) override;
+  bool OnMessageReceived(const IPC::Message&) override;

[nasko, 2015/09/03 18:17:02]

Why did these methods lose the parameter name? Chromium style guide prohibits
nameless parameters (even though Blink allows them).

[Łukasz Anforowicz, 2015/09/03 19:41:24]

Because I saw this practice in Blink and thought that it is more readable (in
cases where the parameter name is the same as the type name).  I guess I need to
revert if the style guide says so... :-/

+  bool OnMessageReceived(const IPC::Message&, RenderFrameHost* source) override;

[nasko, 2015/09/03 18:17:02]

nit: Let's keep the parameter name consistent, s/source/render_frame_host/.

[Łukasz Anforowicz, 2015/09/03 19:41:24]

I think that "source" is a better parameter name, because it doesn't just repeat
the type of the parameter, but explains what is the meaning of this object in
the method.

Oh, I think that by "consistent" you mean "consistent with the name used in the
base virtual method that gets overridden here".  Hmmm... I still prefer
"source"...

[nasko, 2015/09/04 16:03:21]

In general, the Chrome codebase tries to be consistent and keep the parameter
names.

[Łukasz Anforowicz, 2015/09/08 16:23:21]

Ok, I've renamed to reuse the name picked by the base class...

 
   // DownloadItem::Observer implementation.
   void OnDownloadDestroyed(DownloadItem* download) override;
 
   // Update the download history of this item upon completion.
   void FinalizeDownloadEntry();
 
   // Detach from DownloadManager.
   void StopObservation();
 
   // Return max length of a path for a specific base directory.
   // This is needed on POSIX, which restrict the length of file names in
   // addition to the restriction on the length of path names.
   // |base_dir| is assumed to be a directory name with no trailing slash.
   static uint32 GetMaxPathLengthForDirectory(const base::FilePath& base_dir);
 
   static bool GetSafePureFileName(
       const base::FilePath& dir_path,
       const base::FilePath::StringType& file_name_ext,
       uint32 max_file_path_len,
       base::FilePath::StringType* pure_file_name);
 
   // Create a file name based on the response from the server.
   bool GenerateFileName(const std::string& disposition,
                         const GURL& url,
                         bool need_html_ext,
                         base::FilePath::StringType* generated_name);
 
-  // Get all savable resource links from current web page, include main
-  // frame and sub-frame.
-  void GetAllSavableResourceLinksForCurrentPage();
+  // Set of methods to get all savable resource links from current web page,
+  // including main frame and sub-frames.
+  void GetSavableResourceLinksForCurrentPage();
+  void GetSavableResourceLinksForFrame(RenderFrameHost* target);
+  void OnSavableResourceLinksResponse(
+      RenderFrameHost* source,
+      const std::vector<GURL>& resources_list,
+      const std::vector<Referrer>& referrers_list);
+  void OnSavableResourceLinksError(RenderFrameHost* source);
+  void CompleteSavableResourceLinksResponseFromFrame();
+
   // Get html data by serializing all frames of current page with lists
   // which contain all resource links that have local copy.
   void GetSerializedHtmlDataForCurrentPageWithLocalLinks();
 
   // Look up SaveItem by save id from in progress map.
   SaveItem* LookupItemInProcessBySaveId(int32 save_id);
 
   // Remove SaveItem from in progress map and put it to saved map.
   void PutInProgressItemToSavedMap(SaveItem* save_item);
 
   // Retrieves the URL to be saved from the WebContents.
   GURL GetUrlToBeSaved();
 
   void CreateDirectoryOnFileThread(const base::FilePath& website_save_dir,
                                    const base::FilePath& download_save_dir,
                                    bool skip_dir_check,
                                    const std::string& mime_type,
                                    const std::string& accept_langs);
   void ContinueGetSaveInfo(const base::FilePath& suggested_path,
                            bool can_save_as_complete);
   void OnPathPicked(
       const base::FilePath& final_name,
       SavePageType type,
       const SavePackageDownloadCreatedCallback& cb);
-  void OnReceivedSavableResourceLinksForCurrentPage(
-      const std::vector<GURL>& resources_list,
-      const std::vector<Referrer>& referrers_list,
-      const std::vector<GURL>& frames_list);
 
   void OnReceivedSerializedHtmlData(const GURL& frame_url,
                                     const std::string& data,
                                     int32 status);
 
   typedef base::hash_map<std::string, SaveItem*> SaveUrlItemMap;
   // in_progress_items_ is map of all saving job in in-progress state.
   SaveUrlItemMap in_progress_items_;
   // saved_failed_items_ is map of all saving job which are failed.
   SaveUrlItemMap saved_failed_items_;
@@ skipping 35 matching lines @@
 
   // Returns extension for supported MIME types (for example, for "text/plain"
   // it returns "txt").
   static const base::FilePath::CharType* ExtensionForMimeType(
       const std::string& contents_mime_type);
 
   typedef std::queue<SaveItem*> SaveItemQueue;
   // A queue for items we are about to start saving.
   SaveItemQueue waiting_item_queue_;
 
+  // Used to de-dupe urls that are being gathered into |waiting_item_queue_|.
+  std::set<GURL> unique_urls_to_save_;
+
+  // Number of pending FrameMsg_GetSavableResourceLinks without a response.
+  int number_of_frames_with_pending_get_savable_resource_links_;

[nasko, 2015/09/03 18:17:02]

I wonder if we can name this a bit shorter. Do we send any other message that we
wait on for responses? If not, number_of_frames_pending_response_ is a lot
shorter and avoids the actual IPC message name.

[Łukasz Anforowicz, 2015/09/03 19:41:24]

We will probably need something equivalent for per-frame-equivalents of
ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks and
ViewMsg_SavePageAsMHTML.  So, I think I'll stick with the current name for now.

OTOH, For a moment I wondered whether I can have something generic here -
something equivalent to WebContents::SendToAllFrames, but that takes a callback
that is invoked with all the gathered results.  I didn't see an immediately
obvious way to do it, so I didn't go there.  WDYT?

[nasko, 2015/09/04 16:03:21]

The latter message will likely need it. The question is does it really matter
which type of responses we are waiting to hear from the renderer? In general, it
is really a counter of how many messages we've sent that haven't been replied
to. Do we support concurrent save operations?
 
I think the main problem is that each IPC message is independent from the rest
and we don't have formal code way to express that a message is reply to another.
This makes wiring it such that a callback is called on a reply not very obvious.
I'd suggest writing it in the style the owners of this file prefer.

[Łukasz Anforowicz, 2015/09/08 16:23:21]

Ah, makes sense.  Each instance of SavePackage only handles a single
save-page-as + it only does one kind of messages at a time.  So - done.  Thanks.
I was thinking about temporarily instantiating a separate WebContentsObserver
that intercepts messages (so no need to wire-in an external message callback). 
The main problem was with figuring out how to infer-or-specify the type of
values carried in the responses.

+
   typedef base::hash_map<int32, SaveItem*> SavedItemMap;
   // saved_success_items_ is map of all saving job which are successfully saved.
   SavedItemMap saved_success_items_;
 
   // Non-owning pointer for handling file writing on the file thread.
   SaveFileManager* file_manager_;
 
   // DownloadManager owns the DownloadItem and handles history and UI.
   DownloadManagerImpl* download_manager_;
   DownloadItemImpl* download_;
@@ skipping 57 matching lines @@
   friend class SavePackageTest;
   FRIEND_TEST_ALL_PREFIXES(SavePackageTest, TestSuggestedSaveNames);
   FRIEND_TEST_ALL_PREFIXES(SavePackageTest, TestLongSafePureFilename);
 
   DISALLOW_COPY_AND_ASSIGN(SavePackage);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_SAVE_PACKAGE_H_