302601 - top ten list should exclude the bookmarks manager page (about:bookmarks) and history (about:history)

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Maxwell Terpstra]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050728 Camino/0.9a2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050728 Camino/0.9a2+

If you have a bookmark to about:bookmarks (for example, so that you can open the
manager from the bookmarks toolbar), and frequently use the bookmark manager, it
may show up in the "Top Ten" list. However, this doesn't seem to be a very good
use of 1/10th of the Top Ten list :)

Really, the problem is the requirement of making a bookmark to the bookmarks
manager in order to get a button for the manager on the toolbar... Perhaps the
best way to solve this problem would be to provide a preference item (ala
Safari) for this functionality.

Reproducible: Always

Steps to Reproduce:
1. Create a bookmark for about:bookmarks
2. Open and close the bookmarks manager several times in order to increment the
bookmark's visit count (using the toolbar button and cmd-shift-h works well for
this)
3. Open the bookmarks manager one last time and look at the bookmarks included
in the "Top Ten List" group.

Actual Results:  
The bookmark for about:bookmarks appears in the "Top Ten List" bookmark group.

Expected Results:  
Ignore visits to about:bookmarks (and possibly other about: pages as well?).

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> Expected Results:  
> Ignore visits to about:bookmarks (and possibly other about: pages as well?).

Makes sense.  Putting on the 1.1 list for investigation.

Comment 2

[Nick Kreeger]

Trying something new while awaiting for download patch reviews... Taking...

Comment 3

[Wevah (Nate Weaver)]

While we're at it, it should exclude about:history, too.

Comment 4

[Jasper]

Why not all about: pages?

Comment 5

[Maxwell Terpstra]

(In reply to comment #4)
> Why not all about: pages?

about:plugins and the like /should/ show up on the list if they are accessed enough, since they're 
definately end-points.  The history and bookmarks pages, on the other hand, are usually only visited in 
order to reach another page ("non-end-points").

This reasoning however suggests that other "non-end-point" pages shouldn't be on the list either 
(portal pages and the like)...  Perhaps there should simply be an "exclude this page from Top 10" option 
for bookmarks?

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 312127 has been marked as a duplicate of this bug. ***

Comment 7

[Chris Lawson (gone)]

Tweaking summary, hopefully to catch dupes like bug 312127.

cl

Comment 8

[froodian (Ian Leue)]

Attachment: Patch

Hope I'm not stepping on your toes here Nick.  Since it was NEW and not ASSI, I figured I'd risk it.

This patch disables the visit count for about:bookmarks, about:history, and about:config.  about:blank takes care of itself.  Stuart (or anybody else), are there any other about: urls we should be disabling here?  about:plugins is sort of on the line as far as I'm concerned.

Comment 9

[Chris Lawson (gone)]

FWIW, and I don't think I was helping with QA when this was filed, I think this is a WONTFIX.

The Top Ten list should not be excluding ANY page for which a valid bookmark has been created. The fix for this is not to bookmark about:bookmarks (or about:history) if you don't want it showing up in your Top Ten.

Why anyone would be bookmarking about: URLs is beyond me, but I think we should let those folks use said bookmarks just like any other bookmark. (I think a better discussion would be of the topic "should we let people bookmark about: URLs in the first place?", but that's just me.)

If we really do want this, though, the cautions/discussion in bug 159337 would apply here too.

cl

Comment 10

[Samuel Sidler (old account; do not CC)]

(In reply to comment #9)
> Why anyone would be bookmarking about: URLs is beyond me, but I think we should
> let those folks use said bookmarks just like any other bookmark. (I think a
> better discussion would be of the topic "should we let people bookmark about:
> URLs in the first place?", but that's just me.)

Are you crazy? I mean... really. People bookmark about:bookmarks and about:history all the time and use them on their bookmark toolbar with no title. It's a quick way to access them without cluttering the toolbar. And because they bookmark those, why not allow them to bookmark other about: URIs? I might bookmark about:blank so I can quickly hide that NSFW website while at work. Who knows? But we shouldn't disallow it, in my opinion.

Comment 11

[Nick Kreeger]

Comment on attachment 224493 [details] [diff] [review]
Patch

+  if(![loadURL isEqual:@"about:bookmarks"] &&
+     ![loadURL isEqual:@"about:history"] &&
+     ![loadURL isEqual:@"about:config"]) {
+    NSEnumerator* bookmarksEnum = [BookmarkManager enumeratorForBookmarksInMap:mBookmarkURLMap matchingURL:loadURL];
+    Bookmark* curBookmark;
+    while ((curBookmark = [bookmarksEnum nextObject]))
+    {
+      [curBookmark notePageLoadedWithSuccess:successfullLoad];
+    }

Please be consitent on your curly brace placements. Since the while loop has them on the next new line, do the same on the if statement.

Comment 12

[Simon Fraser [no longer active]]

Also, I don't think we want to block notePageLoadedWithSuccess: for these bookmarks; that code might want to do other things in future. You could check for the special URLs inside of notePageLoadedWithSuccess:, or you could set a flag on a bookmark if it is for one of those urls and don't change visit counts. Or you could change the code that builds the top 10 list to ignore bookmarks with those urls.

Comment 13

[Chris Lawson (gone)]

(In reply to comment #12)
> Or you could change the code that builds the top 10 list to ignore
> bookmarks with those urls.

I like this approach. It doesn't mess with the visit counts, and if we have people crazy enough to bookmark about:config, we probably have people crazy enough to care how many times they've visited it.

cl

Comment 14

[Chris Lawson (gone)]

(In reply to comment #12)
> Or you could change the code that builds the top 10 list to ignore
> bookmarks with those urls.

While we're at it, are there other URI prefixes/protocols we should ignore if they're bookmarked when building the top 10 list?

It's possible to create bookmarks to aim:, file:, etc. Do we really want to put stuff in the top 10 that Camino doesn't even *handle*? We should check the protocol against our internal (from Gecko) list of what we know how to handle, and if we don't find it on that list, we shouldn't add it to the top 10. We should also exclude purely "internal" URIs, like about:. I haven't really decided whether file: URIs make sense in the top 10 or not. I tend to think not.

There may be others I haven't thought of, too.

cl

Comment 15

[froodian (Ian Leue)]

Attachment: Excludes about: and external protocols

This 

1. excludes all about: URIs (per IRC)
2. excludes file: URIs
3. excludes all external URIs primarily using code by cl from bug 325839. I tested it out, and it works on things like irc:// aim:// too.
4. removes a bizarre comment that didn't seem to directly apply to the following lines of code.

Comment 16

[Chris Lawson (gone)]

Comment on attachment 224636 [details] [diff] [review]
Excludes about: and external protocols

>Index: bookmarks/KindaSmartFolderManager.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/bookmarks/KindaSmartFolderManager.mm,v
>retrieving revision 1.13
>diff -u -8 -p -r1.13 KindaSmartFolderManager.mm
>--- bookmarks/KindaSmartFolderManager.mm	2 Jun 2006 18:11:17 -0000	1.13
>+++ bookmarks/KindaSmartFolderManager.mm	7 Jun 2006 00:22:18 -0000

>@@ -172,26 +182,28 @@ const unsigned kNumTop10Items = 10;   //
>     {
>       // just resort
>       [mTop10Folder sortChildrenUsingSelector:@selector(compareForTop10:sortDescending:)
>                                   reverseSort:YES
>                                      sortDeep:NO
>                                      undoable:NO];
>     }
>   }
>-  else if (visitCount >= currentMinVisits)
>+  else if (visitCount >= currentMinVisits &&
>+           ![newItemScheme isEqual:@"about"] &&
>+           ![newItemScheme isEqual:@"file"] &&
>+           GeckoUtils::isProtocolValid(newItemSchemeAsCString))

r=me with a comment inserted above the new if statement describing why we want to check for those criteria.

cl

Comment 17

[Simon Fraser [no longer active]]

I'm a little concerned about the performance impact of this. checkForNewTop10: is called on every url load if you have a bookmark for that url. With 3MB of bookmarks, that gets expensive fast.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #17)
> I'm a little concerned about the performance impact of this. checkForNewTop10:
> is called on every url load if you have a bookmark for that url. With 3MB of
> bookmarks, that gets expensive fast.

Ian, the canonical sample file for this is in bug bug 236373.

Comment 19

[froodian (Ian Leue)]

Using the 3.1 MB bookmarks.plist, launch times seemed comparable within 5 seconds when it had to rebuild the top 10 list, and the same when it didn't (duh).  I couldn't tell the difference in page load times using a second-hand timer.  If you want more accurate/delicate performance testing, I'm happy to do it, but will need some pointers.

Comment 20

[froodian (Ian Leue)]

(In reply to comment #19)
> Using the 3.1 MB bookmarks.plist, launch times seemed comparable within 5
> seconds when it had to rebuild the top 10 list

For clarification, launch times were >30 seconds for me in these cases (with or without the patch).

Comment 21

[Chris Lawson (gone)]

Comment on attachment 224636 [details] [diff] [review]
Excludes about: and external protocols

Fixing review requestee.

Comment 22

[Stuart Morgan]

Comment on attachment 224636 [details] [diff] [review]
Excludes about: and external protocols

The issue here isn't startup time, it's impact on pageload. Given that the work should only be done when a page would in fact become a new top-ten item though, I would think the performance impact of this approach should be negligible, and not really affected much by the size of the bookmarks file.

That said, this implementation does extra work on *every* pageload, rather than delaying it until it's actually necessary.

>+  NSString* newItemURL    = [aBookmark url];
>+  NSRange delimiter       = [newItemURL rangeOfString:@":"];
>+  NSString* newItemScheme = @"";
>+
>+  if (delimiter.location == NSNotFound)
>+    newItemScheme = @"file"; // implicitly file:// if no delimiter is found
>+  else
>+    newItemScheme = [newItemURL substringToIndex:delimiter.location];
>+  const char* newItemSchemeAsCString = [newItemScheme UTF8String];

None of this should be done until the inside of the |else if (visitCount >= currentMinVisits)| block, otherwise there is a string scan being done on every URL loaded, when it's only needed a small fraction of the time.

At a more general level:
- Why is file: excluded?  If I have a local page that I bookmark and visit frequently, I don't see why it shouldn't show up.
- Why even go to the trouble of dealing with schemes that aren't handled?  They will have a visit count of at most 1 (and if the creation logic for bookmarks is ever improved, 0), so it's incredibly fringe that any of them would show up in the top 10 anyway.

Basically, I think about: is the only thing worth actually considering here.

Comment 23

[froodian (Ian Leue)]

Attachment: Disables about:URIs and improves performance

Addresses all of smorgan's comments.  On further thought, it makes sense to let files into the top ten list, and there's no way external protocol would ever make it there anyway, so this just excludes about:URIs

Comment 24

[Stuart Morgan]

Comment on attachment 227026 [details] [diff] [review]
Disables about:URIs and improves performance

>+    NSRange delimiter       = [newItemURL rangeOfString:@":"];

I don't like the name |delimiter| here; ':' is the delimiter, not the NSRange. Maybe |firstColon|?

>+    NSString* newItemScheme = [newItemURL substringToIndex:delimiter.location]; //If not found, it's a file, and we don't care

If there's no ':', delimeter.location is NSNotFound, which is not safe to feed to substringToIndex:

Also, rather than the inline comment I think a comment preceding the whole block to the effect of "if it's an 'about:' bookmark, ignore it" would be better to make it clear what the point of the code is.

>+    if(![newItemScheme isEqual:@"about"]) 

Since all the other "nothing to be done" cases in this function do an early return, do that instead of the if.  So the logic becomes:

firstColon = foo
if (firstColon is found && the scheme is about)
  return

Comment 25

[froodian (Ian Leue)]

Attachment: Sylistic changes and null check

Comment 26

[Stuart Morgan]

Comment on attachment 227195 [details] [diff] [review]
Sylistic changes and null check

>+    NSRange firstColon   = [newItemURL rangeOfString:@":"];
>+    if(firstColon.location && [[newItemURL substringToIndex:firstColon.location] isEqual:@"about"])

NSNotFound isn't null--0 is a perfectly legitimate index to find something.  You need to be checking to make sure it's not NSNotFound.

Comment 27

[froodian (Ian Leue)]

Attachment: Properly checks for NSNotFound

Ah, I see.  Sorry for misunderstanding your previous comment.

Comment 28

[Stuart Morgan]

Comment on attachment 227257 [details] [diff] [review]
Properly checks for NSNotFound

r=me

Comment 29

[froodian (Ian Leue)]

Comment on attachment 227257 [details] [diff] [review]
Properly checks for NSNotFound

Simon, are you still worried about perf here?  Now that I know a little more about NSDate I can do some more careful testing, but since all the code is only called now when a bookmark has enough visit counts to get bumped onto the top ten list, I'm not sure it's an issue.

Comment 30

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227257 [details] [diff] [review]
Properly checks for NSNotFound

sr=pink

Comment 31

[Nick Kreeger]

Fixed trunk and branch.