PlzNavigate: fix timing issue in app window creation

extensions/browser/app_window/app_window.cc

Index: extensions/browser/app_window/app_window.cc
diff --git a/extensions/browser/app_window/app_window.cc b/extensions/browser/app_window/app_window.cc
index b6d7754c56a47124255bc3365a36b470696500b1..e799baa85a7822fec3e56c66169ba4b77b4f8b2a 100644
--- a/extensions/browser/app_window/app_window.cc
+++ b/extensions/browser/app_window/app_window.cc
@@ -11,6 +11,8 @@
 #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"
@@ -442,6 +444,26 @@ void AppWindow::DidFirstVisuallyNonEmptyPaint() {
   }
 }
 
+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_) {