OOPIFs: Fixing submitting forms targeting a remote frame.

third_party/WebKit/Source/core/html/HTMLFormElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLFormElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLFormElement.cpp b/third_party/WebKit/Source/core/html/HTMLFormElement.cpp
index f7ea19c53f1eee1af4c92890cace9f99f61136de..22d06ddc47dc16adf96436c2ecc73266d1b38850 100644
--- a/third_party/WebKit/Source/core/html/HTMLFormElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLFormElement.cpp
@@ -36,6 +36,7 @@
 #include "core/events/ScopedEventQueue.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
+#include "core/frame/RemoteFrame.h"
 #include "core/frame/UseCounter.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/html/HTMLCollection.h"
@@ -47,6 +48,7 @@
 #include "core/html/RadioNodeList.h"
 #include "core/html/forms/FormController.h"
 #include "core/inspector/ConsoleMessage.h"
+#include "core/loader/FormSubmission.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/loader/MixedContentChecker.h"
@@ -420,9 +422,12 @@ void HTMLFormElement::scheduleFormSubmission(FormSubmission* submission)
     if (MixedContentChecker::isMixedFormAction(document().frame(), submission->action()))
         UseCounter::count(document().frame(), UseCounter::MixedContentFormsSubmitted);
 
-    // FIXME: Plumb form submission for remote frames.
-    if (targetFrame->isLocalFrame())
+    if (targetFrame->isLocalFrame()) {
         toLocalFrame(targetFrame)->navigationScheduler().scheduleFormSubmission(&document(), submission);
+    } else {
+        FrameLoadRequest frameLoadRequest = submission->createFrameLoadRequest(&document());
+        toRemoteFrame(targetFrame)->navigate(frameLoadRequest);

[dcheng, 2016/06/12 05:50:29]

Crazy idea: is it possible to just call Frame::navigate() for both paths here?
We can just always do this in a timer so we still get the scheduled effect.

[Łukasz Anforowicz, 2016/06/13 17:13:42]

Hmmm.  This does sound like something we should at least seriously attempt.  At
the same time I'd prefer to keep the current CL as-is - the alternative approach
has higher risk of changing behavior for local frames (not really touched in the
current CL) and so containing the changes in a separate CL seems desirable (to
enable evaluating the impact in separation + to enable reverting in separation).
 I also have hidden motivation for keeping the current CL as-is: 1) this will
help me rebase and land the OpenURL CLs and 2) I want to minimize the risk the
POST work won't land before my vacation starts next week... :-/

I see the following aspects of behavior we would need to ensure we are
preserving (if replacing navigationScheduler for the local case with an ad-hoc
timer + virtual Frame::navigate call):

- NavigationScheduler::mustReplaceCurrentItem(LocalFrame* targetFrame) is
currently private and therefore only callable via the current path.  We would
need to figure out how to duplicate the behavior outside of the current code
path without duplicating the code.  OTOH, I do note that calculation of
|replaceCurrentItem| for remote frames seems sketchy in the current CL (well,
there is no calculation - we depend on replaceCurrentItem being set by default
to false by the constructors of FrameLoadRequest).

- It is not immediately clear how to preserve maybeLogScheduledNavigationClobber
outside of navigationScheduler path.

- I don't quite understand the significance of instantiating
OwnPtr<UserGestureIndicator> gestureIndicator = createUserGestureIndicator(); in
ScheduledFormSubmission::fire (as opposed to just calling static methods of
UserGestureIndicator, without constructing an instance).

Additionally - we probably should audit other scheduled navigations in
core/loader/NavigationScheduler.cpp and see if they should also be potentially
replaced with a timer-based call to Frame::navigate (i.e. if these navigations
can target a remote frame).

FWIW, I've added a TODO here...

+    }
 }
 
 void HTMLFormElement::reset()