466422 - Clicking history - show all history doesn't show any history

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Dave Townsend [:mossop]]

If I click History - Show all history then the bookmarks organiser appears but is empty. It really should pre-select the history in the tree on the left in this case.

Comment 1

[Damian Shaw [Quan]]

Confirmed on XP:

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b2pre) Gecko/20081123 Minefield/3.1b2pre ID:20081123131619

Comment 2

[Ria Klaassen (not reading all bugmail)]

Maybe this needs custom settings for I can't reproduce it on Windows XP with the latest trunk hourly with old or new profile.

Comment 3

[Damian Shaw [Quan]]

Attachment: JSON test file

I tried on a clean profile and got WFM.

I then tried restoring my old bookmarks and the bug appeared (simplified test file attached that recreates the bug for me).

I tried again on a clean profile, this time creating a bookmark naturally and the bug did not appear. So appears there is something odd with old bookmark files.

Comment 4

[Ria Klaassen (not reading all bugmail)]

With exactly always the same Steps to reproduce I found this regression range: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2008-05-06+01%3A00&maxdate=2008-05-06+23%3A00
so this could be Bug 417228.

STR:
- Start Firefox with a new profile
- Click History > Show all History
- Close History Window and Firefox
- Start Firefox
- Click Bookmarks > Organize Bookmarks > Import And Restore > Restore 2008-11-24
- Close Library window
- Click History > Show all History

Seems an annoying bug to me, for the problem appears to stay forever.

Comment 5

[Marco Bonardo [:mak]]

my thesis about this, we are restoring from json generating new left pane queries, but we are not updating annotation ORGANIZER_QUERY_ANNO correctly, so that leftPaneQueries['History'] (that we use to select item in the library) points to wrong itemId. Needs to be verified though.

Comment 6

[Dietrich Ayala (:dietrich)]

increasing priority and blocking, since this breaks that menu option forever.

Comment 7

[Marco Bonardo [:mak]]

for some reason we don't remove the old left panel folder, so when getting the anno we receive the old item...

Comment 8

[Marco Bonardo [:mak]]

well exactly we are correctly not removing the left pane folder, but we should not create a new one.

Comment 9

[Marco Bonardo [:mak]]

we are saving the left pane folder to the backup on exit when we should not, since browserglue does not know about left pane. Moreover the code that should exclude the pane does exclude it from removal, but not from creation.

The current excludeItems path for backup/restore is a bit fragile and too much interleaved between frontend and toolkit.

I think i'm going to change that using an annotation EXCLUDE_FROM_BACKUP, we should set that annotation on items we want to be excluded from the backup, and backup utils will automatically exclude items annotated with it, without having to forward an aExcludeItems param between browser functions.

I'll also add a level of protection to the left pane folder generation, so that if we find more then one left pane, we cleanup them all, and create a new correct one. This will protect us from old backups containing the left pane.

Comment 10

[Marco Bonardo [:mak]]

Attachment: wip 0.1

asking a first-pass review to evaluate the approach, i still need to fix current tests and check for their completeness.

Comment 11

[Marco Bonardo [:mak]]

reminder: when removing left pane folder change removeFolder to removeItem, we should still deprecate removeFolder

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 357171 [details] [diff] [review]
wip 0.1

cleaner than the previous method. however, how does this handle upgraders who have an un-annotated leftPane? also, how about when applications like weave sync annotated and non-annotated left panes?

Comment 13

[Marco Bonardo [:mak]]

as i said on IRC, upgraders won't suffer since we are upgrading left pane folder for anyone, while not annotated left panes will be double added, but as soon as we init the Library we will detect we have 2 left pane roots, discard them and build up a clean new one. This will only happen once though.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 357171 [details] [diff] [review]
wip 0.1

actually there's no point in not restoring all contents of a backup since is not guarantee that ids in the backup and ours are still "sync". So i'm going to also remove the changes i made to importJSONnode. We should not put anything bad in a backup since detecting bad entries on restore is really not easy/reliable.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v1.0

this implements above comments.
I've tried to make the test more complete to check we don't remove excluded items on restore.

Comment 16

[Marco Bonardo [:mak]]

reminder: fix indexes in the test to be relative values (some of them are absolute now)

Comment 17

[Marco Bonardo [:mak]]

Attachment: patch v1.1

unbitrot and fix test indexes

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 357662 [details] [diff] [review]
patch v1.1

r=me. my only complaint is the redefinition of anno consts. we should probably hang all the internally-used annotations off of PlacesUtils (and PlacesUIUtils for browser-specific ones). can you file a followup for that?

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a389302fde5f

Comment 20

[Marco Bonardo [:mak]]

(In reply to comment #18)
> (From update of attachment 357662 [details] [diff] [review])
> r=me. my only complaint is the redefinition of anno consts. we should probably
> hang all the internally-used annotations off of PlacesUtils (and PlacesUIUtils
> for browser-specific ones). can you file a followup for that?

filed Bug 474571, i'm unsure about the strategy you want to follow, so please better specify the idea in that bug.

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7139e25ffe16

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on trunk and 1.9.1 with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre

Marco, can we get an automated test here?

Comment 23

[Marco Bonardo [:mak]]

there is a test covering backup effectively i did not mark the flag.

i could probably try creating a browser chrome test that should import a backup containing a left pane library, opening the library pointing to history, and check history is selected in the left tree. Guess if that's excessive, would be far better having a test for all the library, maybe mozmill or a followup?

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now Mozmill cannot handle templetized trees. See bug 477079. So did you had any chance to check for a browser chrome test?

Comment 25

[Marco Bonardo [:mak]]

well, in comment 23 i said a wrong thing, we cannot import a backup containing a left pane folder because we don't backup the left pane folder.
with bug 422163 i've added a test for left pane migration, and with bug 484019 i've added a test for the multiple left panes case...
So this should have enough coverage atm.

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv