Failed offline snapshots won't erase a successful one from the same page load.

Failed offline snapshots won't erase a successful one from the same page load.

If a second offline snapshot starts for the same page load where a
previous one was saved, it should not immediately delete it: as it might
fail the user would end up with no offline copy of the page. This change
fixes this issue.

Three new tests are added: one to cover this precise case and a couple
more to check for related situations. Also many comments were added or
updated.

BUG=655697
Committed: https://crrev.com/96de1925bdf5c051bb903581a427d7f9795c0032
Cr-Commit-Position: refs/heads/master@{#436086}

[carlosk, 2016-12-02 02:41:02]

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

[commit-bot: I haz the power, 2016-12-02 02:41:26]

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

[carlosk, 2016-12-02 02:43:55]

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

[carlosk, 2016-12-02 02:43:56]

fgorski@: PTAL.

[commit-bot: I haz the power, 2016-12-02 03:54:06]

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

[commit-bot: I haz the power, 2016-12-02 03:54:07]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[fgorski, 2016-12-02 19:07:38]

lgtm mod the compilation issue and one nit.

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

https://codereview.chromium.org/2542833003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:234: for (auto it =
ids.begin(); it != ids.end(); it++) {
++it

[carlosk, 2016-12-02 22:44:03]

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

[carlosk, 2016-12-02 22:44:14]

The CQ bit was unchecked by carlosk@chromium.org

[carlosk, 2016-12-02 22:48:51]

Thanks.

The build error was due to my changing a call to base::MakeUnique to directly
having OfflinePageModel::kInvalidOfflineId as an argument and that caused a link
error (for some reason). fgorski@ showed me that dimich@ also dealt with that
same issue and I just reverted the code to using a variable instead.

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

https://codereview.chromium.org/2542833003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:234: for (auto it =
ids.begin(); it != ids.end(); it++) {
On 2016/12/02 19:07:38, fgorski wrote:
> ++it 

Done.

[carlosk, 2016-12-02 22:49:12]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-12-02 22:49:13]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2542833003/#ps20001
(title: "Fix link error and address reviewer comments.")

[commit-bot: I haz the power, 2016-12-02 22:49:56]

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

[dewittj, 2016-12-02 23:21:36]

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

[dewittj, 2016-12-02 23:21:36]

fyi, the link error is due to violating the ODR
http://en.cppreference.com/w/cpp/language/static

the pertinent section is:
"If const static data member [...] is odr-used, a definition at namespace scope
is still required, but it cannot have an initializer."

Informally, an object is odr-used if its address is taken, or a reference is
bound to it. If an object is odr-used, its definition must exist somewhere in
the program; a violation of that is a link-time error.

I can make a separate patch to fix that or you could do it in this patch if you
like.

[carlosk, 2016-12-02 23:50:03]

On 2016/12/02 23:21:36, dewittj wrote:
> fyi, the link error is due to violating the ODR
> http://en.cppreference.com/w/cpp/language/static
> 
> the pertinent section is:
> "If const static data member [...] is odr-used, a definition at namespace
scope
> is still required, but it cannot have an initializer."
> 
> Informally, an object is odr-used if its address is taken, or a reference is
> bound to it. If an object is odr-used, its definition must exist somewhere in
> the program; a violation of that is a link-time error.
> 
> I can make a separate patch to fix that or you could do it in this patch if
you
> like.

Thanks for the investigation and explanation. I'll let you patch that as I
already hit submit on this one.

[commit-bot: I haz the power, 2016-12-03 00:01:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480718952777980,
"parent_rev": "4e1c8ef7641a3fbb69f1e1e7ab16a0270b256591", "commit_rev":
"4c80a0f137c604b510685cb2b48cffbf96aa3b0c"}

[commit-bot: I haz the power, 2016-12-03 00:03:20]

Description was changed from

==========
Failed offline snapshots won't erase a successful one from the same page load.

If a second offline snapshot starts for the same page load where a
previous one was saved, it should not immediately delete it: as it might
fail the user would end up with no offline copy of the page. This change
fixes this issue.

Three new tests are added: one to cover this precise case and a couple
more to check for related situations. Also many comments were added or
updated.

BUG=655697
==========

to

==========
Failed offline snapshots won't erase a successful one from the same page load.

If a second offline snapshot starts for the same page load where a
previous one was saved, it should not immediately delete it: as it might
fail the user would end up with no offline copy of the page. This change
fixes this issue.

Three new tests are added: one to cover this precise case and a couple
more to check for related situations. Also many comments were added or
updated.

BUG=655697

Committed: https://crrev.com/96de1925bdf5c051bb903581a427d7f9795c0032
Cr-Commit-Position: refs/heads/master@{#436086}
==========

[commit-bot: I haz the power, 2016-12-03 00:03:20]

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

[carlosk, 2016-12-08 19:19:27]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2561163002/ by carlosk@chromium.org.

The reason for reverting is: This created a new issue: https://crbug.com/671955.


And the approach to fix this will be different:
- The problem this CL fixes will decrease in importance once MHTML generation
stops failing because of missing iframes: https://crbug.com/655708
- We intend to soon refactor RecentTabHelper and that should finally fix this
issue.
.