Changeset 61207 in webkit


Ignore:
Timestamp:
Jun 15, 2010 12:31:53 PM (14 years ago)
Author:
darin@chromium.org
Message:

2010-06-15 Darin Fisher <darin@chromium.org>

Reviewed by Brady Eidson.

Introduce HistoryItem::itemSequenceNumber and use it to identify
HistoryItems that are clones of one another.

Changes HistoryController::recursiveGoToItem to use itemSequenceNumber
equality instead of isTargetItem as the pre-requisite for not calling
FrameLoader::loadItem.

Changes FrameLoader::loadItem to require equivalent
documentSequenceNumber before initiating a same document navigation.
This alone would appear to fix the bug, but it does not go far enough
since without the itemSequenceNumber equality check, we'd re-load more
often than we should.

Moves documentSequenceNumber assignment into createItemTree as cleanup
and to ensure that it gets called properly whenever we create a cloned
HistoryItem. (createItemTree's mission is to create clones up until
or including the target frame depending on the value of the doClip
parameter.)

Removes the now unused HistoryController::urlsMatchItem.

https://bugs.webkit.org/show_bug.cgi?id=40451

Test: fast/history/history-back-within-subframe.html

http/tests/navigation/history-back-across-form-submission-to-fragment.html

  • history/HistoryItem.cpp: (WebCore::generateSequenceNumber): (WebCore::HistoryItem::HistoryItem):
  • history/HistoryItem.h: (WebCore::HistoryItem::setItemSequenceNumber): (WebCore::HistoryItem::itemSequenceNumber):
  • loader/FrameLoader.cpp: (WebCore::FrameLoader::loadItem):
  • loader/HistoryController.cpp: (WebCore::HistoryController::updateBackForwardListForFragmentScroll): (WebCore::HistoryController::createItemTree): (WebCore::HistoryController::recursiveGoToItem): (WebCore::HistoryController::pushState):
  • loader/HistoryController.h:
Location:
trunk
Files:
5 added
10 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r61205 r61207  
     12010-06-15  Darin Fisher  <darin@chromium.org>
     2
     3        Reviewed by Brady Eidson.
     4
     5        Test that a back navigation following a subframe reference fragment
     6        navigation does the right thing.
     7
     8        https://bugs.webkit.org/show_bug.cgi?id=40451
     9
     10        * fast/history/history-back-within-subframe.html: Added.
     11        * fast/history/resources/history-back-within-subframe-2.html: Added.
     12        * http/tests/navigation/history-back-across-form-submission-to-fragment-expected.txt: Added.
     13        * http/tests/navigation/history-back-across-form-submission-to-fragment.html: Added.
     14        * http/tests/navigation/resources/submit-to-fragment.pl: Added.
     15
    1162010-06-15  Alexey Proskuryakov  <ap@apple.com>
    217
  • trunk/WebCore/ChangeLog

    r61206 r61207  
     12010-06-15  Darin Fisher  <darin@chromium.org>
     2
     3        Reviewed by Brady Eidson.
     4
     5        Introduce HistoryItem::itemSequenceNumber and use it to identify
     6        HistoryItems that are clones of one another.
     7
     8        Changes HistoryController::recursiveGoToItem to use itemSequenceNumber
     9        equality instead of isTargetItem as the pre-requisite for not calling
     10        FrameLoader::loadItem.
     11
     12        Changes FrameLoader::loadItem to require equivalent
     13        documentSequenceNumber before initiating a same document navigation.
     14        This alone would appear to fix the bug, but it does not go far enough
     15        since without the itemSequenceNumber equality check, we'd re-load more
     16        often than we should.
     17
     18        Moves documentSequenceNumber assignment into createItemTree as cleanup
     19        and to ensure that it gets called properly whenever we create a cloned
     20        HistoryItem.  (createItemTree's mission is to create clones up until
     21        or including the target frame depending on the value of the doClip
     22        parameter.)
     23
     24        Removes the now unused HistoryController::urlsMatchItem.
     25
     26        https://bugs.webkit.org/show_bug.cgi?id=40451
     27
     28        Test: fast/history/history-back-within-subframe.html
     29              http/tests/navigation/history-back-across-form-submission-to-fragment.html
     30
     31        * history/HistoryItem.cpp:
     32        (WebCore::generateSequenceNumber):
     33        (WebCore::HistoryItem::HistoryItem):
     34        * history/HistoryItem.h:
     35        (WebCore::HistoryItem::setItemSequenceNumber):
     36        (WebCore::HistoryItem::itemSequenceNumber):
     37        * loader/FrameLoader.cpp:
     38        (WebCore::FrameLoader::loadItem):
     39        * loader/HistoryController.cpp:
     40        (WebCore::HistoryController::updateBackForwardListForFragmentScroll):
     41        (WebCore::HistoryController::createItemTree):
     42        (WebCore::HistoryController::recursiveGoToItem):
     43        (WebCore::HistoryController::pushState):
     44        * loader/HistoryController.h:
     45
    1462010-06-15  Xan Lopez  <xlopez@igalia.com>
    247
  • trunk/WebCore/history/HistoryItem.cpp

    r56825 r61207  
    3838namespace WebCore {
    3939
    40 static long long generateDocumentSequenceNumber()
     40static long long generateSequenceNumber()
    4141{
    4242    // Initialize to the current time to reduce the likelihood of generating
     
    5858    , m_isTargetItem(false)
    5959    , m_visitCount(0)
    60     , m_documentSequenceNumber(generateDocumentSequenceNumber())
     60    , m_itemSequenceNumber(generateSequenceNumber())
     61    , m_documentSequenceNumber(generateSequenceNumber())
    6162{
    6263}
     
    7172    , m_isTargetItem(false)
    7273    , m_visitCount(0)
    73     , m_documentSequenceNumber(generateDocumentSequenceNumber())
     74    , m_itemSequenceNumber(generateSequenceNumber())
     75    , m_documentSequenceNumber(generateSequenceNumber())
    7476{   
    7577    iconDatabase()->retainIconForPageURL(m_urlString);
     
    8688    , m_isTargetItem(false)
    8789    , m_visitCount(0)
    88     , m_documentSequenceNumber(generateDocumentSequenceNumber())
     90    , m_itemSequenceNumber(generateSequenceNumber())
     91    , m_documentSequenceNumber(generateSequenceNumber())
    8992{
    9093    iconDatabase()->retainIconForPageURL(m_urlString);
     
    102105    , m_isTargetItem(false)
    103106    , m_visitCount(0)
    104     , m_documentSequenceNumber(generateDocumentSequenceNumber())
     107    , m_itemSequenceNumber(generateSequenceNumber())
     108    , m_documentSequenceNumber(generateSequenceNumber())
    105109{   
    106110    iconDatabase()->retainIconForPageURL(m_urlString);
     
    134138    , m_dailyVisitCounts(item.m_dailyVisitCounts)
    135139    , m_weeklyVisitCounts(item.m_weeklyVisitCounts)
    136     , m_documentSequenceNumber(generateDocumentSequenceNumber())
     140    , m_itemSequenceNumber(item.m_itemSequenceNumber)
     141    , m_documentSequenceNumber(item.m_documentSequenceNumber)
    137142    , m_formContentType(item.m_formContentType)
    138143{
  • trunk/WebCore/history/HistoryItem.h

    r53950 r61207  
    137137    SerializedScriptValue* stateObject() const { return m_stateObject.get(); }
    138138
     139    void setItemSequenceNumber(long long number) { m_itemSequenceNumber = number; }
     140    long long itemSequenceNumber() const { return m_itemSequenceNumber; }
     141
    139142    void setDocumentSequenceNumber(long long number) { m_documentSequenceNumber = number; }
    140143    long long documentSequenceNumber() const { return m_documentSequenceNumber; }
     
    243246    OwnPtr<Vector<String> > m_redirectURLs;
    244247
     248    long long m_itemSequenceNumber;
     249
    245250    // Support for HTML5 History
    246251    RefPtr<SerializedScriptValue> m_stateObject;
  • trunk/WebCore/loader/FrameLoader.cpp

    r61104 r61207  
    35433543{
    35443544    // We do same-document navigation in the following cases:
    3545     // - The HistoryItem has a history state object
    3546     // - Navigating to an anchor within the page, with no form data stored on the target item or the current history entry,
    3547     //   and the URLs in the frame tree match the history item for fragment scrolling.
    3548     // - The HistoryItem is not the same as the current item, because such cases are treated as a new load.
     3545    // - The HistoryItem corresponds to the same document.
     3546    // - The HistoryItem is not the same as the current item.
    35493547    HistoryItem* currentItem = history()->currentItem();
    3550     bool sameDocumentNavigation = ((!item->formData() && !(currentItem && currentItem->formData()) && history()->urlsMatchItem(item))
    3551                                   || (currentItem && item->documentSequenceNumber() == currentItem->documentSequenceNumber()))
    3552                                   && item != currentItem;
     3548    bool sameDocumentNavigation = currentItem && item != currentItem
     3549        && item->documentSequenceNumber() == currentItem->documentSequenceNumber();
    35533550
    35543551#if ENABLE(WML)
  • trunk/WebCore/loader/HistoryController.cpp

    r59933 r61207  
    107107{
    108108    updateBackForwardListClippedAtTarget(false);
    109    
    110     // Since the document isn't changed as a result of a fragment scroll, we should
    111     // preserve the DocumentSequenceNumber of the previous item.
    112     if (!m_previousItem)
    113         return;
    114 
    115     ASSERT(m_currentItem);
    116     m_currentItem->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber());
    117109}
    118110
     
    230222    page->setGlobalHistoryItem((!settings || settings->privateBrowsingEnabled()) ? 0 : targetItem);
    231223    recursiveGoToItem(targetItem, currentItem, type);
    232 }
    233 
    234 // Walk the frame tree and ensure that the URLs match the URLs in the item.
    235 bool HistoryController::urlsMatchItem(HistoryItem* item) const
    236 {
    237     const KURL& currentURL = m_frame->loader()->documentLoader()->url();
    238     if (!equalIgnoringFragmentIdentifier(currentURL, item->url()))
    239         return false;
    240 
    241     const HistoryItemVector& childItems = item->children();
    242 
    243     unsigned size = childItems.size();
    244     for (unsigned i = 0; i < size; ++i) {
    245         Frame* childFrame = m_frame->tree()->child(childItems[i]->target());
    246         if (childFrame && !childFrame->loader()->history()->urlsMatchItem(childItems[i].get()))
    247             return false;
    248     }
    249 
    250     return true;
    251224}
    252225
     
    522495    if (m_previousItem)
    523496        saveScrollPositionAndViewStateToItem(m_previousItem.get());
    524     if (!(clipAtTarget && m_frame == targetFrame)) {
     497
     498    if (!clipAtTarget || m_frame != targetFrame) {
    525499        // save frame state for items that aren't loading (khtml doesn't save those)
    526500        saveDocumentState();
     501
     502        // clipAtTarget is false for navigations within the same document, so
     503        // we should copy the documentSequenceNumber over to the newly create
     504        // item.  Non-target items are just clones, and they should therefore
     505        // preserve the same itemSequenceNumber.
     506        if (m_previousItem) {
     507            if (m_frame != targetFrame)
     508                bfItem->setItemSequenceNumber(m_previousItem->itemSequenceNumber());
     509            bfItem->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber());
     510        }
     511
    527512        for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling()) {
    528513            FrameLoader* childLoader = child->loader();
     
    537522        }
    538523    }
     524    // FIXME: Eliminate the isTargetItem flag in favor of itemSequenceNumber.
    539525    if (m_frame == targetFrame)
    540526        bfItem->setIsTargetItem(true);
     
    551537    ASSERT(fromItem);
    552538
    553     KURL itemURL = item->url();
    554     KURL currentURL;
    555     if (m_frame->loader()->documentLoader())
    556         currentURL = m_frame->loader()->documentLoader()->url();
    557 
    558     // Always reload the target frame of the item we're going to.  This ensures that we will
    559     // do -some- load for the transition, which means a proper notification will be posted
    560     // to the app.
    561     // The exact URL has to match, including fragment.  We want to go through the _load
    562     // method, even if to do a within-page navigation.
    563     // The current frame tree and the frame tree snapshot in the item have to match.
    564     if (!item->isTargetItem() &&
    565         itemURL == currentURL &&
    566         ((m_frame->tree()->name().isEmpty() && item->target().isEmpty()) || m_frame->tree()->name() == item->target()) &&
    567         childFramesMatchItem(item))
     539    // If the item we're going to is a clone of the item we're at, then do
     540    // not load it again, and continue history traversal to its children.
     541    // The current frame tree and the frame tree snapshot in the item have
     542    // to match.
     543    if (item->itemSequenceNumber() == fromItem->itemSequenceNumber()
     544        && ((m_frame->tree()->name().isEmpty() && item->target().isEmpty()) || m_frame->tree()->name() == item->target())
     545        && childFramesMatchItem(item))
    568546    {
    569547        // This content is good, so leave it alone and look for children that need reloading
     
    660638    item->setURLString(urlString);
    661639
    662     // Since the document isn't changed as a result of a pushState call, we
    663     // should preserve the DocumentSequenceNumber of the previous item.
    664     item->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber());
    665    
    666640    page->backForwardList()->pushStateItem(item.release());
    667641}
  • trunk/WebCore/loader/HistoryController.h

    r57558 r61207  
    5959
    6060    void goToItem(HistoryItem*, FrameLoadType);
    61     bool urlsMatchItem(HistoryItem*) const;
    6261
    6362    void updateForBackForwardNavigation();
  • trunk/WebKit/chromium/ChangeLog

    r61198 r61207  
     12010-06-11  Darin Fisher  <darin@chromium.org>
     2
     3        Reviewed by Brady Eidson.
     4
     5        Expose WebHistoryItem::itemSequenceNumber.
     6
     7        https://bugs.webkit.org/show_bug.cgi?id=40451
     8
     9        * public/WebHistoryItem.h:
     10        * src/WebHistoryItem.cpp:
     11        (WebKit::WebHistoryItem::itemSequenceNumber):
     12        (WebKit::WebHistoryItem::setItemSequenceNumber):
     13
    1142010-06-15  Yury Semikhatsky  <yurys@chromium.org>
    215
  • trunk/WebKit/chromium/public/WebHistoryItem.h

    r55820 r61207  
    3434#include "WebCommon.h"
    3535#include "WebPrivatePtr.h"
     36
     37// FIXME: Remove this once Chromium starts using itemSequenceNumber.
     38#define WEBKIT_BUG_40451_IS_FIXED
    3639
    3740namespace WebCore { class HistoryItem; }
     
    103106    WEBKIT_API void setDocumentState(const WebVector<WebString>&);
    104107
     108    WEBKIT_API long long itemSequenceNumber() const;
     109    WEBKIT_API void setItemSequenceNumber(long long);
     110
    105111    WEBKIT_API long long documentSequenceNumber() const;
    106112    WEBKIT_API void setDocumentSequenceNumber(long long);
  • trunk/WebKit/chromium/src/WebHistoryItem.cpp

    r55820 r61207  
    202202}
    203203
     204long long WebHistoryItem::itemSequenceNumber() const
     205{
     206    return m_private->itemSequenceNumber();
     207}
     208
     209void WebHistoryItem::setItemSequenceNumber(long long itemSequenceNumber)
     210{
     211    ensureMutable();
     212    m_private->setItemSequenceNumber(itemSequenceNumber);
     213}
     214
    204215long long WebHistoryItem::documentSequenceNumber() const
    205216{
Note: See TracChangeset for help on using the changeset viewer.