319879 - Loading bookmarks with keywords doesn't allow popups

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Stuart Morgan]

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

[Simon Fraser [no longer active]]

How are you firing the bookmark from the keyboard?

Comment 2

[Simon Fraser [no longer active]]

Test url in the url field. Drag this to bookmarks, and give it a keyword to test.

Comment 3

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

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

[froodian (Ian Leue)]

Attachment: 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

[Stuart Morgan]

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

[froodian (Ian Leue)]

Attachment: 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

[Stuart Morgan]

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

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 9

[froodian (Ian Leue)]

Attachment: unbitrots it

This is the bitrot police.

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 233415 [details] [diff] [review]
unbitrots it

sr=pink

Comment 11

[Nick Kreeger]

Fixed trunk and MOZILLA_1_8_BRANCH

FYI.
[NSMenuItem setAlternate:YES]; -> Is a 10.3 and later method.

Comment 12

[froodian (Ian Leue)]

this patch doesn't have any alternates in it.

Comment 13

[Nick Kreeger]

cross-commit puked, this did not land on branch, working on it now.

Comment 14

[froodian (Ian Leue)]

Kreeger, see comment 7

Comment 15

[Nick Kreeger]

Please post the 1.8 version of the patch.

Comment 16

[froodian (Ian Leue)]

Attachment: 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

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

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

[Nick Kreeger]

Landed on MOZILLA_1_8_BRANCH.

Comment 19

[Samuel Sidler (old account; do not CC)]

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.