Last Comment Bug 677539 - Bookmarking group of tabs greyed out after restart with session restore
: Bookmarking group of tabs greyed out after restart with session restore
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.5
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
: 670509 (view as bug list)
Depends on: 682531
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-09 07:42 PDT by Martin F.
Modified: 2011-08-28 12:06 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
patch (7.36 KB, patch)
2011-08-12 16:38 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v1a [Checkin: comments 10 and 13] (8.54 KB, patch)
2011-08-12 16:50 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
bugspam.Callek: approval‑comm‑aurora+
bugspam.Callek: approval‑comm‑beta-
Details | Diff | Splinter Review

Description Martin F. 2011-08-09 07:42:09 PDT
When starting Seamonkey with at least 2 windows and 2 tabs in each window by using the option to restore the previous session, the menu entry to bookmark the group of tabs is greyed out from the second window onwards. (e.g. having 5 Windows, each containing several tabs, there are at least 4 windows which have this option greyed out.
When opening another window with new tabs, the option is available/enabled in that window. The "old" windows remain as they are. 
Quitting SM by Ctrl+Q and restarting with that new window in addition to the old makes makes it behaving like the old ones of course.

Illustration of the issue in a localized Seamonkey 2.2 http://img718.imageshack.us/img718/1752/gruppetabslesezeichen.png

Hartmut Figge confirmed issue for SM2.5 Nighty in d.c.s.m.browser <4E4094CD.90009@hfigge.myfqdn.de>

steps to reproduce:
- use option to restore session in browser preferences as starting-page
- open more than one window with more than one tab in each window
- quit SM (e.g. by pressing Ctrl+Q) and restart
- try to bookmark group of tabs in any window except the first one

expected result:
- Bookmarking tab group is enabled as usual

actual result:
- Bookmarking tab group is disabled
Comment 1 therube 2011-08-09 08:30:55 PDT
Confirmed, -safe-mode.

Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20110806 Firefox/6.0 SeaMonkey/2.3
Comment 2 therube 2011-08-09 08:35:48 PDT
Without looking too closely, appears that this has been an issue since SeaMonkey 2.1.

SeaMonkey 2.0.14 is not affected.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-08-09 14:57:15 PDT
*** Bug 670509 has been marked as a duplicate of this bug. ***
Comment 4 Jens Hatlak (:InvisibleSmiley) 2011-08-09 15:16:04 PDT
FWIW, the command is Browser:BookmarkAllTabs (navigator.xul and/or navigatorOverlay.xul). From navigatorOverlay.xul:

     <!-- The command is disabled for the hidden window. Otherwise its enabled
          state is handled by gBookmarkAllTabsHandler. -->
     <command id="Browser:BookmarkAllTabs"
              label="&addCurTabsAsCmd.label;" accesskey="&addCurTabsAsCmd.accesskey;"
              oncommand="gBookmarkAllTabsHandler.doCommand();"
              disabled="true"/>

gBookmarkAllTabsHandler is defined in navigator.js, reacting to tabOpen and tabClose events (and setting the disabled attribute to false in case of a tabOpen event with current tab count == 2). I guess in the case of Session Restore, the event listeners are added too late so the events are not received/processed by gBookmarkAllTabsHandler and subsequently the disabled state of the menuitem is not changed.

Misak, Neil, what can we do about this? [I guess FF doesn't have a case for this; they don't have that menuitem in the menu anymore, only in tab context menus.]
Comment 5 neil@parkwaycc.co.uk 2011-08-09 16:46:32 PDT
One option would be to stop using observer notifications to track windows.
Comment 6 Jens Hatlak (:InvisibleSmiley) 2011-08-09 23:22:01 PDT
Maybe we should simply attach a trigger to the Bookmarks menu/popup (don't know if that's what Neil meant in comment 5).
Comment 7 Jens Hatlak (:InvisibleSmiley) 2011-08-12 16:38:31 PDT
Created attachment 552794 [details] [diff] [review]
patch

I'll give this a try...

Note: The tabbrowser updating is already fine since the updatePopupMenu method takes care of disabling all tbattr=tabbrowser-multiple items if there is only one tab.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-08-12 16:50:51 PDT
Created attachment 552796 [details] [diff] [review]
patch v1a [Checkin: comments 10 and 13]

Bah, forgot the Bookmarks button menu (on the Personal Toolbar).

Note: Using any tab's context menu serves as a workaround until this bug gets fixed (of course that doesn't bring the shortcut back to life; one could bind a call to PlacesCommandHook.bookmarkCurrentPages() to a shortcut using KeyConfig, though.
Comment 9 neil@parkwaycc.co.uk 2011-08-13 03:06:00 PDT
Comment on attachment 552796 [details] [diff] [review]
patch v1a [Checkin: comments 10 and 13]

I have this vague memory that KaiRo wanted the gBookmarkAllTabsHandler for compatibility with Firefox. And then they went and removed it...
Comment 10 Jens Hatlak (:InvisibleSmiley) 2011-08-13 03:44:06 PDT
Comment on attachment 552796 [details] [diff] [review]
patch v1a [Checkin: comments 10 and 13]

http://hg.mozilla.org/comm-central/rev/b76dac043e0f
Comment 11 Jens Hatlak (:InvisibleSmiley) 2011-08-13 10:56:33 PDT
Comment on attachment 552796 [details] [diff] [review]
patch v1a [Checkin: comments 10 and 13]

Just in case we still accept low risk regression fixes for 2.3, I'll request comm-beta approval, too.
Comment 12 Justin Wood (:Callek) (Away until Aug 29) 2011-08-13 11:52:21 PDT
Comment on attachment 552796 [details] [diff] [review]
patch v1a [Checkin: comments 10 and 13]

sorry too late for 2.3
Comment 13 Jens Hatlak (:InvisibleSmiley) 2011-08-13 13:42:23 PDT
Comment on attachment 552796 [details] [diff] [review]
patch v1a [Checkin: comments 10 and 13]

http://hg.mozilla.org/releases/comm-aurora/rev/271501c7d9a2

Note You need to log in before you can comment on or make changes to this bug.