549472 - Add support for fave icons on jump list uri entries

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

To do this - 

1) we'll need the ability to write fave icons out to disk as ico files (bug 549468)

2) code in the widget jump list module to accept fave icon images, cache the files, set the path info on uri items.

3) the ability to manage the file cache in a smart way - it would be good if we didn't have to delete/write out icons every time we refresh the list, since that can happen quite often.

Also, we'll have to be careful about privacy issues.

Comment 3

[Brian R. Bondy [:bbondy]]

I have some a bit of refactoring to finish up for the ICO/BMP encoder and decoder but they are all working and passing a large set of reftests currently.

I'm going to start on this task Wednesday morning.

Re 1: This is done now via Bug 549468, Bug 670466, and Bug 600556
Re 2: I think we have to use @mozilla.org/browser/favicon-service;1 for this
Re 3: We could just check the disk to see if the file is already on disk and if so then we don't need to re-encode the icon.  This however wouldn't help if the favicon of a site changes.  In that case we can maybe store something unique of the source favicon (maybe crc if nothing else) and compare that before re-generating an ico.
Re 4: I think the ico should be stored in the profile directory somewhere.  

The ICO encoder supports both PNG and BMP ICOs, but for this task I'll just use a 16x16 PNG ICO.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot of working jumplists with favicons

Here is a screenshot of the working jumplists.

I will attach an intermediate patch shortly.

Left to do for today:
- Refactoring

- Currently I'm storing cached ico files in c:, I will find a better place for this, probably in the FF user profile dir

- I'm always writing out favicons on each task list build.  I will optimize this to some method based on the last modification time of the ICO file on disk.  I'll add a setting for that timespan like the other jump list settings.  Example: If the favicon is more than 24h long it will re-get them from the favicon service and re-encode them as ICO.

Left to do for other patches that this is based on: (Won't affect this patch)
- Implement review comments
- Refactoring

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Intermediate patch - working jumplist custom file based .ico icons - needs cleanup

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Created attachment 545877 [details]
> Screenshot of working jumplists with favicons
> 
> Here is a screenshot of the working jumplists.


Sweet!

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch for jump list URI icons for the site's favicon

Patch is implemented and ready for review.

Some notes before you review:
- A new shortcut attribute was added for specifying the site to use for the favicon
- If for any reason we can't set the icon based on the URI we will fall back to the application and associated icon index
- Icons are cached inside the profile directory under a new subdir
  Example: C:\Users\bbondy\AppData\Local\Mozilla\Firefox\Profiles\salt\jumpListCache
- I was thinking initially of doing the image ICO encoding stuff in JS but the needed XPCOM components for conversion weren't scriptable so most of the work is in C++, minor changes to the WindowsJumpLists.jsm and transparent to the JS
- xpcshell test was expanded to include the new shortcut attribute
- When icons are removed from the jump list I also clear the icon cache for those associated icons.
- No matter what the favicon data returned is (as long as we have a decoder), it will convert to a 16x16 ICO.  The ICO contains a PNG inside of it.
- We don't build the favicon on each list build.  
  Instead if a favicon ICO is already cached for a site and it's been more than 24h since we re-generated it, we will re-generate the icon. 
  I added a setting to control the 24h span.  If defined it will use it instead: browser.taskbar.lists.icoTimeoutInSeconds


If you want to actually try out the code: 
1. Apply the latest patch from Bug 600556 (BMP decoder refactoring + new ICO decoder), 
2. Optionally apply the reftest from Bug 600556 (reftests for BMP decoder)
3. Apply the latest patch from Bug 670466 (.BMP Encoder),
4. Apply the latest patch from Bug 549468 (.ICO Encoder)
5. Optionally apply the reftest patch from Bug 670466 (reftests for BMP encoder and ICO encoder)
6. Apply the patch itself, from this Bug.  Which is Bug 549472

The patches in steps 1-5 will be enhanced over the next couple days but it won't change the code nor review for this task.

Comment 8

[Brian R. Bondy [:bbondy]]

Cancelling review because:

I'm going to make a change to the base interface to take a image URI instead of a page who I get the image URI from since this interface will be used by others than Firefox.

Also I'll do a try server build before requesting a review.  

Please feel free to leave any feedback otherwise though on the patch.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Base code for XPCOM Jump list favicon support

Task is finished as per comments in Comment 7.

Implemented the change I mentioned in Comment 8.
I also split the patch into 2 parts:
1) All of the base xpcom stuff 
2) The /browser jsm stuff

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: /browser/ code using XPCOM Jump list favicons

Comment 11

[Brian R. Bondy [:bbondy]]

I ran the widget xpcshell tests manually and they are passing, but did not try it on a try server yet as I don't have level 1 access yet (bug for that is in progress). 

For that reason I am not setting r? but please feel free to review if you want to.  Comment 7 includes the instructions for which patches to apply.  You must apply both patches for Step 6 though in this bug (Since I just split the patch for this bug in 2).

Comment 12

[James Ross]

(In reply to comment #7)
> - No matter what the favicon data returned is (as long as we have a
> decoder), it will convert to a 16x16 ICO.  The ICO contains a PNG inside of
> it.

Would it be better to use the small icon size, GetSystemMetrics(SM_CX/YSMICON)?

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Base code for XPCOM Jump list favicon support

The latest patch is now using GetSystemMetrics(SM_CX/YSMICON) as James Ross mentioned.  The only time I could find that these values would be different is when a user adjusts their DPI.

I tried changing my DPI setting bigger and get values of 20x20 for the icons.  

Windows does scale the 16x16 icons for jump lists automatically bigger, but it's better (quality wise and less work for windows) if we do the direct conversion ourselves to 20x20.  All implemented.

Comment 14

[Brian R. Bondy [:bbondy]]

I think this task and its dependents are passing the try server now that I have access:
http://tbpl.mozilla.org/?tree=Try&rev=3aff9a1fbe07
So I think this task is ready for review.

Please see Comment 9 for details of what to apply in order to try the patch.  Or maybe just a quick pass through of the patch first for any initial feedback before you try it out.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Base code for XPCOM Jump list favicon support (rebased)

Fixed formatting things I learnt from other reviews and had to update mime type based on official mime type changed in a dependant patch

Comment 16

[Jim Mathies [:jimm]]

Which patches should I apply to get this working?

Comment 17

[Brian R. Bondy [:bbondy]]

From top to bottom apply in the same order:

Bug 600556:
- https://bugzilla.mozilla.org/attachment.cgi?id=548443
- https://bugzilla.mozilla.org/attachment.cgi?id=546387 (Optional reftests)

Bug 670466:
- https://bugzilla.mozilla.org/attachment.cgi?id=548643
- https://bugzilla.mozilla.org/attachment.cgi?id=548509

Bug 549468:
- https://bugzilla.mozilla.org/attachment.cgi?id=548648
- https://bugzilla.mozilla.org/attachment.cgi?id=548650 (Optional reftests)

Bug 549472: (this task)
- https://bugzilla.mozilla.org/attachment.cgi?id=548680
- https://bugzilla.mozilla.org/attachment.cgi?id=546090

Any problems just let me know, I'm going out for lunch in about an hour (and for about an hour) but otherwise will be around after that.
I just pulled latest source yesterday and implemented latest review comments yesterday for the dependent patches, so they should be OK.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 548680 [details] [diff] [review]
Base code for XPCOM Jump list favicon support (rebased)

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

Requesting review on the fave icon service usage from mak. I wonder if GetFaveIcon can block (looks like it can) and I don't see an alternative call that allows callers to specify "cache only".

r+ though with nits addressed and the fave icon question resolved.

::: widget/public/nsIJumpListItem.idl
@@ +150,5 @@
> +
> +  /**
> +   * Set or get the URI  of an image to use as the jump list item icon.
> +   * The image will be retrieved, scalled to the needed size and converted
> +   * to the needed format.

nits - grammar /spelling

::: widget/src/windows/JumpListItem.cpp
@@ +379,5 @@
> +  }
> +
> +  // Obtain the pref
> +  const char PREF_ICOTIMEOUT[]  = "browser.taskbar.lists.icoTimeoutInSeconds";
> +  nsCOMPtr<nsIPrefBranch> prefService = do_GetService(NS_PREFSERVICE_CONTRACTID);

We have a new preferences module for this work:

Preferences::GetIntPref(..

See mozilla/Preferences.h. There's use of it in nsWindow.cpp for an example.

@@ +396,5 @@
> +
> +// (static) Obtains the ICO data on disk for the specified aIconURI and
> +// fills the path where the ICO file data was stored to.
> +nsresult JumpListShortcut::ObtainCachedIconFile(nsCOMPtr<nsIURI> aIconURI,
> +  nsString &aICOFilePath)

nit - align with the other parameter.

@@ +488,5 @@
> +  nsCString mimeType;
> +  PRUint32 dataLength;
> +  PRUint8 *data;
> +  // Careful! data needs to be freed, GetFaviconData is a pretty unsafe function
> +  nsresult rv = favIconSvc->GetFaviconData(aIconURI, mimeType, 

It looks like this could block if the fave icon isn't cached.

@@ +514,5 @@
> +  // these settings, fall back to 16x16 ICOs.  These values can be different
> +  // 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.
> +  int systemIconWidth = GetSystemMetrics(SM_CXSMICON);

PRInt since it's use is common throughout this code file.

@@ +716,5 @@
> +  // Construct the path of our jump list cache
> +  nsCOMPtr<nsIFile> jumpListCache;
> +  nsresult rv = NS_GetSpecialDirectory("ProfLDS", getter_AddRefs(jumpListCache));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = jumpListCache->AppendNative(NS_LITERAL_CSTRING("jumpListCache"));

You use this string in a couple places, lets move it up to the top of the file as a const.

Comment 19

[Jim Mathies [:jimm]]

Another thing I noticed, if you disable the list generation through prefs, this leaves icons around. That's not a big deal since they'll go with the profile on uninstall, but you might look for an easy way to add to this that purges those when jump lists are disabled. (I only noticed this from testing - it would have been nice to see the icons go away and then come back.)

Comment 20

[Brian R. Bondy [:bbondy]]

> it would have been nice to see the icons go away and then come back

I'll add an observer for this pref, and when it is changed and disabled I'll nuke out the jumplistcache directory's icons. The icons will be regenerated the next time the pref is re-enabled via the jump list build frequency.

Comment 21

[Brian R. Bondy [:bbondy]]

> I wonder if GetFaveIcon can block (looks like it can) and I don't see an alternative call that allows callers to specify "cache only".

I'm not sure either, but is it a problem if it blocks? I think it is in its own thread that re-builds the list on your jump list build frequency.

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)
> > I wonder if GetFaveIcon can block (looks like it can) and I don't see an alternative call that allows callers to specify "cache only".
> 
> I'm not sure either, but is it a problem if it blocks? I think it is in its
> own thread that re-builds the list on your jump list build frequency.

I can't fire it up in a debugger to be sure, but I believe this is on the UI thread.

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #15)
> Created attachment 548680 [details] [diff] [review] [diff] [details] [review]
> Base code for XPCOM Jump list favicon support (rebased)
> 
> Fixed formatting things I learnt from other reviews and had to update mime
> type based on official mime type changed in a dependant patch

Note, there are windows line endings in jump list item. Please clean these up as well.

Comment 24

[Brian R. Bondy [:bbondy]]

> I can't fire it up in a debugger to be sure, but I believe this is on the UI thread.

Yes you were correct, it is in the UI thread.  So if it blocks it is a problem. I'm checking now if it blocks.

Comment 25

[Brian R. Bondy [:bbondy]]

It doesn't look like GetFaviconData blocks:

nsFaviconService::GetFaviconData makes calls to:
  - nsFaviconService::GetDefaultFavicon (obtains a chrome URI)
  - From there if it is a default URI or not it will call either:
    - nsFaviconService::GetDefaultFaviconData (chrome:// URI)
    - Execute an SQLITE statement to get the data

It looks like we do the blocking get of favicons when setting and not getting as in SetFaviconDataFromDataURL.

Comment 26

[Marco Bonardo [:mak]]

I'm not sure what you mean by blocking, that method runs on UI thread and we should try to avoid that wherever possible.
If you want to get only cached icons you should rather get data from a moz-anno:favicon:pageurl channel, or you may code a new async getFavicon method to mozIAsyncFavicons interface.
I don't think we can use any synchronous favicon method here unless we want a choppy UI.

Comment 27

[Jim Mathies [:jimm]]

(In reply to comment #26)
> I'm not sure what you mean by blocking, that method runs on UI thread and we
> should try to avoid that wherever possible.
> If you want to get only cached icons you should rather get data from a
> moz-anno:favicon:pageurl channel, or you may code a new async getFavicon
> method to mozIAsyncFavicons interface.
> I don't think we can use any synchronous favicon method here unless we want
> a choppy UI.

Async would require redesign work on the jump list generator, which we could do, but it'd be great to avoid. Is there any way we could add a "get from cache" call on the fave icon service that would fail if we don't have it available? Or would the call into the db possibly cause chop as well?

Comment 28

[Brian R. Bondy [:bbondy]]

The current call to GetFaviconData will not do an HTTP get for the favicon, it will only get it from the SQLITE cache.  So it depends on if the sqlite execute call is acceptable in the UI thread.

Comment 29

[Marco Bonardo [:mak]]

(In reply to comment #27)
> Is there any way we could add a "get from
> cache" call on the fave icon service that would fail if we don't have it
> available? Or would the call into the db possibly cause chop as well?

The call into the db may easily cause thread contention with other async DB queries, so we should avoid it. We are trying to avoid any new calls that may cause additional SQLite traffic on mainthread, and converting old calls to be async. The old synchronous favicon service interface should be deprecated asap.
I can't see a way to get the icon data without hitting the db, since it should be in memory and this is clearly impossible.

Most likely also reading icons from disk should be done through async streams, as it is now looks like you do those reads on mainthread as well? Is this done only when the user opens the jumplist popup?
In both cases (hitting the Places DB, or hitting disk) the best approach would be to init the request, and update the icon when you get it.  I'm not sure which kind of flexibility winapi gives you in doing that, can you update a jumplist item icon without rebuilding the full list?

Comment 30

[Brian R. Bondy [:bbondy]]

> Most likely also reading icons from disk should be done through async streams, as it is now looks like you do those reads on mainthread as well?

Windows does the loading of icons on disk, we just tell it a file path.  The last submitted patch of this ticket did check for the existence of the icon on disk, but it won't do this on the UI thread soon.  (See below regarding new way)

> can you update a jumplist item icon without rebuilding the full list?

You could update the icon in our icon cache which windows reads from directly independent of the list build. 

---

The current plan is to add a new function similar to addListToBuild called addListToBuildAsync which will return right away and will call a callback when done with adding the list.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 of 5 - Base code for XPCOM Jump list favicon support

This patch is to address the reivew comments of Comment 18. 
I'll be submitting 3 additional patches soon (on top of the current 2):
  - One for an observer on the disable jump list to remove icon cache files
  - One for adding in async support to the base jump list code
  - One for async /browser/ code changes from the base /browser/ patch

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 of 5 - Base code for XPCOM Jump list favicon support

Line ending fix I was going to put in the async patch but I need to rebase it anyway from this patch so I might as well put the line ending fix in patch 1.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 of 4 - Base code for XPCOM Jump list favicon support

Had fixed line endings the wrong way

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 of 5 - Pref observer to clear icon cache when jump list is disabled

It also handles re-enables by re-creating the icons right away because a build happens when the pref is enabled.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 5 - /browser/ code using XPCOM Jump list favicons

Comment 36

[Jim Mathies [:jimm]]

Comment on attachment 550002 [details] [diff] [review]
Patch 2 of 5 - Pref observer to clear icon cache when jump list is disabled

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

r+ w/nits addressed.

::: widget/src/windows/JumpListBuilder.cpp
@@ +223,5 @@
> +  // Loop through each directory entry and remove all ICO files found
> +  do {
> +    PRBool hasMore = PR_FALSE;
> +    rv = entries->HasMoreElements(&hasMore);
> +    if (NS_FAILED(rv) || !hasMore) break;

nit - break on the lower line, or alternatively just ditch rv and check the result inline on these.

@@ +233,5 @@
> +    nsCOMPtr<nsIFile> currFile(do_QueryInterface(supp));
> +    nsAutoString path;
> +    rv = currFile->GetPath(path);
> +    if (NS_FAILED(rv)) {
> +      continue;

nit - no need for paren here.

Comment 37

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 of 4 - Pref observer to clear icon cache when jump list is disabled

Nits are implemented.
The last review was r+ w/ nits implemented.

Comment 38

[Brian R. Bondy [:bbondy]]

Attachment: For reference only: Failed patch attempt with async support 1of2

I have to change the way I was going to do the async. 

I originally implemented the observer pattern and did a new thread and dispatched a new nsIRunnable event to that thread whenever a call to AddListToBuildAsync was called.  I had to first make a deep copy of the passed in JS items array (which took a while to implement right) because you can't use JS objects directly in different threads as of Gecko 2.0 (reference: https://developer.mozilla.org/en/The_Thread_Manager).  When the work was done I would dispatch back to the main thread who would call the observer function to let the observer know of the changes.  This was all working.  

The problem I kept running into though was that a lot of the objects I needed to use did not have an IMPL ISUPPORTS that was thread safe.  I kept having to change more and more to make them thread safe using IMPL_THREADSAFE_ISUPPORTS*, hoping that the underlying code was thread safe (Which it probably wasn't always).  I decided to abandon when I got towards the end and would have to make the fav icon service thread safe.  I'm not sure if we compile sqlite with a threasafe option and it uses nsIUrl which shouldn't be made thread safe. I discussed on IRC and others agreed that I would probably have to change my approach.

If you want to see the async work you have to apply both 1 of 2 and 2 of 2, I never qreresh -X the work properly into the correct places since I'm abandoning this method.  So patch 1of2 is mostly thrown away inside patch 2of2.

The new plan is to use the favicon's async methods to get the icon data and upon retrieving it on a callback to dispatch to a thread to write it out to disk.  Once the file exists on disk nothing else will happen.  On the next list build it will see that the file already exists on disk and simply pass this to Window's jump list builder.  So the first time a favicon is used the first list won't have it, but the lists are generated every 120 seconds so it will pick it up on the next build.

Comment 39

[Brian R. Bondy [:bbondy]]

Attachment: For reference only: Failed patch attempt with async support 2of2

patch 2 of 2 of what was discussed inside of Comment 38 (Most of patch 4 is overwritten in this patch but you need patch 1 to be able to apply it).

Comment 40

[Brian R. Bondy [:bbondy]]

A slight change from the plan I mentioned in the last paragraph of Comment 38, I am going to try to extend nsILocalFileWin.idl to support an Async write method instead.  I think this will eliminate the need for an additional thread.

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Async write is already supported through nsIFileOutputStream, see nsIFileStreams.idl.

https://developer.mozilla.org/en/Code_snippets/File_I%2F%2FO#Copy_a_stream_to_a_file has some more info, but I'm pretty sure that all happens on the main thread.

Comment 42

[Brian R. Bondy [:bbondy]]

I was thinking more along the lines of CreateFile with the FILE_FLAG_OVERLAPPED flag (and OVERLAPPED struct) so that the OS itself does not block on the WriteFile call.

Comment 43

[Brian R. Bondy [:bbondy]]

I have all of this working now async I just need to refactor and move around my code via hg qrefresh -X.  

I added another dependent patch in Bug 676906

Comment 44

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 4 - Converting base code of patch 1 to async

In general:
- A thread is created to do the async operations off of the main thread.
- All writes and deletes happen async in that thread
- A patch was provided in Bug 676906 by me for the favicon service to support getting data async.  Before you could only get the URL of the favicon async which was not good enough.
- The API exposed by the XPIDL stays the same.  Everything happens behind the scenes.  If an icon is not already cached an async request is made and it falls back to the app icon until the next list build.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4 of 4 - /browser/ code

I thought I'd ask for feedback from you for the /browser code before asking for a review from someone in that module.

Comment 46

[Brian R. Bondy [:bbondy]]

I'll also submit a rolled up patch a bit later with everything you need so you can test it out easier than applying 10 or so patches.  Feel free to review independent of that if you want though.

Comment 47

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 of 4 - Base code for XPCOM Jump list favicon support

Small rebase so patch 1 of 4 will apply to latest changes in mozilla-central.

Comment 48

[Brian R. Bondy [:bbondy]]

Attachment: 13 patch rollup for jump list icons 2011-08-08

Here is a roll-up of all patches needed to get the icon jump list working.  This patch is not to be pushed to mozilla-central but just to make trying the code for an easier review.

Comment 49

[Brian R. Bondy [:bbondy]]

By the way I also previously tried setting the icon before it was ready but this can lead to problems if the user tries to open the jump list before the icons are written out.  It caches a bad icon and then it won't refresh until you kill explorer.exe or login/out.

So instead what I do in the patch is async get the icon and use the exe icon until the next rebuild when the icon is available.   

If needed we could later optimize this by sending a flag that there are pending icons that are being written to the jsm so that it could do another list build faster than the usual timeout.  If you want that though I'd prefer we put a new task for it.

Comment 50

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 4 - Converting base code of patch 1 to async v2

Got rid of static thread and use a member variable for the thread now.

Comment 51

[Jim Mathies [:jimm]]

Comment on attachment 552979 [details] [diff] [review]
Patch 3 of 4 - Converting base code of patch 1 to async v2

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

::: widget/src/windows/JumpListBuilder.cpp
@@ +86,5 @@
>    
>    CoCreateInstance(CLSID_DestinationList, NULL, CLSCTX_INPROC_SERVER,
>                     IID_ICustomDestinationList, getter_AddRefs(mJumpListMgr));
>  
> +  NS_NewThread(getter_AddRefs(mIOThread));

Is there any chance the underlying JumpListBuilder could get destroyed out from under this thread, or is there addref'ing taking place on the jlb somewhere while the thread is alive?

@@ +572,5 @@
> +  // Allocate a new buffer that we own and can use out of line in 
> +  // another thread.  Copy the favicon raw data into it.
> +  const fallible_t fallible = fallible_t();
> +  PRUint8 *data = new (fallible) 
> +    PRUint8[aDataLen];

silly nit - please put this all on one line.

@@ +666,5 @@
> +  PRUint32 wrote;
> +  rv = bufferedOutputStream->WriteFrom(iconStream, bufSize, &wrote);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if(bufSize != wrote) {
> +    return NS_ERROR_FAILURE;

Kind of a strange situation to get into. We've failed to write all the output so we fail the call and never call close on the streams. I assume this is unanticipated, maybe this should be an NS_ASSERTION?

Comment 52

[Jim Mathies [:jimm]]

Comment on attachment 551207 [details] [diff] [review]
Patch 4 of 4 - /browser/ code

looks ok to me.

Comment 53

[Brian R. Bondy [:bbondy]]

> is there any chance the underlying JumpListBuilder could get destroyed out from under this thread, or is there addref'ing taking place on the jlb somewhere while the thread is alive?


None of the events on the thread use JumpListBuilder once they are dispatched to the thread. 

The jumplist is created and addref'ed in WindowsJumpList.jsm:
this._builder = _taskbarService.createJumpListBuilder();

I think it's probably best for me to add a thread shutdown in the JumpListBuilder destructor which in turn will call join on the threads.
Shutdown doesn't seem to be called automatically on destruction of the nsThread.

JumpListBuilder::~JumpListBuilder()

{
  
  mIOThread->Shutdown();
  ...
}

Comment 54

[Brian R. Bondy [:bbondy]]

> silly nit - please put this all on one line.

Trying this will give a cast functipn type is illegal error:
> PRUint8 *data = new (fallible_t()) PRUint8[aDataLen];

But I can bring it from 3 lines to 2 though:

> const fallible_t fallible = fallible_t();
> PRUint8 *data = new (fallible) PRUint8[aDataLen];

Comment 55

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 4 - Converting base code of patch 1 to async v3

Implemented latest review comments

Comment 56

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #53)
> > is there any chance the underlying JumpListBuilder could get destroyed out from under this thread, or is there addref'ing taking place on the jlb somewhere while the thread is alive?
> 
> 
> None of the events on the thread use JumpListBuilder once they are
> dispatched to the thread. 
> 
> The jumplist is created and addref'ed in WindowsJumpList.jsm:
> this._builder = _taskbarService.createJumpListBuilder();
> 
> I think it's probably best for me to add a thread shutdown in the
> JumpListBuilder destructor which in turn will call join on the threads.
> Shutdown doesn't seem to be called automatically on destruction of the
> nsThread.
> 
> JumpListBuilder::~JumpListBuilder()
> 
> {
>   
>   mIOThread->Shutdown();
>   ...
> }

I was thinking more along the lines of something like this in a javascript shell:

const Cc = Components.classes; 
const Ci = Components.interfaces;

var taskbar = Cc["@mozilla.org/windows-taskbar;1"].getService(Ci.nsIWinTaskbar);

var builder = taskbar.createJumpListBuilder();

// build list

delete builder;

If the worst that can happen here is the icons don't get written out because the thread and it's tasks get killed, that's ok.

Comment 57

[Jim Mathies [:jimm]]

BTW for testing stuff like this Ted's Extension Builder extension has a nice js shell in it -

https://addons.mozilla.org/en-US/firefox/addon/extension-developer/

You have to rev the compatibility version info manually to get it to run on nightlies.

Comment 58

[Jim Mathies [:jimm]]

Comment on attachment 553456 [details] [diff] [review]
Patch 3 of 4 - Converting base code of patch 1 to async v3

Lets confirm those tasks fire off and shutdown works as advertised. Assuming that, r+.

Comment 59

[Brian R. Bondy [:bbondy]]

Thanks got the extension working on nightly by manually setting the install.rdf accordingly.  I did use this extension quite often previously, I think I had it working with Nightly with an extension to smudge the version numbers of other extensions. 

I checked by putting a 1 minute Sleep inside the thread and it doesn't get past the shutdown until the 1 minute is complete.

Comment 60

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 551207 [details] [diff] [review]
Patch 4 of 4 - /browser/ code

>diff --git a/browser/components/wintaskbar/WindowsJumpLists.jsm b/browser/components/wintaskbar/WindowsJumpLists.jsm

> Components.utils.import("resource://gre/modules/Services.jsm");
> 
>+function makeURI(aURL, aOriginCharset, aBaseURI) {

You can just use Services.io.newURI()

>+const PREF_TASKBAR_ICOTIMEOUT = "icoTimeoutInSeconds";

This appears to be unused?

This otherwise is fairly straightforward, but why is there so much JumpList-specific favicon fetching code? Shouldn't this be making use of the favicon service?

Comment 61

[Brian R. Bondy [:bbondy]]

> Shouldn't this be making use of the favicon service?

Windows unfortunately requires you to have the icons in an ICO file (or windows binary) on disk to generate the jump list.  So this unfortunately required us to write a BMP encoder which is used by an ICO encoder, and then required us to use the favicon service to cache those out to disk.

Comment 62

[Rimas Kudelis [use needinfo to ping]]

(In reply to Brian R. Bondy [:bbondy] from comment #61)
> > Shouldn't this be making use of the favicon service?
> 
> Windows unfortunately requires you to have the icons in an ICO file (or
> windows binary) on disk to generate the jump list.  So this unfortunately
> required us to write a BMP encoder which is used by an ICO encoder, and then
> required us to use the favicon service to cache those out to disk.

Hm, but if this was indeed the only reason for BMP encoder, than maybe it's not actually needed at all? As you know, both Vista and 7 accept icons encoded as PNG, so you would only need the ICO encoder, not BMP.

Comment 63

[Brian R. Bondy [:bbondy]]

Right I think we do actually have it defaulted to use PNG for now.  In any case we have a bunch of ref tests already testing PNG and BMP encoders, and ICO encoders that support both underlying contained PNG and BMP.

Comment 64

[Rimas Kudelis [use needinfo to ping]]

My assumption is that Mozilla doesn't like having unused code in its repos. :)

Comment 65

[Brian R. Bondy [:bbondy]]

> but if this was indeed the only reason for BMP encoder

It was not the only reason.  There were other reasons at the time of doing the BMP encoder. There was a task open that asked for generating ICO files for XULRunner apps. These apps would need to work on Windows XP as well.

> My assumption is that Mozilla doesn't like having unused code

This task uses PNG ICO's, so this argument belongs in the BMP encoder task.  You can bring it up in Bug 670466. There are probably other tasks that will use it though that are currently open.

Comment 66

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4 of 4 - /browser/ code v2

Implemented review comments.

At one point the whole series of tasks passed the try server.  But I'll run it all through try server once again once I get this last patch r+'ed as there were a lot of changes since I last put it on the try server.

Comment 67

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 4 - Converting base code of patch 1 to async v4

Had to update xpcshell test for the async patch (just property name renaming)

Comment 68

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 of 4 - Pref observer to clear icon cache when jump list is disabled v2

Rebased patch for mem leak task which is already on m-c.

Comment 69

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 of 4 - Pref observer to clear icon cache when jump list is disabled v2'

Comment 70

[Brian R. Bondy [:bbondy]]

All of the dependent patches are on mozilla-central.
Pushed this task's patches to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b4e61692184a

Comment 71

[Brian R. Bondy [:bbondy]]

Comment on attachment 551444 [details] [diff] [review]
Patch 1 of 4 - Base code for XPCOM Jump list favicon support

This task is ready to be pushed, I just need a sr on the small interface addition on the IDLs.

Comment 72

[Brian R. Bondy [:bbondy]]

Comment on attachment 555612 [details] [diff] [review]
Patch 3 of 4 - Converting base code of patch 1 to async v4

Interface change that needs sr before I can push the task.  It is just a mod to the interface change that was in Patch 1 of 4.

Comment 73

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 555612 [details] [diff] [review]
Patch 3 of 4 - Converting base code of patch 1 to async v4

>diff --git a/widget/public/nsIJumpListItem.idl b/widget/public/nsIJumpListItem.idl

>    * Set or get the icon displayed with the jump list item.
>    *
>    * Indicates the resource index of the icon contained
>    * within the the handler executable.
>    */
>   attribute long iconIndex;
> 
>   /**
>+   * Set or get the URI of a page who's favicon will be used as the jump
>+   * list item icon.  The favicon will be asynchronously retrieved, scaled
>+   * to the needed size and converted to the needed format.
>    *
>+   * If there is an error setting the faviconPageUri, the
>    * app and iconIndex will be used to generate the icon.
>    */
>+  attribute nsIURI faviconPageUri;

nit: faviconPageURI ?

Seems like it'd be useful to also explain the iconIndex/faviconPageUri prioritization in iconIndex's documentation. Being a little more explicit about the behavior might be good too - is iconIndex ignored forever if a valid faviconPageUri was set (and the data retrieval was successful)? What happens if retrieving the data wasn't successful (does the faviconPageURI get reset)? What happens if I later set faviconPageURI to null, after a valid set?

r=me with those considered.

Comment 74

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 4 - Converting base code of patch 1 to async v5

Updated comments on the interface to explain prioritization more and other factors mentioned by Gavin in Comment 

> nit: faviconPageURI?

I prefer the capitalization you mentioned as well, but I did it that way to stay consistent with other things int he same file:

JumpListLink::GetUriHash
JumpListLink::GetUriTitle
JumpListLink::SetUriTitle

Please advise if you want me to change it to faviconPageURI anyway.

Comment 75

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 558514 [details] [diff] [review]
Patch 3 of 4 - Converting base code of patch 1 to async v5

>diff --git a/widget/public/nsIJumpListItem.idl b/widget/public/nsIJumpListItem.idl

> interface nsIJumpListShortcut : nsIJumpListItem

Sorry to over-rotate on this, but I think rather than repeating the same comment multiple times, this might be clearer as a single comment before faviconPageUri, with an @see faviconPageUri reference next to "app" and "iconIndex". Something like:

When a jump list build occurs, the favicon to be used for the item is obtained using the following steps:

- First, attempt to use asynchronously retrieve and scale the favicon associated with the faviconPageUri.
- If faviconPageUri is null, or if retrieving the favicon fails, fall back to using the handler executable and iconIndex.

Comment 76

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 4 - Converting base code of patch 1 to async v6

> Sorry to over-rotate on this, but I think...

No problem, your review comments now will affect my future patches and work as well.  Thanks for the quick reviews!

Comment 77

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/830e9ce147b0
http://hg.mozilla.org/integration/mozilla-inbound/rev/4ba3fb301ff1
http://hg.mozilla.org/integration/mozilla-inbound/rev/3521d58c2234
http://hg.mozilla.org/integration/mozilla-inbound/rev/a4dc1b53ece1

Comment 78

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/830e9ce147b0
http://hg.mozilla.org/mozilla-central/rev/4ba3fb301ff1
http://hg.mozilla.org/mozilla-central/rev/3521d58c2234
http://hg.mozilla.org/mozilla-central/rev/a4dc1b53ece1

Comment 79

[Jim Jeffery not reading bug-mail 1/2/11]

Testing with the latest hourly build based on cset:
http://hg.mozilla.org/mozilla-central/rev/0c7303e897c5

The 'jump-list' seems to be totally missing, and non-functional as in nothing is show up in the list other than the defaults: Program name, Nightly, Pin, Close Window.

There is also no 'fly-out' list in the start-orb (win7) on the frequently used programs list. 

Nothing abnormal in the Error Console.

Comment 80

[Brian R. Bondy [:bbondy]]

Just to make sure you aren't using a new profile within the first 120 seconds right? It takes 120 seconds to build the first list.  But this is the same as before.  I'll try the nightly build, my debug build works well.

Comment 81

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Brian R. Bondy [:bbondy] from comment #80)
> Just to make sure you aren't using a new profile within the first 120
> seconds right? It takes 120 seconds to build the first list.  But this is
> the same as before.  I'll try the nightly build, my debug build works well.

Yes, indeed.. I was too anxious, the list did populate after a time.  

With that said, we can mark this 'verified fixed'.

Comment 82

[Brian R. Bondy [:bbondy]]

Great! Are the favicons populating for you on the pages that have them Jim?

Comment 83

[Brian R. Bondy [:bbondy]]

That was meant for Jim Jeffery by the way not Jim Mathies.

Comment 84

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Brian R. Bondy [:bbondy] from comment #82)
> Great! Are the favicons populating for you on the pages that have them Jim?

Yes, the favicons are appearing, and looks very good...

Comment 85

[Siddhartha Dugar [:sdrocking]]

The Recent items list doesn't have favicons. Please verify.

Comment 86

[Brian R. Bondy [:bbondy]]

Works for me, likely what you are seeing is the expected behaviour of using the app icon while it async downloads/converts the favicons.  The next jump list build should have the favicon.  The default setting is a new jump list build every 120 seconds.

Comment 87

[SpeciesX]

Pinned pages doesn't have favicons and when a page has no icon, it use still the old icon and not the new from Bug 648668.

Comment 88

[Brian R. Bondy [:bbondy]]

> Pinned pages doesn't have favicons and when a page has no icon

I don't think we manage pinned items ourselves, particularly if you pin before the icon is generated by us I think you'll have whatever icon existed at that time.  When I pin an item we already have the icon for it works for me.   Could you confirm jimm if you know if we manage pinned items?

>  it use still the old icon and not the new from Bug 648668.

Could you attach an example screenshot on how it looks and how you expect it to look?

Comment 89

[Jim Mathies [:jimm]]

That's my understanding as well. I believe Windows moves the shortcut for the item that's pinned to a different location. From the apis, I don't see a way to "get at" these items.

Comment 90

[Brian R. Bondy [:bbondy]]

jimm what do you think about me adding a new observer for the jump list builder that would notify when we have an 'unfinished jump list item'.  I.e. an item who's icon will likely be available next time but currently is not.  

We would register and listen for this in WindowsJumpLists.jsm and we could do a new list rebuild early when we have this case.   It would have a minimum threshold of when to rebuild though.   

Maybe it's not worth it, but let me know what you think.  I think only within the first 240 seconds of a user experience for the lifetime of their browsing experience they would ever really experience this anyway. 

Also it might be nice to handle the very first jump list build faster than 120 seconds on new profiles.

Comment 91

[Brian R. Bondy [:bbondy]]

> Also it might be nice to handle the very first jump list build faster than 120 seconds on new profiles.

Faster but not directly on startup.

Comment 92

[SpeciesX]

Attachment: icon example

>  I don't think we manage pinned items ourselves, particularly if you pin before the icon is generated by us I think you'll have whatever icon existed at that time. 

and when the pinned page came not from the recent list, how it can generate a new icon?

Comment 93

[Brian R. Bondy [:bbondy]]

speciesx, I don't think we can control the pinned icons, but I haven't looked into it deeply yet.  Google Chrome who has had jump list icons for a while now has this same behaviour regarding pinned items from outside.

Comment 94

[Brian R. Bondy [:bbondy]]

Regarding the first list build not happening for 120 seconds.  It may take this long for a user to gather proper frequent or recent lists anyway.

Comment 95

[Francesco]

I just upgraded to FF 37 and noticed that the uri entries on Win7 jump list no longer display the favicon but the default Firefox icon; .ico files in jumpListCache, if any, are deleted. Is this by design? Or a regression on this bug?

Comment 96

[Timothy Nikkel (:tnikkel)]

Might be a regression. Test in safe mode and/or a fresh profile. If it still happens then file a new bug please.

Comment 97

[Francesco]

(In reply to Timothy Nikkel (:tn) from comment #96)
> Might be a regression. Test in safe mode and/or a fresh profile. If it still
> happens then file a new bug please.

Just filed bug 1150534