110894 - Use favicons on webpage shortcuts in Windows

Status: RESOLVED FIXED

(Firefox :: Shell Integration, enhancement)

Description

[Ariel Gonzalez]

If possible, whenever we make a link to a webpage thru a shortcut, if we could
set that link's icon to be the favicon for the page.

For example, if I just drag/drop the url from mozilla to the desktop or
wherever, maybe mozilla could save the .ico to some directory, then set that
icon for the shortcut.

Since favicons can be any image type, maybe there could be a built in converter
into .ico

Also, this could be done for any OS, not just Windows.

Comment 1

[SineSwiper]

Well, for Windows, it would be fairly easy.  For something like X (for Linux),
that's a nightmarish task.  Not to say that the Windows support for it shouldn't
be put in there, but I really don't know about expanding it to other OSs.  I
guess it all depends on the ease of coding involved.

Comment 2

[Blake Ross]

Passing to Dave ``down with the net'' Hyatt, he can do this if he wants. Looks 
like he's still using his netscape.com address in Bugzilla, much to the disgrace 
of the company.

Comment 3

[Terri Preston]

qa contact -> pmac

Comment 4

[Aleksey Nogin]

Slightly related: http://bugs.kde.org/show_bug.cgi?id=21848

Comment 5

[David Mediavilla]

I have filed a similar bug for Phoenix and OS/2 Bug 178756 "Use favicons as icon
for OS/2 URL objects".
I don't know if it is similar enough to be a duplicate.

Comment 6

[Mike Kaply [:mkaply]]

*** Bug 178756 has been marked as a duplicate of this bug. ***

Comment 7

[Mike Kaply [:mkaply]]

Would one of the first steps here be to create an icon encoder in libpr0n?

The images on the websites aren't always icons, so they need to be converted to 
platform specific formats.

Comment 8

[Bogdan Stroe]

*** Bug 241610 has been marked as a duplicate of this bug. ***

Comment 11

[Sandy Brown]

Internet Explorer, even SAFARI, will do this!!!

Sandy Brown

Comment 13

[Dan]

Attachment: Icon bug

When I use MS Explorer and send a shorcut to my desktop, It usually has a grahpic depiction of the site, Firefox doesnt.
For instance, MS IE Explorer will have a yellow pages icon in yellow with the walking fingers. Your is just the earth/globe? and an orange arrow. I was told in a chat at Firefox (zzxc) to "Try dragging the icon from the Firefox address bar to the desktop."  but to drag it to my deaktop I'd have to continually downsize my window to access the desktop and then resize the browser (to full screen again) - Thats WAY too many strokes. Then I was told " that is the only way I know of to automatically set the icon. the other way is to right click on the shortcut, click properties, then click change icon
Dan: MS IE does this automatically...and it is very helpful to have that visual identity to organize...any reason why Firefox doesn't use that featture? if you use bookmarks...organize bookmarks, icons are there"  I said, "When I do as you suggested only three icons are shown..they are all similar to each other...the planet earth/globe
and the arrow described above - no graphic depiction of the site. Here is the rest of the discussion.
zzxc: The graphic descriptions are favicons
Dan: and..?
zzxc: If you want to use one, you need to download it
zzxc: you can get Google's at http://www.google.com/favicon.ico
zzxc: then, from the choose icon window, you can pick one of the favicon files you have downloaded
Dan: That is WAY too much work....I'm going back to MS IE
Dan: I think that the aspect of Firefox
is a ridiculous expenditure of time...
zzxc: It is a bug in Firefox that the favicons aren't always transferred to desktop icons
zzxc: This is a Windows-specific feature
Dan: I never see these with Firefox
zzxc: https://bugzilla.mozilla.org/show_bug.cgi?id=110894 is the bug report for this
Dan: it isn't even a question of "aren't always" it is never
Dan: First you want to send me to google to access favicon then tell me it's a bug and to contact mozilla. When you've corrected the issue then I might defautl Firefox to be my b to be my browser...
zzxc: What I told you was a workaround to make this work properly
Dan: Too much work in this "workaround" for me

Comment 14

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 15

[Mate Srsen]

Shouldn't this be reassigned to Firefox/Shell Integration? It has nothing to do with drag and drop behavior; it's about the method by which Firefox creates shortcut files.

I don't have the permissions to do it myself, can someone else verify and move it?

Comment 16

[Silekonn]

Windows desktop web shortcuts should use site favicons.  A desktop with 100 FF logos is not visually appealing.

****I will add a $20 bounty to desktop-dropped shortcuts with site favicons if someone can successfully code this and it makes a release.

Comment 17

[Mate Srsen]

Is anyone there? Who do I talk to to move this into Firefox/Shell Integration?

Comment 18

[Ralph Graw]

I'll up the ante - $50 to the developer who makes this work on Windows.  Somebody else can motivate for the other OS'...  :-)

Comment 19

[Brian R. Bondy [:bbondy]]

I marked myself as a mentor on this bug because I have a pretty good idea of how to implement it and don't currently have the time to do it.  
I'm open to help in any way if someone wants to take it, feel free to email me as well.

> Getting favicons

Favicons in Firefox can come in any supported image format, so it is not guaranteed you will get a favicon in ICO format.

We can get favicons from the favicon service (async is best) with mozIAsyncFavicons::GetFaviconDataForPage using a callback of nsIFaviconDataCallback.
You can see similar code inside widget/src/Windows/JumpListItem.cpp inside JumpListShortcut::CacheIconFileFromFaviconURIAsync.


> Encoding the icons to disk

Once you have an image from the favicon service you can use imgITools::EncodeScaledImage to create an icon with a requested mime type of image/vnd.microsoft.icon.
You can see the relevent code inside widget/src/windows JumpListBuilder.cpp  AsyncWriteIconToDisk::Run()

The method I used for jump lists was to cache the icons into the user's profile dir here:
C:\Users\bbondy\AppData\Local\Mozilla\Firefox\Profiles\snsq8cgq.default

The filename is a hash of the page for the favicon.

Unlike the jump list task, you should encode the images using the image encoder options for BMP instead of PNG: "bpp=32;format=bmp;".

> Working with shortcut files in Win32 API

There is some code for dealing with shortcut files here:
How to create short-cuts (link files)
http://www.codeproject.com/KB/winsdk/makelink.aspx

Comment 20

[Tomer Cohen :tomer]

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> There is some code for dealing with shortcut files here:
> How to create short-cuts (link files)
> http://www.codeproject.com/KB/winsdk/makelink.aspx
Please note that Windows is using files with the url postfix as shortcut to web pages. These files are plain text, which should make it easier to work with these files than with regular Windows shortcuts.

Below is an example shortcut I've created. After creating the shortcut, I've changed it's default icon from the shortcut properties window.


[InternetShortcut]
URL=http://example.com/
IDList=
IconFile=C:\WINDOWS\system32\url.dll
HotKey=0
IconIndex=2
[{000214A0-0000-0000-C000-000000000046}]
Prop3=19,2

Comment 21

[Tomer Cohen :tomer]

Below is a the file content of a shortcut created by Firefox and its icon changed using File→Properties:

    [InternetShortcut]
    URL=https://www.facebook.com/
    IconFile=C:\Documents and Settings\user\Desktop\favicon.ico
    HotKey=0
    IconIndex=0
    IDList=


We are storing these icons in cache, so it might be possible to link the icon to the cached item, or store these icons in a separated directory in the user profile.

Comment 22

[Amiad]

Users complained on this bug in the forum of Mozilla Israel:
http://mozilla.org.il/board/viewtopic.php?f=9&t=10659 (Hebrew)

Comment 23

[Brian R. Bondy [:bbondy]]

Instead of the codeproject icon code please just modify the shortcut format to include something like this:

[InternetShortcut]
URL=https://url/path
IconFile=C:\Users\user\AppData\Local\Mozilla\Firefox\Profiles\9nvdy8ti.Nightly\ShotcutCache\Filename.ico
IconIndex=0


You can store the favicon inside a new ShortcutCache folder inside the user's profile directory.

I highly recommend looking at the code in bug 549472 which has similar icon handling code you'll need.  This task gets icons from the favicon db and writes them to disk in a folder inside the user's profile directory as well. The favicon file of websites sometimes come in PNG and other formats so you need to convert to ICO as is done in that task.

Comment 24

[Parth Mudgal [:artpar]]

Attachment: Patches nsDataObj and JumpListBuilder to add support for icons in dragged shortcuts

majour changes in nsDataObj.cpp and minor changes in JumpListBuilder.cpp to allow AsyncFaviconDataReady class handle work for both JumpListBuilder and nsDataObj.

To which dir it should write is decided by the variable 'dir' which will have default value 'j' for 'jumplistshortcut' so i need not edit more parts in jumplistbuilder.cpp and the calls i make to AsyncFaviconDataReady will have 'dir' set to 's' for 'shortcutcache'

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 589308 [details] [diff] [review]
Patches nsDataObj and JumpListBuilder to add support for icons in dragged shortcuts

Review of attachment 589308 [details] [diff] [review]:
-----------------------------------------------------------------

Great work so far!
As mentioned in email before you submitted this, I think we'll do few passes, so this list is not complete but it'll get us closer to finishing.

A couple of overall notes:
Please adjust any tabs to use spaces again. 
In VS2010 this setting is located at:
Tools | Options | Text Editor | C/C++ | Tabs 
Adjust Tab size: 2
Indent size: 2
(o) Insert spaces
( ) Keep tabs


Also please use Unix line feeds instead of Windows newlines.
A great extension that takes care of this for you automatically is called Strip'em:
http://grebulon.com/software/stripem.php
After it is installed you just need to set that you want unix line endings once and then never worry about it again.

::: .hgignore
@@ +43,5 @@
> +^bin/
> +\.ncb
> +\.suo
> +_ReSharper\.
> +\.resharper\.user
\ No newline at end of file

\ No newline at end of file

Please propose this change in the context of another bug that you post.

::: widget/windows/JumpListBuilder.cpp
@@ +549,5 @@
>  
>  
>  AsyncFaviconDataReady::AsyncFaviconDataReady(nsIURI *aNewURI, 
> +                                             nsCOMPtr<nsIThread> &aIOThread
> +											 , const char cachedir) //To add support for both "jumplistcache" and "shortcutcache" icon folders

New parameters should begin with a lowercase a.
Please use a bool here instead.

@@ +608,5 @@
>                       : mIconPath(aIconPath),
>                         mMimeTypeOfInputData(aMimeTypeOfInputData),
>                         mBuffer(aBuffer),
> +                       mBufferLength(aBufferLength),
> +					   dir(cachedir)

Adjust spacing here to not use tabs.

@@ +639,5 @@
>    // Windows would scale the 16x16 icon themselves, but it's better
>    // we let our ICO encoder do it.
>    PRInt32 systemIconWidth = GetSystemMetrics(SM_CXSMICON);
>    PRInt32 systemIconHeight = GetSystemMetrics(SM_CYSMICON);
> +  if (systemIconWidth == 0 || systemIconHeight == 0 && dir=='j') {

I think you mean:
if ((systemIconWidth == 0 || systemIconHeight == 0) && dir=='j') {

&& has higher precedence.

@@ +644,5 @@
>      systemIconWidth = 16;
>      systemIconHeight = 16;
>    }
> +  if(dir=='s') {
> +	systemIconWidth = 32;

Use 2 spaces instead of tab here.

::: widget/windows/JumpListBuilder.h
@@ +108,5 @@
>    */
>  class AsyncWriteIconToDisk : public nsIRunnable
>  {
>  public:
> +	const char dir;

Spacing

@@ +117,5 @@
>    AsyncWriteIconToDisk(const nsAString &aIconPath,
>                         const nsACString &aMimeTypeOfInputData,
>                         PRUint8 *aData, 
> +                       PRUint32 aDataLen,
> +					   const char cachedir='j');

Spacing here, also don't use a default value here and change to bool as mentioned in the .cpp file.

::: widget/windows/nsDataObj.cpp
@@ +106,5 @@
>          *buf++ = table[rand()%TABLE_SIZE];
>      }
>      *buf = 0;
>  }
> +//NS_IMPL_ISUPPORTS1(AsyncFaviconDataReady, nsIFaviconDataCallback)

This commented code can be removed.

@@ +1359,5 @@
> +
> +
> +
> +
> +

Remove the extra newlines on this file please.

::: widget/windows/nsDataObj.h
@@ +58,5 @@
>  
> +
> +#include "nsICryptoHash.h"
> +#include "JumpListItem.h"
> +#include "JumpListBuilder.h"

Instead of including these items let's pull out the common code into a new file instead please.

@@ +159,5 @@
>  {
> +
> +public:
> +	static nsresult GetOutputIconPath(nsCOMPtr<nsIURI> aFaviconPageURI, 
> +		nsCOMPtr<nsIFile> &aICOFile);

I think this one can be factored as common code too as well as a couple of the other ones in the new file.

Comment 26

[Parth Mudgal [:artpar]]

I have rectified all the minor errors, like spacing, naming, char -> bool type etc.

I have some question about refractoring

-----------------------------------------------------------------
    ::: widget/windows/nsDataObj.h
    @@ +58,5 @@
    >  
    > +
    > +#include "nsICryptoHash.h"
    > +#include "JumpListItem.h"
    > +#include "JumpListBuilder.h"

    Instead of including these items let's pull out the common code into a new file instead please.

    @@ +159,5 @@
    >  {
    > +
    > +public:
    > +	static nsresult GetOutputIconPath(nsCOMPtr<nsIURI> aFaviconPageURI, 
    > +		nsCOMPtr<nsIFile> &aICOFile);

    I think this one can be factored as common code too as well as a couple of the other ones in the new file.
-----------------------------------------------------------------


the JumpListItem and JumpListBuilder contains a lot of classes which are being used in nsDataObj, actually before importing these files, i did tried to copy all the classes, but the dependency kept on increasing, so i just included these two. Factoring is more appropriate, but i was a scared to do that way (dont want things to go wrong), so since now you have seen this, should i make the new file, more the common classes and edit both nsDataObj and JumpListBuilder to include these files ?  i would then be moving these :

    class AsyncFaviconDataReady
    class AsyncWriteIconToDisk
    class AsyncDeleteIconFromDisk
    class AsyncDeleteAllFaviconsFromDisk


and 4 functions, these functions are currently members of both JumplistBuilder and nsDataObj.

    ObtainCachedIconFile
    HashURI
    GetOutputIconPath
    CacheIconFileFromFaviconURIAsync

so should i proceed ? i can can possibly make a new class out of this.

thanks

Comment 27

[Jim Mathies [:jimm]]

Some of the common utility functions can probably go into the recently added WinUtils static class -
http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.h

Comment 28

[Parth Mudgal [:artpar]]

Attachment: Patch for ShortCut icons on dragged items, and refactoring code.

Refactored the 4 classes and 4 functions mentioned in my previous comment into the file widgets/windows/WinUtils.cpp and minor editing in JumpListBuilder.cpp JumpListItem to make them use the new refactored WinUtils.h

One doubt that i have is, the now probably the dependency list(include "") of both nsDataObj and JumplistBuilder must have reduced, so should of the includes need to be removed, what is the correct way to find these ?

Comment 29

[Brian R. Bondy [:bbondy]]

Comment on attachment 590031 [details] [diff] [review]
Patch for ShortCut icons on dragged items, and refactoring code.

Review of attachment 590031 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome work, I'm really excited to see this land :)

I'll do another pass on this review tomorrow by applying the patch on my computer and trying out the code, but here are some initial comments.
I'll leave the r? flag for now but if you happen to implement the changes first then just reset it on the new patch.
If you'd rather wait until the rest of the comments that's fine too.

> One doubt that i have is, the now probably the dependency 
> list(include "") of both nsDataObj and JumplistBuilder 
> must have reduced, so should of the includes need to be 
> removed, what is the correct way to find these ?

Yes please remove any additional includes that aren't needed.
I don't know of the best way to do this, but I typically just 
comment out one or more at a time and make sure it still builds.
The building should be fast if you do incremental builds.
From within your objdir:
make -C widget
make -C toolkit/library

::: widget/windows/JumpListBuilder.h
@@ +58,5 @@
>  #include "nsIObserver.h"
>  #include "nsIFaviconService.h"
>  #include "nsThreadUtils.h"
>  
> +#include "WinUtils.h"

I think you can remove this include since it is in the .cpp already.

::: widget/windows/JumpListItem.h
@@ +139,5 @@
>    static nsresult GetShellLink(nsCOMPtr<nsIJumpListItem>& item, 
>                                 nsRefPtr<IShellLinkW>& aShellLink, 
>                                 nsCOMPtr<nsIThread> &aIOThread);
> +  static nsresult JumpListShortcut::GetJumpListShortcut(IShellLinkW *pLink, 
> +    nsCOMPtr<nsIJumpListShortcut>& aShortcut);

Please align these params with IShelLinkW (so you are adjusting to the closest open parentheses)

::: widget/windows/nsDataObj.cpp
@@ +419,5 @@
>  nsDataObj::nsDataObj(nsIURI * uri)
>    : m_cRef(0), mTransferable(nsnull),
>      mIsAsyncMode(FALSE), mIsInOperation(FALSE)
>  {
> +	NS_NewThread(getter_AddRefs(mIOThread));

nit: spacing (tab).

@@ +1164,5 @@
>    // will need to change if we ever support iDNS
>    nsCAutoString asciiUrl;
>    LossyCopyUTF16toASCII(url, asciiUrl);
>      
> +  //////////Start Edit

nit: Please remove this comment.

@@ +1175,5 @@
> +  nsAutoString aUriHash;
> +  mozilla::widget::ObtainCachedIconFile(aUri, aUriHash, mIOThread, true);
> +
> +  nsresult rv;
> +

nit: Remove newline after nsresult rv; and just combine with the line below it.
nsresult rv = rv = mozilla::widget::GetOutputIconPath(aUri, icoFile, true);

@@ +1184,5 @@
> +  
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +
> +  //static const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n";

nit: Remove this comment.

@@ +1186,5 @@
> +
> +
> +  //static const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n";
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\nIDList=\r\nHotKey=0\r\nIconFile=%s\r\nIconIndex=0\r\n";
> +  static const int formatLen = strlen(shortcutFormatStr) - 2*2; // don't include %s (2 times) in the len

nit: Space between and after the *, so: 2 * 2
nit: Also comments on the previous line

@@ +1187,5 @@
> +
> +  //static const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n";
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\nIDList=\r\nHotKey=0\r\nIconFile=%s\r\nIconIndex=0\r\n";
> +  static const int formatLen = strlen(shortcutFormatStr) - 2*2; // don't include %s (2 times) in the len
> +  const int totalLen = formatLen + asciiUrl.Length() + aUriHash.Length() + path.Length(); // we don't want a null character on the end

nit: Comment on previous line
Please make sure line length is within 80 chars.  To break up this line you'd put the newline after a + and then align the 2 lines on the right side of the equal sign.

@@ +1213,5 @@
>  
>    return S_OK;
>  } // GetFileContentsInternetShortcut
>  
> +

Remove extra newline here

::: widget/windows/nsDataObj.h
@@ +159,5 @@
>  {
> +
> +public:
> +
> +	static const char kJumpListCacheDir[];

nit: spacing (tab).

@@ +162,5 @@
> +
> +	static const char kJumpListCacheDir[];
> +
> +protected:
> +	 nsCOMPtr<nsIThread> mIOThread;

nit: spacing (tab).

Comment 30

[Brian R. Bondy [:bbondy]]

I discussed a little about this bug with rstrong and I just wanted to mention a good point he brought up. Since we store the icons inside the profile directory, it's possible someone would create a desktop shortcut, and then delete the old profile in favor for a new profile. 

So in that case the shortcut would be pointing to a bad icon location.
I think in that case Windows will just fall back to the icon of the exe like it does now.  But this is something that should be tested.

This is probably not something that we need to be concerned about, but just something I wanted to bring up.

Comment 31

[Brian R. Bondy [:bbondy]]

> So in that case the shortcut would be pointing to a bad icon location.
> I think in that case Windows will just fall back to the icon of the exe 
> like it does now.  But this is something that should be tested.

I tested with a wrong filename and unfortunately it falls back to the windows plain file icon.  This is a pretty rare case though and it's not a show stopper.

Comment 32

[Brian R. Bondy [:bbondy]]

Comment on attachment 590031 [details] [diff] [review]
Patch for ShortCut icons on dragged items, and refactoring code.

Review of attachment 590031 [details] [diff] [review]:
-----------------------------------------------------------------

Here's the rest of the previous review I did yesterday. 

The one thing that is remaining is to figure out a way to invalidate the icon once it is available so it doesn't appear as a plain file icon on the user's desktop.
One way is to resave the shortcut, but I'm not sure if that path is readily available.

I'll get back to you on that point, but for now please go ahead with implementing the rest of the review comments mentioned yesterday and today.

::: widget/windows/JumpListItem.h
@@ +146,5 @@
>    nsCOMPtr<nsIURI> mFaviconPageURI;
>    nsCOMPtr<nsILocalHandlerApp> mHandlerApp;
>  
>    bool ExecutableExists(nsCOMPtr<nsILocalHandlerApp>& handlerApp);
> +

Since you have HashURI inside the WinUtils file now, you can remove it's declaration here inside JumpListItem.h file's JumpListItem class.

::: widget/windows/WinUtils.cpp
@@ +422,5 @@
>  
> +
> +AsyncFaviconDataReady::AsyncFaviconDataReady(nsIURI *aNewURI, 
> +  nsCOMPtr<nsIThread> &aIOThread
> +  , const bool aCachedir) //To add support for both "jumplistcache" and "shortcutcache" icon folders

nit:
Please document the parameter above the function instead.
Javadoc would be best:

/**
 * Constructs an AsyncFaviconDataReady object
 *
 * @param aIOThread The thread to perform the action on
 * @param aCachedir Differentiates between jumpListCache and shortcuteCache folders
*/

@@ +425,5 @@
> +  nsCOMPtr<nsIThread> &aIOThread
> +  , const bool aCachedir) //To add support for both "jumplistcache" and "shortcutcache" icon folders
> +  : mNewURI(aNewURI), 
> +  mIOThread(aIOThread),
> +  dir(aCachedir)

nit: 
alignment of the constructor's initializer list should be aligned to the first parameter (so add 2 spaces)

@@ +442,5 @@
> +
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsresult rv;
> +
> +  rv = GetOutputIconPath(mNewURI, icoFile, dir);

nit: Combine nresult rv; line with this one.

@@ +462,5 @@
> +  //AsyncWriteIconToDisk takes ownership of the heap allocated buffer.
> +  nsCOMPtr<nsIRunnable> event = new AsyncWriteIconToDisk(path, aMimeType, 
> +    data, 
> +    aDataLen,
> +    dir);

nit: Please align these parameters to the opening parentheses.

@@ +652,5 @@
> +{
> +}
> +
> +
> +

nit: Remove all but one of these newline whitespace.

@@ +660,5 @@
> +// for the icon so that next time the method is called it will be available. 
> +nsresult ObtainCachedIconFile(nsCOMPtr<nsIURI> aFaviconPageURI,
> +  nsString &aICOFilePath,
> +  nsCOMPtr<nsIThread> &aIOThread,
> +  bool aCacheDir)                    // False for "JumpListcache", true for "shortcutcache"

Please put the comment on aCacheDir above the function, javadoc as before is best.

@@ +678,5 @@
> +    // Obtain the file's last modification date in seconds
> +    PRInt64 fileModTime = LL_ZERO;
> +    rv = icoFile->GetLastModifiedTime(&fileModTime);
> +    fileModTime /= PR_MSEC_PER_SEC;
> +    PRInt32 icoReCacheSecondsTimeout = 0;

Please use GetICOCacheSecondsTimeout() here.
Please move it to WinUtils inside the helper class with static functions I mentioned in the other comment.

@@ +757,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if(!aCacheDir)
> +    rv = aICOFile->AppendNative(nsDependentCString("jumplistcache"));
> +  else
> +    rv = aICOFile->AppendNative(nsDependentCString("shortcutcache"));

Please use a static const char kJumpListCacheDir[] for this inside of winutils and remove the one in jumplistitem.h.  Reference the new one inside jumplistitem.cpp.

::: widget/windows/WinUtils.h
@@ +232,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIFAVICONDATACALLBACK
> +  const bool dir;

nit: Please name this mCacheDir

@@ +233,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIFAVICONDATACALLBACK
> +  const bool dir;
> +  AsyncFaviconDataReady(nsIURI *aNewURI, nsCOMPtr<nsIThread> &aIOThread, const bool aCachedir=false);

nit: const bool aCachedir = false

@@ +245,5 @@
> +  */
> +class AsyncWriteIconToDisk : public nsIRunnable
> +{
> +public:
> +	const bool dir;

nit: Instead of tab should be spaces.

@@ +254,5 @@
> +  AsyncWriteIconToDisk(const nsAString &aIconPath,
> +                       const nsACString &aMimeTypeOfInputData,
> +                       PRUint8 *aData, 
> +                       PRUint32 aDataLen,
> +					             const bool aCachedir);

nit: Some tabs mixed with spaces here.

@@ +303,5 @@
> +nsresult 
> +  CacheIconFileFromFaviconURIAsync(nsCOMPtr<nsIURI> aFaviconPageURI,
> +  nsCOMPtr<nsIFile> aICOFile,
> +  nsCOMPtr<nsIThread> &aIOThread,
> +  bool aCacheDir);

For these functions: ObtainCachedIconFile, HashURI, GetOutputIconPath, CacheIconFileFromFaviconURIAsync 
Please make them static and put them inside a class called FaviconHelper as public members.
nit: Please fix spacing for all of the parameters of these function declarations so they are aligned with the previous line's opening parentheses.

::: widget/windows/nsDataObj.cpp
@@ +82,5 @@
>  #include <stdlib.h>
>  
>  using namespace mozilla;
>  
> +const char nsDataObj::kJumpListCacheDir[] = "shortCutCache";

Move this inside winutils so you can use it from both places.

@@ +1183,5 @@
> +  rv = icoFile->GetNativePath(path);
> +  
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +

nit: Pls remove extra newline

@@ +1185,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +
> +  //static const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n";
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\nIDList=\r\nHotKey=0\r\nIconFile=%s\r\nIconIndex=0\r\n";

Please remove these values since they aren't needed for the icon URL file:
IDList=
HotKey=0

::: widget/windows/nsDataObj.h
@@ +159,5 @@
>  {
> +
> +public:
> +
> +	static const char kJumpListCacheDir[];

nit: remove newline above this line so this follows directly after public: with no newline after public:

Comment 33

[Brian R. Bondy [:bbondy]]

By the way I wanted to also note that I found on Windows 7 you can use:
IconFile=Some_HTTP_URL_To_A_Fav_Icon_ICO_FORMAT

But this is not a good idea for several reasons:
- Favicons on different sites use different file formats, not only ICO. 
- Some sites use PNG ICOs and these are not supported on XP.
- It would only work on Windows 7 and a large portion of our users are Vista and below.

Internet explorer will use the favicon file from the site if on Windows 7 and if the favicon is in ICO format.  Chrome and Opera do not generate icons on these shortcuts.

Comment 34

[Brian R. Bondy [:bbondy]]

jimm/neil please let me know what you think...

> The one thing that is remaining is to figure out a way to invalidate the 
> icon once it is available so it doesn't appear as a plain file icon on 
> the user's desktop.
> One way is to resave the shortcut, but I'm not sure if that 
> path is readily available.

So the problem is that the icon is created on disk after the shortcut is created. And Windows won't refresh the icon as soon as you write it, it will once you press F5 to refresh though.

I spent a few hours on this.  As far as I can tell, getting the drop filename for program generated content is not possible. It's something I've ran into in the past as well and couldn't solve.  The problem is the data is transferred from source program to target program and the target program can do anything with it.  One option would be to watch for filesystem changes on drag of URL files and look for a .URL drop.  I think the below is a better option though.

This reliably will refresh the icons that show a blank file icon on any and all explorer window:
SendMessage(HWND_BROADCAST, WM_SETTINGCHANGE, SPI_SETNONCLIENTMETRICS, 0);
It would be called not on the main thread once the icon is written to disk.

We could just get all the top level explorer windows to avoid a complete broadcast and send the message to only those windows.

Comment 35

[Brian R. Bondy [:bbondy]]

2 more points:

1) 

ICO files we are currently generating will not work on Windows XP and Windows 2000.

The ICO format is a container format for 1 or more images.  Before Windows Vista they could contain only BMPs.  As of Windows Vista and later they also support PNG files inside.
The way we generate the icon is with the function EncodeScaledImage, passing in the mime type for icon files.  We specify no encoder options, so it uses the default of ICO PNG files. 

These files work great for jump lists because Jump lists only exist on Win7 and later, but for this feature we need to generate ICO BMPs, since it needs to work pre-vista.

The good news is that it is a lot harder to explain this problem than it is to fix it :) 
You just need to change the call to WinUtil's EncodeScaledImage function to pass in encoder options like this: format=bmp&bpp=32 instead of EmptyString().

----

2) The icons that are generated are a bad resolution, you brought this up to me in email.  This is because most favicons are 16x16, but desktop icons are larger than this.

This can be improved by letting Windows handle the resize.  
I'd like to one day improve the way we resize images, but for now Windows does a better job at it than our own code.
There is an equivalent call to EncodeScaledImage you can use called EncodeImage.

This resolution problem could be right out solved when we have smaller icons by drawing a smaller version of the icon on a larger generic file icon.

The first method of letting Windows handle the resize I think would be best for the scope of this task.

Comment 36

[[e-mail]]

(In reply to Brian R. Bondy from comment #33)
> Internet explorer will use the favicon file from the site if on Windows 7
> and if the favicon is in ICO format.  Chrome and Opera do not generate icons
> on these shortcuts.
I tried IE8 (not Windows 7) and it used the favicon from the site that I dragged to the desktop even though the .url file doesn't mention it.

Comment 37

[[e-mail]]

Ah, apparently it automatically does this if the icon is in IE's cache.

Comment 38

[James May [:fowl2]]

I've noticed on 7 at least icons seem to be stored in a alt stream called "favicon".

Comment 39

[Brian R. Bondy [:bbondy]]

> I've noticed on 7 at least icons seem to be stored in a alt stream called "favicon".

Stored on the .URL file?

Alternate data streams would be a great idea as long as the user is on an NTFS filesystem (which is most likely the case).  That would solve the problem of deleting an old profile as well.  But the problem with that is we don't know the path of the .URL file.

For anyone that doesn't know, in NTFS you can have one or more streams associated with a file. There is always an unamed stream that everyone knows about, but you can also have named streams which are referred to as Alternate Data Streams (ADS).

You can create them for a file named test.txt like so:

> notepad test.txt
> notepad test.txt:adsname1
> notepad test.txt:adsname2

All 3 will be shown in explorer as 1 file but they are all different streams attached to that same filename.
You can use normal Win32 APIs with these filenames with ":the_ads_name" at the end and work with the different streams directly.

Comment 40

[James May [:fowl2]]

Dragged out of the favorites menu. It seems the .website files that happen when you drag out tabs or the address bar work differently.

>dir /r
28/Dec/2011  10:55 PM               302 Suggested Sites.url
                                    894 Suggested Sites.url:favicon:$DATA

>type "Suggested Sites.url"
[{000214A0-0000-0000-C000-000000000046}]
Prop3=19,11
[InternetShortcut]
URL=https://ieonline.microsoft.com/#ieslice
IDList=
IconFile=https://ieonline.microsoft.com/favicon.ico
IconIndex=1
[MonitoredItem]
FeedUrl=https://ieonline.microsoft.com/#ieslice
PreviewSize=320x240
IsLivePreview=true

Comment 41

[Parth Mudgal [:artpar]]

Brian,

I made all the changes till comment #32, and now for some reason, the icons in the jumplist dont show up. i deleted the icons in the cache, and the icons come there after some time, but they don't show up in the jumplist. all icons are the blank page with nightly icon on them.

http://imgur.com/Ipl1h

on other side, shortcut is working fine yet. which part of code should i be looking in ? i debugged it but couldn't find any appropriate information.

Some more issues
1) ->
Comment #35, first point
-----------
change the call to WinUtil's EncodeScaledImage function to pass in encoder options like this: format=bmp&bpp=32 instead of EmptyString()
-----------

i changed 
EmptyString(),
to
NS_LITERAL_STRING("format=bmp&bpp=32"),

if i do this, new icons are not build anymore, the command prompt shows :

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file d:/mozilla-src2/image/encoders/ico/nsICOEncoder.cpp, line 260
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file d:/mozilla-src2/image/encoders/ico/nsICOEncoder.cpp, line 102

if i change it back to EmptyString(), icons are fine again.


2) ->


> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\nIDList=\r\nHotKey=0\r\nIconFile=%s\r\nIconIndex=0\r\n";

Please make sure line length is within 80 chars.  To break up this line you'd put the newline after a + and then align the 2 lines on the right side of the equal sign.


Well, i changed it to :

static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" + 
                                 "IconFile=%s\r\nIconIndex=0\r\n";

then i get "Cannot add Pointers" Error while compiling.

Comment 42

[Roasted]

For whatever reason icons for URLs that don't show up on the desktop appear if you once load the URL in Internet Explorer, no matter which browser created the icon in the first place. Just saying in case it might be important.

Comment 43

[Brian R. Bondy [:bbondy]]

> Well, i changed it to :
>
> static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" + 
>                                 "IconFile=%s\r\nIconIndex=0\r\n";
>
> then i get "Cannot add Pointers" Error while compiling.

You can append 2 string literals just by writing one after the other. So just get rid of the + sign.

> static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" 
>                                  "IconFile=%s\r\nIconIndex=0\r\n";



Regarding: 

> NS_LITERAL_STRING("format=bmp&bpp=32"),

You need to use ; instead of &.  So:

> NS_LITERAL_STRING("format=bmp;bpp=32"),


> I made all the changes till comment #32, and now for some reason, 
> the icons in the jumplist dont show up.

This might simply be a cache issue (rebooting windows might fix it) or it might just be that icons take 120 seconds to rebuild the jump list. So it will at first look like that.   If that still doesn't work try seeing if icons are being created at jumpListCache.  If that still doesn't work try using windiff to compare the old classes to the moved ones to see exactly where the changes are that could have caused the problem.

Comment 44

[Parth Mudgal [:artpar]]

Okay, done the two things, & -> ; and remove + ... they work now :)

jumplist icons, icons show up fine in the jumplistcache folder even after deleting, but wont come up in the jumplist. i will try windiff and update here.

Comment 45

[Parth Mudgal [:artpar]]

Attachment: new patch with SendMessage added after .ico file is saved

This is the patch with SendMessage added to AsyncWriteIconToDisk::Run() after .ICO is saved in WinUtils.cpp

All other points in the previous comments were done, until commant #32

file is not stable, jumplist items icons are not appearing and i am not able to locate it.

other querie
1. icons are not fetched for websites which have not been visited already, is that expected ?

Comment 46

[Brian R. Bondy [:bbondy]]

Comment on attachment 590031 [details] [diff] [review]
Patch for ShortCut icons on dragged items, and refactoring code.

Marking the old patch as obsolete, will be doing the review on the new patch shortly.

Comment 47

[Brian R. Bondy [:bbondy]]

Comment on attachment 593542 [details] [diff] [review]
new patch with SendMessage added after .ico file is saved

Review of attachment 593542 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good and you're getting very close :)

Most of the comments are just code cleanup. 
I did see why jump lists aren't working too and I commented inline below, fast fix.
I also put a note on how to fix the icon resolution so that we don't scale the icons ourselves.

I think the only thing left after that is that we should cache the old icon we used to use out to disk at the same path if we find that no favicon can be obtained in the thread.

::: widget/windows/JumpListBuilder.cpp
@@ +237,5 @@
>    nsCOMPtr<nsIFile> jumpListCacheDir;
>    nsresult rv = NS_GetSpecialDirectory("ProfLDS", 
>                                         getter_AddRefs(jumpListCacheDir));
>    NS_ENSURE_SUCCESS(rv, rv);
> +  rv = jumpListCacheDir->AppendNative(nsDependentCString(FaviconHelper::kJumpListCacheDir));

Nit: split this to 80 chars per line.
rv = jumpListCacheDir->AppendNative(
  nsDependentCString(FaviconHelper::kJumpListCacheDir));

::: widget/windows/JumpListItem.cpp
@@ +63,4 @@
>  namespace mozilla {
>  namespace widget {
>  
> +//const char FaviconHelper::kJumpListCacheDir[] = "jumpListCache";

nit: This commented line should be removed.

::: widget/windows/WinUtils.cpp
@@ +61,5 @@
>  #include "nsGUIEvent.h"
>  #include "nsIDOMMouseEvent.h"
>  #include "mozilla/Preferences.h"
>  
> +#include "nsError.h"

Remove '#include "nsError.h"'

@@ +62,5 @@
>  #include "nsIDOMMouseEvent.h"
>  #include "mozilla/Preferences.h"
>  
> +#include "nsError.h"
> +#include "nsCOMPtr.h"

Remove '#include "nsCOMPtr.h"'

@@ +63,5 @@
>  #include "mozilla/Preferences.h"
>  
> +#include "nsError.h"
> +#include "nsCOMPtr.h"
> +#include "nsServiceManagerUtils.h"

Remove '#include "nsServiceManagerUtils.h"'

@@ +64,5 @@
>  
> +#include "nsError.h"
> +#include "nsCOMPtr.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsAutoPtr.h"

Remove '#include "nsAutoPtr.h"'

@@ +65,5 @@
> +#include "nsError.h"
> +#include "nsCOMPtr.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsAutoPtr.h"
> +#include "nsString.h"

Remove '#include "nsAutoPtr.h"'

@@ +66,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsAutoPtr.h"
> +#include "nsString.h"
> +#include "nsArrayUtils.h"

Remove '#include "nsArrayUtils.h"'

@@ +67,5 @@
> +#include "nsServiceManagerUtils.h"
> +#include "nsAutoPtr.h"
> +#include "nsString.h"
> +#include "nsArrayUtils.h"
> +#include "nsIMutableArray.h"

Remove '#include "nsIMutableArray.h"'

@@ +68,5 @@
> +#include "nsAutoPtr.h"
> +#include "nsString.h"
> +#include "nsArrayUtils.h"
> +#include "nsIMutableArray.h"
> +#include "nsWidgetsCID.h"

Remove '#include "nsWidgetsCID.h"'

@@ +69,5 @@
> +#include "nsString.h"
> +#include "nsArrayUtils.h"
> +#include "nsIMutableArray.h"
> +#include "nsWidgetsCID.h"
> +#include "WinTaskbar.h"

Remove '#include "WinTaskbar.h"'

@@ +70,5 @@
> +#include "nsArrayUtils.h"
> +#include "nsIMutableArray.h"
> +#include "nsWidgetsCID.h"
> +#include "WinTaskbar.h"
> +#include "nsDirectoryServiceUtils.h"

Remove '#include "WinTaskbar.h"'

@@ +71,5 @@
> +#include "nsIMutableArray.h"
> +#include "nsWidgetsCID.h"
> +#include "WinTaskbar.h"
> +#include "nsDirectoryServiceUtils.h"
> +#include "nsISimpleEnumerator.h"

Remove '#include "nsISimpleEnumerator.h"'

@@ +72,5 @@
> +#include "nsWidgetsCID.h"
> +#include "WinTaskbar.h"
> +#include "nsDirectoryServiceUtils.h"
> +#include "nsISimpleEnumerator.h"
> +#include "mozilla/Preferences.h"

Remove '#include "mozilla/Preferences.h"'

@@ +77,5 @@
> +#include "imgIContainer.h"
> +#include "imgITools.h"
> +#include "nsStringStream.h"
> +#include "nsNetUtil.h"
> +#include "nsThreadUtils.h"

Remove '#include "nsThreadUtils.h"'

@@ +80,5 @@
> +#include "nsNetUtil.h"
> +#include "nsThreadUtils.h"
> +#include "mozIAsyncFavicons.h"
> +
> +#include "JumpListBuilder.h"

Remove '#include "JumpListBuilder.h"'

@@ +81,5 @@
> +#include "nsThreadUtils.h"
> +#include "mozIAsyncFavicons.h"
> +
> +#include "JumpListBuilder.h"
> +#include "nsDataObj.h"

Remove '#include "nsDataObj.h"

@@ +521,5 @@
> +  }
> +  if(mCacheDir) {
> +    systemIconWidth = 32;
> +    systemIconHeight = 32;
> +  }

I think you'll get a better icon resolution if you don't use EncodeScaledImage and instead use EncodeImage when mCacheDir is true.  If that's the case you can put the if (!mCacheDir) around the whole PRInt32 systemIconWidth variable declaration all the way until after EncodeScaledImage.

@@ +529,5 @@
> +
> +  rv = imgtool->EncodeScaledImage(container, mMimeTypeOfInputData,
> +    systemIconWidth,
> +    systemIconHeight,
> +    //EmptyString(),

nit: Remove commented line of code.

@@ +530,5 @@
> +  rv = imgtool->EncodeScaledImage(container, mMimeTypeOfInputData,
> +    systemIconWidth,
> +    systemIconHeight,
> +    //EmptyString(),
> +    NS_LITERAL_STRING("format=bmp;bpp=32"),

Please keep EmptyString() if dealing with a jump list image. It's better to generate those as PNG since jump lists are only on Win7 and Win Vista and higher support PNG ICOs.  I think that might also fix the problem with jump lists not working.

@@ +553,5 @@
> +  // Setup a buffered output stream from the stream object
> +  // so that we can simply use WriteFrom with the stream object
> +  nsCOMPtr<nsIOutputStream> bufferedOutputStream;
> +  rv = NS_NewBufferedOutputStream(getter_AddRefs(bufferedOutputStream),
> +    outputStream, bufSize);

nit: This should be aligned to just after the opening parentheses.

@@ +559,5 @@
> +
> +  // Write out the icon stream to disk and make sure we wrote everything
> +  PRUint32 wrote;
> +  rv = bufferedOutputStream->WriteFrom(iconStream, bufSize, &wrote);
> +  NS_ASSERTION(bufSize == wrote, "Icon wrote size should be equal to requested write size");

nit: This line is > 80 chars, please split it up.  Align to parentheses.

@@ +564,5 @@
> +
> +  // Cleanup
> +  bufferedOutputStream->Close();
> +  outputStream->Close();
> +  SendMessage(HWND_BROADCAST, WM_SETTINGCHANGE, SPI_SETNONCLIENTMETRICS, 0);

This should not be called for jump list calls.

@@ +619,5 @@
> +  nsCOMPtr<nsIFile> jumpListCacheDir;
> +  nsresult rv = NS_GetSpecialDirectory("ProfLDS", 
> +    getter_AddRefs(jumpListCacheDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = jumpListCacheDir->AppendNative(nsDependentCString(FaviconHelper::kJumpListCacheDir));

nit: For this case since there is no good place to break the line do like this with 2 spaces on the next line:
rv = jumpListCacheDir->AppendNative(
  nsDependentCString(FaviconHelper::kJumpListCacheDir));

@@ +669,5 @@
> +/************************************************************************/
> +nsresult FaviconHelper::ObtainCachedIconFile(nsCOMPtr<nsIURI> aFaviconPageURI,
> +                              nsString &aICOFilePath,
> +                              nsCOMPtr<nsIThread> &aIOThread,
> +                              bool aCacheDir)

nit: This parameters should be aligned to just after the opening parentheses.

@@ +685,5 @@
> +  if (exists) {
> +
> +    // Obtain the file's last modification date in seconds
> +    PRInt64 fileModTime = LL_ZERO;
> +    rv = GetICOCacheSecondsTimeout();

This line is what is causing jump lists to fail.
It should be: 
rv = icoFile->GetLastModifiedTime(&fileModTime);

@@ +687,5 @@
> +    // Obtain the file's last modification date in seconds
> +    PRInt64 fileModTime = LL_ZERO;
> +    rv = GetICOCacheSecondsTimeout();
> +    fileModTime /= PR_MSEC_PER_SEC;
> +    PRInt32 icoReCacheSecondsTimeout = 0;

And this line should be: 
PRInt32 icoReCacheSecondsTimeout = GetICOCacheSecondsTimeout();

::: widget/windows/WinUtils.h
@@ +51,5 @@
>  
>  #include "nscore.h"
>  #include <windows.h>
>  #include <shobjidl.h>
> +#include "nsCOMPtr.h"

Remove '#include "nsCOMPtr.h"'

@@ +232,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIFAVICONDATACALLBACK
> +  const bool mCacheDir;

Move mCacheDir to the end after the mIOThread variable declaration.
nit: Please also do a find/replace in your new code for mCacheDir and replace with mURLShortcut.  I think that's a better name.  Ditto aCacheDir.

@@ +233,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIFAVICONDATACALLBACK
> +  const bool mCacheDir;
> +  AsyncFaviconDataReady(nsIURI *aNewURI, nsCOMPtr<nsIThread> &aIOThread, const bool aCachedir);

nit: Max line should be 80 chars.  Align to opening parentheses.  Newline after a comma.

@@ +245,5 @@
> +  */
> +class AsyncWriteIconToDisk : public nsIRunnable
> +{
> +public:
> +	const bool mCacheDir;

nit: Remove tab in favor of 2 spaces.

@@ +287,5 @@
> +  AsyncDeleteAllFaviconsFromDisk();
> +  virtual ~AsyncDeleteAllFaviconsFromDisk();
> +};
> +
> +class FaviconHelper{

nit: Brace on a newline by itself.

@@ +305,5 @@
> +                                    nsCOMPtr<nsIFile> &aICOFile,
> +                                    bool aCacheDir);
> +
> +  static nsresult 
> +    CacheIconFileFromFaviconURIAsync(nsCOMPtr<nsIURI> aFaviconPageURI,

nit: Move this back 2 spaces and hence also the parameters that follow on the following lines.

::: widget/windows/nsDataObj.cpp
@@ +69,5 @@
>  #include "nsITimer.h"
>  #include "nsThreadUtils.h"
>  
> +#include "WinUtils.h"
> +#include "mozIAsyncFavicons.h"

Remove '#include "mozIAsyncFavicons.h"'

@@ +70,5 @@
>  #include "nsThreadUtils.h"
>  
> +#include "WinUtils.h"
> +#include "mozIAsyncFavicons.h"
> +#include "nsThreadUtils.h"

Remove '#include "nsThreadUtils.h"'

@@ +71,5 @@
>  
> +#include "WinUtils.h"
> +#include "mozIAsyncFavicons.h"
> +#include "nsThreadUtils.h"
> +#include "nsICryptoHash.h"

Remove '#include "nsICryptoHash.h"'

@@ +72,5 @@
> +#include "WinUtils.h"
> +#include "mozIAsyncFavicons.h"
> +#include "nsThreadUtils.h"
> +#include "nsICryptoHash.h"
> +#include "nsIFile.h"

Remove '#include "nsIFile.h"'

@@ +73,5 @@
> +#include "mozIAsyncFavicons.h"
> +#include "nsThreadUtils.h"
> +#include "nsICryptoHash.h"
> +#include "nsIFile.h"
> +#include "nsStringStream.h"

Remove '#include "nsStringStream.h"'

@@ +74,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsICryptoHash.h"
> +#include "nsIFile.h"
> +#include "nsStringStream.h"
> +#include "imgITools.h"

Remove '#include "imgITools.h"'

@@ +75,5 @@
> +#include "nsICryptoHash.h"
> +#include "nsIFile.h"
> +#include "nsStringStream.h"
> +#include "imgITools.h"
> +#include "mozilla/Preferences.h"

Remove '#include "mozilla/Preferences.h"'

@@ +82,5 @@
>  #include <stdlib.h>
>  
>  using namespace mozilla;
>  
> +//const char widget::FaviconHelper::kShortcutCacheDir[] = "shortCutCache";

Remove this comment line

@@ +418,5 @@
>  nsDataObj::nsDataObj(nsIURI * uri)
>    : m_cRef(0), mTransferable(nsnull),
>      mIsAsyncMode(FALSE), mIsInOperation(FALSE)
>  {
> +  NS_NewThread(getter_AddRefs(mIOThread));

This will create a thread a lot of times when it is not needed, please move this closer to where it is actually needed.

@@ +1179,5 @@
> +
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" 
> +                                   "IconFile=%s\r\nIconIndex=0\r\n";
> +
> +  static const int formatLen = strlen(shortcutFormatStr) - 2 * 2; // don't include %s (2 times) in the len

nit: I know this isn't your comment, but please move comment above the line since it exceeds 80 chars.  Maybe even put 2 * 2 in parentheses even know it's not needed, I think it helps code readability in this case as it looks like a bug at first glance but it is not.

@@ +1180,5 @@
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" 
> +                                   "IconFile=%s\r\nIconIndex=0\r\n";
> +
> +  static const int formatLen = strlen(shortcutFormatStr) - 2 * 2; // don't include %s (2 times) in the len
> +  const int totalLen = formatLen + asciiUrl.Length() + aUriHash.Length() + path.Length(); // we don't want a null character on the end

nit: ditto about comment, also line is too long and should be split and aligned to the right of the equal sign.

::: widget/windows/nsDataObj.h
@@ +56,5 @@
>  #include "nsCOMArray.h"
>  #include "nsITimer.h"
>  
> +
> +#include "nsICryptoHash.h"

Remove '#include "nsICryptoHash.h"' and above extra whitespace.

@@ +57,5 @@
>  #include "nsITimer.h"
>  
> +
> +#include "nsICryptoHash.h"
> +#include "WinUtils.h"

Remove '#include "WinUtils.h"'.

@@ +158,5 @@
>                    public IAsyncOperation
>  {
> +
> +public:
> +  static const char kJumpListCacheDir[];

Remove the above 3 lines.

Comment 48

[Parth Mudgal [:artpar]]

Attachment: Updated till comment #47, JumpList icons working

Performed all changed in comment #47.

The icons in the JumpList are working.

One thing which remains, if icons are not fetched, or failed, then we have to put the old icon.

We probably cannot change the .URL (shortcut) file, so that leaves us with copying the Mozilla icon file to the shortcutcache dir with the new file name.

i found mozilla ico file in the source dir in dist/branding but i fail to find it out in the actual installation folder. Where do i take the icon from ?

on the implementation side, what i think of is
i will change 
rv = bufferedOutputStream->WriteFrom(iconStream, bufSize, &wrote);

to write the mozilla icon(stream pointing to mozilla icon file ?) if bufSize is Zero or something.

please advice.

Comment 49

[Brian R. Bondy [:bbondy]]

Comment on attachment 594405 [details] [diff] [review]
Updated till comment #47, JumpList icons  working

Review of attachment 594405 [details] [diff] [review]:
-----------------------------------------------------------------

Everything's looking good.  
There are a few more formatting things that need to be fixed and the main thing left as you 
mentioned is just fetching the icon when a favicon doesn't exist.

> how do I get the icon of the normal URL file 

There are a couple ways you can do this and I'm fine with either one.

1) You can try to load the image via a URL like this:
moz-icon://.url?size=32
And then save it to stream similar to the existing code.

Or 2) you can just use code like this and go the Win32 shell API only way:
http://stackoverflow.com/questions/829843/how-to-get-icon-and-description-from-file-extension-using-delphi

::: widget/windows/JumpListBuilder.cpp
@@ +239,5 @@
>                                         getter_AddRefs(jumpListCacheDir));
>    NS_ENSURE_SUCCESS(rv, rv);
> +  rv = jumpListCacheDir->AppendNative(nsDependentCString(
> +                                      FaviconHelper::kJumpListCacheDir)
> +                                      );

nit: Put closing parentheses on the previous line

::: widget/windows/WinUtils.cpp
@@ +418,5 @@
> +                                             nsCOMPtr<nsIThread> &aIOThread, 
> +                                             const bool aURLShortcut)
> +                                             : mNewURI(aNewURI), 
> +                                             mIOThread(aIOThread),
> +                                             mURLShortcut(aURLShortcut)

nit:
Format the initializer list like this:
Put : on the previous line.
aligh the other 3 lines of paraemter initializations 2 spaces from the start of the line.

@@ +426,5 @@
> +NS_IMETHODIMP
> +  AsyncFaviconDataReady::OnFaviconDataAvailable(nsIURI *aFaviconURI, 
> +  PRUint32 aDataLen,
> +  const PRUint8 *aData, 
> +  const nsACString &aMimeType)

nit:
When you put a type on its own line please align the function name directly under it. 
So remove the 2 spaces before the function name.
Also the parameters should be aligned to the (

@@ +469,5 @@
> +  : mIconPath(aIconPath),
> +  mMimeTypeOfInputData(aMimeTypeOfInputData),
> +  mBuffer(aBuffer),
> +  mBufferLength(aBufferLength),
> +  mURLShortcut(aURLShortcut)

nit:
Parameters of the constructor should be aligned to the (.
The initializer list should have the : on the previous line, but it is formatted correctly here otherwise.

@@ +483,5 @@
> +  nsCOMPtr<nsIInputStream> stream;
> +  nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream),
> +    reinterpret_cast<const char*>(mBuffer.get()), 
> +    mBufferLength,
> +    NS_ASSIGNMENT_DEPEND);

nit: 
I think the correct formatting in this situation is: (it might not show up correctly because of font spacing)
nsresult rv = 
  NS_NewByteInputStream(getter_AddRefs(stream),
                        reinterpret_cast<const char*>(mBuffer.get()),
                        mBufferLength,
                        NS_ASSIGNMENT_DEPEND);

@@ +490,5 @@
> +  // Decode the image from the format it was returned to us in (probably PNG)
> +  nsCOMPtr<imgIContainer> container;
> +  nsCOMPtr<imgITools> imgtool = do_CreateInstance("@mozilla.org/image/tools;1");
> +  rv = imgtool->DecodeImageData(stream, mMimeTypeOfInputData, 
> +    getter_AddRefs(container));

nit: align to open parentheses.

@@ +610,5 @@
> +  nsresult rv = NS_GetSpecialDirectory("ProfLDS", 
> +    getter_AddRefs(jumpListCacheDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = jumpListCacheDir->AppendNative(
> +      nsDependentCString(FaviconHelper::kJumpListCacheDir));

nit: Either remove 2 spaces on the subsequent line or align to one space after the equal sign.

@@ +656,5 @@
> +// exist, or its cache is expired, this method will kick off an async request
> +// for the icon so that next time the method is called it will be available. 
> +/************************************************************************/
> +/* @param aURLShortcut : to distinguish between jumplistcache(false) and shortcutcache(true)
> +/************************************************************************/

Let's just change this block to use javasdoc:

/**
 * (static) If the data is available, will return the path on disk where
 * the favicon for page aFaviconPageURI is stored. If the favicon does not
 * exist, or its cache is expired, this method will kick off an async request
 * for the icon so that next time the method is called it will be available.
 *
 * @param aFaviconPageURI The URI of the page to obtain
 * @param aICOFilePath    The file path of the icon file
 * @param aIOThread       The thread to perform the fetch on
 * @param aURLShortcut    Used to distinguish between jumplistcache(false) and shortcutcache(true)
 * @return NS_OK on success
*/

@@ +685,5 @@
> +    // If the last mod call failed or the icon is old then re-cache it
> +    // This check is in case the favicon of a page changes
> +    // the next time we try to build the jump list, the data will be available.
> +    if (NS_FAILED(rv) ||
> +      (nowTime - fileModTime) > icoReCacheSecondsTimeout) {

nit: add 2 spaces, should be aligned to inside the (

@@ +687,5 @@
> +    // the next time we try to build the jump list, the data will be available.
> +    if (NS_FAILED(rv) ||
> +      (nowTime - fileModTime) > icoReCacheSecondsTimeout) {
> +        CacheIconFileFromFaviconURIAsync(aFaviconPageURI, icoFile, aIOThread, aURLShortcut);
> +        return NS_ERROR_NOT_AVAILABLE;

nit: These lines have too much indent (only 2 spaces)

@@ +703,5 @@
> +  return rv;
> +}
> +
> +nsresult FaviconHelper::HashURI(nsCOMPtr<nsICryptoHash> &aCryptoHash, 
> +  nsIURI *aUri, nsACString& aUriHash)

nit: Align to opening parentheses.

@@ +720,5 @@
> +
> +  rv = aCryptoHash->Init(nsICryptoHash::MD5);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = aCryptoHash->Update(reinterpret_cast<const PRUint8*>(spec.BeginReading()), 
> +    spec.Length());

nit: align to opening parentheses.

@@ +741,5 @@
> +  // Hash the input URI and replace any / with _
> +  nsCAutoString inputURIHash;
> +  nsCOMPtr<nsICryptoHash> cryptoHash;
> +  nsresult rv = HashURI(cryptoHash, aFaviconPageURI,
> +    inputURIHash);

nit: Align to opening parentheses. 
Unfortunately VS reformats when you paste by default (that's probably why these are showing up)

@@ +779,5 @@
> +nsresult 
> +  FaviconHelper::CacheIconFileFromFaviconURIAsync(nsCOMPtr<nsIURI> aFaviconPageURI,
> +  nsCOMPtr<nsIFile> aICOFile,
> +  nsCOMPtr<nsIThread> &aIOThread,
> +  bool aURLShortcut)

Align these parameters to just after the opening parentheses.
The 2 space before line is only for initializer lists on constructors.

@@ +786,5 @@
> +  nsCOMPtr<mozIAsyncFavicons> favIconSvc(
> +    do_GetService("@mozilla.org/browser/favicon-service;1"));
> +  NS_ENSURE_TRUE(favIconSvc, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsIFaviconDataCallback> callback = new mozilla::widget::AsyncFaviconDataReady(aFaviconPageURI, aIOThread, aURLShortcut);

nit: Put this part on a new line 2 spaces from the start of the line.
  new mozilla::widget::AsyncFaviconDataReady(aFaviconPageURI, aIOThread, aURLShortcut);

@@ +810,5 @@
> +
> +  // Obtain the pref
> +  const char PREF_ICOTIMEOUT[]  = "browser.taskbar.lists.icoTimeoutInSeconds";
> +  icoReCacheSecondsTimeout = Preferences::GetInt(PREF_ICOTIMEOUT, 
> +    kSecondsPerDay);

nit: align to just after the opening parentheses.

::: widget/windows/nsDataObj.cpp
@@ -1148,5 @@
>    nsAutoString url;
>    if ( NS_FAILED(ExtractShortcutURL(url)) )
>      return E_OUTOFMEMORY;
>  
> -  // will need to change if we ever support iDNS

nit: Leave this comment since not related to this task.

@@ +1172,5 @@
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" 
> +                                   "IconFile=%s\r\nIconIndex=0\r\n";
> +
> +  // don't include %s (2 times) in the len
> +  static const int formatLen = strlen(shortcutFormatStr) - 2 * 2; 

nit: Put parentheses around 2*2 even know it's not needed for code clarity, or just change to 4.

@@ +1176,5 @@
> +  static const int formatLen = strlen(shortcutFormatStr) - 2 * 2; 
> +  
> +  // we don't want a null character on the end
> +  const int totalLen = formatLen + asciiUrl.Length() + aUriHash.Length() 
> +                       + path.Length(); 

nit: + operator on the previous line

Comment 50

[Parth Mudgal [:artpar]]

Attachment: Patch with formatted code up to comment #49

This patch is with the Formatting as specified up to comment #49.

Another patch for the default icon part will follow this.

Comment 51

[Brian R. Bondy [:bbondy]]

Please put the new code in the same patch, only the one file we talked about should go in its own patch because someone else has to review that file.

Comment 52

[Brian R. Bondy [:bbondy]]

Comment on attachment 595214 [details] [diff] [review]
Patch with formatted code up to comment #49

I'll wait to review this until the default icon stuff.  Please re-request a review at that point.

Comment 53

[Parth Mudgal [:artpar]]

Attachment: Contains In-complete edit for implementing OnFaviconDataNOTavailable()

This is not working. what i did is as follows :

I initially tried to achieve via the size of the content being returned. This happens as early as in the function
NS_IMETHODIMP AsyncGetFaviconDataForPage::Run() in AsyncFavIconHelpers.cpp on line 985

the call backs were made in another function, if the data receieved was valid, by something as
nsCOMPtr<nsIRunnable> event = new NotifyIconObservers(iconData, pageData, mCallback);

i tried to call this in the `if (!iconSpec.Length())` with empty iconData and pageData but things didnt go smooth.
then i tried something like
(void)mCallback->OnFaviconDataAvailable(iconURI,             
                                            mIcon.data.Length(),
                                            TO_INTBUFFER(mIcon.data),
                                            mIcon.mimeType);

<--- This call actually happens in the NotifyIconObservers()

i put this inside the if condition, could made it work partially but later it gives all Memory Access Violation errors.

So i added a new function "void onFaviconDataNotAvailable();" in the FaviconService Interface 'nsIFaviconService.idl', updated winUtils.h/.cpp with the implementation of the function ->

if mURLShortcut -> copy default icon, since icon was not found
else return

In this i am trying to call "CacheIconFileFromFaviconURIAsync" again with mozilla icon URL but same icon filename, but i end up getting Memory Access violation... and various lots of debug breaks "Not valid thread" (like its saying it should be running on the main thread, but this doesnt seems right).


So please guide me some more.

Comment 54

[Brian R. Bondy [:bbondy]]

Just a heads up that I won't be able to do the feedback/review until tomorrow because of another big review I'm doing today.

Comment 55

[Brian R. Bondy [:bbondy]]

Comment on attachment 595221 [details] [diff] [review]
Contains In-complete edit for implementing OnFaviconDataNOTavailable()

Review of attachment 595221 [details] [diff] [review]:
-----------------------------------------------------------------

OK I spent some time looking into this and found a way to move forward.

Just a quick reminder as well that all of the WinUtils code should go in the first patch.  Only the code inside /toolkit should go in this patch.

If you want for now to make it easier just go to the first patch in your patch queue (hg qpop) and then fold the second one onto it (hg qfold second_patch_name).  You can easily split it up later using hg qref -X re:.*AsyncFaviconHelpers.* to get everything in the current patch except for the files that match that regex, then you can do an hg qnew patchname at that point.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +982,5 @@
>    nsresult rv = FetchIconURL(mDB, mPageSpec, iconSpec);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (!iconSpec.Length()) {
> +    (void)mCallback->OnFaviconDataNotAvailable();

I can't think of a reason why a (void) before this line is needed.

The problem with the assertion is here.  This callback should be called on the main thread, but here you are on a non main thread.  You should instead when iconSpec.Length() is 0 not call  these 2 statements:
> rv = FetchIconInfo(mDB, iconData);
> NS_ENSURE_SUCCESS(rv, rv);

So then you'd dispatch the NotifyIconObserver event to the main thread. 
Inside NotifyIconObservers::Run() you would
have something like this:

 nsresult rv = NS_NewURI(getter_AddRefs(iconURI), mIcon.spec);
if (NS_FAILED(rv)) {
if (mCallback) {
// Call OnFaviconDataNotAvailable here
}
return NS_OK;
}

Also note that there is a AsyncGetFavIconURLForPage that should be updated in the same way for symmetry.

::: toolkit/components/places/nsIFaviconService.idl
@@ +367,5 @@
> +   * This callback will be called only if the operation is NOT successful, otherwise
> +   * onFaviconDataAvailable will be called.
> +   *
> +   */
> +  void onFaviconDataNotAvailable();

When you make interface changes you also have to change the uuid.
So update this line:
[scriptable, uuid(2cf188f4-3c96-4bca-b668-36b25aaf7c1d)]

To have a new ID, also nit: make sure the new value is all lowercase.

::: widget/windows/WinUtils.cpp
@@ +423,5 @@
>  {
>  }
>  
>  NS_IMETHODIMP
> +AsyncFaviconDataReady::OnFaviconDataNotAvailable(void)

In addition to adding it here, I couldn't build with this patch applied because nsCopyFaviconCallback was missing the new function from it's calss nsCopyFaviconCallback which also implements the nsIFaviconDataCallback.

So in:
docshell\base\nsDocShell.cpp

You need to add:
NS_IMETHODIMP
OnFaviconDataNotAvailable(void)   
{
  return NS_OK;
}

@@ +437,5 @@
> +
> +  FaviconHelper::CacheIconFileFromFaviconURIAsync(mozillaFaviconURI,
> +    icoFile,
> +    mIOThread,
> +    mURLShortcut);

I see what you are trying to do here but it will lead to infinite recursion.  The favicon DB will not know about this URL.  So instead you need to use the moz-icon decoder to get the data in an image container, and then use the ICO-encoder with an encodeImage call to save the data.  That work should be done off the main thread by the way.

@@ +439,5 @@
> +    icoFile,
> +    mIOThread,
> +    mURLShortcut);
> +
> +}

This function should always return a value.

@@ +450,5 @@
>                                                const nsACString &aMimeType)
>  {
> +  
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut);

I don't think this is used.

::: widget/windows/WinUtils.h
@@ +231,5 @@
>  {
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIFAVICONDATACALLBACK
> +  NS_DECL_NSIFAVICONNODATACALLBACK

This is for the interfaces not the functions.  So you should remove that.  I.e. this should be giving you a compiling error.

Comment 56

[Parth Mudgal [:artpar]]

Note: I will be absent for 2-3 weeks due to my university exams and contribute to Mozilla for this period. I am not permanently absent though. I'll be back :)

thanks.

Comment 57

[Parth Mudgal [:artpar]]

Hello,

I am back at fixing the bug. As suggested by Brian, i have made the following changes:

1. in 'NotifyIconObservers::Run()'

  nsresult rv = NS_NewURI(getter_AddRefs(iconURI), mIcon.spec);

  if (NS_FAILED(rv)) {
    if (mCallback) {
      mCallback->OnFaviconDataNotAvailable();
    }
    return NS_OK;
  }

*havent updated the other place where brian pointed it, have to do it.


2. Changed 
[scriptable, uuid(2cf188f4-3c96-4bca-b668-36b25aaf7c1d)]
to
[scriptable, uuid(2cf188f4-3c96-4bca-b668-36b25aaf7c2e)]

*last `1d` to `2e`... please let me know if there is some other standard procedure for genarating this uuid.



3. I added

NS_IMETHODIMP
OnFaviconDataNotAvailable(void)   
{
  return NS_OK;
}

to  'docshell\base\nsDocShell.cpp'.. inside 'class nsCopyFaviconCallback : public nsIFaviconDataCallback'
next to 'NS_IMETHODIMP OnFaviconDataAvailable'

4. Removed : 'NS_DECL_NSIFAVICONNODATACALLBACK' from winutils.h

5. now when the icon is not found (data 0), this function is called 
AsyncFaviconDataReady::OnFaviconDataNotAvailable(void)

i checked out imgLoader for using it, saw how it is being used, in 'content\base\src\nsContentUtils.cpp' line 2370, 2400.. but i am still unsure how do i use it. the function doesnt returns anything, nor it took any variable passed by address where it wud be saving the data.

i also looked into 'imgIContainer' which is being used in the same file, 2432.. seems like image is being loaded by this line
imgRequest->GetImage(getter_AddRefs(imgContainer));

still i feel lost, how should i proceed ?

6. a random question
a. What is 'ns' ? like everything is starting with a 'ns'
nsCOMPtr
nsIProperties
nsresult
--my wildest guess would be 'namespace' ??

Comment 58

[Brian R. Bondy [:bbondy]]

Nice progress!

re: 
> *last `1d` to `2e`... please let me know if there is some other standard 
> procedure for genarating this uuid.

You can just use guidgen.exe to generate a new ID for here.

re:
> 6. a random question
> a. What is 'ns' ? like everything is starting with a 'ns'

I could be wrong but I think it is a prefix used because of the roots with netscape.

re: 
> 5. .... still i feel lost, how should i proceed ?

I *think* the easiest way to do this is to use nsIconChannel::Open directly from image\decoders\icon\win\nsIconChannel.cpp.   Then you'll have the stream you are looking for.

Comment 59

[Parth Mudgal [:artpar]]

Attachment: Fixed callback for OnFaviconDataNotAvailable + Earlier changes to files

Completed the part for OnFaviconDataNotAvailable in WinUtils, which now copies icon at "moz-icon://.url?size=32" to required location.

The code is almost similar to 
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=731541&attachment=608646

A new class for downloadObserver interface Implementation, what should be the name for it ? right now its "myDownloadObserver" :-p

An issue is that while testing, i get a green Earth Icon, instead of Mozilla/Nightly Icon.

Comment 60

[Parth Mudgal [:artpar]]

Attachment: Changes for the AsyncFaviconHelpers Files for supporting DataNotFound callback.

This patch contains the changes for the file AsyncFaviconHelpers.*

the other patch is imcomplete without this patch.

Comment 61

[Brian R. Bondy [:bbondy]]

I think you can use moz-icon://.html?size=32 instead.
I haven't reviewed yet, but great work getting it to work.

Comment 62

[Brian R. Bondy [:bbondy]]

Comment on attachment 611184 [details] [diff] [review]
Changes for the AsyncFaviconHelpers Files for supporting DataNotFound callback.

Review of attachment 611184 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to review the other patch tonight, please fold this one into that one for the next review.
Minor change only.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +984,5 @@
>  
> +  //if (!iconSpec.Length()) {
> +    //mCallback->OnFaviconDataNotAvailable();
> +    //return NS_ERROR_NOT_AVAILABLE;
> +  //}

Please remove this commented code if it is not needed.

Comment 63

[Brian R. Bondy [:bbondy]]

Comment on attachment 611183 [details] [diff] [review]
Fixed callback for OnFaviconDataNotAvailable + Earlier changes to files

Review of attachment 611183 [details] [diff] [review]:
-----------------------------------------------------------------

Great work getting the default icon downloading working on your own.
We should be able to wrap this task up soon.
Once we're ready I can push this to the try server for you and make sure all of our tests still pass, I'm pretty sure they will be fine.

I tried to apply the patch but could not because it needs to be rebased.
To do this:
First backup your patches directory by copying it out from .hg/patches/
hg qpop -a
hg pull
hg update
hg qpush
<--- After this you'll see a bunch of errors.  For each file that is effected it will have a .rej file.
The .rej file will show you the changes that couldn't be automatically applied.
So then re-apply what is in the .rej files into the source files.
Once you are done you can just:
hg qref

If you qpush'ed something by accident and it lists the errors you can simply qpop to undo it. 
Then when you're ready you can qpush again.

nit: Please remove extra whitespace that was added to nsDocShell.cpp and JumpListItem.cpp.  Also re-add the removed newline from nsDataObj.cpp.

::: toolkit/components/places/nsIFaviconService.idl
@@ +360,5 @@
>                                in unsigned long aDataLen,
>                                [const,array,size_is(aDataLen)] in octet aData,
>                                in AUTF8String aMimeType);
> +
> +    /**

nit: Fix alignment on the start of the comment to match the other functions.

@@ +365,5 @@
> +   * Called when the required favicon's information is NOT available.
> +   *
> +   * This callback will be called only if the operation is NOT successful, otherwise
> +   * onFaviconDataAvailable will be called.
> +   *

nit: Remove extra blank line that just has a * on it.

::: widget/windows/WinUtils.cpp
@@ +454,5 @@
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut);
> +
> +  nsCOMPtr<nsIURI> mozIconURI;
> +  rv = NS_NewURI(getter_AddRefs(mozIconURI), "moz-icon://.url?size=32");

nit: Change to .html to get the right icon

@@ +462,5 @@
> +  rv = NS_NewChannel(getter_AddRefs(channel), mozIconURI);
> +
> +  nsCOMPtr<myDownloadObserver> downloadObserver = new myDownloadObserver;
> +  nsCOMPtr<nsIStreamListener> listener;
> +  rv = NS_NewDownloader(getter_AddRefs(listener), downloadObserver, icoFile);

Please backup yoru patch before trying this chaange.
I think if change we netwerk/base/src nsDownloader.cpp

> mObserver->OnDownloadComplete(this, request, ctxt, status, mLocation);
> mObserver = nsnull;

to:

> if (mObserver) {
>   mObserver->OnDownloadComplete(this, request, ctxt, status, mLocation);
>   mObserver = nsnull;
> }

Then we can remove the class myDOwnloadObserver and pass in NULL. So from:

> rv = NS_NewDownloader(getter_AddRefs(listener), downloadObserver, icoFile);

to:

> rv = NS_NewDownloader(getter_AddRefs(listener), NULL, icoFile);

@@ +518,5 @@
> +  mMimeTypeOfInputData(aMimeTypeOfInputData),
> +  mBuffer(aBuffer),
> +  mBufferLength(aBufferLength),
> +  mURLShortcut(aURLShortcut)
> +

nit: remove blank space.

::: widget/windows/WinUtils.h
@@ +62,5 @@
>  
>  namespace mozilla {
>  namespace widget {
>  
> +class myDownloadObserver: public nsIDownloadObserver

nit: myDownloadObserver -> IconDownloadObserver

::: widget/windows/nsDataObj.cpp
@@ +1155,5 @@
>    LossyCopyUTF16toASCII(url, asciiUrl);
>      
> + 
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsCOMPtr<nsIURI> aUri;

The a prefix is only used for parameter names.  Maybe change to uri and change url to urlSpec above.

@@ +1159,5 @@
> +  nsCOMPtr<nsIURI> aUri;
> +  NS_NewURI(getter_AddRefs(aUri), url);
> +
> +  nsAutoString aUriHash;
> +  NS_NewThread(getter_AddRefs(mIOThread));

Please change this to LazyIdleThread.  This is probably one of the conflicts I mentioned.  You can see how it works from JumpListBuilder.cpp

@@ +1172,5 @@
> +
> +  static char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" 
> +                                   "IconFile=%s\r\nIconIndex=0\r\n";
> +
> +  // don't include %s (2 times) in the len

nit: don't -> Don't

@@ +1175,5 @@
> +
> +  // don't include %s (2 times) in the len
> +  static const int formatLen = strlen(shortcutFormatStr) - (2 * 2); 
> +  
> +  // we don't want a null character on the end

nit: we -> We

Comment 64

[Parth Mudgal [:artpar]]

Hello,

I did the rebasing, took me some considerable time, but than that part is complete.

Now when i try to rebuild the changes files
smartmake.py widgets/windows

i get some compilation errors, in files which i never even touched.

d:\mozilla-src2\widget\xpwidgets\nsBaseWidget.h(127) : error C2061: syntax error : identifier 'WindowAnimationType'
d:\mozilla-src2\widget\xpwidgets\nsBaseWidget.h(209) : error C3668: 'nsBaseWidget::GetGLFrameBufferFormat' : method with override specifier 'override' did not override any base class methods
d:\mozilla-src2\obj-i686-pc-mingw32\dist\include\gfxFont.h(1909) : warning C4244: 'return' : conversion from 'double' to 'float', possible loss of data
d:\mozilla-src2\obj-i686-pc-mingw32\dist\include\gfxFont.h(2096) : error C2197: 'nsMallocSizeOfFun' : too many arguments for call
d:/mozilla-src2/widget/windows/nsWindow.cpp(3211) : error C2660: 'mozilla::layers::LayerManagerD3D10::Initialize' : function does not take 1 arguments
d:/mozilla-src2/widget/windows/nsWindow.cpp(3220) : error C2660: 'mozilla::layers::LayerManagerD3D9::Initialize' : function does not take 1 arguments

So these files : nsBaseWidget.h, gfxFont.h and nsWindow.cpp... i checked the corresponding lines, couldnt imagine how i could fix them

Please give me some advice.

Comment 65

[Josh Matthews [:jdm]]

When rebasing after a pull, it's best to run a full build instead of using smartmake.

Comment 66

[Parth Mudgal [:artpar]]

Okay, i did

pymake -f client.mk build

after a lot of building, i get some fatal error


error: interface 'nsIFaviconDataCallback' has multiple methods, but marked 'function', d:/mozilla-src2/toolkit/components/p
laces/nsIFaviconService.idl line 332:0
interface nsIFaviconDataCallback : nsISupports
^


i made changes to : toolkit/components/places/nsIFaviconService.idl

::: toolkit/components/places/nsIFaviconService.idl
@@ +367,5 @@
> +   * This callback will be called only if the operation is NOT successful, otherwise
> +   * onFaviconDataAvailable will be called.
> +   *
> +   */
> +  void onFaviconDataNotAvailable();

Brian Said:
When you make interface changes you also have to change the uuid.
So update this line:
[scriptable, uuid(2cf188f4-3c96-4bca-b668-36b25aaf7c1d)]

So i changed "2cf188f4-3c96-4bca-b668-36b25aaf7c1d" to "2cf188f4-3c96-4bca-b668-36b25aaf7c2e" 

Those are all the chnages in that file.

So from understanding the error, i see
[scriptable, function, uuid(22ebd780-f4ab-11de-8a39-0800200c9a66)]
and changed it to
[scriptable, uuid(22ebd780-f4ab-11de-8a39-0800200c9a66)]

now doing "pymake -f client.mk build"

Comment 67

[Josh Matthews [:jdm]]

If you're adding a function to nsIFaviconDataCallback, you'll need to remove the function annotation and make sure that any places passing callback objects use full objects instead of bare functions.

Comment 68

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #67)
> If you're adding a function to nsIFaviconDataCallback, you'll need to remove
> the function annotation and make sure that any places passing callback
> objects use full objects instead of bare functions.

just that we don't want that!

Note we are already doing what you are trying to do there in bug 737133, that is about to land.

Comment 69

[Brian R. Bondy [:bbondy]]

Parth can you pull in the change from bug 737133 once it lands and then re-attach the updated patch? That way you can remove that callback code. If you'd rather not wait for it to land you can just push that patch first in your patch queue by using: 

hg qpop -a
qimport path_to_patch_to_import
hg qpush -a

Sorry about the rebasing, ya it can be a pain when you have to do it for some larger tasks.

Comment 70

[Parth Mudgal [:artpar]]

Since i have done the Rebaseing, now whenever i Debug Firefox, it closes as soon as it starts the GUI. That is, the cmd window opens fine, shows data as usual, but as soon as either the "updateing" or actual firefox window appears, the program terminates.

This is still happening even after i have imported the other patch. The build process goes on fine. Also, if i go to the build directory and open a firefox.exe

obj-i686-pc-mingw32\dist\bin\firefox.exe

it opens fine, but also when i close it, there is a message box with an error 

###!!! ASSERTION: Uh, IsInModalState() called w/o a reachable top window?: 'Error', file d:/mozilla-src2/dom/base/nsGlobalWindow.cpp, line 6832

So at this time, i cannot Debug the application (step by step), i have to run the firefox.exe from dist/bin/ dir.

What should i do ?

Comment 71

[Brian R. Bondy [:bbondy]]

Did you try moving your old obj-i686-pc-mingw32 directory out of the way and then rebuilding from scratch?

Also did you try just hitting ignore on the debug assertion? 

You can disable those debug assertions by the way and have them show up in the log instead by having this environment variable:
XPCOM_DEBUG_BREAK=warn

> So at this time, i cannot Debug the application (step by step), 
> i have to run the firefox.exe from dist/bin/ dir.

I'm not sure exactly what you mean here, but I think you should already be running firefox.exe from within your dist/bin directory to debug in the easiest way.

Comment 72

[Parth Mudgal [:artpar]]

Attachment: Patch file AFTER importing patch file of bug 737133

Two files which i was eariler attaching have been merged, since changes in one of them were minor (Suggested by Brian)

This patch is to be applied after importing patch file for bug 737133.

One weird issue:
i changed the Icon url from
moz-icon://.url?size=32
to
moz-icon://.html?size=32

the icon it shows in Mozilla browser for these corresponding URL's were EARTH for first and chrome icon for second. But the downloaded icon is always the Earth icon  ? :-/ please suggest what might the problem be.

Also, I dont know if this problem is related to mozilla browser or my IDE (vs2010), for the part of code in winUtils.cpp

Line 497,
if (!aDataLen || !aData) {
    if(mURLShortcut){
      OnFaviconDataNotAvailable();
    }
    return NS_OK;
  }

I cant Step-in(F11) on the function call, also if i set any breakpoint inside that function (OnFaviconDataNotAvailable), it wont break, but it does its work :-/ lately lot of weird things going on with my IDE :p

Also, all the changes from the IDL file have been reverted, since there was no need for them now.

Comment 73

[Brian R. Bondy [:bbondy]]

I did an hg pull and hg update and I tried first applying 737133 and then the patch attached to this bug, but it did not apply cleanly (there are .rej files).  

There haven't been any changes landed in m-c since your last rebasing affecting this code.  So perhaps you uploaded the wrong patch?  I also first tried applying this patch without bug 737133 but no luck.  


> the icon it shows in Mozilla browser for these corresponding URL's were 
> EARTH for first and chrome icon for second. But the downloaded icon is always 
> the Earth icon  ? :-/ please suggest what might the problem be.

Maybe it is just not built after your change. I will try once I can apply the patches to see.


> I cant Step-in(F11) on the function call, also if i set any breakpoint 
> inside that function (OnFaviconDataNotAvailable), it wont break, but 
> it does its work :-/ lately lot of weird things going on with my IDE :p

It sounds like something is not built correctly for you.

try to move away your old objdir and then from the root of your checked out directory type:
python -OO build/pymake/make.py -f client.mk > out.txt 2>&1

Then after it builds search in your out.txt for:
: error

If it appears there is a build error somewhere and not everything is built correctly.

Comment 74

[Marco Bonardo [:mak]]

bug 737133 landed in inbound, will try to merge it to central asap for you to build on top of it.

Comment 75

[Brian R. Bondy [:bbondy]]

Comment on attachment 613754 [details] [diff] [review]
Patch file AFTER importing patch file of bug 737133

Thanks but I added the patch into my patch queue and this bug's patch doesn't apply after it. So something is not updated in this bug's patch.

Comment 76

[Parth Mudgal [:artpar]]

I am not sure how should i solve this. I checked the "series" file in .hg/patches folder, and i see this :


bug-737133.diff
84447.diff
84448.diff
84449.diff
84450.diff
84451.diff
84452.diff
restore
shortcut2.patch


of which 844*.diff came from pulling/rebasing

I only find the "restore" odd, i dont know why it should be there. I opened the "restore" file (diff file) and it shows various changes. I think maybe this is the file causing the problem.

Comment 77

[Brian R. Bondy [:bbondy]]

You can pop all your patches (qpop -a) then only apply the ones in question.
To do this:

You can qremove the ones you don't want, but this will also remove the patches.
You could also just edit the file in notepad and change the order.
You could also hg qpush --move patchname.diff to move it ahead of the others as the next patch to be pushed.

Comment 78

[Parth Mudgal [:artpar]]

Attachment: Patch file, tried to Fix rej errors

Okay, I have removed the odd "restore" patch which was in the series of my patch's in HG. after removal and pushing the actual patch generated some rej files. I have fixed them all and this is new patch after a qref.

I also did a rebuild after deleting the existing obj dir. the build process completed.

Comment 79

[Brian R. Bondy [:bbondy]]

Comment on attachment 614565 [details] [diff] [review]
Patch file, tried to Fix rej errors

Review of attachment 614565 [details] [diff] [review]:
-----------------------------------------------------------------

I tried to apply this patch again to review it but it is still not applying correctly.
I assume all of the code for this bug is inside shortcut2.patch

You mentioned in comment 76 that you have the following in your patch queue as well:
84447.diff
84448.diff
84449.diff
84450.diff
84451.diff
84452.diff
restore
I don't know what those patches are but this patch should apply without them.

Could you perform the following actions:
> hg qpop -a
> hg pull
> hg update
> hg qremove bug-737133.diff
> hg qpush --move shortcut2.patch

Then you should see a bunch of .rej errors that should be fixed. 
Note: I did a qremove of bug-737133.diff above because it already landed.

Comment 80

[Parth Mudgal [:artpar]]

Attachment: popped all other diff files and fixed all rej conflicts

New patch file after doing what you said.

now my series file looks as follows:

shortcut2.patch
84447.diff
84448.diff
84449.diff
84450.diff
84451.diff
84452.diff

and the status file as:

72d9f2f2f9a22f43a187108f833ccd8eebf07af2:shortcut2.patch

resolved all the rej files that were generated in the process and did a build(smartmake.py widgets/windows) to test it out.

Comment 81

[Brian R. Bondy [:bbondy]]

It applies! thanks :)

Comment 82

[Brian R. Bondy [:bbondy]]

Comment on attachment 615357 [details] [diff] [review]
popped all other diff files and fixed all rej conflicts

Review of attachment 615357 [details] [diff] [review]:
-----------------------------------------------------------------

There are build errors after applying the patch:

c:/projects/mozilla/mozilla-central/widget/windows/nsDataObj.cpp(1163) : error C2065: 'mIOThread' : undeclared identifier

c:/projects/mozilla/mozilla-central/widget/windows/nsDataObj.cpp(1192) : error C2065: 'contents' : undeclared identifier

c:/projects/mozilla/mozilla-central/widget/windows/nsDataObj.cpp(1192) : error C2065: 'totalLen' : undeclared identifier

c:/projects/mozilla/mozilla-central/widget/windows/nsDataObj.cpp(1192) : error C2065: 'shortcutFormatStr' : undeclared identifier

c:/projects/mozilla/mozilla-central/widget/windows/nsDataObj.cpp(1192) : error C2065: 'asciiUrl' : undeclared identifier

c:/projects/mozilla/mozilla-central/widget/windows/nsDataObj.cpp(1192) : error C2228: left of '.get' must have class/struct/union

.... and some others

Comment 83

[Brian R. Bondy [:bbondy]]

sorry I may have applied the wrong patch in Comment 82.  I'll try again and let you know or post the review.

Comment 84

[Parth Mudgal [:artpar]]

(In reply to Brian R. Bondy [:bbondy] from comment #83)
> sorry I may have applied the wrong patch in Comment 82.  I'll try again and
> let you know or post the review.

no you are right, i deleted by obj dir and build from start and i got those errors now. I should have done this earlier itself. Sorry for all the confusion :( . I am fixing it.

Comment 85

[Brian R. Bondy [:bbondy]]

OK thanks, looking forward to seeing this in action :)

Comment 86

[Parth Mudgal [:artpar]]

Attachment: fixed compiler errors

I am really feeling embarassed now after not been able to give a working-for-someone-other-than-me patch.

I saw the errors and i found that the changes i had made to different files, some of them were not in the patch file. somehow they moved to the varioud 84*.diff files i listed earlier. i dont know why or when this happened. so i had to make the changes again manually and qref again.

i deleted the obj dir, compiled, found the errors, fixed and repeted this again until build was complete.

than i deleted the obj dir once more and new build again, and hopefully it will be working for others too now :-/

apologies for the inconvinence

Comment 87

[Brian R. Bondy [:bbondy]]

No problem, it happens :)
I'll try to review tonight.

Comment 88

[Brian R. Bondy [:bbondy]]

Comment on attachment 615444 [details] [diff] [review]
fixed compiler errors

Review of attachment 615444 [details] [diff] [review]:
-----------------------------------------------------------------

There are a lot of formatting changes to nsDataObj.cpp.  Some of the formatting changes are valid (4 spaces to 2) but others make the formatting less valid (parameter alignment).   Please revert all of the unneeded formatting changes for code that isn't related to this task. If we want, to we can post another bug later to clean up the 4 spaces to 2.
I only listed the first few formatting changes to revert in the review, but please check the patch to make sure you get all of them.

The other change that is needed is to use a LazyIdleThread, this was the code that caused you to need to rebase the patch.
It would be like the task is reverted.   (Since the code moved but it is the old code.)

Also I tried using the patch but I no longer get icons for sites that should have favicons available.

Here is an example of one that I tried, I opened it in vim to see this:

[InternetShortcut]
URL=http://www.brianbondy.com/
IDList=
HotKey=0
IconFile=(null)
IconIndex=0
A bunch of binary data here

Good work getting the rebasing to work by the way, I think we'll be able to wrap up this task soon :)

::: widget/windows/nsDataObj.cpp
@@ +39,5 @@
> +* and other provisions required by the GPL or the LGPL. If you do not delete
> +* the provisions above, a recipient may use your version of this file under
> +* the terms of any one of the MPL, the GPL or the LGPL.
> +*
> +* ***** END LICENSE BLOCK ***** */

nit: Looks like you removed a space from the start of each of these lines.  Please revert that so only code that needs to be changed is changed.

@@ +80,5 @@
>  static const char table[] =
> +{ 'a','b','c','d','e','f','g','h','i','j',
> +'k','l','m','n','o','p','q','r','s','t',
> +'u','v','w','x','y','z','0','1','2','3',
> +'4','5','6','7','8','9' };

nit: Ditto please leave the spacing as it was here.

@@ +85,2 @@
>  static void
> +  MakeRandomString(char *buf, PRInt32 bufLen)

nit: Remove the 2 spaces from the start of the line, i.e. leave this line as it was.

@@ +413,2 @@
>  {
> +  NS_NewThread(getter_AddRefs(mIOThread));

As per Comment 63 please change this to LazyIdleThread

::: widget/windows/nsDataObj.h
@@ +121,5 @@
>  {
> +
> +
> +protected:
> +	 nsCOMPtr<nsIThread> mIOThread;

nit: There is a tab here, should be 2 spaces.

Comment 89

[Parth Mudgal [:artpar]]

Attachment: formatting changed reverted, added LazyIdleThread

I fixed the formatting issues (they happened because i did a ctrl+k ctrl+f earlier).

Changed thread to LazyIdleThread.

Fixed the garbage binary stuff in shortcut file.

I icon-not-coming you said, i dont understand too why is it not coming. The ico file appears in the cache DIR just fine. This particular site is showing peculiar behaviour :-/

1. I change the icon for that shortcut from rightclick->properties and choose the same file again, the icon was updated, the contents of the shortcut file remained same though.

2. then i deleted the shortcut, and made another shortcut, same icon comes.

3. then i changed icon again from properties to a different icon, icon changed, file contents changed.

4. then i deleted this shortcut and dropped a new shortcut, now the shorcut file contents icon url to the acctual favicon but the icon which it shows actually is the one i set in step 3.

Comment 90

[Silekonn]

(In reply to Parth Mudgal [:artpar] from comment #89)
> Created attachment 615878 [details] [diff] [review]
> formatting changed reverted, added LazyIdleThread
> 
> I fixed the formatting issues (they happened because i did a ctrl+k ctrl+f
> earlier).
> 
> Changed thread to LazyIdleThread.
> 
> Fixed the garbage binary stuff in shortcut file.
> 
> I icon-not-coming you said, i dont understand too why is it not coming. The
> ico file appears in the cache DIR just fine. This particular site is showing
> peculiar behaviour :-/
> 
> 1. I change the icon for that shortcut from rightclick->properties and
> choose the same file again, the icon was updated, the contents of the
> shortcut file remained same though.
> 
> 2. then i deleted the shortcut, and made another shortcut, same icon comes.
> 
> 3. then i changed icon again from properties to a different icon, icon
> changed, file contents changed.
> 
> 4. then i deleted this shortcut and dropped a new shortcut, now the shorcut
> file contents icon url to the acctual favicon but the icon which it shows
> actually is the one i set in step 3.

Parth and etc.,

There is a problem with MS systems properly updating their icon cache.  See here: http://www.sevenforums.com/tutorials/49819-icon-cache-rebuild.html.  The batch is necessary when icons are stuck.

Silekonn

Comment 91

[Brian R. Bondy [:bbondy]]

I think Comment 34 fixes that though...

Comment 92

[Parth Mudgal [:artpar]]

There is already a 
if(mURLShortcut){
    SendMessage(HWND_BROADCAST, WM_SETTINGCHANGE, SPI_SETNONCLIENTMETRICS, 0);
  }

in AsyncWriteIconToDisk::Run(), which right now does happen after icon is written to disk. I (just now)added that call to the new "OnFaviconDataNotAvailable" function also, but this shortcut does not call that function anyways. So this call does not fix the problem.

Comment 93

[Brian R. Bondy [:bbondy]]

Comment on attachment 615878 [details] [diff] [review]
formatting changed reverted, added LazyIdleThread

Review of attachment 615878 [details] [diff] [review]:
-----------------------------------------------------------------

nsDataObj.cpp still has a lot of formatting changes for parts of the code that aren't related to this task.
Please see the splinter review for this file.  You should be able to remove those changes and qrefresh the patch, repeat until the only changes in that file are the ones related to this task. 
Alternately you can just replace it with the old file and apply the needed changes to that file.

It would be ideal to find a way to center the small favicon like Internet Explorer does instead of having it scaled by windows.  
The favicons currently look pixelated because of the scaling and small size they are provided in. 
We can do that work in another bug though unless you find some easy way to do it via the .url file. 

For some reason the moz-icon://.html is not showing the proper html icon that I get from Windows when creating an html file. I think this is a separate bug though. 

Let's fixup the formatting, remove the unused functions and then push this to try to make sure all the tests still pass.  
We should be ready to land in v15 Firefox Nightly builds.

::: widget/windows/JumpListBuilder.h
@@ +56,5 @@
>  #include "nsIObserver.h"
>  #include "nsIFaviconService.h"
>  #include "nsThreadUtils.h"
>  
> +#include "WinUtils.h"

nit: This include can be removed

::: widget/windows/JumpListItem.cpp
@@ +429,4 @@
>  // If successful, the file path on disk is in the format:
>  // <ProfLDS>\jumpListCache\<hash(aFaviconPageURI)>.ico
>  nsresult JumpListShortcut::GetOutputIconPath(nsCOMPtr<nsIURI> aFaviconPageURI,
>                                               nsCOMPtr<nsIFile> &aICOFile) 

I think JumpListShortcut::GetOutputIconPath is not used anymore, if that's the case please remove.

::: widget/windows/WinUtils.cpp
@@ +464,5 @@
> +
> +
> +nsresult AsyncFaviconDataReady::OnFaviconDataNotAvailable(void)
> +{
> +  if(!mURLShortcut){

nit: if (!mURLShortcut) {

@@ +473,5 @@
> +  nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut);
> +
> +  nsCOMPtr<nsIURI> mozIconURI;
> +  rv = NS_NewURI(getter_AddRefs(mozIconURI), "moz-icon://.html?size=32");
> +  if(NS_FAILED(rv)) return rv;

nit: For debugging purposes it is better not to combine 2 statements inside a single line.
Also should be spaces after the if and before the {:
if (NS_FAILED(rv)) {
  return rv;
}

@@ +495,5 @@
> +                                  const PRUint8 *aData, 
> +                                  const nsACString &aMimeType)
> +{
> +  if (!aDataLen || !aData) {
> +    if(mURLShortcut){

nit: if (mURLShortcut) {

@@ +569,5 @@
> +  // if the user has a different DPI setting other than 100%.
> +  // Windows would scale the 16x16 icon themselves, but it's better
> +  // we let our ICO encoder do it.
> +  nsCOMPtr<nsIInputStream> iconStream;
> +  if (!mURLShortcut){

nit: if (!mURLShortcut) {

@@ +572,5 @@
> +  nsCOMPtr<nsIInputStream> iconStream;
> +  if (!mURLShortcut){
> +    PRInt32 systemIconWidth = GetSystemMetrics(SM_CXSMICON);
> +    PRInt32 systemIconHeight = GetSystemMetrics(SM_CYSMICON);
> +    if((systemIconWidth == 0 || systemIconHeight == 0)){

nit: if ((systemIconWidth == 0 || systemIconHeight == 0)) {

@@ +622,5 @@
> +
> +  // Cleanup
> +  bufferedOutputStream->Close();
> +  outputStream->Close();
> +  if(mURLShortcut){

nit: if (mURLShortcut) {

@@ +731,5 @@
> + * @param aICOFilePath The path of the icon file
> + * @param aIOThread The thread to perform the Fetch on
> + * @param aURLShortcut to distinguish between jumplistcache(false) and shortcutcache(true)
> + */
> +nsresult FaviconHelper::ObtainCachedIconFile(nsCOMPtr<nsIURI> aFaviconPageURI,

ObtainCachedIconFile still exists in JumpListItem.cpp but I don't think it is used.

@@ +829,5 @@
> +
> +  // Obtain the local profile directory and construct the output icon file path
> +  rv = NS_GetSpecialDirectory("ProfLDS", getter_AddRefs(aICOFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if(!aURLShortcut)

nit: if (!aURLShortcut)

@@ +851,5 @@
> +
> +// (static) Asynchronously creates a cached ICO file on disk for the favicon of
> +// page aFaviconPageURI and stores it to disk at the path of aICOFile.
> +nsresult 
> +  FaviconHelper::CacheIconFileFromFaviconURIAsync(nsCOMPtr<nsIURI> aFaviconPageURI,

This function also exists here:
JumpListShortcut::CacheIconFileFromFaviconURIAsync
I think that version is not used and can be removed.

@@ +871,5 @@
> +  return NS_OK;
> +}
> +
> +// Obtains the jump list 'ICO cache timeout in seconds' pref
> +PRInt32 FaviconHelper::GetICOCacheSecondsTimeout() {

I think this function is no longer needed from JumpListItem.cpp and can be removed from JumpListItem.cpp

::: widget/windows/nsDataObj.cpp
@@ +1156,5 @@
>    nsCAutoString asciiUrl;
>    LossyCopyUTF16toASCII(url, asciiUrl);
> +
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsCOMPtr<nsIURI> aUri;

The prefix 'a' as used in aUri is only for parameters.  Local variables should not have this prefix.

@@ +1159,5 @@
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsCOMPtr<nsIURI> aUri;
> +  NS_NewURI(getter_AddRefs(aUri), url);
> +
> +  nsAutoString aUriHash;

There should be no prefix of 'a' on aUriHash since it is a local variable.

@@ +1177,5 @@
> +                                   "IDList=\r\nHotKey=0\r\nIconFile=%s\r\n" 
> +                                   "IconIndex=0\r\n";
> +  static const int formatLen = strlen(shortcutFormatStr) - 2*2; // don't include %s (2 times) in the len
> +  const int totalLen = formatLen + asciiUrl.Length() 
> +                       //+ aUriHash.Length() 

nit: Remove the commented out line.

@@ +1178,5 @@
> +                                   "IconIndex=0\r\n";
> +  static const int formatLen = strlen(shortcutFormatStr) - 2*2; // don't include %s (2 times) in the len
> +  const int totalLen = formatLen + asciiUrl.Length() 
> +                       //+ aUriHash.Length() 
> +                       + path.Length(); // we don't want a null character on the end

nit: + should be on the previous line.

Comment 94

[Brian R. Bondy [:bbondy]]

nit for: static const int formatLen = strlen(shortcutFormatStr) - 2*2
There should be a space before and after the *

Comment 95

[Parth Mudgal [:artpar]]

Attachment: Formatting fixed. Unused methods removed.

Fixed the formatting, repeted until all the editing was related to only this bug. Hopefully i didnt missed any.

Also removed the unused methods.

Thanks :)

Comment 96

[Brian R. Bondy [:bbondy]]

Thanks for cleaning that up Parth, pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=b606fe57d65e

Comment 97

[Brian R. Bondy [:bbondy]]

The try results look good. Parth could you attach a screenshot and mark it for a ui-review? For the reviewer put this:
[e-mail]

I want to see if it's ok to land it as is since it looks pixelated.  Also mention that we're seeking to see if it's OK to land that way or if we should do something else like center the favicon in the middle of a white background like IE does.

Comment 98

[Parth Mudgal [:artpar]]

I have one issue before i think we can proceed.

In some earlier changes, i added this line in the "onDataNotAvailable" function

SendMessage(HWND_BROADCAST, WM_SETTINGCHANGE, SPI_SETNONCLIENTMETRICS, 0);

now i notice that the browser hangs after dragging out a bookmark for site which wont have ico file.

if i remove this line, then it works fine.

so should i remove the line and re-upload the patch ?

Comment 99

[Brian R. Bondy [:bbondy]]

Yup please remove that, I still have to do the final review but last I looked at it, it was really close.

Comment 100

[Parth Mudgal [:artpar]]

Attachment: Removed SendMessage from OnIconDataNotAvailable

Updated patch according to the comment 98.

I have taken screenshots, should i attach them as a ZIP archive or seperatly one by one as i dont see the option to upload multiple files ?

I will upload the screenshots next.

Comment 101

[Brian R. Bondy [:bbondy]]

Just one screenshot that shows how a few of the favicons look would be great.

Comment 102

[Parth Mudgal [:artpar]]

Attachment: Screenshot A few icons dropped from bookmark bar

Screenshot for UI review. This is how the current icons are being formed after being drag-and-dropped from the toolbar.

we're seeking to see if it's OK to land that way or if we should do something else like center the favicon in the middle of a white background like IE does.

Comment 103

[Ralph Graw]

Could we see a screenshot of a favicon dropped from the address bar?  And, some that are not small - i.e. do not have the pixelated problem?  If these look good, I would suggest going with the centering on white background option.

Comment 104

[Brian R. Bondy [:bbondy]]

> Could we see a screenshot of a favicon dropped from the address bar?

That is already included in the screenshot. 

> And, some that are not small 

If you are referring to small icon list view, then that will appear fine.  It is only for larger viewing of the icons that it will look pixelated.  

If you are referring to an icon file that contains larger icon resources, our icon decoder doesn't support this yet.  Also favicons can be provided in a variety of image formats in Firefox.   We re-encode it as an icon so that Windows can use it.

Comment 105

[Brian R. Bondy [:bbondy]]

Comment on attachment 618043 [details] [diff] [review]
Removed SendMessage from OnIconDataNotAvailable

Clearing this review request until we have UX feedback.

Comment 106

[Parth Mudgal [:artpar]]

Attachment: icons dropped from address bar VS dropped from bookmark bar

Another screen shot, containing icons dropped from mozilla. For each icon, one is dropped from the Address bar (the thing on left of address bar which shows the favicon) and other is dropped form the bookmarks bar.

i dont see any difference so i have not pointed out which is dropped from where.

Comment 107

[Ralph Graw]

I was essentially referring to your alternative of "do something else like center the favicon in the middle of a white background like IE does" rather than allowing the icon to pixelate.  One opinion...

Comment 108

[Brian R. Bondy [:bbondy]]

We could do either a white box with the small icon centered or a generic file icon with the favicon in the middle of the file.  Thoughts?

Comment 109

[Ralph Graw]

If the developer went to the trouble of creating an icon, albeit small, I think we should represent it as best we can.  I'd opt for the small icon centered.  Could be a white background or transparent - I like transparent, but if white gets us into a build faster, then that's OK.  Again, one opinion, would like to see others pitch in here.

Comment 110

[Jared Wein [:jaws] (please needinfo? me)]

I like the generic file icon with the favicon in the middle of the file. This is usually used as a "document" icon in Windows, and as such I think it would be consistent with the look-and-feel of other Windows icons.

Comment 111

[Stephen Horlander [:shorlander]]

Attachment: Favicon on white background

(In reply to Brian R. Bondy [:bbondy] from comment #108)
> We could do either a white box with the small icon centered or a generic
> file icon with the favicon in the middle of the file.  Thoughts?

Yes, I think we should center the favicons at 100% on a white background so that they are visible on any wallpaper.

At some point if we can pull larger favicons then we could revisit the presentation.

Comment 112

[Stephen Horlander [:shorlander]]

Comment on attachment 618050 [details]
Screenshot A few icons dropped from bookmark bar

See comment 111

Comment 113

[Parth Mudgal [:artpar]]

Please excuse my for my absence, I am having my university exams on-going and these will go on for approx 2 weeks.

Also, Brain, please let me know how should i go about putting a white background in the ICO. I am hoping to use some Image function and most probably there will be a lot of them in firefox code already, so which files should i look in ?

Thank you

Comment 114

[Brian R. Bondy [:bbondy]]

Let's do that in a follow up bug and then land them at the same time.  I talked to Joe Drew and got some info on how to do this but need to investigate a bit more. Please email me when you are nearly complete your exams, good luck!

Comment 115

[Brian R. Bondy [:bbondy]]

> I am hoping to use some Image function and most probably there will be a
> lot of them in firefox code already

I think the closest thing is probably the implementation of the Canvas element but we'll investigate it more in Bug 752996 when you get back.

Comment 116

[Parth Mudgal [:artpar]]

Attachment: minor modification

There was a path.get() missing, dont know why, so added that.

Comment 117

[Brian R. Bondy [:bbondy]]

Comment on attachment 639227 [details] [diff] [review]
minor modification

Review of attachment 639227 [details] [diff] [review]:
-----------------------------------------------------------------

Pls re-request a review after we finish the work on the favicon centered on white background bug.

::: widget/windows/nsDataObj.cpp
@@ +351,5 @@
>    : m_cRef(0), mTransferable(nsnull),
>      mIsAsyncMode(FALSE), mIsInOperation(FALSE)
>  {
> +  mIOThread = new LazyIdleThread(DEFAULT_THREAD_TIMEOUT_MS, 
> +                                 LazyIdleThread::ManualShutdown);

LazyIdleThread was changed to require a second parameter of a string name for the thread. 
So please add a new param in between the 2 there with something like:
 mIOThread = new LazyIdleThread(DEFAULT_THREAD_TIMEOUT_MS, 
                                NS_LITERAL_CSTRING("nsDataObj"),
                                LazyIdleThread::ManualShutdown);

Comment 118

[Brian R. Bondy [:bbondy]]

By the way I'm excited for this to land because there's also a task for Windows 8 pinned website tiles that will re-use the functionality on the Windows 8 start screen.

Comment 119

[Brian R. Bondy [:bbondy]]

Comment on attachment 639227 [details] [diff] [review]
minor modification

Review of attachment 639227 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinUtils.cpp
@@ +458,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIFile> icoFile;
> +	nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut);

nit: please remove the tab here.

Comment 120

[Parth Mudgal [:artpar]]

Attachment: fixed second param

Nit fixed.

Comment 121

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=502eb26291e0

Comment 122

[Brian R. Bondy [:bbondy]]

Comment on attachment 642201 [details] [diff] [review]
fixed second param

Review of attachment 642201 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinUtils.cpp
@@ +423,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIFile> icoFile;
> +  nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut);

nit: Add after this line:
NS_ENSURE_SUCCESS(rv, rv);

@@ +432,5 @@
> +    return rv;
> +  }
> + 
> +  nsCOMPtr<nsIChannel> channel;
> +  rv = NS_NewChannel(getter_AddRefs(channel), mozIconURI);

nit: Add after this line:
NS_ENSURE_SUCCESS(rv, rv);

@@ +436,5 @@
> +  rv = NS_NewChannel(getter_AddRefs(channel), mozIconURI);
> +
> +  nsCOMPtr<myDownloadObserver> downloadObserver = new myDownloadObserver;
> +  nsCOMPtr<nsIStreamListener> listener;
> +  rv = NS_NewDownloader(getter_AddRefs(listener), downloadObserver, icoFile);

nit: Add after this line:
NS_ENSURE_SUCCESS(rv, rv);

::: widget/windows/nsDataObj.cpp
@@ +1103,5 @@
> +  NS_NewURI(getter_AddRefs(aUri), url);
> +
> +  nsAutoString aUriHash;
> +
> +  mozilla::widget::FaviconHelper::ObtainCachedIconFile(aUri, aUriHash, mIOThread, true);

nit: Reomove the below nsresult rv;
And prepend nsresult rv = to this line.
Then add NS_ENSURE_SUCCESS(rv, rv);

@@ +1112,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCString path;
> +  rv = icoFile->GetNativePath(path);
> +
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: Remove whitespace before this line.

@@ +1119,5 @@
> +                                   "IDList=\r\nHotKey=0\r\nIconFile=%s\r\n" 
> +                                   "IconIndex=0\r\n";
> +  static const int formatLen = strlen(shortcutFormatStr) - 2*2; // don't include %s (2 times) in the len
> +  const int totalLen = formatLen + asciiUrl.Length() 
> +                       //+ aUriHash.Length() 

nit: Remove this comment.

Comment 123

[Brian R. Bondy [:bbondy]]

Try tests all pass, after you implement the nits I'll push to try one last time and then we'll be good to land both patches from this bug and bug 753021

Comment 124

[Parth Mudgal [:artpar]]

Attachment: fixed more nits

Fixed the nits mentioned in previous comment.

Comment 125

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/aa003aa3a18f

Congratulations on landing your first couple patches! Thanks for your contribution and dedication on this bigger bug that's been around for more than 10 years :)

Comment 126

[Jared Wein [:jaws] (please needinfo? me)]

\o/

Comment 127

[Parth Mudgal [:artpar]]

Wow, like finally its Completed !!! :D :D

never noticed its been here for 10 years O.o :D

Comment 128

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aa003aa3a18f

Comment 129

[Silekonn]

Good job, all.  How should the bounty be split?  I would like to compensate those involved.

Comment 130

[N Brooks]

Resolved ???

I'm on V15.0.1 and this still isn't working. It has annoyed me for many years. Do I need to enable it somehow, and if so why is it not 'just on'.

Comment 131

[Mardeg]

(In reply to N Brooks from comment #130)
> I'm on V15.0.1 and this still isn't working. 
The resolution happens on trunk versions, please note the "Target Milestone" field:

Firefox 17

Until it becomes the release, you can test a build of that version from:
http://www.mozilla.org/en-US/firefox/aurora/

Comment 132

[Roasted]

Sorry I have to ask that, but is there a switch to turn this off? 

I was awaiting this eagerly but the two remaining drawbacks (see below, escpecially #1)* makes it worse than before, so I'd like to turn this back to the Firefox default hyperlink icons.

*
1) the icon itself is tiny compared to the surrounding white space so all icons look the same (a white icon with a tiny colored dot inside). Can the proportion icon / white fill be adjusted by the user?
2) the favicons are broken (not even default) when you copy the links on a stick

Comment 133

[Silekonn]

Roasted is correct.  This could be a great addition.  It currently makes a lot of white boxes.  A flag in a configuration file should be available for those who appreciate the stretched (full-sized) icons.

Comment 134

[Silekonn]

As per BBondy I have created https://bugzilla.mozilla.org/show_bug.cgi?id=819360.  It was recommended I "block that [previous] bug number."  I am a novice in Bugzilla and would appreciate suggestions or corrections.

Comment 135

[T. Bug Reporter]

Re: Comment 132 - add my vote for a way to turn this off.  I understand why it was changed, but I never liked this behavior in other browsers, either.  I personally prefer to be able to tell at a glance which of the shortcuts on my desktop will take me to a Web site - more than I care about having a graphic reminder of which Web sites they're going to take me to.  Also, I find that getting rid of the custom icons once they're added to an .URL file is trickier than just editing the contents of the .URL file in Notepad, due to some strange caching effects.

Comment 136

[Yves Goergen [:ygoe]]

This "bug" is no way fixed, its implementation is badly broken! Did anybody ever test this before committing it? We're on a stable release here. So please add an option to turn that off - and set it by default - in Firefox 18, or correct it so that the icons are visible at 16x16 size and on other devices like USB sticks. (Since I don't believe that you can solve the second point, I'd suggest you remove the feature again.)

Comment 137

[Brian R. Bondy [:bbondy]]

Added bug 827784 which will add an option to disable this functionality.

Comment 138

[Roasted]

Regarding the amount of good work that some people invested here, wouldn't it be good not to just add an option to disable this again but also add an option that sets the proportion of icon and white fill so that we just could start using and loving these icons as a feature?

Comment 139

[Brian R. Bondy [:bbondy]]

Hi Roasted, could you post Comment 138 to Bug 827784? No further changes will be done in the context of this bug. That follow up bug and possibly others will be used.

Comment 140

[Roasted]

Sure, done. Thanks for the hint.

Comment 141

[N Brooks]

> Since updating to Firefox 17, shortcut desktop icons have
reverted to generic windows icons

I get exactly the same. This is WORSE than the original which just gave a FF icon for everything, now not only do we not get the FavIcon, we don't get any !

Comment 142

[Yves Goergen [:ygoe]]

WORKAROUND:

As I found out, you can repair those URL shortcuts manually after creating them. Open up the .url file in a plain text editor. Windows Notepad should do, TextPad, UltraEdit, Notepad+ and so on are good. Only keep the first two lines (header and URL) and delete all others which are related to the file's icon, then save the file. If the shortcut is saved on the desktop, it may take a (long) while to update the icon. This change is permanent for each edited shortcut file.

Comment 143

[Russ Hansen]

I understand the desire to make the icons on the desktop more graphically appealing, but on my Windows 7 desktop, I'm using 128x128 scaled icons. The end-result of this feature is that the favicon is a 16x16 blip, difficult to even recognize, in a roughly 48x48 white box, surrounded by a 128x128 frame. This result looks terrible.

Is it possible to make this feature optional, if not in the GUI Options, then at least with an about:config/prefs.js toggle? I prefer a desktop full of large 128x128 Firefox "document" icons to the Firefox 17 desktop full of indecipherable color splotches in the middle of undersized white boxes. I don't think this is universally an improvement to the polish and professional appearance of Firefox in all environments, though it works great in small to medium sized icon environments.

Comment 144

[Russ Hansen]

Hmm. I ran some tests in Windows 8, and now I see what's going on, and why the new handler was written. The new "Internet Shortcut" icon handler is a real pickle. You'd have to set each URL to have an icon of "firefox.exe,1," rather than just toggling it to handle it the old way. Sorry for going off half-cocked, but it really is ugly in Windows 7, and it probably doesn't make Firefox's brand look good on desktops with large icons. Even in Win 8, it still looks very unpolished to me when large icons are on the desktop.

Comment 145

[Brian R. Bondy [:bbondy]]

Work is already being done in this direction, please see Comment 139.

Comment 146

[marcusdoc]

Please allow user setting to disable this "feature" (using about:config) to revert to the previous handling of shortcut creation in the Windows platform.

Thank you.

Comment 147

[Brian R. Bondy [:bbondy]]

There's already a pref called:
browser.shell.shortcutFavicons

Set it to false to disable this.
See bug 827784 for more information.

It was added in v21 though, so if you want to use it now you have to use beta, or you can wait until around May 14th.

Comment 148

[kie000]

The url-icon is shrunk to 1/7th of the icon area available, far too small, especially for 16x16 icons, it's looks totally rubbish.

Comment 149

[GG]

(In reply to Roasted from comment #132)
> 1) the icon itself is tiny compared to the surrounding white space so all
> icons look the same (a white icon with a tiny colored dot inside). Can the
> proportion icon / white fill be adjusted by the user?

YES! Please do that!

Comment 150

[Silekonn]

A fair amount of time a few years ago was invested trying to see someone implement this.  Someone finally took it up and what we received was "it would be best if they were not blown up" to which the obvious response was and even today is "95% of them you can't see."
The programmer then closed the ticket.  When many argued for its reopen his response was it is correct as created, and he closed the ticket.
It was clear then the "fix" was either via lack of time to complete it correctly or some sort of rhyme before reason.  The software has gone a bit downhill since then and around a year ago I was forced to move to something with more realistic performance (e. g. until recently, a 64-bit version, to name one).

Many hope some day Firefox will return to being a good (partial, as the backend is no longer their own) alternative to the ("don't be") evil empire.  Basic features that have lacked for years such as this, while at the same time adding 'sync' features and things that add-ons do and could have waited.  This stands out one more of the reasons users choose other browsers against this one's lack of polish and/or feature bloat.  Here is hoping your progress excels where others have failed.

Comment 151

[Ralph Graw]

Hope.  I'll drink to that.