content/browser/download/mhtml_extra_data.h - Issue 2683493002: Get signals working in the EXTRA_DATA section of MHTML

Side by Side Diff: content/browser/download/mhtml_extra_data.h

Issue 2683493002: Get signals working in the EXTRA_DATA section of MHTML (Closed)

Patch Set: Don't forget the new .h file Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« chrome/browser/android/offline_pages/prerendering_offliner.cc ('K') | « chrome/browser/android/offline_pages/prerendering_offliner.cc ('k') | content/browser/download/mhtml_generation_browsertest.cc » ('j') | content/browser/download/mhtml_generation_browsertest.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright (c) 2017 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4 #ifndef CONTENT_BROWSER_DOWNLOAD_MHTML_EXTRA_DATA_H_

	5 #define CONTENT_BROWSER_DOWNLOAD_MHTML_EXTRA_DATA_H_

	6

	7 #include <string>

	8

	9 #include "base/supports_user_data.h"

	10

	11 namespace content {
	Dmitry Titov 2017/03/16 02:28:20 Not sure why this file should live in content... T Not sure why this file should live in content... This can be part of component/offline_pages/offliner.h for example. It doesn't have to live in content/browser... Crazy idea: in the future, this can be a SnapshotController as well... Pete Williamson 2017/03/17 23:28:32 We dicussed this in person, and agreed to move the Show quoted text On 2017/03/16 02:28:20, Dmitry Titov wrote: > Not sure why this file should live in content... This can be part of > component/offline_pages/offliner.h for example. It doesn't have to live in > content/browser... > > Crazy idea: in the future, this can be a SnapshotController as well... We dicussed this in person, and agreed to move the file into mhtml_generation_manager.h. The implementation got moved into mhtml_generation_manager.cc I like the idea of moving this into our SnapshotController/SignalCollector.
	12

	13 const void* kMHTMLExtraDataKey = (void*)0x14725138;
	Pete Williamson 2017/03/15 22:38:22 This is a kind of clunk way to do a key, just pick This is a kind of clunk way to do a key, just picking some arbitrary number. I tried previously using a string here, but it got put into different compilation units, which means it address was not constant. Reviewers, any suggestions on how to make this nicer? Dmitry Titov 2017/03/16 02:28:20 It is usually done like this: static int kFooKey; It is usually done like this: static int kFooKey; ... SetUserData(&kFooKey, data); fgorski 2017/03/16 17:46:31 It was in different compilation unit because you p Show quoted text On 2017/03/16 02:28:20, Dmitry Titov wrote: > It is usually done like this: > > static int kFooKey; > > ... > SetUserData(&kFooKey, data); It was in different compilation unit because you put it in .h file instead of .cc file... It is best if you actually declare it in .cc and add a static method to this class that will know how to get/set itself from/on web_contents... Otherwise we will run into trouble. E.g.: https://cs.chromium.org/chromium/src/components/offline_pages/core/downloads/... Pete Williamson 2017/03/17 23:28:32 Done. Show quoted text On 2017/03/16 02:28:20, Dmitry Titov wrote: > It is usually done like this: > > static int kFooKey; > > ... > SetUserData(&kFooKey, data); Done. Pete Williamson 2017/03/17 23:28:32 Done. Show quoted text On 2017/03/16 17:46:31, fgorski wrote: > On 2017/03/16 02:28:20, Dmitry Titov wrote: > > It is usually done like this: > > > > static int kFooKey; > > > > ... > > SetUserData(&kFooKey, data); > > It was in different compilation unit because you put it in .h file instead of > .cc file... > > It is best if you actually declare it in .cc and add a static method to this > class that will know how to get/set itself from/on web_contents... Otherwise we > will run into trouble. > > > E.g.: > https://cs.chromium.org/chromium/src/components/offline_pages/core/downloads/... Done.
	14

	15 struct MHTMLExtraData : public base::SupportsUserData::Data {

	16 MHTMLExtraData(std::string data) : data_string(data) {}

	17 ~MHTMLExtraData() override {}

	18 std::string data_string;

	19 };

	20

	21 } // namespace content

	22

	23 #endif // CONTENT_BROWSER_DOWNLOAD_MHTML_EXTRA_DATA_H_

OLD	NEW