Get signals working in the EXTRA_DATA section of MHTML

content/browser/download/mhtml_generation_manager.h

Index: content/browser/download/mhtml_generation_manager.h
diff --git a/content/browser/download/mhtml_generation_manager.h b/content/browser/download/mhtml_generation_manager.h
index e30001535b03735758e09f2734d69ce5cd1c03d7..feab126dd98ce2ee6278395bde4a2525cbb0da54 100644
--- a/content/browser/download/mhtml_generation_manager.h
+++ b/content/browser/download/mhtml_generation_manager.h
@@ -15,7 +15,9 @@
 #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"
 
@@ -34,7 +36,7 @@ class WebContents;
 // 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();
 
@@ -58,6 +60,36 @@ class MHTMLGenerationManager {
       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;