346228 - Use file's actual icon for site icon (favicon) for local files

Status: RESOLVED FIXED

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

Description

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

Since today seems to be "file RFEs" day, I'll file this one.  When we load a local file, we should show the file's actual icon as the favicon.  Safari does this, so it's a nice bit of OS integration polish.  I think Wevah talked about doing this at one time.

Comment 1

[froodian (Ian Leue)]

Attachment: Patch

Does the deed.

Comment 2

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

Pretty :)

Comment 3

[Sean Murphy]

Comment on attachment 248761 [details] [diff] [review]
Patch

Nice addition froodian.  Everything is cool except for a code style nag, so r=me with that fixed.  My other comment does not concern the patch directly, but code right around it.

+  if ([inURIAsNSURL isFileURL]) {
+    faviconImage = [[NSWorkspace sharedWorkspace] iconForFile:[inURIAsNSURL path]];
+  }

This conditional can be written without braces, per our coding style conventions (you obviously follow them since you actually fixed an occurrance of this later in the patch :) ).

NS_DURING
  faviconImage = [[[NSImage alloc] initWithData:data] autorelease];
NS_HANDLER
  NSLog(@"Exception \"%@\" making favicon image for %@", localException, inURI);
faviconImage = nil;
NS_ENDHANDLER

I could very well be wrong here, but can we avoid the exception handling macros?  The NSImage initializer being called will return nil if the method cannot create an image from the data.  Additionally, since we've dropped 10.2 support, exception handling in the future can use the obj-c syntax built into the compiler.

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> This conditional can be written without braces, per our coding style
> conventions

When one of an if/else if/else clauses has braces, we give all of them braces.
 
> I could very well be wrong here, but can we avoid the exception handling
> macros?  The NSImage initializer being called will return nil if the method
> cannot create an image from the data.

It does seem odd, but Simon explicitly added that after the original code had already been written; I'd be very hesitant about removing it without knowing why and being sure that it's no longer a potential problem.

Comment 5

[froodian (Ian Leue)]

Simon, can you glance at comments 3 and 4 and weigh in? :)

Comment 6

[Simon Fraser [no longer active]]

(In reply to comment #4)
> It does seem odd, but Simon explicitly added that after the original code had
> already been written; I'd be very hesitant about removing it without knowing
> why and being sure that it's no longer a potential problem.

I believe that I saw NSImage throwing exceptions when trying to make images from non-image data. Since we can get all sorts of stuff back from the request in addition to images (like 404 pages), I think we should leave it in. If we've turned on Obj-C exceptions, then it could be converted ty @try/@catch (along with the other exception macros in the code).

Comment 7

[froodian (Ian Leue)]

Comment on attachment 248761 [details] [diff] [review]
Patch

That should probably get its own bug (since we should do it everywhere).  Filed as bug 364109.

Comment 8

[froodian (Ian Leue)]

Attachment: Patch

Comment 9

[froodian (Ian Leue)]

Sorry, trigger happy fingers.  Above patch is just a whitespace fix from the previous one.

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 248893 [details] [diff] [review]
Patch

sr=pink

Comment 11

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk (100 fixed bugs!  woohoo!)

Comment 12

[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.