PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

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 08576826a92db9984c60bbbfe2baf32711a173b8..e789093513da1b393ea652b832e7e5202ebfd5cf 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -4,26 +4,161 @@
 
 #include "chrome/browser/android/offline_pages/prerendering_loader.h"
 
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace offline_pages {
 
-PrerenderingLoader::PrerenderingLoader(
-    content::BrowserContext* browser_context) {}
+PrerenderingLoader::PrerenderingLoader(content::BrowserContext* browser_context)
+    : state_(State::IDLE), browser_context_(browser_context) {
+  adapter_.reset(new PrerenderAdapter(this));
+}
+
+PrerenderingLoader::~PrerenderingLoader() {
+  CancelPrerender();
+}
+
+bool PrerenderingLoader::LoadPage(const GURL& url,
+                                  const LoadPageCallback& callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!IsIdle()) {
+    DVLOG(1) << "WARNING: Existing request pending";

[pasko, 2016/05/20 19:04:53]

it might be LOADED and LOADING, not just pending

[dougarnett, 2016/05/20 22:21:12]

Done.

+    return false;
+  }
+  if (!CanPrerender())
+    return false;
 
-PrerenderingLoader::~PrerenderingLoader() {}
+  // Create a WebContents instance to define and hold a SessionStorageNamespace
+  // for this load request.
+  DCHECK(!session_contents_.get());
+  session_contents_.reset(content::WebContents::Create(
+      content::WebContents::CreateParams(browser_context_)));
+  bool accepted = adapter_->StartPrerender(
+      browser_context_, url, GetSessionStorageNamespace(), GetSize());
+  if (!accepted)
+    return false;
 
-bool PrerenderingLoader::LoadPage(
-    const GURL& url,
-    const LoadPageCallback& callback) {
-  // TODO(dougarnett): implement.
-  return false;
+  DCHECK(adapter_->IsActive());
+  callback_ = callback;
+  state_ = State::PENDING;
+  return true;
 }
 
 void PrerenderingLoader::StopLoading() {
-  // TODO(dougarnett): implement.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  CancelPrerender();
+}
+
+bool PrerenderingLoader::CanPrerender() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return adapter_->CanPrerender();
+}
+
+bool PrerenderingLoader::IsIdle() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return state_ == State::IDLE;
+}
+
+bool PrerenderingLoader::IsLoaded() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return state_ == State::LOADED;
+}
+
+void PrerenderingLoader::SetAdapterForTesting(
+    std::unique_ptr<PrerenderAdapter> prerender_adapter) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  adapter_ = std::move(prerender_adapter);
+}
+
+void PrerenderingLoader::OnPrerenderStart() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(state_ == State::PENDING);
+  state_ = State::LOADING;
+}
+
+void PrerenderingLoader::OnPrerenderStopLoading() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // TODO(dougarnett): Implement/integrate to delay policy here.
+  ReportLoaded();
+}
+
+void PrerenderingLoader::OnPrerenderDomContentLoaded() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // TODO(dougarnett): Implement/integrate to delay policy here.
+  ReportLoaded();
+}
+
+void PrerenderingLoader::OnPrerenderStop() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  ReportLoadingStopped();
+}
+
+content::SessionStorageNamespace*
+PrerenderingLoader::GetSessionStorageNamespace() {

[pasko, 2016/05/20 19:04:53]

called only once, does this really need to be a separate method?

[dougarnett, 2016/05/20 22:21:12]

Done.

+  DCHECK(session_contents_);
+  return session_contents_->GetController().GetDefaultSessionStorageNamespace();
+}
+
+const gfx::Size PrerenderingLoader::GetSize() {

[pasko, 2016/05/20 19:04:53]

this method is used only once, and the implementation is one line, but still
nontrivial, so forces the reader of the callsite to lookup for definition. =>
inlining it makes it easier to read.

[dougarnett, 2016/05/20 22:21:12]

Done.

+  DCHECK(session_contents_);
+  return session_contents_->GetContainerBounds().size();
+}
+
+void PrerenderingLoader::ReportLoaded() {

[pasko, 2016/05/20 19:04:53]

it does not only Report, but also performs state transitioning, making sure that
calling it twice is possible.

This makes the state transitioning scattered around non-trivially, and hence I
think the code should rather primarily emphasize how the state transitioning is
done, and what happens in between the transitions is secondary.

I'm wondering how we could make it more obvious. One way is to mandate the
transitioning functions to return the new state, and have the code that checks
for the state be close to the lines of code that modify the state. This way it
would be easy to see, for example, that ReportLoadingStopped() is only executed
once and transitions from LOADING to LOADED. In the current code it requires a
bit of reading to conclude that.

let me know if the above is unclear :)

[dougarnett, 2016/05/20 22:21:12]

Tried improved names and comments for these methods for their actual
responsibilities. 

[pasko, 2016/05/23 20:05:41]

I still find the state transitions here to be complicated (like: re-entering
HandleLoadingStopped() from different codepaths? is it when we asked to cancel,
but prerender crashed at the same time?) - easy to forget to update the state_,
etc.

But since the code is relatively small, and me not having concrete better
suggestions, unblocking the review is the best I can do. Thank you for
clarifications, they helped.

[dougarnett, 2016/05/23 21:43:54]

Will keep this feedback in mind going forward as I add timeouts to the load
events for determining when to snapshot and a timeout from above that will
cancel operation after some time budget and see if I can make clearer.

+  // Check if load is still active to see if reporting is applicable (i.e.,
+  // no other load detection or canceling has already occurred).
+  if (!IsLoaded() && !IsIdle()) {

[pasko, 2016/05/20 19:04:53]

We are not supposed to call ReportLoaded with state_ == IDLE, hence this should
be a DCHECK. Same for PENDING.

AFAICT this should be a DCHECK(state_ == LOADING || state_ == LOADED)

[dougarnett, 2016/05/20 22:21:12]

see preceding comment

+    content::WebContents* contents = adapter_->GetWebContents();
+    if (contents) {
+      state_ = State::LOADED;
+      base::ThreadTaskRunnerHandle::Get()->PostTask(

[pasko, 2016/05/20 19:04:53]

I still do not understand the ThreadTaskRunnerHandle thing.

First, why not just BrowserThread::PostTask(BrowserThread::UI, ...) ?

Second, you were mentioning some crashes you could avoid by doing this. Can you
provide concrete examples of a callbacks that would crash if called here and
work fine if they are re-posted to the UI thread?

[dougarnett, 2016/05/20 22:21:12]

I don't know which type of PostTask is preferred. Although seems nicer to ensure
posting on same thread (for single threaded code) instead of possibly posting on
different, specific thread.

The crash was doing direct callback call (instead of posting and returning up
the call stack before the callback runs). As I understand it, this direct call
end up call an object that was in the process of being deleted.

[pasko, 2016/05/23 20:05:41]

OK, using ThreadTaskRunnerHandle to post to the same thread makes sense, and it
seems commonly used.
This scenario does not seem possible. OnPrerenderStopLoading() and
OnPrerenderDomContentLoaded() are arriving from the prerender stack and are
delivered by posting tasks or sending IPCs from the renderer (or both). I cannot
see a codepath that would trigger one of these events from one of the
offline-related destructors. This could happen only if the destructor called
this method on the same stack, right?

Now there is a tradeoff: be on the safe side by posting a task, or save 10ms or
sometimes 100ms by running the callback earlier. On the large scale of
prerender-related events these savings probably do not matter, WDYT?

[dougarnett, 2016/05/23 21:43:54]

I don't have a strong opinion here. I agree does not look possible so Run() is
simpler/faster but also see Filip's point that it is on boundary of another
subsystem that may change beneath us so good to be conservative. I was inclined
to revisit with Filip and push to make Run()s but instead at this moment I think
I want to leave as is for: 1. getting CL landed to move forward; and 2. it is
not a hot code path (in fact, could be really cold code path wrt system testing)
so maybe conservative approach slightly better than the optimization.

+          FROM_HERE,
+          base::Bind(callback_, Offliner::RequestStatus::LOADED, contents));
+    } else {
+      // No WebContents means that the load failed.
+      ReportLoadingStopped();
+    }
+  }
+}
+
+void PrerenderingLoader::ReportLoadingStopped() {
+  // Check if request is still active to see if reporting is applicable.

[pasko, 2016/05/20 19:04:53]

"Applicable" is not concrete. It would be clearer if we say that we report it
only once per load request.

[dougarnett, 2016/05/20 22:21:12]

reworded

+  if (!IsIdle()) {
+    if (adapter_->IsActive()) {
+      DVLOG(1) << "Load failed: " << adapter_->GetFinalStatus();
+      adapter_->DestroyActive();
+    }
+    // Request status depends on whether we are still loading (failed) or
+    // did load and then loading was stopped (cancel - from prerender stack).
+    Offliner::RequestStatus request_status =
+        IsLoaded() ? Offliner::RequestStatus::CANCELED
+                   : Offliner::RequestStatus::FAILED;
+    // TODO(dougarnett): For failure, determine from final status if retry-able
+    // and report different failure statuses if retry-able or not.
+    session_contents_.reset(nullptr);
+    state_ = State::IDLE;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback_, request_status, nullptr));
+  }
+}
+
+void PrerenderingLoader::CancelPrerender() {
+  if (adapter_->IsActive()) {
+    adapter_->DestroyActive();
+  }
+  session_contents_.reset(nullptr);
+  if (!IsLoaded() && !IsIdle()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback_, Offliner::RequestStatus::CANCELED, nullptr));
+  }
+  state_ = State::IDLE;
 }
 
 }  // namespace offline_pages