Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683493002:
Get signals working in the EXTRA_DATA section of MHTML (Closed)
|
Created:
3 years, 10 months ago by Pete Williamson Modified:
3 years, 7 months ago Reviewers:
David Trainor- moved to gerrit, jochen (gone - plz use gerrit), Steven Holte, fgorski, Dmitry Titov, Bryan McQuade, romax CC:
asanka, cbentzel+watch_chromium.org, chili+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dewittj+watch_chromium.org, dimich+watch_chromium.org, fgorski+watch_chromium.org, gavinp+prer_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org, tburkard+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGet signals working in the EXTRA_DATA section of MHTML
The intent of this change is to add some signal data to the bottom of
a MHTML file when generating an offline file if the new OfflinePagesLoadSignalCollecting flag is set. The signal data
will be analyzed to find a better time to take snapshots, and will eventually be used to decide at runtime when to take snapshots. Design docs here:
https://docs.google.com/document/d/1AcoZOIb81V68_KDSOCatFSYYMLOTSYj6resg-VAxLgo
BUG=706628
Review-Url: https://codereview.chromium.org/2683493002
Cr-Commit-Position: refs/heads/master@{#463407}
Committed: https://chromium.googlesource.com/chromium/src/+/2abcf0413069159c73bceba72f0b6f124021bca5
Patch Set 1 #
Total comments: 7
Patch Set 2 : Approach for writing to the file afterwards instead. #
Total comments: 18
Patch Set 3 : Third approach - write MHTML from browser side. #Patch Set 4 : FIx tests, add unit test #
Total comments: 6
Patch Set 5 : Using WebContents::SupportsUserData #
Total comments: 1
Patch Set 6 : Don't forget the new .h file #
Total comments: 27
Patch Set 7 : Add Arbitrary extra data sections to MHTML. #
Total comments: 17
Patch Set 8 : Dimich CR fixes #
Total comments: 8
Patch Set 9 : Fix build and tests #Patch Set 10 : CR feedback per FGorski #Patch Set 11 : Tentative Compile Fix #Patch Set 12 : Another attempted link fix for the tests. #Patch Set 13 : Another link test. #Patch Set 14 : Move MHTMLExtraData and MHTMLExtraPart out of the .h file #
Total comments: 2
Patch Set 15 : Remove build rule that we probably don't need. #Patch Set 16 : Move content interface to content/public #Patch Set 17 : Attempt to clear trybots. #
Total comments: 20
Patch Set 18 : CR Feedback per FGorski #
Total comments: 28
Patch Set 19 : CR feedback per Dimich #
Total comments: 17
Patch Set 20 : Dimich and DTrainor CR fixes #
Total comments: 4
Patch Set 21 : Fix AboutFlagsHistogramTest for new chrome flag #Patch Set 22 : Even more Dimich CR fixes #
Total comments: 1
Patch Set 23 : Add headers from the prerendering side. #
Total comments: 4
Patch Set 24 : Change header for our data to be X-Chrome- instead of Chromium- #
Total comments: 24
Patch Set 25 : FGorski CR feedback #
Total comments: 3
Patch Set 26 : Fix compile on platforms where lld is not 64 bit int. #Patch Set 27 : Merge changelist with tip of tree #Messages
Total messages: 161 (101 generated)
petewil@chromium.org changed reviewers: + dimich@chromium.org
Dimich - Does this approach look correct to you?
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive-by: From the perspective of extending the interface to SavePage it looks good. (with comments). Could you please include description of how the MHTML snapshot is going to be affected by that change? (If can be in a bug). https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc (right): https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:61: signal_data_ = signal_data; If I understand correctly this will be a copy. Can we avoid that? Perhaps just & instead of const &, followed by a swap? https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:102: params.extra_data = signal_data_; Here as well. https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h (right): https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h:85: // training a recognizer for when to take a snapshot. s/training a recognizer/improving a component/ https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:154: signal_data_.push_back(std::string("OnLoad seen")); "OnLoad" is enough, as "seen" is implied. How about putting the strings in constants? https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:170: signal_data_.push_back(std::string("DCL seen")); can you please expand DCL to full name and remove the "seen"? https://codereview.chromium.org/2683493002/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:238: ipc_params.extra_data = params_.extra_data; do we really want to pass the data over IPC? Did you consider appending to the file when it is already written? (This implies handling the file size appropriately as well)
https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc (right): https://codereview.chromium.org/2683493002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:61: signal_data_ = signal_data; On 2017/02/07 22:06:07, fgorski wrote: > If I understand correctly this will be a copy. Can we avoid that? Perhaps just & > instead of const &, followed by a swap? I think this is also equivalent to passing a std::unique_ptr<...>, with the latter being more explicit about the ownership transferring.
I wonder if we can avoid passing a vector of strings to Blink over IPC for the sole reason that it'll dump it into a file... It seems we create a file on the browser side, then pass the handle to frames one by one (possibly in various renderers). At the end, browser closes the file. Is it possible to just dump the signals section in browser process before closing the file?
Can you do a quick check and see if you like this approach better? This is not ready to commit yet, I'm just looking for feedback on the approach and the TODOs. Thanks!
On 2017/02/27 22:49:14, Pete Williamson wrote: > Can you do a quick check and see if you like this approach better? > > This is not ready to commit yet, I'm just looking for feedback on the approach > and the TODOs. > > Thanks! Let's discuss this in the office tomorrow. Why is the implementation sitting inside of prerendering_offliner?
On 2017/02/28 00:08:55, fgorski wrote: > On 2017/02/27 22:49:14, Pete Williamson wrote: > > Can you do a quick check and see if you like this approach better? > > > > This is not ready to commit yet, I'm just looking for feedback on the approach > > and the TODOs. > > > > Thanks! > > Let's discuss this in the office tomorrow. > Why is the implementation sitting inside of prerendering_offliner? I agree with Filip's comment. It feels like maybe it belongs somewhere closer to MHTML generation, such as in FrameSerializer::Delegate where you can access Document::hasParsingFinished() for DCL and Document::isLoadCompleted() for onLoad.
On 2017/02/28 00:45:07, dewittj wrote: > On 2017/02/28 00:08:55, fgorski wrote: > > On 2017/02/27 22:49:14, Pete Williamson wrote: > > > Can you do a quick check and see if you like this approach better? > > > > > > This is not ready to commit yet, I'm just looking for feedback on the > approach > > > and the TODOs. > > > > > > Thanks! > > > > Let's discuss this in the office tomorrow. > > Why is the implementation sitting inside of prerendering_offliner? > > I agree with Filip's comment. It feels like maybe it belongs somewhere closer > to MHTML generation, such as in FrameSerializer::Delegate where you can access > Document::hasParsingFinished() for DCL and Document::isLoadCompleted() for > onLoad. I agree it does not belong in PrerenderingOffliner, and the recording to file can be done before closing of the file during MHTML generation. I don't think this should live in Blink though, since data is collected on browser side, and file also is opened/closed on bowser side. Also, there can be separate Blinks involved in a single page. Looks like there is enough opinions to discuss in the office tomorrow.
We discussed this in person, and decided on the following approach: The signal data will be one big string (maybe JSON, maybe CSV). The signal data will be collected by the offliner. The signal data will be passed to the OfflinePageModel call to save as MHTML. It will be added to the MhtmlGenerationManager's Params struct. The MhtmlGenerationManager will be modified to add a new section at the end with the signal data. It will use mime type text/plain (or maybe application/json if we decide on JSON), and an x- header that is unique to our signal data. The MhtmlGenerationManager part of the change will need to wait for CarlosK's fix to move the footer generation into MhtmlGenerationManager. The next patch will have these changes.
Considering this is taking some time, please feel free to ping explicitly when the updated patch is uploaded!
https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:157: signal_data_.push_back(std::string("OnLoad signal seen")); OnLoad,time_ms Please :) https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:176: signal_data_.push_back(std::string("Dom Content Loaded signal seen")); DomContentLoaded,time_ms Please :) https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:139: // Append signal data to the savevd file. saved https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:140: base::FilePath filepath_copy(saved_filepath); Is there a reason for copying the path here? https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:295: void PrerenderingOffliner::AppendSignalDataOnFileThread( I'd reverse the names of methods. https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:296: const base::FilePath& file_path) { Add appropriate check for thread here. https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:301: weak_ptr_factory_.GetWeakPtr(), file_path)); weak pointers are not thread-safe If you want a callback from the other thread, it should work then (when posted back here). https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:306: void PrerenderingOffliner::AppendSignalData(const base::FilePath& file_path) { Can we make it into a function (to avoid temptation of accessing the fields from IO thread). You can pass ownership of signal data to IO or simply copy for now. https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:311: if (signals.size() == 0) question 1: What is the point of switching threads if we don't have signals question 2: Would putting in the header section hurt to indicate that there were no signals captured? (Or do we have to figure it out based on missing section) https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:319: DVLOG(0) << "@@@@@@ opened file OK"; how do you know if opened fine? Isn't there a validation you need to do for that? https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:322: std::string boundary = GetBoundaryStringFromFile(saved_file); you don't need this until 344 https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:325: // Chop off the last two hypens from the boundary string. We do this by hyphens? https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:329: // TODO: \n, or \r\f, or something else? would std::endl work? https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:355: std::string PrerenderingOffliner::GetBoundaryStringFromFile( Could this be a function in implementation file? https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:377: boundary = found_character + boundary; Would it make sense to establish position first and then read the string from file that spans the right length? Alternatively boundary.insert(0, found_character); But I still think we could avoid so many string concatenations. https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:383: if (one_dash_seen && found_character == '-') int seen_dashes_count = 0; while (seen_dashes_count < 2 && ...) { if (found_character == '-') ++seen_dashes_count; else seen_dashes_count = 0; https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:407: int64_t PrerenderingOffliner::PositionBeforeTwoDashes(base::File& saved_file) { if you add end position to signature, you could nicely replace the while loop above with a call to this function, as in: find last index of "--" before this position. Special handling of white space might be needed, but perhaps function checking validity of the file could handle that. https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.h (right): https://codereview.chromium.org/2683493002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:88: // TODO: Find a standard function to replace this with. std::isspace ?
Just realized that Pete didn't upload a new patch, but Dmitry made a comment. There are things to think about thou ;)
On 2017/03/08 18:57:30, fgorski wrote: > Just realized that Pete didn't upload a new patch, but Dmitry made a comment. > > There are things to think about thou ;) Right now this is blocked waiting for the MHTML footer work that CarlosK is doing, when that patch lands, I'll have the next update here. When I make my next update, I'll take into account fgorski's latest feedback.
petewil@chromium.org changed reviewers: + romax@chromium.org - fgorski@chromium.org
OK, Third approach, this should meet with our discussion. Does this approach look reasonable?
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Yes, looks much better! Thanks for doing changes. I have couple of comments, one is general: Instead of piping a singnal_data from the top down all the way as additional parameter to archiver/model/etc, woudl it be better to create an object that is connected ot WebContents as UserData, collects data and then serializes it into a string (and mime type) when asked. Since the MHTM Lis generated from WebContents, and signals are collected for a given WebContents, this woudl have the right lifetime and also avoid just passing the parameter all the way through. AT the very bottom leayer, at mhtml generation manager, it'd try to pull it from WebContents and then use it is it's there. WDYT? https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:459: base::StringPrintf("--%s--\r\n%s\r\n%s\r\n", boundary.c_str(), It looks like you are trying to form a 'legit' MHTML part here, with header and body subparts separated with \r\n. This is good, and perhaps we should use legit HTTP headers here as Content-Type: and Content-Location: (the latter with a generated CID). Also, the content type could have been provided by the same code that serializes the 'extra_data' string, because that code is deciding what kind of serialization to use.
https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:459: base::StringPrintf("--%s--\r\n%s\r\n%s\r\n", boundary.c_str(), is it possible to have some kind of signs that this section is used for signal recording? An explicit sign would be pretty helpful when parsing the raw MHTML. https://codereview.chromium.org/2683493002/diff/60001/content/public/common/m... File content/public/common/mhtml_generation_params.h (right): https://codereview.chromium.org/2683493002/diff/60001/content/public/common/m... content/public/common/mhtml_generation_params.h:9: #include <vector> seems like vector is not being used
Next patch, addressing all open concerns. I have added a few concerns of my own that I'd like reviewer's opinions on, please see my comments and let me know what you think would be best. Thanks! https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:459: base::StringPrintf("--%s--\r\n%s\r\n%s\r\n", boundary.c_str(), On 2017/03/14 01:16:02, Dmitry Titov wrote: > It looks like you are trying to form a 'legit' MHTML part here, with header and > body subparts separated with \r\n. This is good, and perhaps we should use legit > HTTP headers here as Content-Type: and Content-Location: (the latter with a > generated CID). > > Also, the content type could have been provided by the same code that serializes > the 'extra_data' string, because that code is deciding what kind of > serialization to use. I moved the Content-Type to where I build the data. I've added Content-Location, but not sure what to use for CID - any ideas? Maybe just the filename for the MHTML file? https://codereview.chromium.org/2683493002/diff/60001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:459: base::StringPrintf("--%s--\r\n%s\r\n%s\r\n", boundary.c_str(), On 2017/03/14 19:08:05, romax wrote: > is it possible to have some kind of signs that this section is used for signal > recording? An explicit sign would be pretty helpful when parsing the raw MHTML. I explicitly kept this section as extra data. Signals is just one potential use. So, from the point of view of the MHTML file, this is just extra data it doesn't know much about, but from the point of view of the offliner, this is signal data. The naming in each file reflects this idea. https://codereview.chromium.org/2683493002/diff/60001/content/public/common/m... File content/public/common/mhtml_generation_params.h (right): https://codereview.chromium.org/2683493002/diff/60001/content/public/common/m... content/public/common/mhtml_generation_params.h:9: #include <vector> On 2017/03/14 19:08:06, romax wrote: > seems like vector is not being used Ended up reverting this file https://codereview.chromium.org/2683493002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/DEPS (right): https://codereview.chromium.org/2683493002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/DEPS:4: "+content/browser/download", Is adding the directory I need to DEPS right, or should I instead move the new .h file into content/public/browser/download (creating a new download folder just for this .h file). I'm also open to other places I could put this. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:13: const void* kMHTMLExtraDataKey = (void*)0x14725138; 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?
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:87: const std::vector<std::string>& GetSignalData() { return signal_data_; } "Signal" is very generic, it would not be clear to the code reader which signals are meant here. Some longer name for those signals could be better, like LoadingCompletenessSignals? https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:88: const char kContentTypeLine[] = "Content-Type: application/json\r\n"; this line would normally be at the beginning of the file in anon namespace... https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:93: for (std::string signal : signals) { I'd move this whole block into a separate function, SerializeLoadCompletenessSignalData or something like this. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:11: namespace content { 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... https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:13: const void* kMHTMLExtraDataKey = (void*)0x14725138; It is usually done like this: static int kFooKey; ... SetUserData(&kFooKey, data); https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:206: web_contents->GetUserData(content::kMHTMLExtraDataKey); It is better to follow other examples that generally look like this: static Foo* Foo::FromWebContents(WebContents*) {} See, for example DownloadUIAdapter or other classes that define FromWebContents static. this allows to keep kFooKey in foo.cc file... https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:442: save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; I think once there is a FILE_WRITING_ERROR, there should be no further writes to the file... likely early return here? https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:473: std::string content_location_value; line 345 in this file is an example of how we generate CIDs in MHTML. This results in something like cid:frame-62691-645341c4-62b3-478e-a8c5-e0dfccc3ca02@mhtml.blink I think we can continue this with something like cid:extra-data-[GUID]@mhtml.blink
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:13: const void* kMHTMLExtraDataKey = (void*)0x14725138; 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/... https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_browsertest.cc (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_browsertest.cc:691: // here. shell()->web_contents()->DetachFromSequence(); What exactly happens here? Comment does not help, especially when it contains commented out code. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:206: web_contents->GetUserData(content::kMHTMLExtraDataKey); On 2017/03/16 02:28:21, Dmitry Titov wrote: > It is better to follow other examples that generally look like this: > > static Foo* Foo::FromWebContents(WebContents*) {} > > See, for example DownloadUIAdapter or other classes that define FromWebContents > static. this allows to keep kFooKey in foo.cc file... +1. Details in my earlier comment. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:482: if (file.WriteAtCurrentPos(extra_data_section.data(), return file.Write... >= 0; https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:483: extra_data_section.size()) < 0) nit: {} if you don't apply above.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Latest version. Based on an offline chat with Dimich, we agreed to put the extra data accessing code into content/browser/download/mhtml_generation_manager.cc/.h. I also made the code more generic, so you can call the API to add any arbitrary extra section to generated MHTML by specifying the content-type, content-location, and body. For a request by FGorski, I added the times for the signals. When our team has consensus, I'll send this to DTrainor to give an OWNERS review of the changes in content/browser/download. https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:87: const std::vector<std::string>& GetSignalData() { return signal_data_; } On 2017/03/16 02:28:20, Dmitry Titov wrote: > "Signal" is very generic, it would not be clear to the code reader which signals > are meant here. Some longer name for those signals could be better, like > LoadingCompletenessSignals? Renamed to GetLoadingSignalData. https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:88: const char kContentTypeLine[] = "Content-Type: application/json\r\n"; On 2017/03/16 02:28:20, Dmitry Titov wrote: > this line would normally be at the beginning of the file in anon namespace... Done. https://codereview.chromium.org/2683493002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:93: for (std::string signal : signals) { On 2017/03/16 02:28:20, Dmitry Titov wrote: > I'd move this whole block into a separate function, > SerializeLoadCompletenessSignalData or something like this. > Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:11: namespace content { 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. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:13: const void* kMHTMLExtraDataKey = (void*)0x14725138; On 2017/03/16 02:28:20, Dmitry Titov wrote: > It is usually done like this: > > static int kFooKey; > > ... > SetUserData(&kFooKey, data); Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_extra_data.h:13: const void* kMHTMLExtraDataKey = (void*)0x14725138; 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. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_browsertest.cc (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_browsertest.cc:691: // here. shell()->web_contents()->DetachFromSequence(); On 2017/03/16 17:46:31, fgorski wrote: > What exactly happens here? Comment does not help, especially when it contains > commented out code. Oops, leftover code, removed. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:206: web_contents->GetUserData(content::kMHTMLExtraDataKey); On 2017/03/16 02:28:21, Dmitry Titov wrote: > It is better to follow other examples that generally look like this: > > static Foo* Foo::FromWebContents(WebContents*) {} > > See, for example DownloadUIAdapter or other classes that define FromWebContents > static. this allows to keep kFooKey in foo.cc file... Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:206: web_contents->GetUserData(content::kMHTMLExtraDataKey); On 2017/03/16 17:46:32, fgorski wrote: > On 2017/03/16 02:28:21, Dmitry Titov wrote: > > It is better to follow other examples that generally look like this: > > > > static Foo* Foo::FromWebContents(WebContents*) {} > > > > See, for example DownloadUIAdapter or other classes that define > FromWebContents > > static. this allows to keep kFooKey in foo.cc file... > > +1. Details in my earlier comment. Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:442: save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; On 2017/03/16 02:28:21, Dmitry Titov wrote: > I think once there is a FILE_WRITING_ERROR, there should be no further writes to > the file... likely early return here? Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:473: std::string content_location_value; On 2017/03/16 02:28:20, Dmitry Titov wrote: > line 345 in this file is an example of how we generate CIDs in MHTML. This > results in something like > cid:frame-62691-645341c4-62b3-478e-a8c5-e0dfccc3ca02@mhtml.blink > > I think we can continue this with something like > cid:extra-data-[GUID]@mhtml.blink Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:482: if (file.WriteAtCurrentPos(extra_data_section.data(), On 2017/03/16 17:46:32, fgorski wrote: > return file.Write... >= 0; Done. https://codereview.chromium.org/2683493002/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:483: extra_data_section.size()) < 0) On 2017/03/16 17:46:32, fgorski wrote: > nit: {} if you don't apply above. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
This is moving in right direction. Please think through who, when triggers the creation of the UserData object. Also, the interface of it - how it can be done tight and minimal but at the same time self-documenting as possible. https://codereview.chromium.org/2683493002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:87: const std::vector<std::string>& GetLoadingSignalData() { do you need it as public? https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:39: extra empty line https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:442: save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; there was a comment about early return here and you replied "Done" but no change in the file. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.h (right): https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:63: // Contained class that represents an extra data section. 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. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:64: struct MHTMLExtraSection { 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. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:75: extra empty line https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:78: std::vector<MHTMLExtraSection>* sections() { return §ions_; } 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. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:86: static void AddToWebContents(content::WebContents* contents, Not sure why this method is needed.
Fixed unit tests (by checking for a nullptr), and addressed Dimich's comments. https://codereview.chromium.org/2683493002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:87: const std::vector<std::string>& GetLoadingSignalData() { On 2017/03/18 00:58:33, Dmitry Titov wrote: > do you need it as public? Done. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:39: On 2017/03/18 00:58:34, Dmitry Titov wrote: > extra empty line Done. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:442: save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; On 2017/03/18 00:58:34, Dmitry Titov wrote: > there was a comment about early return here and you replied "Done" but no change > in the file. Ah, there are now two cases, and only one of them got fixed. Both fixed now. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.h (right): https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:63: // Contained class that represents an extra data section. On 2017/03/18 00:58:34, Dmitry Titov wrote: > 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. Added comments, LMK if I need more. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:64: struct MHTMLExtraSection { On 2017/03/18 00:58:34, Dmitry Titov wrote: > 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. Done. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:75: On 2017/03/18 00:58:34, Dmitry Titov wrote: > extra empty line Done. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:78: std::vector<MHTMLExtraSection>* sections() { return §ions_; } On 2017/03/18 00:58:34, Dmitry Titov wrote: > 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. 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. https://codereview.chromium.org/2683493002/diff/120001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:86: static void AddToWebContents(content::WebContents* contents, On 2017/03/18 00:58:34, Dmitry Titov wrote: > Not sure why this method is needed. 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.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2683493002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:102: // Add this signal to signal_data_. Consider extracting a method accepting signal name. https://codereview.chromium.org/2683493002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:104: signal += std::to_string(base::Time::Now().ToJavaTime()); Why use java time? Please document that somehow. https://codereview.chromium.org/2683493002/diff/140001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/140001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:119: const std::vector<MHTMLExtraPart>* extra_parts); Why is this vector passed by pointer? https://codereview.chromium.org/2683493002/diff/140001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:504: MHTMLGenerationManager::MHTMLExtraData::FromWebContents( Why not return a pointer to extra data?
Forgot to build one of the targets originally, this fixes the build. https://codereview.chromium.org/2683493002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:87: const std::vector<std::string>& GetLoadingSignalData() { On 2017/03/20 18:26:53, Pete Williamson wrote: > On 2017/03/18 00:58:33, Dmitry Titov wrote: > > do you need it as public? > > Done. Oops, this did need to be public, the prerendering offliner uses it to get the data from the prerendering loader.
Addressed CR feedback per FGorski. I also added a #ifndef NDEBUG wrapper around the call that puts the data into MHTML files, so only debug builds will have the signal data. (Technically, that was in the previous change, but I forgot to mention it.) https://codereview.chromium.org/2683493002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:102: // Add this signal to signal_data_. On 2017/03/20 20:43:24, fgorski wrote: > Consider extracting a method accepting signal name. Added a new method for this. https://codereview.chromium.org/2683493002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:104: signal += std::to_string(base::Time::Now().ToJavaTime()); On 2017/03/20 20:43:24, fgorski wrote: > Why use java time? Please document that somehow. What we need is time in milliseconds. There are two methods that do that, ToJavaTime, and ToJavascriptTime. Added a comment in the new method. https://codereview.chromium.org/2683493002/diff/140001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/140001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:119: const std::vector<MHTMLExtraPart>* extra_parts); On 2017/03/20 20:43:24, fgorski wrote: > Why is this vector passed by pointer? When we get the user data from the web contents, that comes by pointer. It seems more parallel with the surrounding code to pass this by pointer too. Do you have a preference for a reference? Also, it passes cleanly as null if the data is not present (which is probably why the user data underneath used a pointer). https://codereview.chromium.org/2683493002/diff/140001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:504: MHTMLGenerationManager::MHTMLExtraData::FromWebContents( On 2017/03/20 20:43:24, fgorski wrote: > Why not return a pointer to extra data? What the client of this method needs is not an ExtraData, but the underlying vector, so why not make life easier for the caller, and encapsulate out the ExtraData here?
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think the design of the classes/lifetime here is still not nailed, we could discuss tomorrow (or whenever first opportunity) in the office, as it doesn't seem to fit in comments here. Some things to tighten: - it feels that design moves from exposing an add-on to WebContents' User Data to simply exposing static "Add this part to WebContents" but is not 100% there, and also "adding parts to web contents" sound strange (we don't really add anything to the web contents itself). - naming is not consistent (parts vs data, etc)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/21 00:06:02, Dmitry Titov wrote: > I think the design of the classes/lifetime here is still not nailed, we could > discuss tomorrow (or whenever first opportunity) in the office, as it doesn't > seem to fit in comments here. > > Some things to tighten: > - it feels that design moves from exposing an add-on to WebContents' User Data > to simply exposing static "Add this part to WebContents" but is not 100% there, > and also "adding parts to web contents" sound strange (we don't really add > anything to the web contents itself). > - naming is not consistent (parts vs data, etc) After we discussed offline, I refactored to move MHTMLExtraData and MHTMLExtraPart out of the interface of the MHTMLGenerationManager and into the .cc file. Let me know if you think we should do more.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683493002/diff/260001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2683493002/diff/260001/content/browser/BUILD.... content/browser/BUILD.gn:21: "//content/browser/download:*", This is a strange dependency, runs counter the comment above. The content stuff should be exposed from 'public' part of the tree.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683493002/diff/260001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2683493002/diff/260001/content/browser/BUILD.... content/browser/BUILD.gn:21: "//content/browser/download:*", On 2017/03/23 17:34:10, Dmitry Titov wrote: > This is a strange dependency, runs counter the comment above. > The content stuff should be exposed from 'public' part of the tree. This is leftover cruft from an earlier try, removed. I also checked for other leftover code, but didn't find any. Here I use CONTENT_EXPORT for the MHTMLGenerationManager class to make the StashMHTMLPartForAdditionToPage() method available as a public symbol.
I think it's more like a FYI for me so no LGTM from me. However the patch looks good :)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Dimich and I discussed in person the best way to expose the interface in content/public, and this change reflects that discussion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: drop (c) from the whole patch. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:38: void StashMHTMLPartForAdditionToPage(const std::string& content_type, consider renaming to: AddExtraMHTMLPart() here and in the interface. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:116: // while on the file thread. Returns true for success. true for success or no data to write. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:443: return std::make_tuple(save_status, file_size); -1 would work better here. (Or some constant). This was hard to understand. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:470: const MHTMLExtraDataImpl* extra_data) { DCHECK thread. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:18: #include "base/supports_user_data.h" needed? https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:20: #include "content/public/browser/web_contents.h" needed? https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... File content/public/browser/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:15: // then call StashMHTMLPartForAdditionToPage to add a new part. please rename https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:21: // Construct and save an extra MHTML part which will be added to the MHTML I believe this had better documentation in implementation of MHTMLExtraData. https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:23: virtual void StashMHTMLPartForAdditionToPage( please rename
https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: drop (c) from the whole patch. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:38: void StashMHTMLPartForAdditionToPage(const std::string& content_type, consider renaming to: AddExtraMHTMLPart() here and in the interface. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:116: // while on the file thread. Returns true for success. true for success or no data to write. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:443: return std::make_tuple(save_status, file_size); -1 would work better here. (Or some constant). This was hard to understand. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:470: const MHTMLExtraDataImpl* extra_data) { DCHECK thread. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:18: #include "base/supports_user_data.h" needed? https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:20: #include "content/public/browser/web_contents.h" needed? https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... File content/public/browser/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:15: // then call StashMHTMLPartForAdditionToPage to add a new part. please rename https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:21: // Construct and save an extra MHTML part which will be added to the MHTML I believe this had better documentation in implementation of MHTMLExtraData. https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:23: virtual void StashMHTMLPartForAdditionToPage( please rename
https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/28 17:01:12, fgorski wrote: > nit: drop (c) from the whole patch. Done. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:38: void StashMHTMLPartForAdditionToPage(const std::string& content_type, On 2017/03/28 17:01:12, fgorski wrote: > consider renaming to: AddExtraMHTMLPart() here and in the interface. Done. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:116: // while on the file thread. Returns true for success. On 2017/03/28 17:01:12, fgorski wrote: > true for success or no data to write. Done. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:443: return std::make_tuple(save_status, file_size); On 2017/03/28 17:01:12, fgorski wrote: > -1 would work better here. (Or some constant). This was hard to understand. Done. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:470: const MHTMLExtraDataImpl* extra_data) { On 2017/03/28 17:01:12, fgorski wrote: > DCHECK thread. Done. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:18: #include "base/supports_user_data.h" On 2017/03/28 17:01:12, fgorski wrote: > needed? Done. https://codereview.chromium.org/2683493002/diff/320001/content/browser/downlo... content/browser/download/mhtml_generation_manager.h:20: #include "content/public/browser/web_contents.h" On 2017/03/28 17:01:12, fgorski wrote: > needed? Done. https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... File content/public/browser/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:15: // then call StashMHTMLPartForAdditionToPage to add a new part. On 2017/03/28 17:01:12, fgorski wrote: > please rename Done. https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:21: // Construct and save an extra MHTML part which will be added to the MHTML On 2017/03/28 17:01:12, fgorski wrote: > I believe this had better documentation in implementation of MHTMLExtraData. Moved comments from the interface to here. https://codereview.chromium.org/2683493002/diff/320001/content/public/browser... content/public/browser/mhtml_extra_data.h:23: virtual void StashMHTMLPartForAdditionToPage( On 2017/03/28 17:01:12, fgorski wrote: > please rename Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for changes! BackgroundLoader does not call the AddExtraMHTMLPart method... I also recommend to ping a content OWNER at this point. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:96: signal += std::to_string(base::Time::Now().ToJavaTime()); this is not using ToJavaTime() for what it is intended. Not sure why we need a wall clock timestamps BTW. Isn't it a delay from the navigation that you want? If so, it'd be more straightforward to snapshot a TickTime at the start and then compute TimeDeltas and use its ToMilliseconds method. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:91: #ifndef NDEBUG why this specific part is debug-only? https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:122: // TODO(petewil): Convert this to JSON, use json_writer.h either do this in this patch, or it should remain a TODO and content-type should be text/plain https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:125: signal_string += "\r\n"; does it have to be \r\n in the body? https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.cc (right): https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.cc:8: const int kMHTMLExtraDataKey = 0; no need to initialize this value. But adding a short comment that only an address of this variable is used would be nice. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.h (right): https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:22: // one MHTML part. Arbitrary extra MHTML parts to be added into the complete incomplete sentence... https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:30: // Return the vector of parts to be disassembled. disassembled? https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:34: void AddExtraMHTMLPart(const std::string& content_type, This shoudl have a comment saying it's implementation of MHTMLExtraData. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:428: MHTMLGenerationManager::Job::CloseFileOnFileThread( It's adding a side effect to the "CloseFile" function.. At least it should be renamed to something like FinalizeAndCloseFileOnFileThread. It could have a check for thread and then call other methods, WriteFooter, WriteExtraDataPart and CloseFile (not necessarily having the "OnFileThread" suffix but checking for the thread)... https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:491: "--%s--\r\n%s%s\r\n%s%s\r\n%s\r\n", boundary.c_str(), kContentLocation, the boundary, if it is not the last one in the file, should not have trailing "--". https://tools.ietf.org/html/rfc2557 https://codereview.chromium.org/2683493002/diff/340001/content/public/browser... File content/public/browser/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/340001/content/public/browser... content/public/browser/mhtml_extra_data.h:16: class CONTENT_EXPORT MHTMLExtraData : public base::SupportsUserData::Data { Naming sugegstion: ExtraData -> ExtraParts? It'd be more specific.
https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:96: signal += std::to_string(base::Time::Now().ToJavaTime()); On 2017/03/29 18:53:06, Dmitry Titov wrote: > this is not using ToJavaTime() for what it is intended. > Not sure why we need a wall clock timestamps BTW. Isn't it a delay from the > navigation that you want? If so, it'd be more straightforward to snapshot a > TickTime at the start and then compute TimeDeltas and use its ToMilliseconds > method. Reading the descriptions at the top of time.h, it says that TimeTicks may stand still if the computer is suspended. I'm not sure what "suspended" means here, and was concerned that it might consider running in background as "suspended". I've recoded using TimeTicks assuming that suspended means the CPU is not running. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:91: #ifndef NDEBUG On 2017/03/29 18:53:06, Dmitry Titov wrote: > why this specific part is debug-only? My intent here is that we only write the signals into the MHTML file in debug mode, but that we collect the signals in all modes since we eventually want to use them for decision making. Since they are not being used for decision making now, if you prefer, I can make the other signal gathering code debug only, but since it is a small amount of code sprinkled throughout, I though it better for readability to leave the code in for both debug and non-debug. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:122: // TODO(petewil): Convert this to JSON, use json_writer.h On 2017/03/29 18:53:06, Dmitry Titov wrote: > either do this in this patch, or it should remain a TODO and content-type should > be text/plain Changed to text/plain for now. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:125: signal_string += "\r\n"; On 2017/03/29 18:53:06, Dmitry Titov wrote: > does it have to be \r\n in the body? No, but it does make it easier to read and parse until we have JSON. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.cc (right): https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.cc:8: const int kMHTMLExtraDataKey = 0; On 2017/03/29 18:53:06, Dmitry Titov wrote: > no need to initialize this value. But adding a short comment that only an > address of this variable is used would be nice. Comment added. The compiler really wanted me to initialize it though, so I left the '= 0' in. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... File content/browser/download/mhtml_extra_data_impl.h (right): https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:22: // one MHTML part. Arbitrary extra MHTML parts to be added into the complete On 2017/03/29 18:53:06, Dmitry Titov wrote: > incomplete sentence... Done. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:30: // Return the vector of parts to be disassembled. On 2017/03/29 18:53:06, Dmitry Titov wrote: > disassembled? Oops, serialized. Fixed. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_extra_data_impl.h:34: void AddExtraMHTMLPart(const std::string& content_type, On 2017/03/29 18:53:06, Dmitry Titov wrote: > This shoudl have a comment saying it's implementation of MHTMLExtraData. I don't think I understood your suggestion, but I did add a comment, please check to see if that is what you wanted. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:428: MHTMLGenerationManager::Job::CloseFileOnFileThread( On 2017/03/29 18:53:06, Dmitry Titov wrote: > It's adding a side effect to the "CloseFile" function.. > At least it should be renamed to something like > FinalizeAndCloseFileOnFileThread. It could have a check for thread and then call > other methods, WriteFooter, WriteExtraDataPart and CloseFile (not necessarily > having the "OnFileThread" suffix but checking for the thread)... Done. https://codereview.chromium.org/2683493002/diff/340001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:491: "--%s--\r\n%s%s\r\n%s%s\r\n%s\r\n", boundary.c_str(), kContentLocation, On 2017/03/29 18:53:06, Dmitry Titov wrote: > the boundary, if it is not the last one in the file, should not have trailing > "--". https://tools.ietf.org/html/rfc2557 Done. https://codereview.chromium.org/2683493002/diff/340001/content/public/browser... File content/public/browser/mhtml_extra_data.h (right): https://codereview.chromium.org/2683493002/diff/340001/content/public/browser... content/public/browser/mhtml_extra_data.h:16: class CONTENT_EXPORT MHTMLExtraData : public base::SupportsUserData::Data { On 2017/03/29 18:53:06, Dmitry Titov wrote: > Naming sugegstion: ExtraData -> ExtraParts? It'd be more specific. Done.
petewil@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor for files under content. DTrainor, the intent of this change is to add some signal data to the bottom of a MHTML file when generating an offline file for DEBUG only. The signal data will be analyzed to find a better time to take snapshots, and will eventually be used to decide at runtime when to take snapshots. Design docs here: https://docs.google.com/document/d/1AcoZOIb81V68_KDSOCatFSYYMLOTSYj6resg-VAxLgo
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please add bug number to the CL. I don't think surrounding the section with #ifdef NDEBUG is the right thing to do. What is our approach on loading data collection here? I can see: - enable for all - have chorme://flag - enable for non-official channels, canary/dev Where and how exactly will we collect this data? https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:91: #ifndef NDEBUG So what is the plan? Do we build debug to run in harness to collect data? Do we have a chrome://flag to enable data collection in mhtml files? I doubt debug builds of Clank are awesome, especially on low-end phones. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:125: signal_string += "\r\n"; On 2017/03/29 22:05:42, Pete Williamson wrote: > On 2017/03/29 18:53:06, Dmitry Titov wrote: > > does it have to be \r\n in the body? > > No, but it does make it easier to read and parse until we have JSON. What I meant is why both, the regular "\n" would suffice. Seeing this special combo can make others think it's essential to do both. https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:92: void PrerenderingLoader::SetSnapshotStartTime() { looks like nobody calls it. https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:115: void SetSnapshotStartTime(); Does it sets a time of snapshot start? It should be called at the start of loading, likely in LoadPage(). Perhaps shoudl be called MarkLoadStartTime? https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.cc:15: MHTMLExtraPartsImpl::~MHTMLExtraPartsImpl() {} need a blank line between ctor and dtor https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:31: std::vector<MHTMLExtraDataPart>& parts() { return parts_; } It's weird to have both methods like this. There should be one. Do you need to modify the vector beyond AddExtraHTMLPart?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Get signals working in the EXTRA_DATA section of MHTML BUG= ========== to ========== Get signals working in the EXTRA_DATA section of MHTML BUG=706628 ==========
I liked the description you put in the comment where you added me to the patch. Can you update/elaborate the main CL description with more info like that? Thanks! content/ lgtm % nits. https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.cc:14: MHTMLExtraPartsImpl::MHTMLExtraPartsImpl() {} = default for both? https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_generation_browsertest.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_generation_browsertest.cc:688: std::string signal_data1(kFakeSignalData1); Do you need these? https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:508: if (file.WriteAtCurrentPos(footer.data(), footer.size()) < 0) Keep this consistent with the return style above? Either is fine though.
Description was changed from ========== Get signals working in the EXTRA_DATA section of MHTML BUG=706628 ========== to ========== Get signals working in the EXTRA_DATA section of MHTML The intent of this change is to add some signal data to the bottom of a MHTML file when generating an offline file if the new OfflinePagesLoadSignalCollecting flag is set. The signal data will be analyzed to find a better time to take snapshots, and will eventually be used to decide at runtime when to take snapshots. Design docs here: https://docs.google.com/document/d/1AcoZOIb81V68_KDSOCatFSYYMLOTSYj6resg-VAxLgo BUG=706628 ==========
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:91: #ifndef NDEBUG On 2017/03/29 23:26:29, Dmitry Titov wrote: > So what is the plan? Do we build debug to run in harness to collect data? Do we > have a chrome://flag to enable data collection in mhtml files? I doubt debug > builds of Clank are awesome, especially on low-end phones. After offline discussion, we decided on a new command line flag. https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:125: signal_string += "\r\n"; On 2017/03/29 23:26:29, Dmitry Titov wrote: > On 2017/03/29 22:05:42, Pete Williamson wrote: > > On 2017/03/29 18:53:06, Dmitry Titov wrote: > > > does it have to be \r\n in the body? > > > > No, but it does make it easier to read and parse until we have JSON. > > What I meant is why both, the regular "\n" would suffice. Seeing this special > combo can make others think it's essential to do both. Ah, that's different. I was copying what I saw elsewhere in generating the file. The same newline combination is used when we generate the footer and the headers and the body (see MHTMLArchive.cpp, line 165) https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:92: void PrerenderingLoader::SetSnapshotStartTime() { On 2017/03/29 23:26:29, Dmitry Titov wrote: > looks like nobody calls it. Oops, fixed. LoadPage now calls this instead of adding a start time signal. https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/2683493002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:115: void SetSnapshotStartTime(); On 2017/03/29 23:26:29, Dmitry Titov wrote: > Does it sets a time of snapshot start? It should be called at the start of > loading, likely in LoadPage(). Perhaps shoudl be called MarkLoadStartTime? Done. https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.cc:14: MHTMLExtraPartsImpl::MHTMLExtraPartsImpl() {} On 2017/03/30 05:26:40, David Trainor-ping if over 24h wrote: > = default for both? I tried that, but I got "Complex constructor has an inlined body." https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.cc:15: MHTMLExtraPartsImpl::~MHTMLExtraPartsImpl() {} On 2017/03/29 23:26:29, Dmitry Titov wrote: > need a blank line between ctor and dtor Done. https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:31: std::vector<MHTMLExtraDataPart>& parts() { return parts_; } On 2017/03/29 23:26:29, Dmitry Titov wrote: > It's weird to have both methods like this. There should be one. Do you need to > modify the vector beyond AddExtraHTMLPart? 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). https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_generation_browsertest.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_generation_browsertest.cc:688: std::string signal_data1(kFakeSignalData1); On 2017/03/30 05:26:40, David Trainor-ping if over 24h wrote: > Do you need these? Removed https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:508: if (file.WriteAtCurrentPos(footer.data(), footer.size()) < 0) On 2017/03/30 05:26:40, David Trainor-ping if over 24h wrote: > Keep this consistent with the return style above? Either is fine though. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I wish we had design doc on this. Significant part of discussions in this CL could be discussed in a short design doc. It would be faster too. Still some comments: https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:125: signal_string += "\r\n"; On 2017/03/31 00:29:48, Pete Williamson wrote: > On 2017/03/29 23:26:29, Dmitry Titov wrote: > > On 2017/03/29 22:05:42, Pete Williamson wrote: > > > On 2017/03/29 18:53:06, Dmitry Titov wrote: > > > > does it have to be \r\n in the body? > > > > > > No, but it does make it easier to read and parse until we have JSON. > > > > What I meant is why both, the regular "\n" would suffice. Seeing this special > > combo can make others think it's essential to do both. > > Ah, that's different. I was copying what I saw elsewhere in generating the > file. The same newline combination is used when we generate the footer and the > headers and the body (see MHTMLArchive.cpp, line 165) The other places are in header/boundary parts, it's regulated by MHTML spec. The content of the body is regulated by Content-Type. For images, we just dump binary data w/o any encoding. For test/plain, it's just text. it can contain whatever CRLF sequences but I think it's a bit clearer to avoid CRLF in any text bodies that we generate to avoid perception it's done for some specific purpose. LF is usually enough. https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:31: std::vector<MHTMLExtraDataPart>& parts() { return parts_; } On 2017/03/31 00:29:49, Pete Williamson wrote: > On 2017/03/29 23:26:29, Dmitry Titov wrote: > > It's weird to have both methods like this. There should be one. Do you need to > > modify the vector beyond AddExtraHTMLPart? > > 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). What test is using it? I don't see. https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... chrome/browser/flag_descriptions.cc:2034: "Enables collecting load timing data for offline page snapshots."; Same note as in .h file. I think we are not really after 'timing' signals because times in themselves say little to us. For example, percentage of loaded resources says more... Trying to avoid confusion for others reading the code. "telemetry" or "loading completeness" are probably better than "timing". https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... chrome/browser/flag_descriptions.h:2219: // Name for the flag to enable collecting load timing signals to improve Unclear sentence... The signals will not only be time-based... Maybe "the flag to enable collecting loading telemetry in MHTML snapshots"? same below.
On 2017/03/31 18:05:53, Dmitry Titov wrote: > For test/plain, it's just text. it can contain > whatever CRLF sequences but I think it's a bit clearer to avoid CRLF in any text > bodies that we generate to avoid perception it's done for some specific purpose. > LF is usually enough. I'd like to maintain compatibility with the spec, so probably using a Content-Transfer-Encoding: binary header would be appropriate if you don't choose to use CRLF.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixes as suggested by Dimich and DewittJ https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:125: signal_string += "\r\n"; On 2017/03/31 18:05:53, Dmitry Titov wrote: > On 2017/03/31 00:29:48, Pete Williamson wrote: > > On 2017/03/29 23:26:29, Dmitry Titov wrote: > > > On 2017/03/29 22:05:42, Pete Williamson wrote: > > > > On 2017/03/29 18:53:06, Dmitry Titov wrote: > > > > > does it have to be \r\n in the body? > > > > > > > > No, but it does make it easier to read and parse until we have JSON. > > > > > > What I meant is why both, the regular "\n" would suffice. Seeing this > special > > > combo can make others think it's essential to do both. > > > > Ah, that's different. I was copying what I saw elsewhere in generating the > > file. The same newline combination is used when we generate the footer and > the > > headers and the body (see MHTMLArchive.cpp, line 165) > > The other places are in header/boundary parts, it's regulated by MHTML spec. The > content of the body is regulated by Content-Type. For images, we just dump > binary data w/o any encoding. For test/plain, it's just text. it can contain > whatever CRLF sequences but I think it's a bit clearer to avoid CRLF in any text > bodies that we generate to avoid perception it's done for some specific purpose. > LF is usually enough. OK, done, and added the Content-Transfer-Encoding as suggested by DewittJ https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.cc (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.cc:14: MHTMLExtraPartsImpl::MHTMLExtraPartsImpl() {} On 2017/03/31 00:29:48, Pete Williamson wrote: > On 2017/03/30 05:26:40, David Trainor-ping if over 24h wrote: > > = default for both? > > I tried that, but I got "Complex constructor has an inlined body." Ah, I can use = default in the .cc file, changed to do that (my earlier attempt was to use it in the .h file). https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/360001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:31: std::vector<MHTMLExtraDataPart>& parts() { return parts_; } On 2017/03/31 18:05:53, Dmitry Titov wrote: > On 2017/03/31 00:29:49, Pete Williamson wrote: > > On 2017/03/29 23:26:29, Dmitry Titov wrote: > > > It's weird to have both methods like this. There should be one. Do you need > to > > > modify the vector beyond AddExtraHTMLPart? > > > > 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). > > What test is using it? I don't see. Ah, sorry, it looks like that test got refactored out of existence. Removing the non-const accessor. https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... chrome/browser/flag_descriptions.cc:2034: "Enables collecting load timing data for offline page snapshots."; On 2017/03/31 18:05:53, Dmitry Titov wrote: > Same note as in .h file. > > I think we are not really after 'timing' signals because times in themselves say > little to us. For example, percentage of loaded resources says more... Trying to > avoid confusion for others reading the code. "telemetry" or "loading > completeness" are probably better than "timing". Changed to "Enables load completeness data colllection " ... https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2683493002/diff/380001/chrome/browser/flag_de... chrome/browser/flag_descriptions.h:2219: // Name for the flag to enable collecting load timing signals to improve On 2017/03/31 18:05:53, Dmitry Titov wrote: > Unclear sentence... > > The signals will not only be time-based... Maybe "the flag to enable collecting > loading telemetry in MHTML snapshots"? > > same below. Changed "load timing" to "load completeness"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683493002/diff/420001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/420001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:501: part.content_type.c_str(), kContentTransferEncodingBinary, Thinking about this just a bit more, I think I'll move the ContentTransferEncoding header into the string supplied by the client instead of assuming it makes sense to set it to binary for all parts, new patch coming Monday. I'll also add a x-google-offline-pages-signal-collector header into the string supplied by the prerendering offliner to make our parsing easier.
This version of the patch fixes my own comment that we should put the ContentTransferEncoding header in with the code that generates the part, on the theory that other extra parts might want to use different encodings. This patch also adds a "x-google-..." style header to mark the body content as loading signal data. Any feedback on the name I've picked for the header is welcome.
petewil@chromium.org changed reviewers: + jochen@chromium.org
+jochen: Please provide an OWNERS review for the following files: content/browser/BUILD.gn content/public/browser/BUILD.gn content/public/browser/mhtml_extra_parts.h tools/metrics/histograms/histograms.xml MHTMLExtraParts is a new API that allows code to add an arbitrary extra MHTML part to a MHTML file that is generated by offline pages. The change to histograms.xml is for adding a flag to control the data collection feature of this changelist. Current plans are to use it in the lab for data generation only, but we added the flag to allow data to be generated from remote sites like in emerging markets.
lgtm with nits: https://codereview.chromium.org/2683493002/diff/440001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/440001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:27: const char kXHeaderForSignals[] = "Chromium-Loading-Metrics-Data: 1"; that header doesn't look like x-google-... I htink it should literally start with this. https://codereview.chromium.org/2683493002/diff/440001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:99: kXHeaderForSignals); For the further CL: consider refactoring by adding 'extra_headers' parameter so they can be passed w/o concatenating with the body... Not necessarily for this patch.
content stuff looks good, however, I can't approve general histograms.xml changes (see the comment in the owners file). Please ask a member of the metrics team to review
petewil@chromium.org changed reviewers: + holte@chromium.org
+holte for histograms.xml - The histogram change is due to adding a new flag to turn on our feature. +ryansturm for metrics (Jochen suggested I loop in the metrics team). This is for gathering test data in the lab about loading metrics while offlining a page. We are putting the metrics into the MHTML file that we save so that it stays with the data. https://codereview.chromium.org/2683493002/diff/440001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/440001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:27: const char kXHeaderForSignals[] = "Chromium-Loading-Metrics-Data: 1"; On 2017/04/05 00:18:06, Dmitry Titov wrote: > that header doesn't look like x-google-... I htink it should literally start > with this. After discussion offline, we decided to change this to "X-Chrome-...". There is guidance on the ietf website to stop using X- headers, but that seems to be for only things that are likely to become standards someday, and this will be chrome specific only. https://codereview.chromium.org/2683493002/diff/440001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:99: kXHeaderForSignals); On 2017/04/05 00:18:06, Dmitry Titov wrote: > For the further CL: consider refactoring by adding 'extra_headers' parameter so > they can be passed w/o concatenating with the body... Not necessarily for this > patch. Acknowledged.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mostly looks good. A few comments to address thou. https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:102: std::string content_location = "cid:signal-data-" + is there a good reason not to base::StringPrintf this one too? https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:108: DCHECK(extra_parts); this is worrisome: * DCHECK in debug is great. * But that is also going to end up being a crash in production if you don't skip another line... Do we want that? https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:19: // Class used as a data object for WebContents UserData to represent a MHTML nit: an MHTML https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:34: int64_t size() override; Could you make this one const as well? Here and in the interface. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:451: return std::make_tuple(save_status, kInvalidFileSize); I believe this return is wrong: This whole method uses save_status, instead of doing direct returns, so that we can attempt to close the file at the end. If you return from here, you will not give it the opportunity. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:457: return std::make_tuple(save_status, kInvalidFileSize); same here https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:464: MhtmlSaveStatus close_save_status = CloseFileIfValid(file, file_size); Because there are only 2 outcomes of CloseFileIfValid, you could return boolean and do this: if (!CloseFileIfValid(file, file_size)) save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:519: MhtmlSaveStatus MHTMLGenerationManager::Job::CloseFileIfValid( This should be a function in anonymous namespace (no need to expose it). https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:521: int64_t& file_size) { This should be a pointer (and because of above it would be ok to simply DCHECK it). Based on: https://google.github.io/styleguide/cppguide.html#Reference_Arguments Specifically: """ Within function parameter lists all references must be const: void Foo(const string &in, string *out); In fact it is a very strong convention in Google code that input arguments are values or const references while output arguments are pointers. Input parameters may be const pointers, but we never allow non-const reference parameters except when required by convention, e.g., swap(). """ And I couldn't find a contradiction in Chromium style guide. Alternative design for this function is to return the file_size or kInvalidFileSize... But the calling code will be nicer with out param and boolean. https://codereview.chromium.org/2683493002/diff/460001/content/public/browser... File content/public/browser/mhtml_extra_parts.h (right): https://codereview.chromium.org/2683493002/diff/460001/content/public/browser... content/public/browser/mhtml_extra_parts.h:30: virtual int64_t size() = 0; make this method const, please, per earlier comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
petewil@chromium.org changed reviewers: + ryansturm@chromium.org
+ryansturm for metrics (Jochen suggested I loop in the metrics team). This is for gathering test data in the lab about loading metrics while offlining a page. We are putting the metrics into the MHTML file that we save so that it stays with the data. https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:102: std::string content_location = "cid:signal-data-" + On 2017/04/05 18:59:12, fgorski wrote: > is there a good reason not to base::StringPrintf this one too? It was a style choice, in my opinion, it is more readable as is. Changed to use base::StringPrintf(). https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:108: DCHECK(extra_parts); On 2017/04/05 18:59:12, fgorski wrote: > this is worrisome: > * DCHECK in debug is great. > * But that is also going to end up being a crash in production if you don't skip > another line... > Do we want that? It should be safe since FromWebContents creates a new MHTMLExtraParts if there is not one already, but to be extra safe, I added an if check. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:19: // Class used as a data object for WebContents UserData to represent a MHTML On 2017/04/05 18:59:12, fgorski wrote: > nit: an MHTML Done. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:34: int64_t size() override; On 2017/04/05 18:59:12, fgorski wrote: > Could you make this one const as well? Here and in the interface. Changing this to const produces a build error: error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers] I think it is basically saying there is no point in having a const int as a return value, since the value is getting copied anyway. It could be potentially made into a const reference, is that your suggestion? Left as is. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:451: return std::make_tuple(save_status, kInvalidFileSize); On 2017/04/05 18:59:13, fgorski wrote: > I believe this return is wrong: This whole method uses save_status, instead of > doing direct returns, so that we can attempt to close the file at the end. If > you return from here, you will not give it the opportunity. Done. Thanks for catching this! https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:457: return std::make_tuple(save_status, kInvalidFileSize); On 2017/04/05 18:59:12, fgorski wrote: > same here Done. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:464: MhtmlSaveStatus close_save_status = CloseFileIfValid(file, file_size); On 2017/04/05 18:59:13, fgorski wrote: > Because there are only 2 outcomes of CloseFileIfValid, you could return boolean > and do this: > > if (!CloseFileIfValid(file, file_size)) > save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; Done, but it ended up a bit more complex than that to avoid overwriting the file writing error which happened first. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:519: MhtmlSaveStatus MHTMLGenerationManager::Job::CloseFileIfValid( On 2017/04/05 18:59:13, fgorski wrote: > This should be a function in anonymous namespace (no need to expose it). It is already part of the MHTMLGenerationManager::Job object which is scoped to this file, so it is already not exposed. It seems to fit better as a member of the class, so left as is. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:521: int64_t& file_size) { On 2017/04/05 18:59:13, fgorski wrote: > This should be a pointer (and because of above it would be ok to simply DCHECK > it). > Based on: https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > Specifically: > """ > Within function parameter lists all references must be const: > > void Foo(const string &in, string *out); > > In fact it is a very strong convention in Google code that input arguments are > values or const references while output arguments are pointers. Input parameters > may be const pointers, but we never allow non-const reference parameters except > when required by convention, e.g., swap(). > """ > > And I couldn't find a contradiction in Chromium style guide. > > Alternative design for this function is to return the file_size or > kInvalidFileSize... But the calling code will be nicer with out param and > boolean. Ah, I had forgotten that. Fixed. https://codereview.chromium.org/2683493002/diff/460001/content/public/browser... File content/public/browser/mhtml_extra_parts.h (right): https://codereview.chromium.org/2683493002/diff/460001/content/public/browser... content/public/browser/mhtml_extra_parts.h:30: virtual int64_t size() = 0; On 2017/04/05 18:59:13, fgorski wrote: > make this method const, please, per earlier comment. Left as is, same reason as earlier comment.
lgtm with extra comment. https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/460001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:108: DCHECK(extra_parts); On 2017/04/05 22:11:30, Pete Williamson wrote: > On 2017/04/05 18:59:12, fgorski wrote: > > this is worrisome: > > * DCHECK in debug is great. > > * But that is also going to end up being a crash in production if you don't > skip > > another line... > > Do we want that? > > It should be safe since FromWebContents creates a new MHTMLExtraParts if there > is not one already, but to be extra safe, I added an if check. If you are sure this is never null CHECK(extra_parts); is probably better, but it is up to you. Resulting crash, if any will be easier to pin-point that way. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... File content/browser/download/mhtml_extra_parts_impl.h (right): https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_extra_parts_impl.h:34: int64_t size() override; On 2017/04/05 22:11:30, Pete Williamson wrote: > On 2017/04/05 18:59:12, fgorski wrote: > > Could you make this one const as well? Here and in the interface. > > Changing this to const produces a build error: > error: 'const' type qualifier on return type has no effect > [-Werror,-Wignored-qualifiers] > > I think it is basically saying there is no point in having a const int as a > return value, since the value is getting copied anyway. It could be potentially > made into a const reference, is that your suggestion? > > Left as is. Acknowledged. Good point. I blanked on return type. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:464: MhtmlSaveStatus close_save_status = CloseFileIfValid(file, file_size); On 2017/04/05 22:11:30, Pete Williamson wrote: > On 2017/04/05 18:59:13, fgorski wrote: > > Because there are only 2 outcomes of CloseFileIfValid, you could return > boolean > > and do this: > > > > if (!CloseFileIfValid(file, file_size)) > > save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR; > > Done, but it ended up a bit more complex than that to avoid overwriting the file > writing error which happened first. Still looks cleaner. I like it. https://codereview.chromium.org/2683493002/diff/460001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:519: MhtmlSaveStatus MHTMLGenerationManager::Job::CloseFileIfValid( On 2017/04/05 22:11:30, Pete Williamson wrote: > On 2017/04/05 18:59:13, fgorski wrote: > > This should be a function in anonymous namespace (no need to expose it). > > It is already part of the MHTMLGenerationManager::Job object which is scoped to > this file, so it is already not exposed. It seems to fit better as a member of > the class, so left as is. Acknowledged. https://codereview.chromium.org/2683493002/diff/480001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2683493002/diff/480001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:449: if (!WriteExtraDataParts(boundary, file, extra_parts)) { minor thing which I didn't realize until now that I see it. This could pretty much be a single if statement with both calls: if (!WriteExtraDataParts(boundary, file, extra_parts) || !WriteFooter(boundary, file)) { save_status = ...; } Comments can be put on top, but the names of methods are good in this case, and comments don't really add stuff here. Consider applying if you are comfortable with such change.
https://codereview.chromium.org/2683493002/diff/480001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/480001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:99: kXHeaderForSignals); will this header actually hit the network, or is that just locally stored?
https://codereview.chromium.org/2683493002/diff/480001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2683493002/diff/480001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:99: kXHeaderForSignals); On 2017/04/06 13:02:03, jochen wrote: > will this header actually hit the network, or is that just locally stored? This header is just locally stored. When parsing, we want to be able to find our load progress signal data in the MHTML file, and this is how we plan to identify it. Since the MHTML is for an offline version of a page, it doesn't go back out onto the network, but just stays on the user's phone until they delete it.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
histograms lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
petewil@chromium.org changed reviewers: + bmcquade@chromium.org - ryansturm@chromium.org
+bcmcquace for looking at from a PageLoadMetrics point of view. This is for offline pages. To find the right time to take a snapshot, we are collecting page loading signal data. Today we only collect a few, but we hope to add FirstMeaningfulPaint, TimeToInteractive, and more. To store the signals for processing, we are adding them to the offline page's MHTML file. Jochen@ suggests we have somebody from the PLM team take a look, and I'd like to ask you to look. The individual files should already be LGTMed by folks, so I'm mostly looking to see if this is a good approach. Design Doc here: https://docs.google.com/document/d/1qx-UwSWqfybRIzkndD9MUFE7MK_MaafagsOTNqIgXwo
On 2017/04/07 at 18:10:52, petewil wrote: > +bcmcquace for looking at from a PageLoadMetrics point of view. This is for offline pages. To find the right time to take a snapshot, we are collecting page loading signal data. Today we only collect a few, but we hope to add FirstMeaningfulPaint, TimeToInteractive, and more. To store the signals for processing, we are adding them to the offline page's MHTML file. Jochen@ suggests we have somebody from the PLM team take a look, and I'd like to ask you to look. The individual files should already be LGTMed by folks, so I'm mostly looking to see if this is a good approach. > > Design Doc here: https://docs.google.com/document/d/1qx-UwSWqfybRIzkndD9MUFE7MK_MaafagsOTNqIgXwo Hi Pete, Thanks for looping me in. I think this is fine for now and as you note when you are looking to add more signals, such as first contentful paint, etc, I'd advise integrating into page_load_metrics by implementing a PageLoadMetricsObserver. You could use android_page_load_metrics_observer as a model - that class collects metrics and forwards them to the chrome custom tabs infra for use there. Your goals seem similar - you want to collect and forward metrics. When you do add support for this, I'd suggest replacing any existing metrics you are logging with page load metrics equivalents. This allows us to be confident that we're logging the same thing in various components so we can compare e.g. domcontentloaded in your infra with domcontentloaded across all page loads, and know we're talking about the exact same metrics. So, for now, this LGTM, but when you're ready to add more metrics such as FCP, you can model a change after the android page load metrics observer class, and feel free to add me as reviewer. Thanks! -Bryan
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from romax@chromium.org, dtrainor@chromium.org, dimich@chromium.org, fgorski@chromium.org, jochen@chromium.org, holte@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/2683493002/#ps520001 (title: "Merge changelist with tip of tree")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 520001, "attempt_start_ts": 1491856315700740,
"parent_rev": "4baea33bdc3f22c101176a43e694cbc1cbb56650", "commit_rev":
"2abcf0413069159c73bceba72f0b6f124021bca5"}
Message was sent while issue was closed.
Description was changed from ========== Get signals working in the EXTRA_DATA section of MHTML The intent of this change is to add some signal data to the bottom of a MHTML file when generating an offline file if the new OfflinePagesLoadSignalCollecting flag is set. The signal data will be analyzed to find a better time to take snapshots, and will eventually be used to decide at runtime when to take snapshots. Design docs here: https://docs.google.com/document/d/1AcoZOIb81V68_KDSOCatFSYYMLOTSYj6resg-VAxLgo BUG=706628 ========== to ========== Get signals working in the EXTRA_DATA section of MHTML The intent of this change is to add some signal data to the bottom of a MHTML file when generating an offline file if the new OfflinePagesLoadSignalCollecting flag is set. The signal data will be analyzed to find a better time to take snapshots, and will eventually be used to decide at runtime when to take snapshots. Design docs here: https://docs.google.com/document/d/1AcoZOIb81V68_KDSOCatFSYYMLOTSYj6resg-VAxLgo BUG=706628 Review-Url: https://codereview.chromium.org/2683493002 Cr-Commit-Position: refs/heads/master@{#463407} Committed: https://chromium.googlesource.com/chromium/src/+/2abcf0413069159c73bceba72f0b... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001) as https://chromium.googlesource.com/chromium/src/+/2abcf0413069159c73bceba72f0b... |
