359662 - Single-page bookmarking broken in recent nightlies

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Hayden]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20061105 Camino/1.1a1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20061105 Camino/1.1a1+

With more than one tab open, the option to "Bookmark Current Page" is shaded out in recent nightly builds. I hope this is a bug and not a feature. The option to "Bookmark Current Tabs as Tab Group" still works.

Reproducible: Always

Steps to Reproduce:
1. Open more than one tab


Expected Results:  
User should be able to bookmark current page regardless of how many tabs are open.

Comment 1

[philippe (part-time)]

Regressed between
Version 2006102622 (1.1a1+) OK 
Version 2006102822 (1.1a1+) NO

Comment 2

[philippe (part-time)]

On trunk:
Version 2006102722 (1.2+) fails
Version 2006102701 (1.2+) works

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Camino&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-10-26+16%3A00%3A01&maxdate=2006-10-27+22%3A00%3A01&cvsroot=%2Fcvsroot

Bug 159337 or bug 343767 ?

Comment 3

[froodian (Ian Leue)]

As a workaround, you can still Cmd-D, or change focus to the finder and change back to Camino.  The item will then be enabled for that tab (for as long as it's open, AFAICS).

At first glance, I have no idea what caused this (there are some other bugs in that query that could well be the culprit too).  Then again, maybe after I've been awake for more than 15 minutes it'll all be clear.  I'll look deeper, but eyeballs on the problem would be a good thing.

Comment 4

[froodian (Ian Leue)]

This was broken by the patch portion of bug 159337.  I'll try to track down more as I go.  Stuart, any ideas?

Comment 5

[Stuart Morgan]

IIRC the bookmark menu doesn't auto-validate, and this looks like we just aren't triggering the validation at the right times (you'll see the same thing if you open several tabs then close all but one; the multi-bookmark stays enabled).  I think it's only appearing to be a regression because there was never any validation of that menu item; before the consolidation only the toolbar item was being validated.

Comment 6

[froodian (Ian Leue)]

(In reply to comment #5)
> (you'll see the same thing
> if you open several tabs then close all but one; the multi-bookmark stays
> enabled).

FWIW, this didn't happen before the consolidation either.

Comment 7

[Eiichi]

Just a reference;

1) Open a new tab.
2) Go back to the previous tab or close the new tab.
3) Then it works.

Comment 8

[froodian (Ian Leue)]

Attachment: Patch

Properly validates bookmark menu items.  Note that I had to factor out the heart of addBookmark and create |addTabGroup| so that I could validate the "add tab group" menu item using the central method.

Comment 9

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

This just changes the target of the "add tab group" menu items to the new method.

Comment 10

[Stuart Morgan]

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

This leaves bookmark menu validation in a weird half-and-half scheme, where some of the non-bookmark items are updated when it's shown and some are updated on window changes (and some are validated both, which is definitely not what we want).

I think we need to either decide that the whole menu is going to be validated when it's shown, in which case we call doBookmarksMenuEnabling from the menu event handler and remove all calls to delayedAdjustBookmarksMenuItemsEnabling, or we make sure that delayedAdjustBookmarksMenuItemsEnabling is called everywhere we need it to be.

My feeling is that the former is better.  The calls to delayedAdjustBookmarksMenuItemsEnabling from the various tab handlers seems uglier and more error prone, and I can't imagine there's more than negligible added cost for validating a few more items when it's shown.

Comment 11

[Simon Fraser [no longer active]]

(In reply to comment #10)

> I think we need to either decide that the whole menu is going to be validated
> when it's shown,

Does this also cover the appropriate enabling of keyboard shortcuts?

Comment 12

[Stuart Morgan]

(In reply to comment #11)
> Does this also cover the appropriate enabling of keyboard shortcuts?

Sigh. I knew there must be a reason it wasn't that easy.

So we'll need to add more calls to update the menu when the number of tabs changes.

Comment 13

[froodian (Ian Leue)]

Attachment: Patch

We don't just need it to update when the number of tabs changes, but at every page load (so addBookmark and addBookmarkWithoutPrompt can validate properly).  So, this patch:

- Still creates addTabGroup
- Adds a delayedAdjustBookmarksMenuItemsEnabling call every time we finish loading a page (BrowserWrapper portion is in |onLoadingCompleted|)
- Consolidates tab-closing code
- Validates Cmd-D too
- Removes the custom enabling calls |doBookmarksMenuEnabling| - they're redundant.

Comment 14

[froodian (Ian Leue)]

Attachment: This is the bitrot police (MainMenu.nib)

Comment 15

[Stuart Morgan]

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

>Index: src/browser/BrowserWrapper.mm
>... 
>+  // Validate the bookmark menu items
>+  [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling];

I haven't had time to do a full review yet, but shouldn't this be done in BWC?

Comment 16

[froodian (Ian Leue)]

Attachment: Patch

Yeah.

Comment 17

[Stuart Morgan]

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

Looks good, r=me with the minor changes below.

>+  else if (!mClosingWindow) {
>       [[self window] close];
>+      [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling];
>   }

Indentiation is off here.

>+    if (tabViewItem) {
>+      [self closeTab:tabViewItem];
>     }

Remove the braces.

Lastly, please update the comment on doBookmarksMenuEnabling, since the part about when it needs to be called is now inaccurate.

I am concerned with how fragile this system is. Did we get all the places we needed to? Will we forget something later? We should file a bug for post-1.1 to see if we can come up with a new scheme for bookmark menu maintenance.

Comment 18

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 19

[froodian (Ian Leue)]

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

Comment 20

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 248740 [details] [diff] [review]
r=smorgan patch

sr=pink

Comment 21

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.  Followup is bug 364252.