PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

components/test_runner/test_runner_for_specific_view.cc

 // Copyright 2016 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 "components/test_runner/test_runner_for_specific_view.h"
 
 #include <stddef.h>
 #include <limits>
 #include <utility>
 
@@ skipping 541 matching lines @@
 }
 
 void TestRunnerForSpecificView::DidLosePointerLockInternal() {
   bool was_locked = pointer_locked_;
   pointer_locked_ = false;
   if (was_locked)
     web_view()->didLosePointerLock();
 }
 
 bool TestRunnerForSpecificView::CallShouldCloseOnWebView() {
-  return web_view()->mainFrame()->dispatchBeforeUnloadEvent();
+  return web_view()->mainFrame()->toWebLocalFrame()->dispatchBeforeUnloadEvent(

[nasko, 2016/05/09 21:56:04]

I don't think you can make this assumption here. It is better off being an if
statement to check if the frame is local or remote. You can ping lukasza@, who
has been working on making the test runner work properly in OOPIF mode.

[Łukasz Anforowicz, 2016/05/09 22:43:23]

nasko@ - you are correct that web_view()->mainFrame() might be a WebRemoteFrame
here.  At the same time:

1) It seems that testRunner.callShouldCloseOnWebView() is called only from a
single layout test (fast/events/onbeforeunload-focused-iframe.html) and this
particular test only involves single-origin (file:) frames.

2) There is already one existing binding (i.e. testRunner.findString aka
TestRunnerForSpecificView::FindString) that assumes that main frame is a
WebLocalFrame.

3) I don't see an easy way to allow calling these bindings from any frame
(including frames in another renderer from the main frame) - allowing this would
probably require some IPCs to forward the call to the right renderer (and this
seems like an overkill for future, hypothetical test needs)

Given above, I think the changes in the CL-under-review are almost okay - I
would just add an explicit check if web_view()->mainFrame()->isWebLocalFrame()
and do something smart if it isn't (I guess violently failing a test is best to
avoid surprising future maintainers - a CHECK(false) + a comment might
accomplish this).

[clamy, 2016/05/10 00:39:56]

With the current architecture, you would get a NOTREACHED() if we tried to call
dispatchBeforeUnloadEvent on a WebRemoteFrame. We could either have that or a
CHECK(false) << "This function should only be called when the main frame is a
WebLocalFrame.".

[clamy, 2016/05/10 01:58:08]

Went with CHECK(false).

+      false);
 }
 
 void TestRunnerForSpecificView::SetDomainRelaxationForbiddenForURLScheme(
     bool forbidden,
     const std::string& scheme) {
   web_view()->setDomainRelaxationForbidden(forbidden,
                                            WebString::fromUTF8(scheme));
 }
 
 v8::Local<v8::Value>
@@ skipping 91 matching lines @@
 
 blink::WebView* TestRunnerForSpecificView::web_view() {
   return web_test_proxy_base_->web_view();
 }
 
 WebTestDelegate* TestRunnerForSpecificView::delegate() {
   return web_test_proxy_base_->delegate();
 }
 
 }  // namespace test_runner