502396 - Properly autocomplete redirected URLs

Status: RESOLVED WONTFIX

(Camino Graveyard :: Location Bar & Autocomplete, enhancement)

Description

[Dan Weber]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US) AppleWebKit/525.18 (KHTML, like Gecko, Safari/525.20) OmniWeb/v622.6.1.0.111015
Build Identifier: Trunk

URLs that are redirected don't show up properly in the autocomplete results--they have blank titles and generic favicons. The title and favicon should be pulled from the new URL that we are redirected to, and the old/new URLs should be merged (i.e. shouldn't show up in the autocomplete results at the same time).

Reproducible: Always

Comment 1

[Dan Weber]

Attachment: Patch 1

Comment 2

[Dan Weber]

Attachment: Patch 2

Comment 3

[Stuart Morgan]

Comment on attachment 387273 [details] [diff] [review]
Patch 2

The new files need license blocks.

>+  if ([mRegexTest evaluateWithObject:url]) {
>+    // We want to return NO if there exists a URL that redirects to this URL
>+    // and also matches the current search string.
>+    return NO;
>+  }

Why? Won't de-duping take care of this?

>-  NSImage* cachedFavicon = [[SiteIconProvider sharedFavoriteIconProvider] favoriteIconForPage:[info url]];
>+  NSString *faviconURL = [info url];
>+
>+  RedirectedURLManager *redirectedURLManager = [RedirectedURLManager sharedInstance];
>+  NSString *url = [redirectedURLManager redirectedURLForURL:[info url]];
>+  if (url) {
>+    for (unsigned int i = 0; i < [mHistoryData count]; i++) {
>+      if ([[[mHistoryData objectAtIndex:i] url] isEqualToString:url]) {
>+        [info setTitle:[[mHistoryData objectAtIndex:i] title]];
>+        faviconURL = url;
>+      }
>+    }
>+  }
>+
>+  NSImage* cachedFavicon = [[SiteIconProvider sharedFavoriteIconProvider] favoriteIconForPage:faviconURL];

You don't really need the favicon involved in the loop; just check for -favoriteIconForPage:url, and if that doesn't work, check with [info url]. Also, maybe rename url to redirectURL or finalURL or something like that, so it's easier to track what's being used where.

The loop is missing a |break;|. Also, can't we do this only in the case where there is no title so the non-redirect matches don't incur the cost?

>+#import <Cocoa/Cocoa.h>

Looks like Foundation is all you actually need for this class.


>+@interface RedirectedURLManager : NSObject {

Needs a class description.

>++ (RedirectedURLManager *)sharedInstance;
>+
>+- (void)setOriginalURL:(NSString *)url;
>+- (void)setNewURL:(NSString *)url;
>+- (void)setAllowsRedirects:(BOOL)flag;
>+- (BOOL)allowsRedirects;
>+- (NSString *)redirectedURLForURL:(NSString *)url;
>+- (NSString *)URLForRedirectedURL:(NSString *)url;

These all need comments.

>+    NSString *profileDir = [[PreferenceManager sharedInstance] profilePath];

Just to be on the safe side, use sharedInstanceDontCreate here.

>+  if (mOriginalURL)
>+    [mOriginalURL release];

Get rid of the |if| check; messaging nil is fine.

>+- (void)setOriginalURL:(NSString *)url
...
>+- (void)setNewURL:(NSString *)url
...

It feels weird to have the responsibility for doing this in this class. Couldn't the client keep track of where we start and where we end up, and the call a method to add a pair?

>+  if ([self allowsRedirects]) {
>+    [mURLToRedirectedURLDict setObject:url forKey:mOriginalURL];
>+    [mRedirectedURLToURLDict setObject:mOriginalURL forKey:url];
>+    [self writeToFile];
>+  }

We should only write if something new is added, not every time, since hitting the same redirect URL frequently is a likely use case.

>+- (void)setAllowsRedirects:(BOOL)flag

Again, this feels like something that the client should handle.

>--- ./src/embedding/CHBrowserListener.mm	2009-07-07 12:17:13.000000000 -0700
...
>+#include "RedirectedURLManager.h"

This is a problem, since we try to maintain a boundary between CH code and Camino code; the idea is that the CH stuff could be used as a stand-alone WebView-like view without any of the Camino code. It's not quite there right now, but we don't want to make the coupling worse.

It looks like onLoadingStarted and onLoadingCompleted: give you half of what you need (you can add the URL to onLoadingStarted if BrowserWrapper doesn't already have the information you need), and you could make similar new callbacks for the other spots, then have BrowserWrapper take care of this logic.

>+      // The headers have been received and the document is starting to load,
>+      // so we no longer allow redirects.
>+      [sharedRedirectedURLManager setAllowsRedirects:NO];

The comment needs to make it clear why you are doing this; I only know because we talked about it in irc.

Comment 4

[Dan Weber]

Attachment: Patch 3

Comment 6

[Stuart Morgan]

Comment on attachment 388990 [details] [diff] [review]
Patch 3

>+  [...] However, this can be reversed under
>+    // one condition: if the redirected URL is shorter than the original URL, the
>+    // redirected URL will match and the original URL will be ignored.
>+    return NO;

So I was thinking about this some more, and it seems risky. Let's say URL A redirects to B, where B is longer, and both match a given search. Half the time I type A, and half the time I get to B by following a direct link from somewhere. My visit count for B is twice the visit count for A, so B may be in my top 5 for a given query while A isn't. This logic causes the site to be dropped from autocomplete suggestions.

We could do a full scan for B and promote it, I guess, but that seems like a lot of extra work (although I guess we are sort of doing it anyway to get the title). Maybe we just shouldn't care while URL is used? The new look makes the title much more prominent than the URL.

> - (AutoCompleteResult *)autoCompleteResultForItem:(id)item
[...]
>+    for (unsigned int i = 0; i < [mHistoryData count]; i++) {
>+      if ([[[mHistoryData objectAtIndex:i] url] isEqualToString:redirectedURL]) {

I just realized that this breaks the whole "chunked to prevent hangs" approach. Imagine the only 5 matches to your string are near the end of your 1000 history items, along with their targets. In one chunk you are only supposed to work with ~100 items, but now you are doing 5000 string comparisons. I think you need to pre-generate a URL-to-item dictionary for history when the autocomplete object is created (which should be reasonably fast) and then use that here.

Oh, and random thought: what if A is a redirect to B, and B redirects to C? We should actually follow the chain until it ends. Using a dictionary should make lookup cheap enough that following the whole chain will be fast.

>+  NSString *redirectedURL = [redirectedURLManager redirectedURLForURL:[info url]];
>...
>+  NSImage* cachedFavicon = [[SiteIconProvider sharedFavoriteIconProvider] favoriteIconForPage:redirectedURL];

Won't this break favicon lookup for everything that doesn't redirect?

>+// 1. With the redirectedURLForURL method, the final URL is returned for the
>+//    URL passed, if it was redirected.
>+// 2. With the URLForRedirectedURL method, the original URL is returned for
>+//    the URL passed, if there is a URL that redirected to it.

This part doesn't really belong in the class-level comment, since it's really about individual methods.

>+@interface RedirectedURLManager : NSObject {
>+  NSMutableDictionary*  mURLToRedirectedURLDict;
>+  NSMutableDictionary*  mRedirectedURLToURLDict;
>+  NSString*             mRedirectsPath;
>+}

This should have a @private at the beginning, and the members need ownership markers. (Yes, it would be better if we assumed owned/strong and only marked weak, but unfortunately that ship has already sailed and being inconsistent would make it really confusing.)

>+// Returns the new, redirected URL for the URL passed.
>+- (NSString *)redirectedURLForURL:(NSString *)url;
>+
>+// Returns the original URL that redirected to the URL passed.
>+- (NSString *)URLForRedirectedURL:(NSString *)url;

These should say what is returned when the given URL doesn't have a redirect.

Here's a question: what if 5 different URLs all redirect to the same place? This may to map from URLs to arrays in one direction instead of being one-to-one. I'm okay with that being a follow-up bug though so we can wait and see if it's necessary in real-world usage.

>+// Removes the URL of the specified history item from the redirection map.
>+- (void)removeItem:(HistorySiteItem *)item;

Define this in terms of URL so it's not needlessly dependent on another class.

>+// Adds a RedirectedURLBrowserListener to the specified browser's view.
>+- (void)addListenerToBrowser:(BrowserWrapper *)browser;

Why is this part of the redirect class, rather than BrowserWrapper (or maybe the Listener class). Doing it this way makes this class dependent on two other classes that it doesn't actually use in any meaningful way.

>+@interface RedirectedURLBrowserListener : NSObject<CHBrowserListener>
>+{
>+  BrowserWrapper* mBrowserWrapper;
>+  NSString*       mOriginalURL;

Needs @private and ownership markers.

>+    NSFileManager *fileManager = [NSFileManager defaultManager];
>+    if ([fileManager fileExistsAtPath:mRedirectsPath]) {
>+      mURLToRedirectedURLDict = [[NSMutableDictionary alloc] initWithContentsOfFile:mRedirectsPath];
>+      mRedirectedURLToURLDict = [[NSMutableDictionary alloc] initWithObjects:[mURLToRedirectedURLDict allKeys]
>+                                                                     forKeys:[mURLToRedirectedURLDict allValues]];
>+    } else {
>+      mURLToRedirectedURLDict = [[NSMutableDictionary alloc] init];
>+      mRedirectedURLToURLDict = [[NSMutableDictionary alloc] init];
>+    }

Make readFromFile or loadFromFile method, and put this there.

>+- (void)setRedirectedURL:(NSString *)newURL forURL:(NSString *)oldURL;

Can we come up with less ambiguous names for the two URLs? In this and the fetcher methods, I can't tell without reading the comments which is which (and in fact I would guess wrong; if A redirects to B, then isn't A a redirected URL?) Maybe targetURLForRedirectSource:, sourceURLForRedirectTarget:, and setTargetURL:forRedirectSource: ?



I really need to fix the problem of the listener protocol being non-optional :P

>   NS_IMETHOD ItemRemoved(nsIHistoryItem* inHistoryItem)

Do we know if this gets called for history expiration, or things like using Reset Camino to wipe history? If not, we'll want to handle those sepecially (perhaps doing a sweep every so often to handle expiration?)

It would also be good to test with history set to 0 days, to see how that gets handled with your current code.

Comment 7

[Dan Weber]

Attachment: Patch 4

Comment 8

[Stuart Morgan]

Comment on attachment 390114 [details] [diff] [review]
Patch 4

Nice job puzzling out the history implementation to get the expiration/removal stuff! Looks great :)


>+  RedirectionManager *redirectionManager = [RedirectionManager sharedInstance];
>+  NSString *url = [redirectionManager sourceURLForRedirectTarget:[item url]];
>+  if (!url)
>+    url = [redirectionManager targetURLForRedirectSource:[item url]];
>+  if (!url || ![mRegexTest evaluateWithObject:url])
>+    return item;
>+  if ([mURLToBookmarkDict objectForKey:[item url]]) {
>+    [mIgnoredURLs addObject:url];
>+    return [mURLToHistoryItemDict objectForKey:[item url]];
>+  } else if ([url length] <= [[item url] length]) {
>+    [mIgnoredURLs addObject:[item url]];
>+    return [mURLToHistoryItemDict objectForKey:url];
>+  }
>+  return nil;

The logic here is hard to follow; it could use some clearly-named helpers or some comments.

I'm confused why you are checking both directions in the redirect manager; don't we only care about targetURLForRedirectSource (i.e., does this item redirect somewhere, and if so where?).

Why do you check to see if it's in the bookmark dictionary, but then return from the history dictionary if it is? And isn't |[mURLToHistoryItemDict objectForKey:[item url]];| just |item|, since only history items get this far in the method?

>+// Removes the URL of the specified history item from the redirection map.
>+- (void)removeURL:(NSString *)url;

Comment needs fixing; not a history item any more. You should also clarify which direction in the mapping it removes.

>+  if ([fileManager fileExistsAtPath:mRedirectsPath]) {

It's theoretically possible to end up with nil for mRedirectsPath (if the DontCreate prefs manager call returns nil), so it would be good to check for that first in this condition.

>     [obj onLocationChange:location isNewPage:(aRequest != nsnull) requestStatus:requestStatus];
>+    if (mSourceURL && ![mSourceURL isEqualToString:location])
>+      [obj onRedirectFromURL:mSourceURL toURL:location];

It would make more sense to call these in the other order, in case someone were listening for both.

Comment 9

[Gordon P. Hemsley [:GPHemsley]]

This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]