Get signals working in the EXTRA_DATA section of MHTML

chrome/browser/android/offline_pages/prerendering_offliner.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/prerendering_offliner.h"
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/offline_pages/core/background/offliner_policy.h"
 #include "components/offline_pages/core/background/save_page_request.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/downloads/download_ui_adapter.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 
 namespace offline_pages {
 namespace {
 const char kDownloadUIAdapterKey[] = "download-ui-adapter";
+const char kTextSignal[] = "Content-Type: text/signal-data";
+const char kNewline[] = "\n";
 }
 
 PrerenderingOffliner::PrerenderingOffliner(
     content::BrowserContext* browser_context,
     const OfflinerPolicy* policy,
     OfflinePageModel* offline_page_model)
     : browser_context_(browser_context),
       policy_(policy),
       offline_page_model_(offline_page_model),
       pending_request_(nullptr),
@@ skipping 63 matching lines @@
     // Clear pending request and app listener then run completion callback.
     pending_request_.reset(nullptr);
     app_listener_.reset(nullptr);
     completion_callback_.Run(request, load_status);
   }
 }
 
 void PrerenderingOffliner::OnSavePageDone(
     const SavePageRequest& request,
     SavePageResult save_result,
-    int64_t offline_id) {
+    int64_t offline_id,
+    const base::FilePath& saved_filepath) {
   // Check if request is still pending receiving a callback.
   if (!pending_request_)
     return;
 
   // Also check that this completed request is same as the pending one
   // (since SavePage request is not cancel-able currently and could be old).
   if (request.request_id() != pending_request_->request_id()) {
     DVLOG(1) << "Ignoring save callback for old request";
     return;
   }
 
   // Clear pending request and app listener here and then inform loader we
   // are done with WebContents.
   pending_request_.reset(nullptr);
   app_listener_.reset(nullptr);
   GetOrCreateLoader()->StopLoading();
 
-  // Determine status and run the completion callback.
+  // Determine status.
   Offliner::RequestStatus save_status;
   if (save_result == SavePageResult::SUCCESS) {
     save_status = RequestStatus::SAVED;
   } else {
     // TODO(dougarnett): Consider reflecting some recommendation to retry the
     // request based on specific save error cases.
     save_status = RequestStatus::SAVE_FAILED;
   }
+
+  // Append signal data to the savevd file.

[fgorski, 2017/03/08 18:56:42]

saved

+  base::FilePath filepath_copy(saved_filepath);

[fgorski, 2017/03/08 18:56:42]

Is there a reason for copying the path here?

+  AppendSignalDataOnFileThread(filepath_copy);
+  // TODO: Do we need to update the file size in the OfflinePageModel?
+  // TODO: Should we wait for the file IO to be done before we return?
+
+  // Run the completion callback (in the RequestCoordinator).
   completion_callback_.Run(request, save_status);
 }
 
 bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request,
                                        const CompletionCallback& callback) {
   DCHECK(!pending_request_.get());
 
   if (pending_request_) {
     DVLOG(1) << "Already have pending request";
     return false;
@@ skipping 129 matching lines @@
       application_state ==
           base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
     DVLOG(1) << "App became active, canceling current offlining request";
     SavePageRequest* request = pending_request_.get();
     Cancel();
     completion_callback_.Run(*request,
                              Offliner::RequestStatus::FOREGROUND_CANCELED);
   }
 }
 
+void PrerenderingOffliner::AppendSignalDataOnFileThread(

[fgorski, 2017/03/08 18:56:42]

I'd reverse the names of methods.

+    const base::FilePath& file_path) {

[fgorski, 2017/03/08 18:56:43]

Add appropriate check for thread here.

+  base::FilePath file_path_copy(file_path);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&PrerenderingOffliner::AppendSignalData,
+                 weak_ptr_factory_.GetWeakPtr(), file_path));

[fgorski, 2017/03/08 18:56:42]

weak pointers are not thread-safe

If you want a callback from the other thread, it should work then (when posted
back here).

+}
+
+// TODO: Write unit tests for these new functions, fix unit tests that will
+// now try to write to the MHTML file.
+void PrerenderingOffliner::AppendSignalData(const base::FilePath& file_path) {

[fgorski, 2017/03/08 18:56:42]

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.

+  DVLOG(0) << "@@@@@@ FilePath " << file_path.value() << " " << __func__;
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  std::vector<std::string> signals = loader_->GetSignalData();
+  if (signals.size() == 0)

[fgorski, 2017/03/08 18:56:42]

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)

+    return;
+
+  // Use the FilePath to create a FILE object, opened for read and write.  File
+  // will be closed when the File object goes out of scope.
+  base::File saved_file =
+      base::File(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                                base::File::FLAG_WRITE);
+  DVLOG(0) << "@@@@@@ opened file OK";

[fgorski, 2017/03/08 18:56:42]

how do you know if opened fine? Isn't there a validation you need to do for
that?

+
+  // Get the MHTML boundary string.
+  std::string boundary = GetBoundaryStringFromFile(saved_file);

[fgorski, 2017/03/08 18:56:42]

you don't need this until 344

+  DVLOG(0) << "@@@@@@ saw a MHTML boundary of " << boundary;
+
+  // Chop off the last two hypens from the boundary string.  We do this by

[fgorski, 2017/03/08 18:56:42]

hyphens?

+  // backing up the current position to just before them, and overwriting.
+  int64_t position = PositionBeforeTwoDashes(saved_file);
+  saved_file.Seek(base::File::FROM_BEGIN, position);
+  // TODO: \n, or \r\f, or something else?

[fgorski, 2017/03/08 18:56:42]

would std::endl work?

+  saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
+
+  // Add a mime type of text/SignalData.
+  saved_file.WriteAtCurrentPos(kTextSignal, sizeof(kTextSignal));
+  saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
+
+  // Add signals that the loader collected.
+  for (std::string signal : signals) {
+    saved_file.WriteAtCurrentPos(signal.c_str(), signal.length());
+    saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
+  }
+
+  // End with a boundary string followed by a terminating double dash.
+  saved_file.WriteAtCurrentPos("--", 2);
+  saved_file.WriteAtCurrentPos(boundary.c_str(), boundary.length());
+  saved_file.WriteAtCurrentPos("--", 2);
+  saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
+
+  // Exiting will let the file object go out of scope, which closes the file.
+  DVLOG(0) << "@@@@@@ Done writing MHTML file.";
+}
+
+// Returns an empty string if none can be found.
+// TODO: What is the best way?  Read backwards from the file?  Read boundary=
+// from file?
+std::string PrerenderingOffliner::GetBoundaryStringFromFile(

[fgorski, 2017/03/08 18:56:42]

Could this be a function in implementation file?

+    base::File& saved_file) {
+  // We've chosen to get the last boundary so we don't have to read the whole
+  // file and parse it looking for the boundary="" string
+  // Seek to just before the two dashes.
+  int64_t position = PositionBeforeTwoDashes(saved_file);
+  if (position == 0)
+    return std::string();
+  char found_character = '\0';
+
+  saved_file.Read(--position, &found_character, 1);
+
+  // Everything between preceeding dashes and end of dashes is boundary, build
+  // it up as we go along (backwards).
+  std::string boundary;
+  boundary = boundary + found_character;
+  bool one_dash_seen = false;
+  bool two_dashes_seen = false;
+  // We have reached the start of the boundary when we see two consecutive
+  // dashes.
+  while (!two_dashes_seen && position > 0) {
+    saved_file.Read(--position, &found_character, 1);
+    boundary = found_character + boundary;

[fgorski, 2017/03/08 18:56:42]

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.

+
+    // TODO: Verify a boundary may not contain dash dash.  If it may, I need to
+    // get the boundary by parsing instead of reading backwards.  Alternately, I
+    // could pass the boudary down from where we set it.  Is this safe if we
+    // know that *our* boundaries won't contain dash dash?
+    if (one_dash_seen && found_character == '-')

[fgorski, 2017/03/08 18:56:42]

int seen_dashes_count = 0;

while (seen_dashes_count < 2 && ...) {
  
if (found_character == '-')
  ++seen_dashes_count;
else
  seen_dashes_count = 0;

+      two_dashes_seen = true;
+    else if (found_character == '-')
+      one_dash_seen = true;
+    else
+      one_dash_seen = false;
+  }
+  if (!two_dashes_seen)
+    return std::string();
+
+  // Our string now has two dashes at the front that we don't want.
+  return boundary.substr(2);
+}
+
+// TODO: Find and use the appropriate utility function instead, there should
+// be one somewhere in Chromium.
+bool PrerenderingOffliner::IsWhitespace(char next) {
+  if (next == '\f' || next == '\r' || next == '\t' || next == ' ')
+    return true;
+
+  return false;
+}
+
+// returns 0 for failure.
+int64_t PrerenderingOffliner::PositionBeforeTwoDashes(base::File& saved_file) {

[fgorski, 2017/03/08 18:56:42]

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.

+  // We've chosen to get the last boundary so we don't have to read the whole
+  // file and parse it looking for the boundary="" string
+  // Seek to end
+  int64_t position = saved_file.Seek(base::File::FROM_END, 0);
+
+  // move backwards past any whitespace then last two dashes find preceeding
+  // dashes.  We are using UTF-8.  Dash cannot be a lead or trail byte, so we
+  // should be safe.
+  char found_character = '\0';
+  saved_file.Read(--position, &found_character, 1);
+
+  // Eat any whitespace at the end of the file.
+  while (IsWhitespace(found_character))
+    saved_file.Read(--position, &found_character, 1);
+
+  // Eat two dashes
+  if (found_character != '-')
+    return 0;
+  saved_file.Read(--position, &found_character, 1);
+  if (found_character != '-')
+    return 0;
+
+  return --position;
+}
+
 }  // namespace offline_pages