[webkit-reviews] review denied: [Bug 40451] history.back() example that works in a main frame breaks in an iframe : [Attachment 58520] v2 patch [fixed style error]

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Darin Fisher (:fishd, Google)
<fishd at chromium.org>'s request for review:
Bug 40451: history.back() example that works in a main frame breaks in an
iframe
https://bugs.webkit.org/show_bug.cgi?id=40451

Attachment 58520: v2 patch [fixed style error]
https://bugs.webkit.org/attachment.cgi?id=58520&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
I like the approach here, and think it is fundamentally right.	But...

> Index: WebCore/loader/FrameLoader.cpp
> ===================================================================
> --- WebCore/loader/FrameLoader.cpp	(revision 60978)
> +++ WebCore/loader/FrameLoader.cpp	(working copy)
> @@ -3542,14 +3542,11 @@ void FrameLoader::navigateToDifferentDoc
>  void FrameLoader::loadItem(HistoryItem* item, FrameLoadType loadType)
>  {
>      // We do same-document navigation in the following cases:
> -    // - The HistoryItem has a history state object
> -    // - Navigating to an anchor within the page, with no form data stored
on the target item or the current history entry,
> -    //   and the URLs in the frame tree match the history item for fragment
scrolling.
> -    // - The HistoryItem is not the same as the current item, because such
cases are treated as a new load.
> +    // - The HistoryItem corresponds to the same document.
> +    // - The HistoryItem is not the same as the current item.
>      HistoryItem* currentItem = history()->currentItem();
> -    bool sameDocumentNavigation = ((!item->formData() && !(currentItem &&
currentItem->formData()) && history()->urlsMatchItem(item))
> -				     || (currentItem &&
item->documentSequenceNumber() == currentItem->documentSequenceNumber()))
> -				     && item != currentItem;
> +    bool sameDocumentNavigation = currentItem && item != currentItem
> +	   && item->documentSequenceNumber() ==
currentItem->documentSequenceNumber();

I'm concerned about the removal of the formData check here.  It's possible that
it's not an issue because of the expressiveness of the item sequence number
concept, but I haven't had enough coffee this afternoon to really think it
through just by reading this patch.


> @@ -659,10 +656,6 @@ void HistoryController::pushState(PassRe
>      item->setStateObject(stateObject);
>      item->setURLString(urlString);
>  
> -    // Since the document isn't changed as a result of a pushState call, we
> -    // should preserve the DocumentSequenceNumber of the previous item.
> -   
item->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber());
> -    
>      page->backForwardList()->pushStateItem(item.release());
>  }

I'm not entirely clear why this change is needed.

I'm provisionally r-'ing this patch, waiting for an explanation on both of
these points.  It should be pretty easy to convince me, at which point I'll r+ 
:)