Bug 177734 - [Patch] Search top-level domain for favicon.ico if bz domain doesn't have it
Summary: [Patch] Search top-level domain for favicon.ico if bz domain doesn't have it
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: eclipse-bugzilla
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Igor Foox
QA Contact:
URL:
Whiteboard:
Depends On: 178077
Blocks:
TreeView+ depends on / blocked
 
Reported: 2006-01-13 16:49 UTC by Bug Testing
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2006-01-18 22:03:02 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
patch (I'll remove the unnecessary printlns) (3.83 KB, patch)
2006-01-17 16:25 UTC, Andrew Overholt
no flags Details | Diff
Updated patch with Igor's suggestions and removed printlns. (3.69 KB, patch)
2006-01-17 18:35 UTC, Andrew Overholt
no flags Details | Diff

Description Bug Testing 2006-01-13 16:49:14 UTC
Description of problem:
Currently we just search the BZ domain for favicon.ico.  Often times, the BZ
installation is on a subdomain with no favicon.ico (ex. bugs.eclipse.org).  This
patch attempts to get favicon.ico from the top-level domain before defaulting to
no icon.

I have also added bin/ to .cvsignore.

Okay to apply?

2006-01-13  Andrew Overholt  <overholt>

	* .cvsignore: Add bin/.
	* src/org/eclipse/team/bugzilla/operations/BugzillaOperations.java
	(getBugzillaIcon): Search main domain for favicon.ico if not found
	at bugzilla URL.

Comment 1 Andrew Overholt 2006-01-17 16:25:35 UTC
Created attachment 123304 [details]
patch (I'll remove the unnecessary printlns)

Comment 2 Igor Foox 2006-01-17 16:45:00 UTC
Looks ok overall, a few comments.

   1. You're doing:
iconUrl = new URL(base.getProtocol() + "://" + newHost + "favicon.ico");
and then a few lines after:
iconUrl = new URL("http://" + newHost + "/" + "favicon.ico");

I think you might need the extra "/" in the first case too. 

   2. It will be easier to read if you change the top level if statement from:
if ((defaultIcon == null) || (defaultIcon.getImageData() == null)) {
    ... try top host ...
} else {
    ... return non-null icon ...
}

to


if ((defaultIcon != null) && (defaultIcon.getImageData() != null)) {
    return defaultIcon;
}
... otherwise try to find in top host ...


   3. In getBugzillaIconImageDescriptor() you do 
  ImageDescriptor defaultIcon = ImageDescriptor.createFromURL(iconUrl);

  URLConnection con = iconUrl.openConnection();
  int len = con.getContentLength();

but in the original code they first opened the connection and checked the
length, and then got the full icon if not 0. Is there a reason you switched
them? Not sure if it makes a big (or any) difference but might be better to
check for 0 length first.

Comment 3 Andrew Overholt 2006-01-17 18:35:53 UTC
Created attachment 123316 [details]
Updated patch with Igor's suggestions and removed printlns.

I'm committing this.

Comment 4 Igor Foox 2006-01-17 21:48:22 UTC
As a result of the patch there's a problem with the Ubuntu bugzilla, adding
dependency on that bug.

Comment 5 Igor Foox 2006-01-18 22:03:02 UTC
Patch commited.


Note You need to log in before you can comment on or make changes to this bug.