Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(136)

Side by Side Diff: chrome/browser/android/offline_pages/prerendering_offliner.cc

Issue 2683493002: Get signals working in the EXTRA_DATA section of MHTML (Closed)
Patch Set: Approach for writing to the file afterwards instead. Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
OLDNEW
1 // Copyright 2016 The Chromium Authors. All rights reserved. 1 // Copyright 2016 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include "chrome/browser/android/offline_pages/prerendering_offliner.h" 5 #include "chrome/browser/android/offline_pages/prerendering_offliner.h"
6 6
7 #include "base/bind.h" 7 #include "base/bind.h"
8 #include "base/metrics/histogram_macros.h" 8 #include "base/metrics/histogram_macros.h"
9 #include "base/sys_info.h" 9 #include "base/sys_info.h"
10 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h" 10 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
11 #include "chrome/browser/android/offline_pages/offliner_helper.h" 11 #include "chrome/browser/android/offline_pages/offliner_helper.h"
12 #include "chrome/browser/profiles/profile.h" 12 #include "chrome/browser/profiles/profile.h"
13 #include "components/offline_pages/core/background/offliner_policy.h" 13 #include "components/offline_pages/core/background/offliner_policy.h"
14 #include "components/offline_pages/core/background/save_page_request.h" 14 #include "components/offline_pages/core/background/save_page_request.h"
15 #include "components/offline_pages/core/client_namespace_constants.h" 15 #include "components/offline_pages/core/client_namespace_constants.h"
16 #include "components/offline_pages/core/downloads/download_ui_adapter.h" 16 #include "components/offline_pages/core/downloads/download_ui_adapter.h"
17 #include "components/offline_pages/core/offline_page_model.h" 17 #include "components/offline_pages/core/offline_page_model.h"
18 #include "content/public/browser/browser_context.h" 18 #include "content/public/browser/browser_context.h"
19 #include "content/public/browser/browser_thread.h"
19 #include "content/public/browser/web_contents.h" 20 #include "content/public/browser/web_contents.h"
20 21
21 namespace offline_pages { 22 namespace offline_pages {
22 namespace { 23 namespace {
23 const char kDownloadUIAdapterKey[] = "download-ui-adapter"; 24 const char kDownloadUIAdapterKey[] = "download-ui-adapter";
25 const char kTextSignal[] = "Content-Type: text/signal-data";
26 const char kNewline[] = "\n";
24 } 27 }
25 28
26 PrerenderingOffliner::PrerenderingOffliner( 29 PrerenderingOffliner::PrerenderingOffliner(
27 content::BrowserContext* browser_context, 30 content::BrowserContext* browser_context,
28 const OfflinerPolicy* policy, 31 const OfflinerPolicy* policy,
29 OfflinePageModel* offline_page_model) 32 OfflinePageModel* offline_page_model)
30 : browser_context_(browser_context), 33 : browser_context_(browser_context),
31 policy_(policy), 34 policy_(policy),
32 offline_page_model_(offline_page_model), 35 offline_page_model_(offline_page_model),
33 pending_request_(nullptr), 36 pending_request_(nullptr),
(...skipping 63 matching lines...) Expand 10 before | Expand all | Expand 10 after
97 // Clear pending request and app listener then run completion callback. 100 // Clear pending request and app listener then run completion callback.
98 pending_request_.reset(nullptr); 101 pending_request_.reset(nullptr);
99 app_listener_.reset(nullptr); 102 app_listener_.reset(nullptr);
100 completion_callback_.Run(request, load_status); 103 completion_callback_.Run(request, load_status);
101 } 104 }
102 } 105 }
103 106
104 void PrerenderingOffliner::OnSavePageDone( 107 void PrerenderingOffliner::OnSavePageDone(
105 const SavePageRequest& request, 108 const SavePageRequest& request,
106 SavePageResult save_result, 109 SavePageResult save_result,
107 int64_t offline_id) { 110 int64_t offline_id,
111 const base::FilePath& saved_filepath) {
108 // Check if request is still pending receiving a callback. 112 // Check if request is still pending receiving a callback.
109 if (!pending_request_) 113 if (!pending_request_)
110 return; 114 return;
111 115
112 // Also check that this completed request is same as the pending one 116 // Also check that this completed request is same as the pending one
113 // (since SavePage request is not cancel-able currently and could be old). 117 // (since SavePage request is not cancel-able currently and could be old).
114 if (request.request_id() != pending_request_->request_id()) { 118 if (request.request_id() != pending_request_->request_id()) {
115 DVLOG(1) << "Ignoring save callback for old request"; 119 DVLOG(1) << "Ignoring save callback for old request";
116 return; 120 return;
117 } 121 }
118 122
119 // Clear pending request and app listener here and then inform loader we 123 // Clear pending request and app listener here and then inform loader we
120 // are done with WebContents. 124 // are done with WebContents.
121 pending_request_.reset(nullptr); 125 pending_request_.reset(nullptr);
122 app_listener_.reset(nullptr); 126 app_listener_.reset(nullptr);
123 GetOrCreateLoader()->StopLoading(); 127 GetOrCreateLoader()->StopLoading();
124 128
125 // Determine status and run the completion callback. 129 // Determine status.
126 Offliner::RequestStatus save_status; 130 Offliner::RequestStatus save_status;
127 if (save_result == SavePageResult::SUCCESS) { 131 if (save_result == SavePageResult::SUCCESS) {
128 save_status = RequestStatus::SAVED; 132 save_status = RequestStatus::SAVED;
129 } else { 133 } else {
130 // TODO(dougarnett): Consider reflecting some recommendation to retry the 134 // TODO(dougarnett): Consider reflecting some recommendation to retry the
131 // request based on specific save error cases. 135 // request based on specific save error cases.
132 save_status = RequestStatus::SAVE_FAILED; 136 save_status = RequestStatus::SAVE_FAILED;
133 } 137 }
138
139 // Append signal data to the savevd file.
fgorski 2017/03/08 18:56:42 saved
140 base::FilePath filepath_copy(saved_filepath);
fgorski 2017/03/08 18:56:42 Is there a reason for copying the path here?
141 AppendSignalDataOnFileThread(filepath_copy);
142 // TODO: Do we need to update the file size in the OfflinePageModel?
143 // TODO: Should we wait for the file IO to be done before we return?
144
145 // Run the completion callback (in the RequestCoordinator).
134 completion_callback_.Run(request, save_status); 146 completion_callback_.Run(request, save_status);
135 } 147 }
136 148
137 bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request, 149 bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request,
138 const CompletionCallback& callback) { 150 const CompletionCallback& callback) {
139 DCHECK(!pending_request_.get()); 151 DCHECK(!pending_request_.get());
140 152
141 if (pending_request_) { 153 if (pending_request_) {
142 DVLOG(1) << "Already have pending request"; 154 DVLOG(1) << "Already have pending request";
143 return false; 155 return false;
(...skipping 129 matching lines...) Expand 10 before | Expand all | Expand 10 after
273 application_state == 285 application_state ==
274 base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) { 286 base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
275 DVLOG(1) << "App became active, canceling current offlining request"; 287 DVLOG(1) << "App became active, canceling current offlining request";
276 SavePageRequest* request = pending_request_.get(); 288 SavePageRequest* request = pending_request_.get();
277 Cancel(); 289 Cancel();
278 completion_callback_.Run(*request, 290 completion_callback_.Run(*request,
279 Offliner::RequestStatus::FOREGROUND_CANCELED); 291 Offliner::RequestStatus::FOREGROUND_CANCELED);
280 } 292 }
281 } 293 }
282 294
295 void PrerenderingOffliner::AppendSignalDataOnFileThread(
fgorski 2017/03/08 18:56:42 I'd reverse the names of methods.
296 const base::FilePath& file_path) {
fgorski 2017/03/08 18:56:43 Add appropriate check for thread here.
297 base::FilePath file_path_copy(file_path);
298 content::BrowserThread::PostTask(
299 content::BrowserThread::FILE, FROM_HERE,
300 base::Bind(&PrerenderingOffliner::AppendSignalData,
301 weak_ptr_factory_.GetWeakPtr(), file_path));
fgorski 2017/03/08 18:56:42 weak pointers are not thread-safe If you want a c
302 }
303
304 // TODO: Write unit tests for these new functions, fix unit tests that will
305 // now try to write to the MHTML file.
306 void PrerenderingOffliner::AppendSignalData(const base::FilePath& file_path) {
fgorski 2017/03/08 18:56:42 Can we make it into a function (to avoid temptatio
307 DVLOG(0) << "@@@@@@ FilePath " << file_path.value() << " " << __func__;
308 DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
309
310 std::vector<std::string> signals = loader_->GetSignalData();
311 if (signals.size() == 0)
fgorski 2017/03/08 18:56:42 question 1: What is the point of switching threads
312 return;
313
314 // Use the FilePath to create a FILE object, opened for read and write. File
315 // will be closed when the File object goes out of scope.
316 base::File saved_file =
317 base::File(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ |
318 base::File::FLAG_WRITE);
319 DVLOG(0) << "@@@@@@ opened file OK";
fgorski 2017/03/08 18:56:42 how do you know if opened fine? Isn't there a vali
320
321 // Get the MHTML boundary string.
322 std::string boundary = GetBoundaryStringFromFile(saved_file);
fgorski 2017/03/08 18:56:42 you don't need this until 344
323 DVLOG(0) << "@@@@@@ saw a MHTML boundary of " << boundary;
324
325 // Chop off the last two hypens from the boundary string. We do this by
fgorski 2017/03/08 18:56:42 hyphens?
326 // backing up the current position to just before them, and overwriting.
327 int64_t position = PositionBeforeTwoDashes(saved_file);
328 saved_file.Seek(base::File::FROM_BEGIN, position);
329 // TODO: \n, or \r\f, or something else?
fgorski 2017/03/08 18:56:42 would std::endl work?
330 saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
331
332 // Add a mime type of text/SignalData.
333 saved_file.WriteAtCurrentPos(kTextSignal, sizeof(kTextSignal));
334 saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
335
336 // Add signals that the loader collected.
337 for (std::string signal : signals) {
338 saved_file.WriteAtCurrentPos(signal.c_str(), signal.length());
339 saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
340 }
341
342 // End with a boundary string followed by a terminating double dash.
343 saved_file.WriteAtCurrentPos("--", 2);
344 saved_file.WriteAtCurrentPos(boundary.c_str(), boundary.length());
345 saved_file.WriteAtCurrentPos("--", 2);
346 saved_file.WriteAtCurrentPos(kNewline, sizeof(kNewline));
347
348 // Exiting will let the file object go out of scope, which closes the file.
349 DVLOG(0) << "@@@@@@ Done writing MHTML file.";
350 }
351
352 // Returns an empty string if none can be found.
353 // TODO: What is the best way? Read backwards from the file? Read boundary=
354 // from file?
355 std::string PrerenderingOffliner::GetBoundaryStringFromFile(
fgorski 2017/03/08 18:56:42 Could this be a function in implementation file?
356 base::File& saved_file) {
357 // We've chosen to get the last boundary so we don't have to read the whole
358 // file and parse it looking for the boundary="" string
359 // Seek to just before the two dashes.
360 int64_t position = PositionBeforeTwoDashes(saved_file);
361 if (position == 0)
362 return std::string();
363 char found_character = '\0';
364
365 saved_file.Read(--position, &found_character, 1);
366
367 // Everything between preceeding dashes and end of dashes is boundary, build
368 // it up as we go along (backwards).
369 std::string boundary;
370 boundary = boundary + found_character;
371 bool one_dash_seen = false;
372 bool two_dashes_seen = false;
373 // We have reached the start of the boundary when we see two consecutive
374 // dashes.
375 while (!two_dashes_seen && position > 0) {
376 saved_file.Read(--position, &found_character, 1);
377 boundary = found_character + boundary;
fgorski 2017/03/08 18:56:42 Would it make sense to establish position first an
378
379 // TODO: Verify a boundary may not contain dash dash. If it may, I need to
380 // get the boundary by parsing instead of reading backwards. Alternately, I
381 // could pass the boudary down from where we set it. Is this safe if we
382 // know that *our* boundaries won't contain dash dash?
383 if (one_dash_seen && found_character == '-')
fgorski 2017/03/08 18:56:42 int seen_dashes_count = 0; while (seen_dashes_cou
384 two_dashes_seen = true;
385 else if (found_character == '-')
386 one_dash_seen = true;
387 else
388 one_dash_seen = false;
389 }
390 if (!two_dashes_seen)
391 return std::string();
392
393 // Our string now has two dashes at the front that we don't want.
394 return boundary.substr(2);
395 }
396
397 // TODO: Find and use the appropriate utility function instead, there should
398 // be one somewhere in Chromium.
399 bool PrerenderingOffliner::IsWhitespace(char next) {
400 if (next == '\f' || next == '\r' || next == '\t' || next == ' ')
401 return true;
402
403 return false;
404 }
405
406 // returns 0 for failure.
407 int64_t PrerenderingOffliner::PositionBeforeTwoDashes(base::File& saved_file) {
fgorski 2017/03/08 18:56:42 if you add end position to signature, you could ni
408 // We've chosen to get the last boundary so we don't have to read the whole
409 // file and parse it looking for the boundary="" string
410 // Seek to end
411 int64_t position = saved_file.Seek(base::File::FROM_END, 0);
412
413 // move backwards past any whitespace then last two dashes find preceeding
414 // dashes. We are using UTF-8. Dash cannot be a lead or trail byte, so we
415 // should be safe.
416 char found_character = '\0';
417 saved_file.Read(--position, &found_character, 1);
418
419 // Eat any whitespace at the end of the file.
420 while (IsWhitespace(found_character))
421 saved_file.Read(--position, &found_character, 1);
422
423 // Eat two dashes
424 if (found_character != '-')
425 return 0;
426 saved_file.Read(--position, &found_character, 1);
427 if (found_character != '-')
428 return 0;
429
430 return --position;
431 }
432
283 } // namespace offline_pages 433 } // namespace offline_pages
OLDNEW

Powered by Google App Engine
This is Rietveld 408576698