[Offline Pages] Snapshotting on timeout of last retry.

[Offline Pages] Snapshotting on timeout of last retry.

Added the logic which would do a snapshot when the offliner times out
on the last retry, also it must meet the low bar for quality control.
As for now the low bar is to get DOMContentLoaded callback.

BUG=680363
Review-Url: https://codereview.chromium.org/2637563002
Cr-Commit-Position: refs/heads/master@{#453053}
Committed: https://chromium.googlesource.com/chromium/src/+/250c4a05efdb4ec893d99ed7fe63236686caec4d

[romax, 2017-01-24 20:36:07]

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

[romax, 2017-01-24 20:36:08]

I realized there's a bug in the other patch(the one you and Doug already
reviewed) and this way can easily avoid that.
PTAL at this one.
Thanks!

[Pete Williamson, 2017-01-25 00:06:17]

The changes all look good, but the results don't show the improvement we hoped
for.  Let's investigate and see why, maybe something is missing from this
change, then decide whether to continue to pursue it or abandon it.

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

https://codereview.chromium.org/2637563002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:77: return
false;
This should have a TODO: decide if we want to also take a snapshot on the last
timeout for the background loader offliner.

[romax, 2017-01-25 02:02:08]

Patchset #2 (id:20001) has been deleted

[romax, 2017-01-25 02:10:50]

romax@chromium.org changed reviewers:
+ dougarnett@chromium.org

[romax, 2017-01-25 02:10:50]

merged with master(patch 2) and addressed comments(patch 3).
also +dougarnett@

[romax, 2017-02-01 20:38:39]

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

[commit-bot: I haz the power, 2017-02-01 20:38:57]

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, 2017-02-01 20:41:04]

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

[commit-bot: I haz the power, 2017-02-01 20:41:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[romax, 2017-02-01 21:19:59]

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

[commit-bot: I haz the power, 2017-02-01 21:20:22]

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, 2017-02-01 22:18:31]

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

[commit-bot: I haz the power, 2017-02-01 22:18:32]

Dry run: This issue passed the CQ dry run.

[romax, 2017-02-24 21:32:54]

just updated with TOT, and run 'git cl format' which returned a minor change in
format.
PTAL, thanks!

[romax, 2017-02-24 21:33:07]

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

[commit-bot: I haz the power, 2017-02-24 21:33:51]

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

[Pete Williamson, 2017-02-24 22:29:03]

lgtm with nit.

https://codereview.chromium.org/2637563002/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2637563002/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/background_loader_offliner.cc:127: //
TODO(romax) device if we want to also take a snapshot on the last timeout
Nit: Device -> Decide

[romax, 2017-02-24 22:31:30]

The CQ bit was checked by romax@chromium.org

[romax, 2017-02-24 22:31:30]

The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2637563002/#ps160001
(title: "comments.")

[commit-bot: I haz the power, 2017-02-24 22:32:47]

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-02-25 00:08:09]

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

[commit-bot: I haz the power, 2017-02-25 00:08:18]

Failed to apply patch for
chrome/browser/android/offline_pages/prerendering_loader.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/android/offline_pages/prerendering_loader.cc:244
  error: chrome/browser/android/offline_pages/prerendering_loader.cc: patch does
not apply

Patch:       chrome/browser/android/offline_pages/prerendering_loader.cc
Index: chrome/browser/android/offline_pages/prerendering_loader.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_loader.cc
b/chrome/browser/android/offline_pages/prerendering_loader.cc
index
f6e1b6e435b50bdf91776f1b4446b41b388834fe..6153b308da87b4165545921f91c300e39df578f3
100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -79,7 +79,8 @@ Offliner::RequestStatus ClassifyFinalStatus(
 PrerenderingLoader::PrerenderingLoader(content::BrowserContext*
browser_context)
     : state_(State::IDLE),
       snapshot_controller_(nullptr),
-      browser_context_(browser_context) {
+      browser_context_(browser_context),
+      is_lowbar_met_(false) {
   adapter_.reset(new PrerenderAdapter(this));
 }
 
@@ -157,11 +158,14 @@ void PrerenderingLoader::OnPrerenderDomContentLoaded() {
   if (!adapter_->GetWebContents()) {
     // Without a WebContents object at this point, we are done.
     HandleLoadingStopped();
-  } else if (kConsiderDclForSnapshot) {
-    // Inform SnapshotController of DomContentLoaded event so it can
-    // determine when to consider it really LOADED (e.g., some multiple
-    // second delay from this event).
-    snapshot_controller_->DocumentAvailableInMainFrame();
+  } else {
+    is_lowbar_met_ = true;
+    if (kConsiderDclForSnapshot) {
+      // Inform SnapshotController of DomContentLoaded event so it can
+      // determine when to consider it really LOADED (e.g., some multiple
+      // second delay from this event).
+      snapshot_controller_->DocumentAvailableInMainFrame();
+    }
   }
 }
 
@@ -175,6 +179,10 @@ void PrerenderingLoader::StartSnapshot() {
   HandleLoadEvent();
 }
 
+bool PrerenderingLoader::IsLowbarMet() {
+  return is_lowbar_met_;
+}
+
 void PrerenderingLoader::HandleLoadEvent() {
   // If still loading, check if the load succeeded or not, then update
   // the internal state (LOADED for success or IDLE for failure) and post
@@ -244,6 +252,7 @@ void PrerenderingLoader::HandleLoadingStopped() {
   snapshot_controller_.reset(nullptr);
   session_contents_.reset(nullptr);
   state_ = State::IDLE;
+  is_lowbar_met_ = false;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback_, request_status, nullptr));
 }
@@ -255,6 +264,7 @@ void PrerenderingLoader::CancelPrerender() {
   snapshot_controller_.reset(nullptr);
   session_contents_.reset(nullptr);
   state_ = State::IDLE;
+  is_lowbar_met_ = false;
 }
 
 }  // namespace offline_pages

[romax, 2017-02-25 00:32:46]

The CQ bit was checked by romax@chromium.org

[romax, 2017-02-25 00:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2637563002/#ps180001
(title: "merging with master again")

[commit-bot: I haz the power, 2017-02-25 00:33:19]

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-02-25 00:34:51]

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

[commit-bot: I haz the power, 2017-02-25 00:34:52]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[romax, 2017-02-25 02:26:44]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2017-02-25 02:27:01]

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-02-25 03:08:18]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487989604040170,
"parent_rev": "d0b25b0a993e6623ddf9deb789cbd6f9ae35435d", "commit_rev":
"250c4a05efdb4ec893d99ed7fe63236686caec4d"}

[commit-bot: I haz the power, 2017-02-25 03:08:53]

Description was changed from

==========
[Offline Pages] Snapshotting on timeout of last retry.

Added the logic which would do a snapshot when the offliner times out
on the last retry, also it must meet the low bar for quality control.
As for now the low bar is to get DOMContentLoaded callback.

BUG=680363
==========

to

==========
[Offline Pages] Snapshotting on timeout of last retry.

Added the logic which would do a snapshot when the offliner times out
on the last retry, also it must meet the low bar for quality control.
As for now the low bar is to get DOMContentLoaded callback.

BUG=680363

Review-Url: https://codereview.chromium.org/2637563002
Cr-Commit-Position: refs/heads/master@{#453053}
Committed:
https://chromium.googlesource.com/chromium/src/+/250c4a05efdb4ec893d99ed7fe63...
==========

[commit-bot: I haz the power, 2017-02-25 03:08:54]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/250c4a05efdb4ec893d99ed7fe63...