15212 2007-09-14 07:22:57 -0700 Useless checking for Item in FrameLoader::goBackOrForward(int distance) 2007-10-14 04:59:29 -0700 1 1 1 Unclassified WebKit History 523.x (Safari 3) Mac OS X 10.4 RESOLVED FIXED P2 Minor --- 1 mbritto webkit-unassigned sroret oldest_to_newest 487 0 mbritto 2007-09-14 07:22:57 -0700 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(); 484 1 16290 mbritto 2007-09-14 07:51:28 -0700 Created attachment 16290 proposed patch I wasn't sure if I had to put an ASSERT for the item, so I didn't. 57061 2 16397 mbritto 2007-09-26 02:31:24 -0700 Created attachment 16397 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); 57062 3 16397 mitz 2007-09-26 03:07:31 -0700 Comment on attachment 16397 modified patch r=me 58493 4 mrowe 2007-10-14 04:59:29 -0700 Landed in r26588. 16290 2007-09-14 07:51:28 -0700 2007-09-26 02:31:24 -0700 proposed patch 20070914mBrittoBug15212.txt text/plain 1581 mbritto SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNTU2MSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTMgQEAKKzIwMDctMDktMTQgIE1heGltZSBCcml0dG8gIDxtYnJpdHRvQHBsZXlv LmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBC dWcgMTUyMTIgOiBSZW1vdmluZyBhIHVzZWxlc3MgcGFydCBvZiBjb2RlIGluIHRoZSBnb0JhY2tP ckZvcndhcmQoKSBmdW5jdGlvbi4KKyAgICAgICAgaHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTUyMTIKKworICAgICAgICAqIGxvYWRlci9GcmFtZUxvYWRlci5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpGcmFtZUxvYWRlcjo6Z29CYWNrT3JGb3J3YXJkKToKKwogMjAwNy0w OS0xNCAgU3ZlbiBIZXJ6YmVyZyAgPHN2ZW5AaW1lbmRpby5jb20+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgQWRhbSBSb2Jlbi4KSW5kZXg6IFdlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBXZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAJKHJldmlzaW9uIDI1 NTYxKQorKysgV2ViQ29yZS9sb2FkZXIvRnJhbWVMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC0xNDExLDIwICsxNDExLDcgQEAgdm9pZCBGcmFtZUxvYWRlcjo6Z29CYWNrT3JGb3J3YXJkKGlu dCBkaQogICAgIGlmICghbGlzdCkKICAgICAgICAgcmV0dXJuOwogICAgIAotICAgIEhpc3RvcnlJ dGVtKiBpdGVtID0gbGlzdC0+aXRlbUF0SW5kZXgoZGlzdGFuY2UpOwotICAgIGlmICghaXRlbSkg ewotICAgICAgICBpZiAoZGlzdGFuY2UgPiAwKSB7Ci0gICAgICAgICAgICBpbnQgZm9yd2FyZExp c3RDb3VudCA9IGxpc3QtPmZvcndhcmRMaXN0Q291bnQoKTsKLSAgICAgICAgICAgIGlmIChmb3J3 YXJkTGlzdENvdW50ID4gMCkgCi0gICAgICAgICAgICAgICAgaXRlbSA9IGxpc3QtPml0ZW1BdElu ZGV4KGZvcndhcmRMaXN0Q291bnQpOwotICAgICAgICB9IGVsc2UgewotICAgICAgICAgICAgaW50 IGJhY2tMaXN0Q291bnQgPSBsaXN0LT5mb3J3YXJkTGlzdENvdW50KCk7Ci0gICAgICAgICAgICBp ZiAoYmFja0xpc3RDb3VudCA+IDApCi0gICAgICAgICAgICAgICAgaXRlbSA9IGxpc3QtPml0ZW1B dEluZGV4KC1iYWNrTGlzdENvdW50KTsKLSAgICAgICAgfQotICAgIH0KLSAgICBpZiAoaXRlbSkK LSAgICAgICAgcGFnZS0+Z29Ub0l0ZW0oaXRlbSwgRnJhbWVMb2FkVHlwZUluZGV4ZWRCYWNrRm9y d2FyZCk7CisgICBwYWdlLT5nb1RvSXRlbShsaXN0LT5pdGVtQXRJbmRleChkaXN0YW5jZSksIEZy YW1lTG9hZFR5cGVJbmRleGVkQmFja0ZvcndhcmQpOwogfQogCiB2b2lkIEZyYW1lTG9hZGVyOjpy ZWRpcmVjdGlvblRpbWVyRmlyZWQoVGltZXI8RnJhbWVMb2FkZXI+KikK 16397 2007-09-26 02:31:24 -0700 2007-09-26 03:07:31 -0700 modified patch 20070926mBrittoBug15212b.txt text/plain 1410 mbritto SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNTc2MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTMgQEAKKzIwMDctMDktMjYgIE1heGltZSBCcml0dG8gIDxtYnJpdHRvQHBsZXlv LmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBG aXggYSB0eXBvIGVycm9yIGluIHRoZSBnb0JhY2tPckZvcndhcmQoKSBmdW5jdGlvbiA6IGNvbmZ1 c2lvbiBiZXR3ZWVuIGZvcndhcmRMaXN0Q291bnQgYW5kIGJhY2tMaXN0Q291bnQgLgorICAgICAg ICBodHRwOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTIxMgorCisgICAgICAg ICogbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZyYW1lTG9hZGVy Ojpnb0JhY2tPckZvcndhcmQpOgorCiAyMDA3LTA5LTI1ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHpl ckB3ZWJraXQub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0uCkluZGV4OiBXZWJDb3Jl L2xvYWRlci9GcmFtZUxvYWRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9sb2FkZXIvRnJh bWVMb2FkZXIuY3BwCShyZXZpc2lvbiAyNTc2MCkKKysrIFdlYkNvcmUvbG9hZGVyL0ZyYW1lTG9h ZGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTQxNCwxMSArMTQxNCwxNCBAQCB2b2lkIEZyYW1l TG9hZGVyOjpnb0JhY2tPckZvcndhcmQoaW50IGRpCiAgICAgICAgICAgICBpZiAoZm9yd2FyZExp c3RDb3VudCA+IDApIAogICAgICAgICAgICAgICAgIGl0ZW0gPSBsaXN0LT5pdGVtQXRJbmRleChm b3J3YXJkTGlzdENvdW50KTsKICAgICAgICAgfSBlbHNlIHsKLSAgICAgICAgICAgIGludCBiYWNr TGlzdENvdW50ID0gbGlzdC0+Zm9yd2FyZExpc3RDb3VudCgpOworICAgICAgICAgICAgaW50IGJh Y2tMaXN0Q291bnQgPSBsaXN0LT5iYWNrTGlzdENvdW50KCk7CiAgICAgICAgICAgICBpZiAoYmFj a0xpc3RDb3VudCA+IDApCiAgICAgICAgICAgICAgICAgaXRlbSA9IGxpc3QtPml0ZW1BdEluZGV4 KC1iYWNrTGlzdENvdW50KTsKICAgICAgICAgfQogICAgIH0KKyAgICAKKyAgICBBU1NFUlQoaXRl bSk7IC8vd2Ugc2hvdWxkIG5vdCByZWFjaCB0aGlzIGxpbmUgd2l0aCBhbiBlbXB0eSBiYWNrL2Zv cndhcmQgbGlzdAorICAgIAogICAgIGlmIChpdGVtKQogICAgICAgICBwYWdlLT5nb1RvSXRlbShp dGVtLCBGcmFtZUxvYWRUeXBlSW5kZXhlZEJhY2tGb3J3YXJkKTsKIH0K