374417 - Disable "Use as Dock menu" CM item for the History collection

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

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

The collection context menu includes a "Use as Dock menu" item, which is valid for every collection except History (since History items aren't bookmarks).  

We should disable this context menu item in the collection context menu when the History collection is selected.

Comment 1

[Chris Lawson (gone)]

...or maybe we should enable users to get at their history through the Dock menu instead?

Either way, we need to do something about it.

Comment 2

[froodian (Ian Leue)]

Attachment: Patch 1

This does comment 0.

Comment 3

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

cl, you should probably file another history/bookmarks parity bug on comment 1, because it'll be more than re-arranging a few lines of code ;)  Rearranging lines now gets us to stop misbehaving immediately.

Comment 4

[Sean Murphy]

Comment on attachment 258972 [details] [diff] [review]
Patch 1

Looks good, r=me with two details to consider (neither of them really necessary, though):

> +      if (!(aFolder == [[BookmarkManager sharedBookmarkManager] historyFolder])) {

This could be expressed as…

if (aFolder != [[BookmarkManager sharedBookmarkManager] historyFolder]) {

…which seems to make the condition slightly more obvious at first glance.  No big deal though, especially if other code in the project tends to follow the first approach; either way evaluates correctly.

In the collections contextual menu, a separator item is added whenever there are more items underneath "New Collection."  In the patch, this separator is added within the "if aFolder != historyFolder" block of code.  This causes no trouble now, since the only other candidate for addition is the "Delete" item, which is also left out for history.  My point here is that if we ever decide to introduce another contextual menu item, one that is applicable to the history collection, there won't be a separator item above it.

Comment 5

[froodian (Ian Leue)]

Attachment: r=murph patch

(In reply to comment #4)
> This could be expressed as…
> 
> if (aFolder != [[BookmarkManager sharedBookmarkManager] historyFolder]) {

Yes, that's far less insane.

> In the collections contextual menu, a separator item is added whenever there
> are more items underneath "New Collection."  In the patch, this separator is
> added within the "if aFolder != historyFolder" block of code.  This causes no
> trouble now, since the only other candidate for addition is the "Delete" item,
> which is also left out for history.  My point here is that if we ever decide to
> introduce another contextual menu item, one that is applicable to the history
> collection, there won't be a separator item above it.

I personally don't think this is really a concern at this point - if other options are added at a later point, new logic for adding the separator will make itself apparent (and relevant).  There's also plenty of precedent for adding separators programatically in this way.  I've left that logic as it is for now.

Comment 6

[Stuart Morgan]

Comment on attachment 259016 [details] [diff] [review]
r=murph patch

sr=smorgan

Comment 7

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.