Get signals working in the EXTRA_DATA section of MHTML

content/browser/download/mhtml_generation_manager.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_MHTML_GENERATION_MANAGER_H_
 #define CONTENT_BROWSER_DOWNLOAD_MHTML_GENERATION_MANAGER_H_
 
 #include <stdint.h>
 
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
 
 #include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "base/process/process.h"
+#include "base/supports_user_data.h"
 #include "content/common/download/mhtml_save_status.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/mhtml_generation_params.h"
 #include "ipc/ipc_platform_file.h"
 
 namespace base {
 class FilePath;
 }
 
 namespace content {
 
 class RenderFrameHostImpl;
 class WebContents;
 
 // The class and all of its members live on the UI thread.  Only static methods
 // are executed on other threads.
 //
 // MHTMLGenerationManager is a singleton.  Each call to SaveMHTML method creates
 // a new instance of MHTMLGenerationManager::Job that tracks generation of a
 // single MHTML file.
-class MHTMLGenerationManager {
+class CONTENT_EXPORT MHTMLGenerationManager {
  public:
   static MHTMLGenerationManager* GetInstance();
 
   // GenerateMHTMLCallback is called to report completion and status of MHTML
   // generation.  On success |file_size| indicates the size of the
   // generated file.  On failure |file_size| is -1.
   typedef base::Callback<void(int64_t file_size)> GenerateMHTMLCallback;
 
   // Instructs the RenderFrames in |web_contents| to generate a MHTML
   // representation of the current page.
   void SaveMHTML(WebContents* web_contents,
                  const MHTMLGenerationParams& params,
                  const GenerateMHTMLCallback& callback);
 
   // Handler for FrameHostMsg_SerializeAsMHTMLResponse (a notification from the
   // renderer that the MHTML generation finished for a single frame).
   void OnSerializeAsMHTMLResponse(
       RenderFrameHostImpl* sender,
       int job_id,
       MhtmlSaveStatus save_status,
       const std::set<std::string>& digests_of_uris_of_serialized_resources,
       base::TimeDelta renderer_main_thread_time);
 
+  // Contained class that represents an extra data section.

[Dmitry Titov, 2017/03/18 00:58:34]

This mechanism needs a great deal of comment, to explain what capability it
provides, and how to use it. I think a good comment is needed here.

[Pete Williamson, 2017/03/20 18:26:53]

Added comments, LMK if I need more.

+  struct MHTMLExtraSection {

[Dmitry Titov, 2017/03/18 00:58:34]

Naming suggestion: MHTMLExtraSection -> ExtraPart. They are calling those Mime
"parts", not "sections".
Another possibility is to not expose this at all, and have AddExtraPart on the
class/interface below, with 3 string parameters. Internally it can be a struct.

[Pete Williamson, 2017/03/20 18:26:53]

Done.

+    std::string content_type;
+    std::string content_location;
+    std::string body;
+  };
+
+  // Helper class to put extra sections into the WebContents user data, and take
+  // them back out again.
+  class MHTMLExtraData : public base::SupportsUserData::Data {
+   public:
+    MHTMLExtraData();
+

[Dmitry Titov, 2017/03/18 00:58:34]

extra empty line

[Pete Williamson, 2017/03/20 18:26:53]

Done.

+    ~MHTMLExtraData() override;
+
+    std::vector<MHTMLExtraSection>* sections() { return &sections_; }

[Dmitry Titov, 2017/03/18 00:58:34]

It feels that the access to the vector is not needed - we don't expect users of
this modifying it, right? Just adding. So how about exposing
AddSection(MHTMLExtraSection) method and keeping the vector itself a private
member with MHTMLGenerationManager being a friend class?

Another possibility would be to expose an interface here, and then have a class
in .cc file that implements that interface, and has a public vector.

[Pete Williamson, 2017/03/20 18:26:53]

Turns out I didn't really need this after all, I was calling it from within the
class, I can use the member variable instead.  Method removed. 

+
+    // Get the data string out of the web contents.
+    // The web contents retains ownership of the vector
+    static std::vector<MHTMLExtraSection>* FromWebContents(
+        content::WebContents* contents);
+
+    // Stash the data string in the web contents user data.
+    static void AddToWebContents(content::WebContents* contents,

[Dmitry Titov, 2017/03/18 00:58:34]

Not sure why this method is needed.

[Pete Williamson, 2017/03/20 18:26:53]

This method is for adding a part to the vector stored on the web contents.  I
changed the name to AddPartToWebContents, and clarified the comment.

+                                 MHTMLExtraSection& section);
+
+   private:
+    std::vector<MHTMLExtraSection> sections_;
+  };
+
  private:
   friend struct base::DefaultSingletonTraits<MHTMLGenerationManager>;
   class Job;
 
   MHTMLGenerationManager();
   virtual ~MHTMLGenerationManager();
 
   // Called on the file thread to create |file|.
   static base::File CreateFile(const base::FilePath& file_path);
 
@@ skipping 23 matching lines @@
   std::map<int, std::unique_ptr<Job>> id_to_job_;
 
   int next_job_id_;
 
   DISALLOW_COPY_AND_ASSIGN(MHTMLGenerationManager);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_MHTML_GENERATION_MANAGER_H_