Closed Bug 493557 Opened 13 years ago Closed 11 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

3.5 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- -

People

(Reporter: whimboo, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fixed-in-places])

Attachments

(1 file, 2 obsolete files)

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
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
Blocks: 484263
Flags: wanted-firefox3.6?
based on comment 1 getting this from regressionwindow-wanted list.
Marco, anything we can do here for 3.7 at least? Looks like it was falling off your radar.
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.
(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?
assigning to have it on radar, but for the next days I have higher priorities :(
Assignee: nobody → mak77
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?
blocking2.0: --- → ?
Flags: wanted-firefox3.6?
I do not think that the order of these menus blocks the release.
blocking2.0: ? → -
Blocks: 560198
Attached patch patch v1.0 (obsolete) — Splinter Review
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).
Attachment #489049 - Flags: review?(dietrich)
Status: NEW → ASSIGNED
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.
Attachment #489049 - Flags: review?(dietrich)
Attached patch patch v1.1 (obsolete) — Splinter Review
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!
Attachment #489049 - Attachment is obsolete: true
Attachment #489131 - Flags: review?(dietrich)
Flags: in-testsuite?
Summary: Entries for "Recent Tags" and "Recently Bookmarked" are flipped in Minefield for profiles created with Shiretoko → "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated.
Attachment #489131 - Flags: review?(dietrich) → review+
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).
Attachment #489131 - Flags: approval2.0?
Comment on attachment 489131 [details] [diff] [review]
patch v1.1

a+ for b9. let's not muddy the b8 waters without really good reason.
Attachment #489131 - Flags: approval2.0? → approval2.0+
Attached patch patch v1.2Splinter Review
with PlacesUtils as a lazy getter
Attachment #489131 - Attachment is obsolete: true
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
Flags: in-testsuite? → in-testsuite+
Whiteboard: [fixed-in-places]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
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.
(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).
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus+
Blocks: 743692
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.