[Offline Pages] Make new archives have random file names.

[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276
Committed: https://crrev.com/26b982a36584ba683f760383210ab64a83b24bb3
Cr-Commit-Position: refs/heads/master@{#441243}

[dewittj, 2016-12-16 19:19:10]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 19:19:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 23:56:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 23:56:31]

Dry run: This issue passed the CQ dry run.

[dewittj, 2016-12-23 01:20:37]

Description was changed from

==========
[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276
==========

to

==========
[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276
==========

[dewittj, 2016-12-23 01:20:37]

dewittj@chromium.org changed reviewers:
+ dimich@chromium.org

[dewittj, 2016-12-23 01:20:55]

dewittj@chromium.org changed reviewers:
+ carlosk@chromium.org

[dewittj, 2016-12-23 01:23:26]

PTAL, though Dmitry implied that overwriting archives /is/ actually closer to
what we want than just failing and removing the new one.

[carlosk, 2016-12-30 00:11:13]

LGTM after removing unneeded includes.

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:17: #include
"base/strings/utf_string_conversions.h"
It seems these 3 imports above aren't needed anymore.

[Pete Williamson, 2017-01-03 20:32:30]

petewil@chromium.org changed reviewers:
+ petewil@chromium.org

[Pete Williamson, 2017-01-03 20:32:30]

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:98:
archives_dir.Append(base::GenerateGUID()).AddExtension(kMHTMLExtension));
Drive by: This means that if we generate another page for the same file, it
won't collide or overwrite.  Are we sure there is nothing depending on a
collision in case of a duplicate?

[Dmitry Titov, 2017-01-03 22:09:26]

lgtm

[dewittj, 2017-01-03 22:23:39]

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:17: #include
"base/strings/utf_string_conversions.h"
On 2016/12/30 00:11:13, carlosk wrote:
> It seems these 3 imports above aren't needed anymore.

Done.

https://codereview.chromium.org/2579343002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:98:
archives_dir.Append(base::GenerateGUID()).AddExtension(kMHTMLExtension));
On 2017/01/03 20:32:30, Pete Williamson wrote:
> Drive by: This means that if we generate another page for the same file, it
> won't collide or overwrite.  Are we sure there is nothing depending on a
> collision in case of a duplicate?

We do currently exhibit this behavior when we manage to save 2 copies of a page
with RecentTabHelper.  This is the cause of the ALREADY_EXISTS error.  However,
it is better to clarify the behavior (already exists error does not update
archive) and then to change policy if necessary rather than leave it in this
strange state.

[dewittj, 2017-01-03 22:23:48]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2017-01-03 22:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
carlosk@chromium.org
Link to the patchset: https://codereview.chromium.org/2579343002/#ps20001
(title: "Remove unnecessary includes")

[commit-bot: I haz the power, 2017-01-03 22:24:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 23:15:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483482228011700,
"parent_rev": "2c5414cd19702d9dddc21d3e894ee2a6d03085b3", "commit_rev":
"4182a556c17cb6343bb76b2352b113667d9b540b"}

[commit-bot: I haz the power, 2017-01-03 23:16:08]

Description was changed from

==========
[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276
==========

to

==========
[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276

Review-Url: https://codereview.chromium.org/2579343002
==========

[commit-bot: I haz the power, 2017-01-03 23:16:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2017-01-03 23:18:56]

Description was changed from

==========
[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276

Review-Url: https://codereview.chromium.org/2579343002
==========

to

==========
[Offline Pages] Make new archives have random file names.

Currently offline pages with the same offline ID, title and URL will
produce archive files with the same filename.  This leads to unexpected
circumstances where archives could be overwritten even if the offline
page model is not updated.

This improvement also allows for more reliable updates to existing offline
pages by making the update process 3-step: create an archive, then update
the offline page to point to the newly created archive, then clean up the
old archive.

BUG=638276

Committed: https://crrev.com/26b982a36584ba683f760383210ab64a83b24bb3
Cr-Commit-Position: refs/heads/master@{#441243}
==========

[commit-bot: I haz the power, 2017-01-03 23:18:57]

Patchset 2 (id:??) landed as
https://crrev.com/26b982a36584ba683f760383210ab64a83b24bb3
Cr-Commit-Position: refs/heads/master@{#441243}