PlzNavigate: fix timing issue in app window creation

extensions/browser/app_window/app_window.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/app_window/app_window.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/media_stream_request.h"
@@ skipping 411 matching lines @@
 
 void AppWindow::DidFirstVisuallyNonEmptyPaint() {
   first_paint_complete_ = true;
   if (show_on_first_paint_) {
     DCHECK(delayed_show_type_ == SHOW_ACTIVE ||
            delayed_show_type_ == SHOW_INACTIVE);
     Show(delayed_show_type_);
   }
 }
 
+void AppWindow::SetOnFirstCommitCallback(base::Callback<void(void)> callback) {
+  on_first_commit_callback_ = callback;

[Devlin, 2015/09/16 21:44:04]

Can we DCHECK(on_first_commit_callback_.is_null()) here?

[clamy, 2015/09/16 22:19:39]

Done.

+}
+
+void AppWindow::OnReadyToCommit() {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      ::switches::kEnableBrowserSideNavigation));
+  WindowEventsReady();
+  if (on_first_commit_callback_.is_null())
+    return;
+  // It is important that the callback executes after the calls to
+  // WebContentsObserver::AboutToCommitNavigation have been processed. The

[Devlin, 2015/09/16 21:44:04]

Two notes:
- AboutToCommitNavigation seems to have been renamed.
- This worries me.  AboutToCommitNavigation() is a new signal, and we're already
abusing it by saying "this is near what we want, but not quite, but we hope that
it'll be ready after some other tasks run."  Is there any way to make this a
little bit better?

[clamy, 2015/09/16 22:19:39]

Updated the comment.

As for the ReadyToCommitNavigation , we had planned on adding this call to the
WebContentsObserver interface for some time, as it will be needed by other
components such as DevTools (which require to have it happen exactly at the time
it happens there). At the same time, we'd like to avoid having to add a signal
after we've sent the IPC telling the renderer to commit the navigation, because
we want to limit the number of signals available to the WebContentsObserver.

The PostTask here is to ensure that any IPC sent to the renderer will be sent
after the CommitNavigation IPC. Given the way IPC queuing work, this also
ensures that the extension IPC will be processed after the CommitNavigation one,
therefore the Renderer will have been properly initialized.

[Devlin, 2015/09/16 22:37:14]

But even after all these IPCs, we still have a guarantee that the callback from
the extension API call (basically this task, +some IPCs) will definitely finish
before the onload handler for the window, right?

[clamy, 2015/09/16 23:08:25]

Yes I have verified that. Since we block requests from the renderer in the
network stack until the callback gets to execute, the renderer cannot actually
get the body of the response, and doesn't commit until the requests are
unblocked in the network stack. If it cannot commit,then we're sure the onload
handler will not be called.

+  // CommitNavigation IPC that will properly set up the renderer will only be
+  // sent after these, and it must be sent before the callback gets to run,
+  // hence the use of PostTask.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                on_first_commit_callback_);

[Devlin, 2015/09/16 21:44:04]

nit: base::ResetAndReturn(&on_first_commit_callback_)

[clamy, 2015/09/16 22:19:39]

Done.

+  on_first_commit_callback_.Reset();
+}
+
 void AppWindow::OnNativeClose() {
   AppWindowRegistry::Get(browser_context_)->RemoveAppWindow(this);
   if (app_window_contents_) {
     WebContentsModalDialogManager* modal_dialog_manager =
         WebContentsModalDialogManager::FromWebContents(web_contents());
     if (modal_dialog_manager)  // May be null in unit tests.
       modal_dialog_manager->SetDelegate(nullptr);
     app_window_contents_->NativeWindowClosed();
   }
   delete this;
@@ skipping 632 matching lines @@
         region.bounds.x(),
         region.bounds.y(),
         region.bounds.right(),
         region.bounds.bottom(),
         region.draggable ? SkRegion::kUnion_Op : SkRegion::kDifference_Op);
   }
   return sk_region;
 }
 
 }  // namespace extensions