Bug 15212 – Useless checking for Item in FrameLoader::goBackOrForward(int distance)

• Component: History
• Status: RESOLVED FIXED

[Maxime BRITTO]

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();

[Maxime BRITTO]

Created attachment 16290 [details]
proposed patch

I wasn't sure if I had to put an ASSERT for the item, so I didn't.

[Maxime BRITTO]

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 on attachment 16397 [details]
modified patch

r=me

[Mark Rowe (bdash)]

Landed in r26588.