[webkit-reviews] review granted: [Bug 25223] REGRESSION: Back button after form submission to the same URL fails to navigate : [Attachment 29538] v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Darin Fisher (:fishd, Google)
<fishd at chromium.org>'s request for review:
Bug 25223: REGRESSION: Back button after form submission to the same URL fails
to navigate
https://bugs.webkit.org/show_bug.cgi?id=25223

Attachment 29538: v2 patch: same as before, but with equivalent changes to
win/FrameLoadDelegate.cpp
https://bugs.webkit.org/attachment.cgi?id=29538&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      // We also do not do anchor-style navigation if we're posting a form.
>      
>  #if ENABLE(WML)
> -    if (!formData && urlsMatchItem(item) &&
!m_frame->document()->isWMLDocument()) {
> +    if (!formData && !m_currentHistoryItem->formData() &&
urlsMatchItem(item) && !m_frame->document()->isWMLDocument()) {
>  #else
> -    if (!formData && urlsMatchItem(item)) {
> +    if (!formData && !m_currentHistoryItem->formData() &&
urlsMatchItem(item)) {
>  #endif
>	   // Must do this maintenance here, since we don't go through a real
page reload
>	   saveScrollPositionAndViewStateToItem(m_currentHistoryItem.get());

It seems that if you're changing the code you also should change the comment
before it, which describes the condition in a high-level way.

Not your fault, and maybe best to not do this in this patch, but that
WMLDocument check is done in a particuarly ugly way. Maybe we could just set up
a boolean before the if statement instead of repeating the whole if statement
twice. The extra open brace inside an #if also tends to confuse syntax
highlighters.

> +    // if another load started, then wait for it to complete.
> +    if (topLoadingFrame != nil)
> +	   return;

Normally we wouldn't say != nil here.

r=me