Bug 15212 - Useless checking for Item in FrameLoader::goBackOrForward(int distance)
Summary: Useless checking for Item in FrameLoader::goBackOrForward(int distance)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-09-14 07:22 PDT by Maxime BRITTO
Modified: 2007-10-14 04:59 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (1.54 KB, patch)
2007-09-14 07:51 PDT, Maxime BRITTO
no flags Details | Formatted Diff | Diff
modified patch (1.38 KB, patch)
2007-09-26 02:31 PDT, Maxime BRITTO
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime BRITTO 2007-09-14 07:22:57 PDT
There is no need to check if there is an item at this distance because the check has already been performed in a previously :

This is the piece of code I'm talking about :
------------------------------------
    HistoryItem* item = list->itemAtIndex(distance);
    if (!item) {
        if (distance > 0) {
            int forwardListCount = list->forwardListCount();
            if (forwardListCount > 0) 
                item = list->itemAtIndex(forwardListCount);
        } else {
            int backListCount = list->forwardListCount();
            if (backListCount > 0)
                item = list->itemAtIndex(-backListCount);
        }
    }


There are three calls for FrameLoader::goBackOrForward(int distance) :
-----------------------------------------------------------
  1 - From FrameLoader::scheduleHistoryNavigation(int steps)
          -> which checks before the call if the step is "reachable" with canGoBackOrForward(steps).
          -> if the step is unreachable, goBackOrForward() isn't called at all

  2 - From FrameLoader::redirectionTimerFired(Timer<FrameLoader>*)
          -> in case we have a ScheduledRedirection::historyNavigation
          -> a ScheduledRedirection::historyNavigation can only be created from scheduleHistoryNavigation() (see point 1)

  3 - From ContextMenuController::contextMenuItemSelected(ContextMenuItem* item)
          -> the context menu choice "back" or "forward" is disabled if there no previous or next item

-

To sum up, I don't know how the item (from HistoryItem* item = list->itemAtIndex(distance);) in goBackOrForward() could be null. 
Moreover there is something wrong in this useless part of code : int backListCount = list->forwardListCount();
Comment 1 Maxime BRITTO 2007-09-14 07:51:28 PDT
Created attachment 16290 [details]
proposed patch

I wasn't sure if I had to put an ASSERT for the item, so I didn't.
Comment 2 Maxime BRITTO 2007-09-26 02:31:24 PDT
Created attachment 16397 [details]
modified patch

After the talk mitzpettel and I had, we decided to let the check for the reachable item in the function because it's more conveniant for the future updates.
Though I added an assert to help future debug.
I let that last check to avoid an eventually crash on release versions :
    if (item)
        page->goToItem(item, FrameLoadTypeIndexedBackForward);
Comment 3 mitz 2007-09-26 03:07:31 PDT
Comment on attachment 16397 [details]
modified patch

r=me
Comment 4 Mark Rowe (bdash) 2007-10-14 04:59:29 PDT
Landed in r26588.