We can't really prevent people from making multiple bookmarks with the same URL, and they might even want that if the bookmarks are in different folders, but it looks wrong to be showing those as two separate entries in "Top Sites". I think we should combine bookmarks with the same URL in the "Top Sites" list/grid.

Nom-ing as a potential softblocker, since I think most people don't have many duplicate bookmarks (no data to back that up), but when they do, duplicate entries in Top Sites looks weird. Also, I'm concerned that there may be no good solution to bug 742381, so this would at least cover that up in the Top Sites UI.

Ian, how much does UX care about this bug?

I think this is pretty important. Duplicates in the top sites, even if technically "correct," makes us look broken and squanders a pretty important bit of real estate. Just to connect some dots, bug 691870 would help people correct the underlying problem.

(In reply to Madhava Enros [:madhava] from comment #3) > I think this is pretty important. Duplicates in the top sites, even if > technically "correct," makes us look broken and squanders a pretty important > bit of real estate. > > Just to connect some dots, bug 691870 would help people correct the > underlying problem. It depends on what the solution to that bug is (I actually just marked it as a dupe of bug 671131) - that is, if we decide to remove bookmarks as well as history records when a user chooses to remove a site, that would get rid of it entirely from top sites, but I don't think we necessarily want to be getting rid of the bookmark. Also, if it's one of these problem bookmarks that appears two places in your folder tree, we'd be deleting a random one of them, and that's no good. Other than preventing users from making multiple bookmarks with the same site (which we can't control), I think we need the UI to just combine bookmarks with the same URL into one "top sites" entry.

I can take this. I don't think there's any more information we need from UX.

Calling setDistinct(true) prevents the query from returning duplicate entries. This will still return two bookmark entries with the same URL if they have different titles, but I think that's okay. If we really only want one entry per URL, we can add a "GROUP BY url" to the query instead of calling setDistinct(true), but then the entry would randomly have the title of one of those entries (unless we add something more complicated to choose one). Ian, do you think it's important to only return one entry per URL, or are entries with different titles okay? (I know, this is a real edge case.) I will write a test for this as well, since it should be simple.

Comment on attachment 614231 [details] [diff] [review] patch Review of attachment 614231 [details] [diff] [review]: ----------------------------------------------------------------- Look good. Any performance impact with this change? Please post numbers here. I'll only r+ with tests.

Margaret, I think the behaviour you've implemented makes sense. It seems super rare, but I can see the benefit of displaying both entries (same URL, different titles), if for no other reason than that I can't think of how we could determine which is the more meaningful title to display if we were to just show one.

I have four about:home's and three about:firefox's.

I decided to add some duplicate bookmarks to the test to make sure the DISTINCT code would actually be doing something in the test. I only added 10, since this seems like something fairly uncommon, but I can up that if you think that would be better. I found that my patch seemed to have a very slight perf impact, but I don't know if this is statistically significant, especially since the majority of the runs were in the same range with and without the patch: Without patch: 390, 385, 386, 386, 408 (mean 391) With patch: 388, 395, 433, 402, 390 (mean 401.6)

This tests the functionality of adding the setDistinct(true). I found that I had to set a projection for my test queries (similar to what LocalBrowserDB does), or else the rows wouldn't be combined, since they have distinct bookmark ids (among other things).

Comment on attachment 614442 [details] [diff] [review] add a few duplicate bookmarks to testBrowserProviderPerf Review of attachment 614442 [details] [diff] [review]: ----------------------------------------------------------------- Good.

Comment on attachment 614444 [details] [diff] [review] test for duplicate bookmark combination Review of attachment 614444 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 614231 [details] [diff] [review] patch Review of attachment 614231 [details] [diff] [review]: ----------------------------------------------------------------- I'm slightly mixed about this patch due the small performance hit. Have you tried changing the Combined view to use SELECT DISTINCT when querying the bookmarks? Just wondering if using distinct on the inner queries of the view instead of on the whole view would make any difference...

(In reply to Lucas Rocha (:lucasr) from comment #14) > Comment on attachment 614231 [details] [diff] [review] > patch > > Review of attachment 614231 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm slightly mixed about this patch due the small performance hit. Have you > tried changing the Combined view to use SELECT DISTINCT when querying the > bookmarks? Just wondering if using distinct on the inner queries of the view > instead of on the whole view would make any difference... I haven't tried that, but I don't think it will work, since the view includes bookmark ids, so it would still return all the rows, since they would be unique. I could try seeing what the performance impact of a GROUP BY url would be, both inside/outside the view - we'd have the title issue I mentioned before, but I don't think it's the end of the world, since the title would still be correct for at least one of the bookmark entries.

Comment on attachment 614231 [details] [diff] [review] patch This isn't going to work anymore when the patches for bug 701330 land, since we'll also be querying for bookmark id. I'm starting to think it may not be worth it to fix this bug, since it's a pretty big edge case for people with who never used a really early version of sync.

I agree. Let's see if this becomes a problem in real world usage. Re-noming to minus.

FWIW, I've seen this in nightly builds primarily in my history with 10 or so about:home items at the very top of my history. Sounds like that's something that's resolved by this patch, but in case it's not then maybe that needs to be tracked someplace as well. I don't know if what I've seen is an artifact of sync and nightly builds or some bigger systematic problem, but after updating to aurora and starting fresh I have not seen it happen again. If it does I'll let people know...