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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Location Bar & Autocomplete
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1.1, polish})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1, polish

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.83 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Camino1.2
(Assignee)

Updated

11 years ago
Assignee: nobody → stridey
(Assignee)

Comment 1

11 years ago
Created attachment 248761 [details] [diff] [review]
Patch

Does the deed.
Attachment #248761 - Flags: review?(camino)
Pretty :)

Comment 3

11 years ago
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.
Attachment #248761 - Flags: review?(camino) → review+

Comment 4

11 years ago
(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.
(Assignee)

Comment 5

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

Comment 6

11 years ago
(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).
(Assignee)

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
Created attachment 248893 [details] [diff] [review]
Patch
Attachment #248761 - Attachment is obsolete: true
Attachment #248893 - Flags: superreview?(mikepinkerton)
(Assignee)

Comment 9

11 years ago
Sorry, trigger happy fingers.  Above patch is just a whitespace fix from the previous one.
Comment on attachment 248893 [details] [diff] [review]
Patch

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

Comment 11

11 years ago
Checked in on 1.8branch and trunk (100 fixed bugs!  woohoo!)
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
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.