Last Comment Bug 319879 - Loading bookmarks with keywords doesn't allow popups
: Loading bookmarks with keywords doesn't allow popups
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: 1.8 Branch
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
javascript:void(window.open('http://b...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-11 09:22 PST by Stuart Morgan
Modified: 2007-03-31 18:04 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Implements comment 0 (4.54 KB, patch)
2006-05-31 17:16 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
New Patch (4.62 KB, patch)
2006-06-01 19:39 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
r=smorgan patch (4.59 KB, patch)
2006-06-06 13:02 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
unbitrots it (4.56 KB, patch)
2006-08-12 16:23 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review
Patch ported to 1.8 branch (4.59 KB, patch)
2006-08-23 12:25 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review

Description User image Stuart Morgan 2005-12-11 09:22:41 PST
If you click on a bookmark for a site that needs to open a popup (for example, a BugMeNot bookmarklet) it works fine, but it fails if you fire off the bookmark using a keyword.

From a quick glance at the code, it looks like resolveBookmarksKeyword just needs to return nil when there's no match (so we know if it is giving back a match or just the url that was passed in), then goToLocationFromToolbarURLField:inView:inBackground: can pass YES for all the allowPopups params in the case where it's loading based on bookmarks.
Comment 1 User image Simon Fraser 2005-12-11 11:43:10 PST
How are you firing the bookmark from the keyboard?
Comment 2 User image Simon Fraser 2005-12-11 15:00:13 PST
Test url in the url field. Drag this to bookmarks, and give it a keyword to test.
Comment 3 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-04-28 15:52:10 PDT
Is this as easy a fix as it seems to me from Stuart's comment?  If so, can someone whip up a quick patch?
Comment 4 User image froodian (Ian Leue) 2006-05-31 17:16:35 PDT
Created attachment 224001 [details] [diff] [review]
Implements comment 0

This does more or less what comment 0 says.  The check to see if it's null is so the URL gets passed on anyway (to the error page or keyword.URL).

Also includes some whitespace cleaning and detabbing.
Comment 5 User image Stuart Morgan 2006-05-31 18:52:56 PDT
Comment on attachment 224001 [details] [diff] [review]
Implements comment 0

>+  if (resolvedURLs == nil) {
>+    if (inDest == kDestinationNewTab)
>+      [self openNewTabWithURL:theURL referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>+    else if (inDest == kDestinationNewWindow)
>+      [self openNewWindowWithURL:theURL referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>+    else // if it's not a new window or a new tab, load into the current view
>+      [self loadURL:theURL referrer:nil activate:YES allowPopups:NO];
>+  }
>+  else if ([resolvedURLs count] == 1) {
>     resolvedURL = [resolvedURLs lastObject];
>     if (inDest == kDestinationNewTab)
>-      [self openNewTabWithURL:resolvedURL referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>+      [self openNewTabWithURL:resolvedURL referrer:nil loadInBackground:inLoadInBG allowPopups:YES];
>     else if (inDest == kDestinationNewWindow)
>-      [self openNewWindowWithURL:resolvedURL referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>+      [self openNewWindowWithURL:resolvedURL referrer:nil loadInBackground:inLoadInBG allowPopups:YES];

This duplicates a bunch of code. I think it would be cleaner to do something like:

if (!resolvedURLs || [resolvedURLs count] == 1) {
  NSString *targetURL = resolvedURLs ? [resolvedURLs lastObject] : theURL;
  allowPopups = resolvedURLs ? YES : NO;
  ...
}

It's perhaps a bit less clear, but easier to maintain.

>   if (resolvedArray)
>     return resolvedArray;
>-  return [NSArray arrayWithObject:keyword];
>+  return nil;

Just return |resolvedArray|, since that's what the three-line version is doing anyway.
Comment 6 User image froodian (Ian Leue) 2006-06-01 19:39:42 PDT
Created attachment 224151 [details] [diff] [review]
New Patch

Addresses smorgan's comments.  Also allows popups if |resolvedURLs| is longer than 1 element, so that folders full of popups will work too.
Comment 7 User image Stuart Morgan 2006-06-06 00:03:43 PDT
Comment on attachment 224151 [details] [diff] [review]
New Patch

>-  if (resolvedURL && [theURL isEqualToString:resolvedURL] &&
>+  if (targetURL && [theURL isEqualToString:targetURL] &&

Change this to
  if (!resolvedURLs &&
since that's what it's trying to get at.

r=me with that change.

This will need either a new patch or some commit love for the 1.8 branch because of the NS_ConvertUTF16toUTF8 name change right in the middle of the context.
Comment 8 User image froodian (Ian Leue) 2006-06-06 13:02:38 PDT
Created attachment 224594 [details] [diff] [review]
r=smorgan patch
Comment 9 User image froodian (Ian Leue) 2006-08-12 16:23:10 PDT
Created attachment 233415 [details] [diff] [review]
unbitrots it

This is the bitrot police.
Comment 10 User image Mike Pinkerton (not reading bugmail) 2006-08-21 14:00:33 PDT
Comment on attachment 233415 [details] [diff] [review]
unbitrots it

sr=pink
Comment 11 User image Nick Kreeger 2006-08-23 07:36:31 PDT
Fixed trunk and MOZILLA_1_8_BRANCH

FYI.
[NSMenuItem setAlternate:YES]; -> Is a 10.3 and later method.
Comment 12 User image froodian (Ian Leue) 2006-08-23 07:43:15 PDT
this patch doesn't have any alternates in it.
Comment 13 User image Nick Kreeger 2006-08-23 07:48:02 PDT
cross-commit puked, this did not land on branch, working on it now.
Comment 14 User image froodian (Ian Leue) 2006-08-23 07:52:12 PDT
Kreeger, see comment 7
Comment 15 User image Nick Kreeger 2006-08-23 11:31:47 PDT
Please post the 1.8 version of the patch.
Comment 16 User image froodian (Ian Leue) 2006-08-23 12:25:22 PDT
Created attachment 235111 [details] [diff] [review]
Patch ported to 1.8 branch

Here's a 1.8 version of the patch.  I don't know if a) this is what's needed, or if there's special workarounding that needs to be done b) it builds or works, since I don't have a 1.8 tree (or the kind of connection power to pull one).  If this is the right thing, could you check it in kreeger?  If not, just tell me what it needs and I'll happily do what needs to be done. :)  ty.
Comment 17 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-23 13:29:23 PDT
attachment 235111 [details] [diff] [review] applies (with offsets, but my 1.8 tree is not clean atm) and works.

We need to keep adding the [needs checkin] comments after sr so that we don't lose patches if no one's around to check in ASAP; we can track patches while they're still in the queues, but once they have sr, [needs checkin] is the only way to make sure patches don't get lost/forgotten waiting on checkin.... 
Comment 18 User image Nick Kreeger 2006-08-23 20:26:31 PDT
Landed on MOZILLA_1_8_BRANCH.
Comment 19 User image Samuel Sidler (old account; do not CC) 2007-03-31 18:04:17 PDT
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.

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