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

content/browser/download/save_package.h

Index: content/browser/download/save_package.h
diff --git a/content/browser/download/save_package.h b/content/browser/download/save_package.h
index 071a5b7adead60a4eebbad9f4208bd2b957dd38d..ca77da5bd2a26898afca710e0e421d8aeb5e881f 100644
--- a/content/browser/download/save_package.h
+++ b/content/browser/download/save_package.h
@@ -146,7 +146,8 @@ class CONTENT_EXPORT SavePackage
   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;
@@ -175,9 +176,17 @@ class CONTENT_EXPORT SavePackage
                         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();
@@ -202,10 +211,6 @@ class CONTENT_EXPORT SavePackage
       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,
@@ -261,6 +266,12 @@ class CONTENT_EXPORT SavePackage
   // 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_;