Last Comment Bug 346228 - Use file's actual icon for site icon (favicon) for local files
: Use file's actual icon for site icon (favicon) for local files
Status: RESOLVED FIXED
: fixed1.8.1.1, polish
Product: Camino Graveyard
Classification: Graveyard
Component: Location Bar & Autocomplete (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-27 22:25 PDT by Smokey Ardisson (offline for a while; not following bugs - do not email)
Modified: 2007-03-31 18:04 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (4.80 KB, patch)
2006-12-15 12:18 PST, froodian (Ian Leue)
murph: review+
Details | Diff | Splinter Review
Patch (4.83 KB, patch)
2006-12-17 00:27 PST, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-27 22:25:14 PDT
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 User image froodian (Ian Leue) 2006-12-15 12:18:01 PST
Created attachment 248761 [details] [diff] [review]
Patch

Does the deed.
Comment 2 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-12-15 17:00:39 PST
Pretty :)
Comment 3 User image Sean Murphy 2006-12-15 23:58:00 PST
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 User image Stuart Morgan 2006-12-16 07:57:07 PST
(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 User image froodian (Ian Leue) 2006-12-16 08:41:52 PST
Simon, can you glance at comments 3 and 4 and weigh in? :)
Comment 6 User image Simon Fraser 2006-12-16 21:25:38 PST
(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 User image froodian (Ian Leue) 2006-12-17 00:19:54 PST
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 User image froodian (Ian Leue) 2006-12-17 00:27:24 PST
Created attachment 248893 [details] [diff] [review]
Patch
Comment 9 User image froodian (Ian Leue) 2006-12-17 00:28:16 PST
Sorry, trigger happy fingers.  Above patch is just a whitespace fix from the previous one.
Comment 10 User image Mike Pinkerton (not reading bugmail) 2006-12-18 09:49:59 PST
Comment on attachment 248893 [details] [diff] [review]
Patch

sr=pink
Comment 11 User image froodian (Ian Leue) 2006-12-18 10:29:51 PST
Checked in on 1.8branch and trunk (100 fixed bugs!  woohoo!)
Comment 12 User image Samuel Sidler (old account; do not CC) 2007-03-31 18:04:41 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.