Get signals working in the EXTRA_DATA section of MHTML

content/browser/download/mhtml_extra_parts_impl.h

+// Copyright 2017 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_EXTRA_PARTS_IMPL_H_
+#define CONTENT_BROWSER_DOWNLOAD_MHTML_EXTRA_PARTS_IMPL_H_
+
+#include "content/public/browser/mhtml_extra_parts.h"
+
+namespace content {
+
+// Data fields used to build an additional MHTML part in the output file.
+struct MHTMLExtraDataPart {
+  std::string content_type;
+  std::string content_location;
+  std::string body;
+};
+
+// Class used as a data object for WebContents UserData to represent a MHTML
+// part that we plan to write into the output MHTML file.  Each MHTMLExtraPart
+// object in the contained vector lets us hold enough information to generate
+// one MHTML part.  This allows arbitrary extra MHTML parts to be added into the
+// complete file.  For instance, this might be used for gathering load time
+// signals in debug code for analysis.
+class MHTMLExtraPartsImpl : public content::MHTMLExtraParts {
+ public:
+  MHTMLExtraPartsImpl();
+  ~MHTMLExtraPartsImpl() override;
+
+  // Return the vector of parts to be serialized.
+  std::vector<MHTMLExtraDataPart>& parts() { return parts_; }

[Dmitry Titov, 2017/03/29 23:26:29]

It's weird to have both methods like this. There should be one. Do you need to
modify the vector beyond AddExtraHTMLPart?

[Pete Williamson, 2017/03/31 00:29:49]

The test needs the writeable version of parts(), so we can add more parts to be
tested.

The chrome code needs the const version, since we pass in a const
MHTMLExtraPartsImpl to the WriteExtraDataParts function.

I could fix this by passing a non-const MHTMLExtraPartsImpl, but I didn't want
to limit the code safety to make the test easier (and sometimes it is hard to
pass a non-const pointer across threads).  I could fix this by making the test
class a friend, but having two accessors seemed cleaner to me.  I'm happy to use
one of the other solutions if you disagree (or add comments about why there are
two).

[Dmitry Titov, 2017/03/31 18:05:53]

What test is using it? I don't see.

[Pete Williamson, 2017/03/31 21:58:03]

 Ah, sorry, it looks like that test got refactored out of existence.  Removing
the non-const accessor.

+  const std::vector<MHTMLExtraDataPart>& parts() const { return parts_; }
+
+  // Creates a MHTMLExtraDataPart and adds it to our vector of parts.
+  void AddExtraMHTMLPart(const std::string& content_type,
+                         const std::string& content_location,
+                         const std::string& body) override;
+
+ private:
+  std::vector<MHTMLExtraDataPart> parts_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_MHTML_EXTRA_PARTS_IMPL_H_