382397 - fix nsNavHistoryContainerResultNode::ReverseUpdateStats() to not notify unsorted views

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[(not reading, please use [e-mail] instead)]

fix nsNavHistoryContainerResultNode::ReverseUpdateStats() to not notify unsorted views

spun off from bug #379591

mano wrote, about itemChanged(), how can aNode.parent be null?

Here's how it happens when updating livemarks:
 
     places.dll!nsNavHistoryContainerResultNode::ReverseUpdateStats(int aAccessCountChange=-1)  Line 530    C++
     places.dll!nsNavHistoryContainerResultNode::RemoveChildAt(int aIndex=0, int aIsTemporary=0)  Line 1375    C++
     places.dll!nsNavHistoryFolderResultNode::OnItemRemoved(__int64 aItemId=53, __int64 aParentFolder=11, int aIndex=0)  Line 3130    C++
     places.dll!nsNavHistoryResult::OnItemRemoved(__int64 aItemId=53, __int64 aFolder=11, int aIndex=0)  Line 3718 + 0x8e bytes    C++
     places.dll!nsNavBookmarks::RemoveItem(__int64 aItemId=53)  Line 968 + 0xa7 bytes    C++
     places.dll!nsNavBookmarks::RemoveFolderChildren(__int64 aFolder=11)  Line 1356 + 0x21 bytes    C++

The livemark container (on the personal toolbar folder) has a mParent, and it is the toolbar folder (itemId  = 4)

The personal toolbar folder's parent is null.

nsNavHistoryContainerResultNode::ReverseUpdateStats() calls ItemChanged() with the mParent (so the personal toolbar folder),
so aNode.parent in toolbar.xml's itemChanged() is is null.

A couple of questions / comments:

a)  should the toolbar folder's parent be null, or should it be the root?

b)  for the toolbar.xml view, we are not not sorted.  so we don't need the back end to notify the view in nsNavHistoryContainerResultNode::ReverseUpdateStats(). 
perhaps part of the fix here is to fix is to not call ItemChanged() on the view if our sort type is SORT_BY_NONE.

Comment 1

[Asaf Romano (gone)]

a) we query to the toolbar folder directly it's supposed to be null.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

from what i see in actual code ReverseUpdateStats notifies the view only if mParent is valid, and a time or visit_count resorting has happened.
Looks like this bug, as it is, is no more valid.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I misread this as a problem with mParent, but we notify itemChanged(mParent) and we practically use mParent->mParent. Sorry for bad interpretation.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this should be fixed by bug 498130, the code changed a bit, and we check mParent->mParent in ReverseUpdateStats

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

no more fixed due to backout

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

relanded

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

on 1.9.2 too