blimp: Fix page load status tracking.

blimp/engine/session/page_load_tracker.cc

Index: blimp/engine/session/page_load_tracker.cc
diff --git a/blimp/engine/session/page_load_tracker.cc b/blimp/engine/session/page_load_tracker.cc
index 24e3a4871aeb134c601a3479d1120608bf7fed9a..ec61d3ee8499025016369753b9a2e33c5f8d77cf 100644
--- a/blimp/engine/session/page_load_tracker.cc
+++ b/blimp/engine/session/page_load_tracker.cc
@@ -4,6 +4,7 @@
 
 #include "blimp/engine/session/page_load_tracker.h"
 
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_widget_host_view.h"
 
 namespace blimp {
@@ -11,93 +12,69 @@ namespace engine {
 
 namespace {
 
-content::RenderWidgetHost* GetRenderWidgetHostIfMainFrame(
-    content::RenderFrameHost* render_frame_host) {
-  if (render_frame_host->GetParent() != nullptr)
-    return nullptr;
+bool ShouldIgnoreNavigation(content::NavigationHandle* navigation_handle) {
+  // We change the progress bar for main frame navigations only.
+  if (!navigation_handle->IsInMainFrame())
+    return true;
 
-  return render_frame_host->GetView()->GetRenderWidgetHost();
+  // Same page navigations don't need to trigger a progress bar update.
+  if (navigation_handle->IsSamePage())
+    return true;
+
+  return false;
 }
 
 }  // namespace
 
 PageLoadTracker::PageLoadTracker(content::WebContents* web_contents,
                                  PageLoadTrackerClient* client)
-    : client_(client) {
+    : client_(client), weak_factory_(this) {
   DCHECK(web_contents);
   Observe(web_contents);
 }
 
 PageLoadTracker::~PageLoadTracker() {}
 
-void PageLoadTracker::DidStartProvisionalLoadForFrame(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    bool is_error_page,
-    bool is_iframe_srcdoc) {
-  content::RenderWidgetHost* render_widget_host =
-      GetRenderWidgetHostIfMainFrame(render_frame_host);
-  if (!render_widget_host)
+void PageLoadTracker::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (ShouldIgnoreNavigation(navigation_handle))
     return;
 
-  render_widget_load_status_[render_widget_host] = LoadStatus();
-
-  // Notify the client that a navigation was initiated.
+  DCHECK(!navigation_pending_)
+      << "There should be only one navigation in the main frame for a tab";

[nasko, 2016/11/08 16:39:27]

I don't think this is a correct DCHECK. It is quite possible to have a slow
navigation to a cross-site URL which will require a cross-process navigation
while in the meantime the existing document is invoking pushState, which is
classified as "same-page" navigation.

[Khushal, 2016/11/08 20:52:25]

I made sure to explicitly ignore same page navigations above in
ShouldIgnoreNavigation. Given that notifications from this code are only being
used for the progress bar, does that sound reasonable?

[nasko, 2016/11/08 21:55:15]

I still think it is possible without same page navigations. Feel free to leave
it in, but don't be surprised if this DCHECK is hit.

[Khushal, 2016/11/08 23:54:06]

Oh oh. I already came here trying to fix one DCHECK. I'll remove it then.

[Khushal, 2016/11/11 01:53:35]

Done.

+  navigation_pending_ = true;
   client_->SendPageLoadStatusUpdate(PageLoadStatus::LOADING);
 }
 
-void PageLoadTracker::DidFinishLoad(content::RenderFrameHost* render_frame_host,
-                                    const GURL& validated_url) {
-  content::RenderWidgetHost* render_widget_host =
-      GetRenderWidgetHostIfMainFrame(render_frame_host);
-  if (!render_widget_host)
+void PageLoadTracker::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (ShouldIgnoreNavigation(navigation_handle))
     return;
 
-  RenderWidgetLoadStatusMap::iterator it =
-      render_widget_load_status_.find(render_widget_host);
-  DCHECK(it != render_widget_load_status_.end());
-
-  it->second.page_loaded = true;
-  if (it->second.Loaded()) {
+  DCHECK(navigation_pending_);
+  navigation_pending_ = false;
+
+  if (navigation_handle->HasCommitted()) {

[nasko, 2016/11/08 16:39:27]

Does this code care about error page vs real document?

[Khushal, 2016/11/08 20:52:25]

Not at the moment. This code is very ad-hoc, and should change when a more
structured approach to navigation state sync is in place and how that will
integrate with the client side UI.

[nasko, 2016/11/08 21:55:15]

Acknowledged.

+    // Make sure that at least one frame after the navigation commits is sent to

[nasko, 2016/11/08 16:39:27]

nit: Be specific when you say "frame" as it is a very overloaded term. You
probably mean a drawing frame, not a RenderFrame, right?

[Khushal, 2016/11/08 20:52:25]

Yes, it means an update to the compositor's content for this RenderFrame on the
client. I'll change it to that and make it more explicit. 

[Khushal, 2016/11/11 01:53:35]

Done.

+    // the client.
+    // Note that a visual state update in our case implies that this callback
+    // will be invoked after a frame is queued to be sent to the client.
+    navigation_handle->GetRenderFrameHost()->InsertVisualStateCallback(
+        base::Bind(&PageLoadTracker::DidPaintAfterNavigationCommitted,

[nasko, 2016/11/08 16:39:27]

This seems open to raciness. It could very well be that the browser has updated
its state to have committed the navigation, but the IPC channel still has queued
up a paint IPC from the old document.

[Khushal, 2016/11/08 20:52:25]

Good point. Could I use a cancelable callback then? It can be reset each time a
new navigation is committed?

I think in the long run we would want tie compositor content updates with the
navigation they correspond to. So this logic can be driven completely on the
client. For instance, the client will have its own pending navigation, while
still receiving content updates from the previous one.

[Khushal, 2016/11/08 21:20:57]

Sorry, I meant each time we get a new pending navigation.

[nasko, 2016/11/08 21:55:15]

I'm not sure it will fully eliminate the race. If you have a document A and a
navigation to document B, when document B commits, you might still have a
drawing frame from document A in the IPC queue. kenrb@ has hit various different
cases where navigations correlation to painting is hard, so maybe you can pull
him in for a more expert review of that specific code.

[Khushal, 2016/11/08 23:54:06]

Its true, we could have a drawing frame in the IPC queue for document A, but the
callback corresponding to that frame should get dropped the moment that
navigation is committed in the browser. We attach the id for the callback with
the IPC message that is expected to be reflected back by the renderer. So the
browser will still process the message, but the callback shouldn't run.

+kenrb to take a look too. In case there is some other case we're missing.

[kenrb, 2016/11/10 18:11:42]

The issues I had to deal with (and still haven't entirely fixed) relating to
FirstPaintAfterLoad came from the fact that DidCommitProvisionalLoad and the
first CompositorFrameSwap are not necessarily ordered, and also not necessarily
paired (because for example the renderer could crash after sending one and never
send the other). It sounds like associating an identifier with the callback
should make it possible to solve this.

[Khushal, 2016/11/11 01:53:34]

Thanks Ken. The id is already attached by the RenderFrameHostImpl when queuing
the callback. I was just going to rely on that, and cancel the queued callback
when receiving a DidStartNavigation for any new navigations. Do you mind taking
another look?

+                   weak_factory_.GetWeakPtr()));
+  } else {
+    // Inform the client to update the progress bar right away.
     client_->SendPageLoadStatusUpdate(PageLoadStatus::LOADED);
-    render_widget_load_status_.erase(it);
   }
 }
 
-void PageLoadTracker::DidFailLoad(content::RenderFrameHost* render_frame_host,
-                                  const GURL& validated_url,
-                                  int error_code,
-                                  const base::string16& error_description,
-                                  bool was_ignored_by_handler) {
-  content::RenderWidgetHost* render_widget_host =
-      GetRenderWidgetHostIfMainFrame(render_frame_host);
-  if (!render_widget_host)
+void PageLoadTracker::DidPaintAfterNavigationCommitted(bool result) {
+  // If a navigation is pending, early out. We'll send a complete event based
+  // on the status of the currently pending navigation.
+  if (navigation_pending_)
     return;
 
-  RenderWidgetLoadStatusMap::iterator it =
-      render_widget_load_status_.find(render_widget_host);
-  DCHECK(it != render_widget_load_status_.end());
-
-  // If the navigation failed, the client should dismiss the load indicator.
   client_->SendPageLoadStatusUpdate(PageLoadStatus::LOADED);
-  render_widget_load_status_.erase(it);
-}
-
-void PageLoadTracker::DidFirstPaintAfterLoad(
-    content::RenderWidgetHost* render_widget_host) {
-  RenderWidgetLoadStatusMap::iterator it =
-      render_widget_load_status_.find(render_widget_host);
-  DCHECK(it != render_widget_load_status_.end());
-
-  it->second.did_first_paint = true;
-  if (it->second.Loaded()) {
-    client_->SendPageLoadStatusUpdate(PageLoadStatus::LOADED);
-    render_widget_load_status_.erase(it);
-  }
-}
-
-bool PageLoadTracker::LoadStatus::Loaded() const {
-  return page_loaded && did_first_paint;
 }
 
 }  // namespace engine