Closed Bug 333765 Opened 19 years ago Closed 18 years ago

Opening a bookmark in a new tab no longer works when pushing Command after opening a bookmark folder, only before

Categories

(Camino Graveyard :: Toolbars & Menus, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: davedit, Assigned: froodian)

References

Details

(Keywords: fixed1.8.1, regression)

Attachments

(1 file, 6 obsolete files)

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

I believe this regressed somewhere along the way, but I'm not sure where. Basically, in past builds, you could open a bookmark folder (toolbar or menu), then push Command, and choose a bookmark and it would open a new tab. In recent nightlies (both trunk and branch), it requires you to push Command BEFORE you open a bookmark folder for it to open a new tab. If you push Command after opening a bookmark folder, it opens the bookmark in the current tab. 

Reproducible: Always

Steps to Reproduce:
1. Click open a bookmark folder
2. Push Command
3. Select a bookmark

Actual Results:  
Bookmark opened in current tab

Expected Results:  
Bookmark should have opened in a new tab
Keywords: regression
I thought this was what bug 317214 was all about.
Yeah I'm not sure. I know the behavior I'm describing used to work in my 2/5 build, so I dunno if the bugs are describing exactly the same thing.
This also sounds similar to the problems we ran into in bug 186591.

In fact, this may be the same bug as bug 186591 comment 14.

cl
This regressed somewhere between the 03.27 and 04.01 official nightly Trunk build.
Meaning it works as expected in the 03.27 build, but are broken in the 04.01 build. There are no 03.28, 29, 30 or 31 trunk build, so I can not track it down more accurate.

"Checkins to directory /mozilla/camino between 2006-03-27 12:00 and 2006-04-01 12:00:" http://tinyurl.com/fqwor

Regression from bug 325939 maybe?
Is it a problem in the 1.8UniExperimental builds, too?  Can you get a better window using them?
(In reply to comment #5)
> Is it a problem in the 1.8UniExperimental builds, too?  Can you get a better
> window using them?

Yes, its a problem in the 1.8UniExperimental builds as well.
My test show that it regressed in the 04.01 build, while it work as expected in the 03.31 build.
1.8Uni window: http://tinyurl.com/fqs3k

That pretty well fingers Chris's patch for bug 320746.  

I don't have a current build, so I can't confirm the bug, so someone with one should check it and then thump the patch author and reviewers ;)
Assignee: mikepinkerton → nobody
Component: Bookmarks → Toolbars & Menus
QA Contact: bookmarks → toolbars
Confirmed, promoting it to NEW.  Apparently, [NSApp currentEvent] isn't giving us events corresponding to the modifier key going down.  While tracking the menu, it's probably just giving the event that caused the menu to open.

What was it that we were saying about alternates?  :P
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Camino1.1
Bug 186591 landed using the same "buggy" method that's problematic here (better to get the code in than let it bitrot until we come up with a solution for this), so when fixing this, be sure to fix the case(s) there, too.
Bump, this is a really annoying regression, it's the only reason I saty away from any trunk builds. Would someone please take a look?
Alternates' shift key mask can't be different, FWIW.
Apparently I was wrong; I just tested this. You just can't set it to be shift in IB; you can programatically.

Also, we can use alternates here (barring memory concerns), we just have to make sure that the "normal" items have their modifier mask cleared since menu items have NSCommandKeyMask by default (as froodian also discovered, IIRC).
Assignee: nobody → stridey
Attached patch Patch (obsolete) — Splinter Review
Well, I've got a patch, and I'm pretty sure that I didn't break anything new.  That said, this could use some serious QA lovin', since it directly affects every bookmark load (Bookmark bar, context menus, bookmark manager, everything).  Let me repeat that.

This could use some serious QS lovin'.

I also fixed bug 186591 for the command key modifier, but since that contextual menu is made in IB, the only way to fix the shift key modifier is to port the whole damn thing to code.  I'll file a seperate bug for that.

Something to think about: Now that we're using alternate menu items, it would be arbitrarily easy to change the text when shift/comman is held down (for bookmark bar menus, and all bookmark context menus).  Do we want to?  My first reaction is no, since "open in new tab in background" is a mouthful.
Attachment #227126 - Flags: review?(hwaara)
Status: NEW → ASSIGNED
Attached file New BrowserWindow.nib (obsolete) —
This just adds an alternate menu item for "View image in new tab."

Oh.  Now that I think about it, that's bad, isn't it (since not everybody uses tabs).  Maybe the only solution is to port that context menu entirely to code before fixing any of that.
No longer blocks: 186591
Attached patch Patch (obsolete) — Splinter Review
The view image issue is now bug 342780.
Attachment #227126 - Attachment is obsolete: true
Attachment #227127 - Attachment is obsolete: true
Attachment #227132 - Flags: review?(hwaara)
Attachment #227126 - Flags: review?(hwaara)
Comment on attachment 227132 [details] [diff] [review]
Patch

Review with froodian on IRC. 

Except for code changes, needs to be perf-checked (see bug 228840 which is more or less the same, unfortunately)
Attachment #227132 - Flags: review?(hwaara) → review-
Oops, wrong bug. the bug that is duplicate of this is (still, see comment 1) bug 317214.
OK.  I did some perf testing on this.  I launched Camino (with the 3.1MB bookmarks file), opened the biggest bookmark bar menu I could find and logged the time it took to run -rebuildMenuIncludingSubmenus.

After ten runs, the current implementation has a minimum time of .075013 seconds and a maximum of .250750, with an average of .136014.

After ten runs, my implementation has a minimum time of .105287, a maximum of .334555, and an average time of .231449.

Simon: is this perf impact ok?
Attached patch Addresses hwaara's IRC comments (obsolete) — Splinter Review
This addresses the review hwaara and I did on IRC.

hwaara: I'm still moving the [self addItem:menuItem] message in BookmarkMenu (even though it means it needs multiple instances) because it needs to be added before the alternate menu items for them to be set as alternates.

Also, I didn't integrate your middle-click changes, but I did use eBookmarkOpenBehavior_NewPreferred, so you just need to call loadBookmark with that behavior on middle click.
Attachment #227132 - Attachment is obsolete: true
Attachment #227291 - Flags: review?(hwaara)
I'm still not keen on making 2 bookmark items for every single bookmark. Isn't there a smarter way to fix this? Maybe get the event modifiers again when handling the action, rather than using [NSApp currentEvent] which was left over from before tracking started?
Simon, could you elaborate on that?  [NSApp currentEvent] is the only way I know of getting the event modifiers...
(In reply to comment #21)
> Simon, could you elaborate on that?  [NSApp currentEvent] is the only way I
> know of getting the event modifiers...

You can get them via a carbon call (see bug 320746 for discussion).
How much memory or performance degradation are we talking here for using alternates?  I don't really like the idea of mixing Carbon calls up into Cocoa code, especially when the Carbon code is dependent on state maintained by other Carbon code, and the future of the underlying implementations is unclear.  The case we're hitting here is exactly the case that alternates are intended to handle.  Like I've said before, I'll happily shut up if Apple's made a promise to maintain GetCurrentEventKeyModifiers compatibility in Cocoa.  On the other hand, I'll cautiously acquiesce if someone shows that alternates bring a huge memory/performance cost.
We're building twice as many menu items for the bookmarks. I think if we were to change the appearance of the menu items when you hold a key down then alternates would make sense. For what we're doing, I think a cheaper (if slightly ugly) fix is fine.
Attached patch Uses carbon call (obsolete) — Splinter Review
This essentially just backs out the loadBookmarks portion of cl's patch in bug 320746.
Attachment #227854 - Flags: review?(hwaara)
Isn't the other reason why we moved from that call was that the call doesn't work with various mice?
(In reply to comment #26)
> Isn't the other reason why we moved from that call was that the call doesn't
> work with various mice?

Yes. Wevah and I were talking about this on IRC the other day. As far as anyone can tell thus far, here's the dilemma:

Either we break third-party mice by using the Carbon call, or we combine the Carbon call with a Cocoa call and by doing so, we break the user's ability to hold down Command initially and then change his mind and release Command (which should then load the bookmark in the same tab/window).

I don't see any way to do this without breaking one or the other.

cl
(In reply to comment #27)
> I don't see any way to do this without breaking one or the other.

Hmm... maybe we could use alternate menu items instead? ;)
i don't follow, what's the issue with breaking 3rd party mice?
Attachment #227291 - Flags: review?(hwaara)
Comment on attachment 227854 [details] [diff] [review]
Uses carbon call

Clearing review flags because it's unfair to hwaara to leave them both on r? and it doesn't look like anything is getting decided here.

The choices in front of us are
a) Use an arguably evil carbon call that breaks some third party mice

b) Use alternate menu items, which multiplies the amount of memory used

I agree that neither solution is ideal, but frankly, what we have right now blows.  We have to do something.  Now we just need decision-making people to make decisions.

Personally, I'm in favor of alternates, but just because they were more fun to code, and that's a bad reason. ;)
Attachment #227854 - Flags: review?(hwaara)
IMHO, if we have to choose, third-party mice > people having strang cmd-toggling behavior.

Alternate menuitems would fix all use cases though, so even if it ain't pretty, I think we should do that as long as we can prove that it won't hurt any performance.
Blocks: 317952
Blocks: 290212
Mike, Simon, Mark: 

In addition to being a really annoying (dataloss in some cases) regression, this bug is blocking an increasingly-large number of our other menu-related fixes for 1.1.  

Can the three of you please make a decision soon on which method of fixing to take (and then r or sr the patch for that method) so we can keep moving?
I think that we should try this with alternates first.  If perf totally sucks, we can back off of that and use the Carbon call (provided it'll work in Leopard) and hope that we won't get toasted by it in a subsequent release.
Attachment #227291 - Flags: review?(stuart.morgan)
Attachment #227854 - Attachment is obsolete: true
Attached patch Unbitrots it (obsolete) — Splinter Review
This is the bitrot police.
Attachment #227291 - Attachment is obsolete: true
Attachment #234547 - Flags: review?(hwaara)
Attachment #227291 - Flags: review?(stuart.morgan)
Comment on attachment 234547 [details] [diff] [review]
Unbitrots it

>Index: src/application/MainController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/application/MainController.mm,v
>retrieving revision 1.195
>diff -u -8 -r1.195 MainController.mm
>--- src/application/MainController.mm	15 Aug 2006 15:35:20 -0000	1.195
>+++ src/application/MainController.mm	19 Aug 2006 05:13:15 -0000
>@@ -489,58 +489,57 @@
>   [self updateDockMenuBookmarkFolder];
> }
> 
> - (void)updateDockMenuBookmarkFolder
> {
>   [mDockMenu setBookmarkFolder:[[BookmarkManager sharedBookmarkManager] dockMenuFolder]];
> }
> 
>-// a central place for bookmark opening logic.
>-- (void)loadBookmark:(BookmarkItem*)item withWindowController:(BrowserWindowController*)browserWindowController openBehavior:(EBookmarkOpenBehavior)behavior
>+// a central place for bookmark opening logic
>+- (void)loadBookmark:(BookmarkItem*)item
>+             withBWC:(BrowserWindowController*)browserWindowController
>+        openBehavior:(EBookmarkOpenBehavior)behavior
>+     reverseBgToggle:(BOOL)reverseBackgroundPref
> {
>   if (!browserWindowController)
>     browserWindowController = [self getMainWindowBrowserController];
>   
>-  BOOL openInNewWindow = (browserWindowController == nil);
>+  BOOL openInNewWindow = !browserWindowController;
>   BOOL openInNewTab = NO;
>   BOOL newTabInBackground = NO;
>-  
>+
>   BOOL loadNewTabsInBackgroundPref = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];

I don't see this change as an improvement, please refrain from changing unless it helps readability.

>@@ -1223,17 +1222,22 @@
>     [[BookmarkManager sharedBookmarkManager] writeHTMLFile:[savePanel filename]];
>   else
>     [[BookmarkManager sharedBookmarkManager] writeSafariFile:[savePanel filename]];
> }
> 
> -(IBAction) openMenuBookmark:(id)aSender
> {
>   BookmarkItem*  item = [aSender representedObject];
>-  [self loadBookmark:item withWindowController:[self getMainWindowBrowserController] openBehavior:eBookmarkOpenBehavior_Preferred];
>+  BOOL reverseBackgroundPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) ? YES : NO;

skip the ? and what comes after it. That goes for all such checks that are in the patch.

The bitwise operation will already be zero or non-zero and thus works like a boolean check. YES is probably defined to 1 and NO to 0 in objc.h, anyway.

>+  EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
>+  if ([aSender keyEquivalentModifierMask] & NSCommandKeyMask)
>+    openBehavior = eBookmarkOpenBehavior_NewPreferred;

You can wrap places where you have alternate menuitem-specific code inside if ([aSender isAlternate]) blocks, so it's clear that they are special cases and seldom executed.


>@@ -187,24 +187,50 @@
> {
>   NSString *title = [[inItem title] stringByTruncatingTo:MENU_TRUNCATION_CHARS at:kTruncateAtMiddle];
> 
>   NSMenuItem *menuItem = nil;
>   if ([inItem isKindOfClass:[Bookmark class]])
>   {
>     if (![(Bookmark *)inItem isSeparator])  // normal bookmark
>     {
>-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>+      menuItem                     = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>+      NSMenuItem *cmdMenuItem      = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>+      NSMenuItem *cmdShiftMenuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];

I don't think it was ever discussed to also have a cmd-shift alternate, but I suppose we'll have to for consistency... Let's just hope it won't be the straw that broke the camel's back performance wise...

>+
>       [menuItem setTarget:[NSApp delegate]];
>       [menuItem setAction:@selector(openMenuBookmark:)];
>       [menuItem setImage:[inItem icon]];
>+      [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask
>+
>+      [cmdMenuItem setTarget:[NSApp delegate]];
>+      [cmdMenuItem setAction:@selector(openMenuBookmark:)];
>+      [cmdMenuItem setImage:[inItem icon]];
>+      [cmdMenuItem setKeyEquivalentModifierMask:NSCommandKeyMask];
>+      [cmdMenuItem setRepresentedObject:inItem];
>+
>+      [cmdShiftMenuItem setTarget:[NSApp delegate]];
>+      [cmdShiftMenuItem setAction:@selector(openMenuBookmark:)];
>+      [cmdShiftMenuItem setImage:[inItem icon]];
>+      [cmdShiftMenuItem setKeyEquivalentModifierMask:NSCommandKeyMask | NSShiftKeyMask];
>+      [cmdShiftMenuItem setRepresentedObject:inItem];
>+
>+      [self addItem:menuItem];
>+      [self addItem:cmdMenuItem];
>+      [self addItem:cmdShiftMenuItem];
>+
>+      [cmdMenuItem      setAlternate:YES];
>+      [cmdShiftMenuItem setAlternate:YES];
>+      [cmdMenuItem      release];
>+      [cmdShiftMenuItem release];

Move the setAlternate: call before the addItem: call. It makes sense to setup things before you add them to the menu. After adding, the menu will retain the items, and that's why it's natural to release them right after that.

>     }
>     else    //separator
>     {
>       menuItem = [[NSMenuItem separatorItem] retain];
>+      [self addItem:menuItem];
>     }
>   }
>   else if ([inItem isKindOfClass:[BookmarkFolder class]])
>   {
>     BookmarkFolder* curFolder = (BookmarkFolder*)inItem;
>     if (![curFolder isGroup])     // normal folder
>     {
>       menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>@@ -220,20 +246,21 @@
>     }
>     else // group
>     { 
>       menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>       [menuItem setTarget:[NSApp delegate]];
>       [menuItem setAction:@selector(openMenuBookmark:)];
>       [menuItem setImage:[inItem icon]];      
>     }
>+
>+    [self addItem:menuItem];
>   }
> 
>   [menuItem setRepresentedObject:inItem];
>-  [self addItem:menuItem];
>   [menuItem release];
> }
> 
> 
> - (void)addLastItems
> {
>   // add the "Open In Tabs" option to open all items in this subfolder
>   if (mAppendTabsItem && [[mFolder childURLs] count] > 0)
>Index: src/bookmarks/BookmarkButton.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkButton.mm,v
>retrieving revision 1.23
>diff -u -8 -r1.23 BookmarkButton.mm
>--- src/bookmarks/BookmarkButton.mm	16 May 2006 08:50:59 -0000	1.23
>+++ src/bookmarks/BookmarkButton.mm	19 Aug 2006 05:13:16 -0000
>@@ -160,34 +160,40 @@
> {
>   return mItem;
> }
> 
> -(IBAction)openBookmark:(id)aSender
> {
>   BrowserWindowController* brController = [[self window] windowController];
>   BookmarkItem *item = [self bookmarkItem];
>+  BOOL reverseBGPref = (([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0);
>+  EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
>+  if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
>+    openBehavior = eBookmarkOpenBehavior_NewPreferred;

Wasn't this what we wanted to get rid of in the first place? :)

>Index: src/bookmarks/BookmarkManager.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkManager.mm,v
>retrieving revision 1.62
>diff -u -8 -r1.62 BookmarkManager.mm
>--- src/bookmarks/BookmarkManager.mm	2 Jun 2006 18:11:17 -0000	1.62
>+++ src/bookmarks/BookmarkManager.mm	19 Aug 2006 05:13:17 -0000
>@@ -729,35 +729,58 @@
>     menuTitle = NSLocalizedString(@"Open Tabs in New Window", @"");
>   else if (multipleItems)
>     menuTitle = NSLocalizedString(@"Open in New Windows", @"");
>   else
>     menuTitle = NSLocalizedString(@"Open in New Window", @"");
> 
>   NSMenuItem *menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) keyEquivalent:@""] autorelease];
>   [menuItem setTarget:target];
>+  [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask
>   [contextMenu addItem:menuItem];
>-  
>+
>+  NSMenuItem *shiftMenuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) keyEquivalent:@""] autorelease];
>+  [shiftMenuItem setTarget:target];
>+  [menuItem setKeyEquivalentModifierMask:NSShiftKeyMask];

Is this a bug? You set the |menuItem| keyequiv in the block that sets up the |shiftMenuItem|.

>+  [shiftMenuItem setAlternate:YES];
>+  [contextMenu addItem:shiftMenuItem];
>+
>   // open in new tabs in new window
>   if (multipleItems)
>   {
>     menuTitle = NSLocalizedString(@"Open in Tabs in New Window", @"");
>+
>     menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarksInTabsInNewWindow:) keyEquivalent:@""] autorelease];
>+    [menuItem setKeyEquivalentModifierMask:0];
>     [menuItem setTarget:target];
>     [contextMenu addItem:menuItem];
>+
>+    shiftMenuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarksInTabsInNewWindow:) keyEquivalent:@""] autorelease];
>+    [shiftMenuItem setKeyEquivalentModifierMask:NSShiftKeyMask];
>+    [shiftMenuItem setTarget:target];
>+    [shiftMenuItem setAlternate:YES];
>+    [contextMenu addItem:shiftMenuItem];
>   }
> 
>   // open in new tab in current window
>   if (itemsContainsFolder || multipleItems)
>     menuTitle = NSLocalizedString(@"Open in New Tabs", @"");
>   else
>     menuTitle = NSLocalizedString(@"Open in New Tab", @"");
>+
>   menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewTab:) keyEquivalent:@""] autorelease];
>+  [menuItem setKeyEquivalentModifierMask:0];
>   [menuItem setTarget:target];
>   [contextMenu addItem:menuItem];
>+  
>+  shiftMenuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewTab:) keyEquivalent:@""] autorelease];
>+  [shiftMenuItem setKeyEquivalentModifierMask:NSShiftKeyMask];
>+  [shiftMenuItem setTarget:target];
>+  [shiftMenuItem setAlternate:YES];
>+  [contextMenu addItem:shiftMenuItem];

It seems like you're doing the same thing over and over in different places. Is there some simple way you can share the code a bit, to avoid repeating it?
Attachment #234547 - Flags: review?(hwaara) → review-
(In reply to comment #35)
> >+  BOOL reverseBackgroundPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) ? YES : NO;
> 
> skip the ? and what comes after it. That goes for all such checks that are in
> the patch.
> 
> The bitwise operation will already be zero or non-zero and thus works like a
> boolean check. YES is probably defined to 1 and NO to 0 in objc.h, anyway.

YES being defined to 1 is irrelevant; the arbitrary value you get from bitwise-anding with a mask is likely to be something other than 1.  What's wrong with normalizing to actual BOOL values on assignment?
(In reply to comment #36)
> YES being defined to 1 is irrelevant; the arbitrary value you get from
> bitwise-anding with a mask is likely to be something other than 1.  What's
> wrong with normalizing to actual BOOL values on assignment?

This isn't just a style thing, it's actually dangerous to not account for this.  Here's why it's really important to normalize.  Keep in mind that sizeof(BOOL) = 1, but most other types (especially bitfields) are wider.  What does this code do?

  int i = 0x100;
  BOOL b = i;
  if (b) {
    // we should wind up here, right?  we don't!
  }

What this code really wants to do instead is one of:

  BOOL b = i != 0;
  BOOL b = !!i; // correct, but looks weird
  BOOL b = i ? YES : NO; // as in the proposed patch
Thanks for the correction. 

I find |(foo & bar) ? YES : NO;| a bit chatty and would prefer: |(foo & bar) != 0|.
> I don't see this change as an improvement, please refrain from changing unless
> it helps readability.

I'm pretty sure moz style guidelines dictate using !foo instead of foo == nil, but I'll save it for another day.  Removed.

> I don't think it was ever discussed to also have a cmd-shift alternate, but I
> suppose we'll have to for consistency... Let's just hope it won't be the straw
> that broke the camel's back performance wise...

FWIW, my perf testing in comment 18 was done with the cmd-shift alternate as well.

> >Index: src/bookmarks/BookmarkButton.mm
> >===================================================================
> >RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkButton.mm,v
> >retrieving revision 1.23
> >diff -u -8 -r1.23 BookmarkButton.mm
> >--- src/bookmarks/BookmarkButton.mm	16 May 2006 08:50:59 -0000	1.23
> >+++ src/bookmarks/BookmarkButton.mm	19 Aug 2006 05:13:16 -0000
> >@@ -160,34 +160,40 @@
> > {
> >   return mItem;
> > }
> > 
> > -(IBAction)openBookmark:(id)aSender
> > {
> >   BrowserWindowController* brController = [[self window] windowController];
> >   BookmarkItem *item = [self bookmarkItem];
> >+  BOOL reverseBGPref = (([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0);
> >+  EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
> >+  if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
> >+    openBehavior = eBookmarkOpenBehavior_NewPreferred;
> 
> Wasn't this what we wanted to get rid of in the first place? :)

|aSender| in this case is the bookmark button itself, and buttons still don't have alternates (oh, that I wish they did).  It's not a problem, though, since the bug is only for menu items, so this works.

All other comments addressed.  I went with != 0, especially considering that that's what the current event key modifier checks do.
Attachment #234547 - Attachment is obsolete: true
Attachment #234645 - Flags: review?(hwaara)
(In reply to comment #39)
> > I don't see this change as an improvement, please refrain from changing unless
> > it helps readability.
> 
> I'm pretty sure moz style guidelines dictate using !foo instead of foo == nil,
> but I'll save it for another day.  Removed.

It all depends on the context. For if-statements, using !foo looks better, but if you want to set a boolean according to whether something is null, I think it's more readable to make the code explicit.
Attachment #234645 - Flags: review?(hwaara) → review+
*** Bug 317214 has been marked as a duplicate of this bug. ***
Attachment #234645 - Flags: superreview?(mikepinkerton)
Comment on attachment 234645 [details] [diff] [review]
Addresses hwaara's comments

sr=pink

sucks we have to update so much code in so many places to do something like this. OH well.
Attachment #234645 - Flags: superreview?(mikepinkerton) → superreview+
I just checked this in. Thanks for the patch froodian!
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Clearing the blocking request since this landed.

Please keep an eye on perf when opening the Bookmarks menu and bookmark toolbar folder menus in tomorrow's and subsequent builds (and file new bugs on issues).
Flags: camino1.1?
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: