493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre ID:20090517031025

For profiles created with Shiretoko the entries "Recent Tags" and "Recently Bookmarked" inside the Bookmarks menu are flipped. Probably it could be a regression from bug 428690 or bug 484263. I'll investigate.

Steps:
1. Create a fresh profile with Shiretoko
2. Start with this profile and shutdown immediately
3. Start Minefield and observer Bookmarks menu

Comment 1

[Marco Bonardo [:mak]]

probably bug 484263, won't be a problem for 3.5 since we don't upgrade smart bookmarks, should look into it for 3.6 though

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

based on comment 1 getting this from regressionwindow-wanted list.

Comment 3

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

Marco, anything we can do here for 3.7 at least? Looks like it was falling off your radar.

Comment 4

[Marco Bonardo [:mak]]

hm, i'm unsure we can do much now that they have been inverted in 3.6, we will just think the user inverted them and avoid touching them. btw will look into it next week, nag me in case.

Comment 5

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

(In reply to comment #4)
> hm, i'm unsure we can do much now that they have been inverted in 3.6, we will
> just think the user inverted them and avoid touching them. btw will look into
> it next week, nag me in case.

Any update on this Marco?

Comment 6

[Marco Bonardo [:mak]]

assigning to have it on radar, but for the next days I have higher priorities :(

Comment 7

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

We get more and more Litmus test failures on the Bookmarks menu test. Even you don't have time to fix this bug, do we have an idea what we wanna do here?

Comment 8

[Shawn Wilsher :sdwilsh]

I do not think that the order of these menus blocks the release.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I agree that this doesn't block, it would be cool to have it fixed though, since as it is it's going to mess up the bookmarks menu ordering for some user, and it's broken from 3.5 on (as Henrik said this generates false litmus failures).

So, in case Dietrich has any spare time for a review, this should not take a lot of time, since all of the touched code paths are covered by the test.

Actually, looking at this I found Sync bug 610501, so my investigation was not completely out of place :)

This patch applies on top of bug 574514, since it is making PlacesUtils a lazyGetter in the bg scope (that's the only dependency).

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 489049 [details] [diff] [review]
patch v1.0

hm, there is possibly another subtle bug in this old code, so I'm suspending the review request till I have a test for it.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Luckily I was just too picky and code was better than me, the patch is fine, I just added another test, better test coverage is a win!

Comment 12

[Marco Bonardo [:mak]]

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

ehr, needs approval too

and since bug 574514 is still waiting review, I could just import the PlacesUtils lazygetter change (it's trivial).

Comment 13

[Dietrich Ayala (:dietrich)]

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

a+ for b9. let's not muddy the b8 waters without really good reason.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.2

with PlacesUtils as a lazy getter

Comment 15

[Marco Bonardo [:mak]]

I've decided to push to Places branch for some reasons:
1. we will merge for sure after b8
2. I'm re-enabling a test that was random on central (on Windows), I want to check what happens
3. I need to push something to check some talos noise, pushing something useful is better

http://hg.mozilla.org/projects/places/rev/6cdfd382478d

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> http://hg.mozilla.org/projects/places/rev/6cdfd382478d

http://hg.mozilla.org/mozilla-central/rev/6cdfd382478d

Comment 17

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

Checked with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20101217 Firefox/4.0b9pre ID:20101217030324 and it looks ok, even not all paths have been caught. That means users who have upgraded from 3.5 to 3.6 before will still have the wrong order of the menu items with Minefield.

Marco, not sure if it is worth fixing the remaining issue. But would be great to get feedback from you.

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> Checked with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre)
> Gecko/20101217 Firefox/4.0b9pre ID:20101217030324 and it looks ok, even not all
> paths have been caught. That means users who have upgraded from 3.5 to 3.6
> before will still have the wrong order of the menu items with Minefield.

Sorry, but there is no way to recognize a wrong movement made by us from a wanted movement made by the user... This means it would be really really hard (or impossible) to fix that remaining issue (we should special case the specific fact those 2 are in the right place but inverted, but that's not worth it imo).

Comment 19

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

Sounds fine with me. Marking as verified fixed based on my last testing. Also updated the Litmus test (10033) and removed the known issue from the 4.0 branch.