Bug 213341

Created attachment 402208 [details]
Patch

Comment on attachment 402208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402208&action=review

> Source/WebCore/ChangeLog:3
> +        Move form submission logic into HTMLFrameElement

You probably mean HTMLFormElement. HTMLFrameElement would not make much sense.

Comment on attachment 402208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402208&action=review

>> Source/WebCore/ChangeLog:3
>> +        Move form submission logic into HTMLFrameElement
> 
> You probably mean HTMLFormElement. HTMLFrameElement would not make much sense.

Thanks Chris, you are correct, will fix. I was doing too many things at the same time...

Created attachment 402210 [details]
Patch

Created attachment 402220 [details]
Patch

Created attachment 402539 [details]
Patch

Created attachment 402548 [details]
Patch

Created attachment 402562 [details]
Patch

Created attachment 402662 [details]
Patch

Comment on attachment 402662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402662&action=review

> Source/WebCore/ChangeLog:9
> +        Move some form submission logic into HTMLFormElement, this
> +        allows us to remove some state and complexity out of FrameLoader.

Generally looks OK. It’s a little sad when an HTML element class has so much of this sort of logic, but on balance I agree that this is an improvement.

> Source/WebCore/html/HTMLFormElement.cpp:383
> +    ASSERT(!formSubmission->state().sourceDocument().frame() || formSubmission->state().sourceDocument().frame() == frame.get());

I think this should be something you can write without "get()".

> Source/WebCore/html/HTMLFormElement.cpp:386
> +    if (!frame->page())
> +        return;

This is a significant change in behavior. Before, in a case like this, the code below would run. We will leave m_shouldSubmit set to true now, for example. Is this a desirable change?

> Source/WebCore/html/HTMLFormElement.cpp:389
> +        return;

Ditto.

> Source/WebCore/html/HTMLFormElement.cpp:394
> +        return;

Ditto.

> Source/WebCore/loader/FrameLoader.cpp:2222
> -    m_quickRedirectComing = (lockBackForwardList == LockBackForwardList::Yes || history().currentItemShouldBeReplaced()) && m_documentLoader && !m_isExecutingJavaScriptFormAction;
> +    m_quickRedirectComing = (lockBackForwardList == LockBackForwardList::Yes || history().currentItemShouldBeReplaced()) && m_documentLoader;

What makes this change safe? Was this boolean doing anything useful before? If so, why isn’t it needed now?