Loading bookmarks with keywords doesn't allow popups

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

1.8 Branch
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
How are you firing the bookmark from the keyboard?

Comment 2

12 years ago
Test url in the url field. Drag this to bookmarks, and give it a keyword to test.
Is this as easy a fix as it seems to me from Stuart's comment?  If so, can someone whip up a quick patch?
Assignee: mikepinkerton → nobody
QA Contact: bookmarks
Target Milestone: --- → Camino1.2
(Assignee)

Updated

11 years ago
Whiteboard: [Good First Bug]
(Assignee)

Comment 4

11 years ago
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.
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #224001 - Flags: review?(stuart.morgan)
(Reporter)

Comment 5

11 years ago
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.
Attachment #224001 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 6

11 years ago
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.
Attachment #224001 - Attachment is obsolete: true
Attachment #224151 - Flags: review?(stuart.morgan)
(Reporter)

Comment 7

11 years ago
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.
Attachment #224151 - Flags: superreview?
Attachment #224151 - Flags: review?(stuart.morgan)
Attachment #224151 - Flags: review+
(Assignee)

Comment 8

11 years ago
Created attachment 224594 [details] [diff] [review]
r=smorgan patch
Attachment #224151 - Attachment is obsolete: true
Attachment #224594 - Flags: superreview?
Attachment #224151 - Flags: superreview?
(Assignee)

Updated

11 years ago
Attachment #224594 - Flags: superreview? → superreview?(sfraser_bugs)
(Assignee)

Comment 9

11 years ago
Created attachment 233415 [details] [diff] [review]
unbitrots it

This is the bitrot police.
Attachment #224594 - Attachment is obsolete: true
Attachment #233415 - Flags: superreview?(mikepinkerton)
Attachment #224594 - Flags: superreview?(sfraser_bugs)
Comment on attachment 233415 [details] [diff] [review]
unbitrots it

sr=pink
Attachment #233415 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [Good First Bug] → [Good First Bug] [needs checkin]

Comment 11

11 years ago
Fixed trunk and MOZILLA_1_8_BRANCH

FYI.
[NSMenuItem setAlternate:YES]; -> Is a 10.3 and later method.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [Good First Bug] [needs checkin]
(Assignee)

Comment 12

11 years ago
this patch doesn't have any alternates in it.

Comment 13

11 years ago
cross-commit puked, this did not land on branch, working on it now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

11 years ago
Kreeger, see comment 7
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1

Comment 15

11 years ago
Please post the 1.8 version of the patch.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

11 years ago
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.
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.... 
Whiteboard: [needs checkin 1.8branch]

Comment 18

11 years ago
Landed on MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
Whiteboard: [needs checkin 1.8branch]
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.
Target Milestone: Camino1.2 → Camino1.1
You need to log in before you can comment on or make changes to this bug.