User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre Build Identifier: In Windows 7, IE8 allows tabs to be browsed via the task bar. That's quite useful, IMHO as it allows to select the right tab to go even in another application. Reproducible: Always Expected Results: See: http://blogs.msdn.com/ie/archive/2009/01/10/ie8-in-windows-7-beta.aspx who has a screen copy of the feature (Is there a meta-bug for Windows 7 OS integration?)

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 Ubiquity/0.1.4 A more general meta-bug would be nice in order to avoid to have the several features in different bugs. For instance the Jump List is also not implemented in Firefox. More information here: http://blogs.msdn.com/yochay/archive/2009/01/06/windows-7-taskbar-part-1-the-basics.aspx

I would (In reply to comment #1) > Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.0.5) Gecko/2008120122 > Firefox/3.0.5 Ubiquity/0.1.4 > > A more general meta-bug would be nice in order to avoid to have the several > features in different bugs. > For instance the Jump List is also not implemented in Firefox. > More information here: > http://blogs.msdn.com/yochay/archive/2009/01/06/windows-7-taskbar-part-1-the-basics.aspx Lets make this the meta, change the title and spin off bugs from this one. There are a number of things we can consider here, we should put some effort into coming up with new ideas. Ars dug into Jump Lists in a recent article that's worth a read - http://arstechnica.com/articles/paedia/windows-7-beta.ars/2

I wonder if the behaviour I described in the description is part of what Microsoft call the Jump List feature (which is interested per se, btw) or not. The Jump List seems to work even when the app is not running (a kind of improved MRU display available from outside the apps, defaulting to the MRU I believe), the tabs display appearing only when IE is running. Anyway, a similar bug exists for Mac as OS X 10.5 allows the customization of the dock menu. An API is being worked there by Tom Dryas. It may be an interesting starting point. See bug 415463 .

(In reply to comment #3) > I wonder if the behaviour I described in the description is part of what > Microsoft call the Jump List feature (which is interested per se, btw) or not. > The Jump List seems to work even when the app is not running (a kind of > improved MRU display available from outside the apps, defaulting to the MRU I > believe), the tabs display appearing only when IE is running. > > Anyway, a similar bug exists for Mac as OS X 10.5 allows the customization of > the dock menu. An API is being worked there by Tom Dryas. It may be an > interesting starting point. See bug 415463 . I don't believe so. As far as I can tell, jump lists are just the application-specific lists that appear when right-clicking on an icon in the taskbar. What you seem to be referring to in the description is a tab preview on scroll-over, which seems to behave just like the window preview.

For reference, the COM API to implement custom Jump Lists is available at <http://msdn.microsoft.com/en-us/library/dd378402%28VS.85%29.aspx>

I think that when Firefox is closed, one of the jump list options should be to restore the last session. While I don't like this behavior by default, it would be nice to have the option there for starting up. Also, the jump list should include a search bar. Maybe all your search engines, but Google for sure. That would be really convenient to have always available.

What about a folder (like done in bookmarks) to show your bookmarks?

(In reply to comment #6) > I think that when Firefox is closed, one of the jump list options should be to > restore the last session. While I don't like this behavior by default, it > would be nice to have the option there for starting up. I think this could vary depending on your setting. E.g., if my setting is to always restore the last session, the possibility to start with an empty session would probably be useful. This also clues that both options (with appropriate names) would make most sense. Or just the default one.

(In reply to comment #7) > What about a folder (like done in bookmarks) to show your bookmarks? I would add that, but hide it by default. By default I'd add the links on your links bar to a "link" folder or similar. The entire bookmark menu seems like overkill to me (as a default setting). Depending on the number of options we come up with, making what shows up here configurable through a config UI may make sense.

Also, getting the Recent or Frequent list tied into Spaces might be interesting.

Here is an implementation of Jump List in Firefox done by Brent with an external program : https://brentf.com/blog/software/winfox-ndash-firefox-and-windows-7/

Comment on attachment 390246 [details] [diff] [review] widget v.1 jst -> changes to the handler app interface. These are non-intrusive changes, I just needed a good place to store additional params for local handler apps (description, command line args, win icon res id).

New patch including the upper level bits. In this rev I'm sourcing this into browser.js, but I'll be moving it, maybe to components. This is about as far as I'm going to take this until the taskbar preview code lands. A also need to work out bug 505925 which is causing problems for registering recent and frequent lists.

Comment on attachment 390246 [details] [diff] [review] widget v.1 - interface nsILocalHandlerApp : nsIHandlerApp { +#ifdef XP_WIN + /** + * Optional icon resource id. Defaults to 1. + */ + attribute short iconResID; +#endif Could we make this platform neutral and remove the #ifdef XP_WIN in this interface? And maybe make this a 32-bit integer rather than a 16-bit one to be more likely to be usable on other platforms for similar purposes? The Windows implementations could throw if the value doesn't fit in 16 bits etc... - In nsLocalHandlerApp::EqualsStrict(): + nsresult rv = aHandlerApp->GetExecutable(getter_AddRefs(executable)); + if (NS_FAILED(rv) || !executable || !mExecutable) { + *_retval = PR_FALSE; + return NS_OK; Should we return rv here instead of always returning NS_OK? I don't think I care strongly here, but propagating errors to the caller seems like the right thing to do here IMO. Same thing in several other places following this particular one. - In several files in this patch: \ No newline at end of file Add a newline at the end of file please :) r=jst with that. Please have bz look at the handler app parts of an update patch.

(In reply to comment #16) > - In nsLocalHandlerApp::EqualsStrict(): > > + nsresult rv = aHandlerApp->GetExecutable(getter_AddRefs(executable)); > + if (NS_FAILED(rv) || !executable || !mExecutable) { > + *_retval = PR_FALSE; > + return NS_OK; > > Should we return rv here instead of always returning NS_OK? I don't think I > care strongly here, but propagating errors to the caller seems like the right > thing to do here IMO. Same thing in several other places following this > particular one. In the exe case, I think it's right since a failure implies there's no file set. Although I might update this code to something like: nsCOMPtr<nsIFile> executable; nsresult rv = aHandlerApp->GetExecutable(getter_AddRefs(executable)); if (NS_FAILED(rv) || !executable) { if (!mExecutable) // both empty *_retval = PR_TRUE; else *_retval = PR_FALSE; return NS_OK; } The original code was pulled from equals(). I think the two calls should probably match up when doing the same comparisons so if I update one in any way I should probably update the other.

(In reply to comment #17) > nsresult rv = aHandlerApp->GetExecutable(getter_AddRefs(executable)); > if (NS_FAILED(rv) || !executable) { > if (!mExecutable) // both empty > *_retval = PR_TRUE; > else > *_retval = PR_FALSE; Just *_retval = !mExecutable would do, but GetExecutable is infallble and it returns an addref'ed mExecutable: http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsLocalHandlerApp.cpp#125 so this code doesn't make sense on several fronts. /be

Breaking this down into separate patches to simplify reviews. This patch has the handler app parameter changes.

Comment on attachment 392252 [details] [diff] [review] mime patch v.1 I have no idea how this got lost... I seem to have no bugmail for this. > + attribute AString description; This seems to be somewhat duplicating nsIMIMEInfo::defaultDescription. Can the nsIMIMEInfo property just call through to your new code? >+ attribute AString parameters; An array would seem to make more sense to me. Certainly a string is not useful for our nsIProcess implementation, and if we have platform-specific code that wants to have a flattened string, then this should either be on a platform-specific interface or the code should flatten itself. > + attribute long iconResID; Does this make sense anywhere but Windows? If not, should it be on a Windows-specific interface? >+ // Compare the icon resource id There was no mention of this in the IDL. Do we really want to be comparing these? > + // Compare the apps Can this just fall through to nsIHandlerApp::Equals?

(In reply to comment #22) > (From update of attachment 392252 [details] [diff] [review]) > I have no idea how this got lost... I seem to have no bugmail for this. > > > + attribute AString description; > > This seems to be somewhat duplicating nsIMIMEInfo::defaultDescription. Can the nsIMIMEInfo property just call through to your new code? We have a couple of these in here, the one I'm adding is oriented toward a long winded description of the app tailored for a tooltip. We also have description - "A human readable description of the handler type" defaultDescription - "A pretty name description of the associated default application. Only usable if hasDefaultHandler is true." My use really doesn't fit with either of these and I don't want to populate something with the wrong info. However there's little risk of anything going wrong. Your call. > > >+ attribute AString parameters; > > An array would seem to make more sense to me. Certainly a string is not useful > for our nsIProcess implementation, and if we have platform-specific code that > wants to have a flattened string, then this should either be on a > platform-specific interface or the code should flatten itself. will do. > > > + attribute long iconResID; > > Does this make sense anywhere but Windows? If not, should it be on a > Windows-specific interface? Do macs/unix have the concept of a resource id that points into a library? I'll move it out on the assumption they don't. > > >+ // Compare the icon resource id > > There was no mention of this in the IDL. Do we really want to be comparing > these? I put it in there since equalsStrict implies a strict comparison. There's no harm in comparing them, if the icon index is different, technically the two shortcuts aren't equal. > > > + // Compare the apps > > Can this just fall through to nsIHandlerApp::Equals? will do.

> the one I'm adding is oriented toward a long winded description of the app > tailored for a tooltip Ah, worth making that clear in the comments. The one on nsIMIMEInfo is basically something like "Adobe Acrobat Reader" as opposed to "acroread.exe". It sounds like yours would be something distinct from both of those, right? Can we change the name to something that makes it clear? I guess "description" is not too bad if we document it well enough (in particular document how it differs from the nsIMIMEInfo stuff). > Do macs/unix have the concept of a resource id that points into a library? Unix does not, to my knowledge. Not sure about Mac. > I put it in there since equalsStrict implies a strict comparison OK, but then just change the idl comments to make it clear that all available information will be compared except the description.

(In reply to comment #24) > > the one I'm adding is oriented toward a long winded description of the app > > tailored for a tooltip > > Ah, worth making that clear in the comments. The one on nsIMIMEInfo is > basically something like "Adobe Acrobat Reader" as opposed to "acroread.exe". > It sounds like yours would be something distinct from both of those, right? > Can we change the name to something that makes it clear? I guess "description" > is not too bad if we document it well enough (in particular document how it > differs from the nsIMIMEInfo stuff). > > > Do macs/unix have the concept of a resource id that points into a library? > > Unix does not, to my knowledge. Not sure about Mac. > > > I put it in there since equalsStrict implies a strict comparison > > OK, but then just change the idl comments to make it clear that all available > information will be compared except the description. Will do, thanks Boris.

Please excuse the spam - but I have a related question, and I'm not sure if this bug, or one that it's tracking, addresses it or not. This is specific to SeaMonkey - where "seamonkey -browser" and "seamonkey -mail" open up, respectively, the browser component and the mail component of the suite. When I drag these shortcuts to the Windows 7 taskbar, they do not get grouped into the same overall SeaMonkey icon. Rather, I will get two different icons - one for the browser, one for mail. This seems reasonable, until I actually go to launch them. Regardless of which order I launch them in, the now running windows become grouped under the first launched icon. So, for example, if I launch a browser session then a mail session, the taskbar shows 3 "grouped" icons for the browser. (Similarly, 3 "grouped" icons for mail if I launch that one first.) It doesn't seem right that it should treat them separately when pinned to taskbar - but together (randomly) when actually launched. Either everything should be grouped together, or the different functions should be treated as individual applications. Should I file a new bug for this problem (and add it to those tracked here)?

Definitely a different bug, and the spam here doesn't help.

I had to comment because I didn't know if it was related or not. If it *was* related, it wouldn't have been spam. :)

(In reply to comment #26) > Please excuse the spam - but I have a related question, and I'm not sure if > this bug, or one that it's tracking, addresses it or not. > > This is specific to SeaMonkey - where "seamonkey -browser" and "seamonkey > -mail" open up, respectively, the browser component and the mail component of > the suite. > > When I drag these shortcuts to the Windows 7 taskbar, they do not get grouped > into the same overall SeaMonkey icon. Rather, I will get two different icons - > one for the browser, one for mail. This seems reasonable, until I actually go > to launch them. Regardless of which order I launch them in, the now running > windows become grouped under the first launched icon. So, for example, if I > launch a browser session then a mail session, the taskbar shows 3 "grouped" > icons for the browser. (Similarly, 3 "grouped" icons for mail if I launch that > one first.) > > It doesn't seem right that it should treat them separately when pinned to > taskbar - but together (randomly) when actually launched. Either everything > should be grouped together, or the different functions should be treated as > individual applications. > > Should I file a new bug for this problem (and add it to those tracked here)? The grouping is controlled through app ids, see - http://msdn.microsoft.com/en-us/library/dd378459%28VS.85%29.aspx They can be applied per processes / exe or in the case of an app that has multiple uses, per window. The shortcuts you create also have the appid of the window you right click - dock embedded in them. The m-c widget code for jump lists will include process registration with a default app id, and I was thinking of overriding that using compile directives that set it based on what is being built. There may be some issues though because app ids are also tied in with vista+ file and protocol registration, which we do not version. That might cause problems for firefox and tb and depending on how it's handled in the code/install, seamonkey. All the taskbar stuff should be landing here in the next couple weeks on m-c and 1.9.2, so I would suggest waiting for that so you can leverage whatever we learn from testing Fx. I also have a general bug filed on the app registration stuff - bug 505925.

> All the taskbar stuff should be landing here in the next couple weeks on > m-c and 1.9.2, so I would suggest waiting for that so you can leverage > whatever we learn from testing Fx. Well, based on Benjamin's response I already went ahead and filed bug 515734 on my specific issue.

Requesting blocking 3.6 on this because I think it should get in along with the aero peek patches. Note, if approved, there are some dependencies that should be considered blocking as well - 495988 - this will be fixed by the patch in bug 491947 which I'm finishing up today. It's a must fix for Jump Lists. 506955 - if we want "private browsing mode" visible in the jump list task list, this is a must fix. 515785 - icons for the tasks should probably block as well.

On the handler app changes, I ended up placing the icon down in my jump list item object, so the only thing I needed here was the new command line parameter management, detailedDescription, and some changes to equals().

Rob if you don't have the time to go over this, please let me know and I'll track down some other unwitting victim.

Note on blocking status, this has both icons and strings for the jump list tasks list, so unless we want to allow for late strings or can reuse existing strings, this can't block. The strings tie to the four desired tasks, new tab, new window, private browsing mode, and profile manager. On the first two, we may be able to pilfer strings from other locations - +taskbar.tasks.one.label=Open new tab +taskbar.tasks.one.description=Open a new tab in the current window. +taskbar.tasks.two.label=Open new window +taskbar.tasks.two.description=Open a new window. +taskbar.frequent.label=Frequent +taskbar.recent.label=Recent On the last two, private browsing mode is currently held up by a dependency, and the profile manager tasks isn't displayed by default and could be dropped. +taskbar.tasks.three.label=Private Browsing +taskbar.tasks.three.description=Open in private browsing mode. +taskbar.tasks.four.label=Profiles +taskbar.tasks.four.description=Open the Profile Manager.

The profile manager should under no circumstances have jumplist UI.

Sorry for the spam. Added tests for the new parameters interface and an additional equality test since I made some changes to equals().

Added unit tests for the jump list item and task bar interfaces.

Comment on attachment 402199 [details] [diff] [review] mime patch v.3 >+++ b/netwerk/mime/public/nsIMIMEInfo.idl >+ * Detailed description for this handler. Suitable for >+ * for a tooltip or short informative sentence. s/for for/for/ >+++ b/uriloader/exthandler/nsLocalHandlerApp.cpp >-nsLocalHandlerApp::Equals(nsIHandlerApp *aHandlerApp, PRBool *_retval) Why did you move this method wholesale to a different part of the file instead of editing it in-place? >+nsLocalHandlerApp::GetParameters(nsIStringEnumerator **aParameters) >+ return NS_NewStringEnumerator(aParameters, &mParameters, this); Might want to file a separate bug on having the string enumerator support cycle collection. Or something. It's pretty trivial to make this code leak by just having the handler app hold a ref to the enumerator in JS, as far as I can tell. >+nsLocalHandlerApp::ParameterExists(const nsAString & param, PRBool *_retval) >+{ >+ *_retval = PR_FALSE; >+ for (PRUint32 idx = 0; idx < mParameters.Length(); idx++) { >+ if (mParameters[idx].Equals(param)) { >+ *_retval = PR_TRUE; >+ return NS_OK; >+ } >+ } Was there a reason that mParameters.Contains(param) didn't work here? Also, aParam (and similar in other methods where your arg is called |param|), right? >+nsLocalHandlerApp::Equals(nsIHandlerApp *aHandlerApp, PRBool *_retval) >+ if ((executable && !mExecutable) || (!executable && mExecutable)) >+ return NS_OK; >+ >+ // Equality for two empty nsIHanderApp >+ if (!executable && !mExecutable) { >+ *_retval = PR_TRUE; >+ return NS_OK; >+ } s/nsIHanderApp/nsIHandlerApp/ I'd switch the order of those two ifs, and simplify the condition in the one that checks for one executable being null and the other being non-null to just |!executable || !mExecutable|. >+ // Check the command line params lists >+ for (PRUint32 idx = 0; idx < mParameters.Length(); idx++) { >+ PRBool exists; >+ localHandlerApp->ParameterExists(mParameters[idx], &exists); >+ if (!exists) { >+ return NS_OK; >+ } >+ } That ensures that the parameter list of |this| is a subset of the parameter list of aHandlerApp (and the same length, of course). That doesn't sound sufficient for equality. In particular, it would treat the following pairs as equal: this: cut -d 1 -f 2 aHandlerApp: cut -f 1 -d 2 this: ssh -v -v -v aHandlerApp: ssh -v -l othername You probably want to get the enumerator here and check that it's element-for-element equal to your mParameters. And add tests for the above.

We want this, but won't block on it. Jim, can you file a bug for the taskbar backend and ask for blocking on that, as it feels like the most important part?

(In reply to comment #40) > >+nsLocalHandlerApp::GetParameters(nsIStringEnumerator **aParameters) > >+ return NS_NewStringEnumerator(aParameters, &mParameters, this); > > Might want to file a separate bug on having the string enumerator support cycle > collection. Or something. It's pretty trivial to make this code leak by just > having the handler app hold a ref to the enumerator in JS, as far as I can > tell. This will be used through script, so I ditched the enumerator and just went with: AString getParameter(in long parameterIndex); on the app. The consumer can handle walking across the list. > > >+nsLocalHandlerApp::ParameterExists(const nsAString & param, PRBool *_retval) > >+{ > >+ *_retval = PR_FALSE; > >+ for (PRUint32 idx = 0; idx < mParameters.Length(); idx++) { > >+ if (mParameters[idx].Equals(param)) { > >+ *_retval = PR_TRUE; > >+ return NS_OK; > >+ } > >+ } > > Was there a reason that mParameters.Contains(param) didn't work here? That certainly would be simpler. :) > >+ // Check the command line params lists > >+ for (PRUint32 idx = 0; idx < mParameters.Length(); idx++) { > >+ PRBool exists; > >+ localHandlerApp->ParameterExists(mParameters[idx], &exists); > >+ if (!exists) { > >+ return NS_OK; > >+ } > >+ } > > That ensures that the parameter list of |this| is a subset of the parameter > list of aHandlerApp (and the same length, of course). That doesn't sound > sufficient for equality. In particular, it would treat the following pairs as > equal: > > this: cut -d 1 -f 2 > aHandlerApp: cut -f 1 -d 2 cut -d 1 -f 2 cut -f 1 -d 2 This being equal was ok in my specific use of this but I see the issue. Will update.

Updated and added tests for the parameter ordering.

same patch, just cleaned up some bracing in equals().

This is where the ui stands at this point after all the changes and comments here.

Comment on attachment 402260 [details] [diff] [review] widget v.3 >+interface nsIWinJumpListItem : nsISupports >+{ >+ const short JUMPLIST_ITEM_UNK = 0; // Empty list item >+ const short JUMPLIST_ITEM_LINK = 1; // Web link item >+ const short JUMPLIST_ITEM_SHORTCUT = 2; // Application shortcut >+ const short JUMPLIST_ITEM_SEPARATOR = 3; // Separator >+ >+ /** >+ * Retrieves the jump list item type. >+ */ >+ readonly attribute short type; >+ >+ /** >+ * A 'separator' jump list item. >+ * >+ * Sets the item to a separator type. (This just sets the type, >+ * separators don't hold any other information.) >+ */ >+ attribute boolean separator; This seems to conflict with the type being readonly above. Why are separators special? Can this be renamed to isSeparator or some boolean-implying name? >+ >diff --git a/widget/public/nsIWinTaskbar.idl b/widget/public/nsIWinTaskbar.idl >new file mode 100644 >--- /dev/null >+++ b/widget/public/nsIWinTaskbar.idl This doesn't seem to be a diff against the interface presented in bug 501490. I thought we were going to share some common infrastructure. At the very least, the names conflict. >+[scriptable, uuid(180F1D93-A94D-4D8A-AE37-A388372269B3)] >+interface nsIWinTaskbar : nsISupports >+{ >+ /** >+ * Indicates whether or not Win7+ superbar features are available. superbar/taskbar - we should be consistent here in terminology. >+ /** >+ * JumpLists >+ * >+ * Jump lists are built and then applied. Modifying an applied jump list is not >+ * permitted. Callers should initialize the creation of a new jump list using >+ * initListBuild, add sub lists using addListBuild, then commit the jump list >+ * using commitListBuild. >+ * >+ * The default number of allowed items in a jump list is ten. Users can change >+ * the number through system preferences. User may also pin items to jump lists, >+ * which take up additional slots. Applications do not have control over the >+ * number of items allowed in jump lists; excess items added are dropped by the >+ * system. Item insertion priority is defined as first to last added. >+ * >+ * Users may remove items from jump lists after they are commited. The system >+ * tracks removed items between commits. A list of these items is returned by >+ * a call to initListBuild. nsIWinTaskbar does not filter entries added that >+ * have been removed since the last commit. To prevent repeatedly adding entires >+ * users have removed, applications are encoraged to track removed items >+ * internally. >+ * >+ * Each list is made up of an array of nsIWinJumpListItems representing local >+ * handlers, links, and separators. >+ */ >+ >+ /** >+ * List Types >+ */ >+ >+ /** >+ * Task List >+ * >+ * Tasks are common actions performed by users within the application. A task >+ * can be represented by an application shortcut and associated command line >+ * parameters or a URI. Task lists should generally be static lists that do not >+ * change often, if at all - similar to an application menu. >+ * >+ * Tasks are given the highest priority of all lists when space is limited. >+ */ >+ const short JUMPLIST_CATEGORY_TASKS = 0; >+ >+ /** >+ * Recent or Frequent list >+ * >+ * Recent and frequent lists are based on Window's recent document lists. The >+ * lists are generated automatically by Windows. Applications that use recent >+ * or frequent lists should keep document use tracking up to date by calling >+ * the SHAddToRecentDocs shell api. >+ */ >+ const short JUMPLIST_CATEGORY_RECENT = 1; >+ const short JUMPLIST_CATEGORY_FREQUENT = 2; >+ >+ /** >+ * Custom Lists >+ * >+ * Custom lists can be made up of tasks, links, and separators. The title of >+ * of the list is passed through the optional string parameter of addBuildList. >+ */ >+ const short JUMPLIST_CATEGORY_CUSTOMLIST = 3; >+ >+ /** >+ * JumpList management >+ * >+ * @throw NS_ERROR_NOT_AVAILABLE on all calls if taskbar functionality >+ * is not supported by the operating system. >+ */ >+ >+ /** >+ * Indicates if a commit has already occured in this session. >+ */ >+ readonly attribute boolean listCommited; >+ >+ /** >+ * The maximum number of jump list items the current desktop can support. >+ */ >+ readonly attribute short maxListItems; >+ >+ /** >+ * Initializes a jump list build and returns a list of items the user removed >+ * since the last time a jump list was committed. >+ * >+ * @param removedItems >+ * A list of items that were removed by the user since the last commit. >+ * >+ * @returns true if the operation completed successfully. >+ */ >+ boolean initListBuild(in nsIMutableArray removedItems); >+ >+ /** >+ * Adds a list and if required, a set of items for the list. >+ * >+ * @param aCatType >+ * The type of list to add. >+ * @param items >+ * An array of nsIWinJumpListItem items to add to the list. >+ * @param catName >+ * For custom lists, the title of the list. >+ * >+ * @returns true if the operation completed successfully. >+ * >+ * @throw NS_ERROR_INVALID_ARG if incorrect parameters are passed for >+ * a particular category or item type. >+ * @throw NS_ERROR_ILLEGAL_VALUE if an item is added that was removed >+ * since the last commit. >+ * @throw NS_ERROR_UNEXPECTED on internal errors. >+ */ >+ boolean addListBuild(in short aCatType, in nsIMutableArray items, [optional] in AString catName); I don't suppose there's any way to use a regular JS array here for convenience? >+ >+ /** >+ * Aborts and clears the current jump list build. >+ */ >+ void abortListBuild(); >+ >+ /** >+ * Commits the current jump list build to the Taskbar. >+ * >+ * @returns true if the operation completed successfully. >+ */ >+ boolean commitListBuild(); >+ >+ /** >+ * Deletes any currently applied Taskbar jump list for this application. >+ * Common uses would be the enabling of a privacy mode and uninstallation. >+ * >+ * @returns true if the operation completed successfully. >+ * >+ * @throw NS_ERROR_UNEXPECTED on internal errors. >+ */ >+ boolean deleteActiveList(); >+}; The current API does not allow for a client to manipulate multiple jump lists but the underlying COM APIs seem to support this (please tell me if I'm wrong). Instead of having these methods on the nsIWinTaskbar interface, what if we instead have an nsIJumpList interface that loosely wraps the COM interface (as it does now) with a property on nsIWinTaskbar to get/set the current jumplist? >diff --git a/widget/src/windows/WinJumpListItem.cpp b/widget/src/windows/WinJumpListItem.cpp >new file mode 100644 >--- /dev/null >+++ b/widget/src/windows/WinJumpListItem.cpp >@@ -0,0 +1,332 @@ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is mozilla.org code. >+ * >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2009 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Jim Mathies <jmathies@mozilla.com> >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * 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 ***** */ >+ >+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7 >+ >+#include "WinJumpListItem.h" >+ >+#include <windows.h> >+#include <shobjidl.h> >+ >+#include "nsIFile.h" >+#include "nsILocalFile.h" >+#include "nsNetUtil.h" >+ >+namespace mozilla { >+namespace widget { >+ >+NS_IMPL_ISUPPORTS1(WinJumpListItem, >+ nsIWinJumpListItem) I don't see why we need to prefix Win to the concrete class's name (I only did this with the taskbar because other widget backends have a taskbar - in hindsight maybe it's not actually necessary). This might be a good topic for a widget super-reviewer to comment on. >+ >+NS_IMETHODIMP WinJumpListItem::SetSeparator(PRBool aSeparator) >+{ >+ mItemType = nsIWinJumpListItem::JUMPLIST_ITEM_SEPARATOR; This doesn't actually change the type! The underlying object is still whatever you created it to be. Also, what happens if aSeparator = PR_FALSE? >+/* boolean equals(nsIWinJumpListItem item); */ >+NS_IMETHODIMP WinJumpListItem::Equals(nsIWinJumpListItem *aItem, PRBool *aResult) >+{ >+ NS_ENSURE_ARG_POINTER(aItem); >+ >+ *aResult = PR_FALSE; >+ >+ PRInt16 theType = nsIWinJumpListItem::JUMPLIST_ITEM_UNK; >+ aItem->GetType(&theType); For sanity we should check the return value. I'm not sure if there's a guarantee that theType is unmodified in the event of an error (I would hope so). >+NS_IMETHODIMP WinJumpListLink::SetUri(nsIURI *aURI) >+{ >+ mURI = aURI; >+ >+ mItemType = nsIWinJumpListItem::JUMPLIST_ITEM_LINK; Why does the type change? This worries me - it's not clear what really defines the type of an nsIJumpListItem. >+ nsCOMPtr<nsIWinJumpListLink> link = do_QueryInterface(aItem, &rv); >+ if (NS_FAILED(rv) || !link) >+ return rv; What does it mean if the types match but QI fails? >+ >+ // Confirm the app is present on the system >+ if (!ExecutableExists(mHandlerApp)) >+ return NS_ERROR_FILE_NOT_FOUND; While this is a nice sanity check, what is the harm if it doesn't exist? What about executables on network drives? (and say the network isn't attached right now). >+ >+NS_IMETHODIMP WinJumpListShortcut::SetIconIndex(PRInt32 aIconIndex) >+{ >+ mIconIndex = aIconIndex; >+ return NS_OK; >+} Is there any validation we can do here? Using indexes seems fragile. Ex: update adds a new resource, now our jumplists have the wrong icon right? Can we use resource names? >+const PRUnichar kShellLibraryName[] = L"shell32.dll"; >+ >+static NS_DEFINE_CID(kJumpListItemCID, NS_WIN_JUMPLISTITEM_CID); >+ >+// Unique identifier for a particular application. Windows uses this to group >+// application windows from the same process and to track application's jump >+// list. Should be unique on a per application basis. (Win7 specific.) >+// Format: CompanyName.ProductName.SubProduct.VersionInformation >+#ifndef MOZ_TASKBAR_ID >+#define MOZ_COMPANY Mozilla >+#define MOZ_SUBPRODUCT - >+#define MOZTBID1(x) L#x >+#define MOZTBID2(a,b,c,d) MOZTBID1(a##.##b##.##c##.##d) Do you know if this works for MinGW? Not a big issue (especially since there's lag time on the SDK for them). >+#define MOZTBID(a,b,c,d) MOZTBID2(a,b,c,d) >+#if MOZ_BUILD_APP != browser >+#define MOZ_TASKBAR_ID MOZTBID(MOZ_COMPANY, MOZ_BUILD_APP, MOZ_SUBPRODUCT, MOZILLA_VERSION_U) >+#else >+// A white wash to get around the issue of not versioning our application registration ids >+// in firefox. We end up sharing jump list data between version due to this, but jump list >+// links will not register unless the prog id of the protocol hander registered for http/https >+// matches our jump list prog id. (Bug 505925 has been filed to try and address this.) >+#define MOZ_TASKBAR_ID L"Firefox" Can we pull this from the build system so that this changes with official branding on/off? I think you need to explicitly release mJumpListMgr here or else (in the unlikely event this is the last CoUninitialize call), bad things will happen since the member destructors fire after the class destructor does. >+ ::CoUninitialize(); >+} >+/* readonly attribute short available; */ >+NS_IMETHODIMP WinTaskbar::GetAvailable(PRInt16 *aAvailable) >+{ >+ NS_ENSURE_ARG_POINTER(aAvailable); >+ >+ *aAvailable = PR_FALSE; >+ >+ if (mJumpListMgr) >+ *aAvailable = PR_TRUE; >+ >+ return NS_OK; >+} This definition of available is different from the one defined in bug 501490 but I think they are equivalent. >+/* readonly attribute short available; */ >+NS_IMETHODIMP WinTaskbar::GetListCommited(PRBool *aCommit) >+{ Comment is wrong. Wouldn't a name like "isListCommitted" be better? (Also Firefox's spellchecker informs me that "commited" is misspelled). >+/* boolean addListBuild(in short aCatType, in nsIMutableArray items, [optional] in AString catName); */ >+NS_IMETHODIMP WinTaskbar::AddListBuild(PRInt16 aCatType, nsIMutableArray *items, const nsAString &catName, PRBool *_retval) >+{ >+ nsresult rv; >+ >+ if (!mJumpListMgr) >+ return NS_ERROR_NOT_AVAILABLE; nit: retval not assigned here before potential return. However, it seems that *retval <=> rv == NS_OK so I think retval is just unnecessary. Actually, on further review, it's not clear what the error semantics are. >+/* boolean commitListBuild(); */ >+NS_IMETHODIMP WinTaskbar::CommitListBuild(PRBool *_retval) >+{ >+ if (!mJumpListMgr) >+ return NS_ERROR_NOT_AVAILABLE; >+ >+ *_retval = PR_FALSE; >+ >+ HRESULT hr = mJumpListMgr->CommitList(); >+ >+ // XXX We might want some specific error data here. >+ if (SUCCEEDED(hr)) { >+ *_retval = PR_TRUE; >+ mHasCommit = PR_TRUE; >+ } >+ >+ return NS_OK; >+} Same ambiguity with error handling. Seems like we return NS_OK even if the call actually failed. >+/* internal */ >+ >+PRBool WinTaskbar::IsSeparator(nsCOMPtr<nsIWinJumpListItem>& item) item.GetSeparator? >+// ShellLinks are used to encapsulate local app handlers for the taskbar interface. >+nsresult WinTaskbar::GetShellLink(nsCOMPtr<nsIWinJumpListItem>& item, nsRefPtr<IShellLinkW>& shellLink) Later on, shellLink is assigned to. I think it makes sense to follow the coding convention of prefixing a for arguments. >+{ >+ HRESULT hres; nit: hr is more common in our codebase and Microsoft sample code. >+// ShellItems are used to encapsulate links for the taskbar interface. >+nsresult WinTaskbar::GetShellItem(nsCOMPtr<nsIWinJumpListItem>& item, nsRefPtr<IShellItem2>& shellItem) Same comments as above (this function seems very similar too). >+nsresult WinTaskbar::GetJumpListItem(IShellLinkW *pLink, nsCOMPtr<nsIWinJumpListItem>& item) As with the previous two methods, it's not clear to me if item is supposed to be initialized before being passed in. >+nsresult WinTaskbar::GetJumpListItem(IShellItem *pItem, nsCOMPtr<nsIWinJumpListItem>& item) Ooh, overloading. Same comments as above. >+#undef NTDDI_VERSION >+#undef _WIN32_IE >+#define _WIN32_IE _WIN32_IE_IE70 >+#define NTDDI_VERSION NTDDI_WIN7 >+#include <shobjidl.h> These defines are somewhat dangerous - why are they needed? See http://blogs.msdn.com/oldnewthing/archive/2003/12/12/56061.aspx for more details (there's another good reference but I can't seem to find it). >diff --git a/widget/src/windows/nsWinGesture.h b/widget/src/windows/nsWinGesture.h >--- a/widget/src/windows/nsWinGesture.h >+++ b/widget/src/windows/nsWinGesture.h >@@ -43,17 +43,17 @@ > * nsWinGesture - Touch input handling for tablet displays. > */ > > #include "nsdefs.h" > #include <winuser.h> > #include "nsPoint.h" > #include "nsGUIEvent.h" > >-#if !defined(NTDDI_WIN7) || NTDDI_VERSION < NTDDI_WIN7 >+#ifndef HGESTUREINFO // needs WINVER >= 0x0601 > > DECLARE_HANDLE(HGESTUREINFO); > > /* > * Gesture flags - GESTUREINFO.dwFlags > */ > #define GF_BEGIN 0x00000001 > #define GF_INERTIA 0x00000002 Unrelated change? Also, why not use the SDK #defines? >+ >+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7 >+ mozilla::widget::WinTaskbar::WinTaskbarSetAppUserModelID(); >+#endif This is potentially called multiple times for embedders (if they startup & shutdown). How does nsAppShell::Init sound? >+function test_links() >+{ >+ // links: >+ var link = Cc["@mozilla.org/windows-jumplistlink;1"] >+ .createInstance(Ci.nsIWinJumpListLink); >+ >+ link.uriSpec = "http://www.test.com/"; >+ link.uriTitle = "Test"; >+ >+ // the uri hash should be constant, we store these in prefs >+ do_check_eq(link.uriSpecHash, 575108772); How stable is the hashing algorithm? I assume it's deterministic across runs but these tests will break if the algorithm changes.

(In reply to comment #46) > (From update of attachment 402260 [details] [diff] [review]) > >+ readonly attribute short type; > >+ > >+ /** > >+ * A 'separator' jump list item. > >+ * > >+ * Sets the item to a separator type. (This just sets the type, > >+ * separators don't hold any other information.) > >+ */ > >+ attribute boolean separator; > > This seems to conflict with the type being readonly above. Why are separators > special? Can this be renamed to isSeparator or some boolean-implying name? Been looking at this lately, it just didn't seem worth it to create a whole new item interface just for separators. But I think to clear up the ambiguity it's probably worth it. > > >+ > >diff --git a/widget/public/nsIWinTaskbar.idl b/widget/public/nsIWinTaskbar.idl > >new file mode 100644 > >--- /dev/null > >+++ b/widget/public/nsIWinTaskbar.idl > > This doesn't seem to be a diff against the interface presented in bug 501490. I > thought we were going to share some common infrastructure. At the very least, > the names conflict. I'll clean that up. > > >+[scriptable, uuid(180F1D93-A94D-4D8A-AE37-A388372269B3)] > >+interface nsIWinTaskbar : nsISupports > >+{ > >+ /** > >+ * Indicates whether or not Win7+ superbar features are available. > > superbar/taskbar - we should be consistent here in terminology. yep, will update. > >+ boolean addListBuild(in short aCatType, in nsIMutableArray items, [optional] in AString catName); > > I don't suppose there's any way to use a regular JS array here for convenience? This could be an nsIArray, it's not modified. > > >+ > >+ /** > >+ * Aborts and clears the current jump list build. > >+ */ > >+ void abortListBuild(); > >+ > >+ /** > >+ * Commits the current jump list build to the Taskbar. > >+ * > >+ * @returns true if the operation completed successfully. > >+ */ > >+ boolean commitListBuild(); > >+ > >+ /** > >+ * Deletes any currently applied Taskbar jump list for this application. > >+ * Common uses would be the enabling of a privacy mode and uninstallation. > >+ * > >+ * @returns true if the operation completed successfully. > >+ * > >+ * @throw NS_ERROR_UNEXPECTED on internal errors. > >+ */ > >+ boolean deleteActiveList(); > >+}; > The current API does not allow for a client to manipulate multiple jump lists > but the underlying COM APIs seem to support this (please tell me if I'm wrong). Not sure what you mean here. Building the current list is a one shot deal tied to the app id. You init (getting back a list of removed items), build, and commit. > Instead of having these methods on the nsIWinTaskbar interface, what if we > instead have an nsIJumpList interface that loosely wraps the COM interface (as > it does now) with a property on nsIWinTaskbar to get/set the current jumplist? I don't mind splitting that out, but it sort of leaves nsIWinTaskbar doing little to nothing other than supporting that property. I guess with all the other stuff we're adding it wouldn't hurt. > >+namespace mozilla { > >+namespace widget { > >+ > >+NS_IMPL_ISUPPORTS1(WinJumpListItem, > >+ nsIWinJumpListItem) > > I don't see why we need to prefix Win to the concrete class's name (I only did > this with the taskbar because other widget backends have a taskbar - in > hindsight maybe it's not actually necessary). This might be a good topic for a > widget super-reviewer to comment on. Sounds good. > > >+ > >+NS_IMETHODIMP WinJumpListItem::SetSeparator(PRBool aSeparator) > >+{ > >+ mItemType = nsIWinJumpListItem::JUMPLIST_ITEM_SEPARATOR; > > This doesn't actually change the type! The underlying object is still whatever > you created it to be. Also, what happens if aSeparator = PR_FALSE? That is sort of worthless, although it did what I wanted it to. :) Breaking seps out into an interface will address this. > > >+/* boolean equals(nsIWinJumpListItem item); */ > >+NS_IMETHODIMP WinJumpListItem::Equals(nsIWinJumpListItem *aItem, PRBool *aResult) > >+{ > >+ NS_ENSURE_ARG_POINTER(aItem); > >+ > >+ *aResult = PR_FALSE; > >+ > >+ PRInt16 theType = nsIWinJumpListItem::JUMPLIST_ITEM_UNK; > >+ aItem->GetType(&theType); > > For sanity we should check the return value. I'm not sure if there's a > guarantee that theType is unmodified in the event of an error (I would hope > so). updated. > > >+NS_IMETHODIMP WinJumpListLink::SetUri(nsIURI *aURI) > >+{ > >+ mURI = aURI; > >+ > >+ mItemType = nsIWinJumpListItem::JUMPLIST_ITEM_LINK; > > Why does the type change? This worries me - it's not clear what really defines > the type of an nsIJumpListItem. Actually since I broke these out and init the type value on creation, this is redundant. Will update. > > >+ nsCOMPtr<nsIWinJumpListLink> link = do_QueryInterface(aItem, &rv); > >+ if (NS_FAILED(rv) || !link) > >+ return rv; > > What does it mean if the types match but QI fails? Overkill error checking - removed. > > >+ > >+ // Confirm the app is present on the system > >+ if (!ExecutableExists(mHandlerApp)) > >+ return NS_ERROR_FILE_NOT_FOUND; > > While this is a nice sanity check, what is the harm if it doesn't exist? What > about executables on network drives? (and say the network isn't attached right > now). I wanted to be sure they were present to prevent people from broken links. I'm 50/50 on that. The list is generated every time the app starts up, so presumably at some point they'll launch it with whatever resource that contains the exe available, at which point the link becomes available. Personally I'd start out sticking with the way I have it and see if there are complaints. > > >+ > >+NS_IMETHODIMP WinJumpListShortcut::SetIconIndex(PRInt32 aIconIndex) > >+{ > >+ mIconIndex = aIconIndex; > >+ return NS_OK; > >+} > > Is there any validation we can do here? Using indexes seems fragile. Ex: update > adds a new resource, now our jumplists have the wrong icon right? Can we use > resource names? Not in this case, it's got to be an integer index or a path to an ico. It would be the responsibility of the developer to keep their indexes current. (Note, I'm working on adding some additional support here, image support and the ability to point to a different file other than the exe.) > > >+const PRUnichar kShellLibraryName[] = L"shell32.dll"; > >+ > >+static NS_DEFINE_CID(kJumpListItemCID, NS_WIN_JUMPLISTITEM_CID); > >+ > >+// Unique identifier for a particular application. Windows uses this to group > >+// application windows from the same process and to track application's jump > >+// list. Should be unique on a per application basis. (Win7 specific.) > >+// Format: CompanyName.ProductName.SubProduct.VersionInformation > >+#ifndef MOZ_TASKBAR_ID > >+#define MOZ_COMPANY Mozilla > >+#define MOZ_SUBPRODUCT - > >+#define MOZTBID1(x) L#x > >+#define MOZTBID2(a,b,c,d) MOZTBID1(a##.##b##.##c##.##d) > > Do you know if this works for MinGW? Not a big issue (especially since there's > lag time on the SDK for them). No idea, but I imagine they'll let me know if it's broken. :) > > >+#define MOZTBID(a,b,c,d) MOZTBID2(a,b,c,d) > >+#if MOZ_BUILD_APP != browser > >+#define MOZ_TASKBAR_ID MOZTBID(MOZ_COMPANY, MOZ_BUILD_APP, MOZ_SUBPRODUCT, MOZILLA_VERSION_U) > >+#else > >+// A white wash to get around the issue of not versioning our application registration ids > >+// in firefox. We end up sharing jump list data between version due to this, but jump list > >+// links will not register unless the prog id of the protocol hander registered for http/https > >+// matches our jump list prog id. (Bug 505925 has been filed to try and address this.) > >+#define MOZ_TASKBAR_ID L"Firefox" > > Can we pull this from the build system so that this changes with official > branding on/off? Not sure why you would want to, "Firefox" is our generic app id we use in protocol and file handling registration regardless of the build. These have to match up. > > I think you need to explicitly release mJumpListMgr here or else (in the > unlikely event this is the last CoUninitialize call), bad things will happen > since the member destructors fire after the class destructor does. > > >+ ::CoUninitialize(); > >+} > Added. > >+/* readonly attribute short available; */ > >+NS_IMETHODIMP WinTaskbar::GetAvailable(PRInt16 *aAvailable) > >+{ > >+ NS_ENSURE_ARG_POINTER(aAvailable); > >+ > >+ *aAvailable = PR_FALSE; > >+ > >+ if (mJumpListMgr) > >+ *aAvailable = PR_TRUE; > >+ > >+ return NS_OK; > >+} > > This definition of available is different from the one defined in bug 501490 > but I think they are equivalent. We can sort out how we make this decision depending on who lands first. (Honestly I'm partial to available vs. ready myself.) > > >+/* readonly attribute short available; */ > >+NS_IMETHODIMP WinTaskbar::GetListCommited(PRBool *aCommit) > >+{ > > Comment is wrong. Wouldn't a name like "isListCommitted" be better? (Also > Firefox's spellchecker informs me that "commited" is misspelled). Updated. > >+/* boolean addListBuild(in short aCatType, in nsIMutableArray items, [optional] in AString catName); */ > >+NS_IMETHODIMP WinTaskbar::AddListBuild(PRInt16 aCatType, nsIMutableArray *items, const nsAString &catName, PRBool *_retval) > >+{ > >+ nsresult rv; > >+ > >+ if (!mJumpListMgr) > >+ return NS_ERROR_NOT_AVAILABLE; > > nit: retval not assigned here before potential return. However, it seems that > *retval <=> rv == NS_OK so I think retval is just unnecessary. Actually, on > further review, it's not clear what the error semantics are. You're right it should be *_retval = PR_FALSE; if (!mJumpListMgr) return NS_ERROR_NOT_AVAILABLE; These routines throw on major unexpected errors. > > >+// ShellLinks are used to encapsulate local app handlers for the taskbar interface. > >+nsresult WinTaskbar::GetShellLink(nsCOMPtr<nsIWinJumpListItem>& item, nsRefPtr<IShellLinkW>& shellLink) > > Later on, shellLink is assigned to. I think it makes sense to follow the coding > convention of prefixing a for arguments. Good point, updated. > > >+{ > >+ HRESULT hres; > > nit: hr is more common in our codebase and Microsoft sample code. updated. > >+#undef NTDDI_VERSION > >+#undef _WIN32_IE > >+#define _WIN32_IE _WIN32_IE_IE70 > >+#define NTDDI_VERSION NTDDI_WIN7 > >+#include <shobjidl.h> > > These defines are somewhat dangerous - why are they needed? > See http://blogs.msdn.com/oldnewthing/archive/2003/12/12/56061.aspx for more > details (there's another good reference but I can't seem to find it). My com interfaces fall out if I don't have - #define NTDDI_VERSION NTDDI_WIN7 I think you spoke with someone about this recently, we don't up this for some reason? I'm not entirely sure how else to get around it. The IE define I need elevated to get at SHCreateItemFromParsingName. > > #define GF_BEGIN 0x00000001 > > #define GF_INERTIA 0x00000002 > > Unrelated change? Also, why not use the SDK #defines? Totally, I'll remove this. > > >+ > >+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7 > >+ mozilla::widget::WinTaskbar::WinTaskbarSetAppUserModelID(); > >+#endif > > This is potentially called multiple times for embedders (if they startup & > shutdown). How does nsAppShell::Init sound? WFM, I'll move it there. > >+function test_links() > >+{ > >+ // links: > >+ var link = Cc["@mozilla.org/windows-jumplistlink;1"] > >+ .createInstance(Ci.nsIWinJumpListLink); > >+ > >+ link.uriSpec = "http://www.test.com/"; > >+ link.uriTitle = "Test"; > >+ > >+ // the uri hash should be constant, we store these in prefs > >+ do_check_eq(link.uriSpecHash, 575108772); > > How stable is the hashing algorithm? I assume it's deterministic across runs > but these tests will break if the algorithm changes. Yes, they are ok.

> > > #define GF_BEGIN 0x00000001 > > > #define GF_INERTIA 0x00000002 > > > > Unrelated change? Also, why not use the SDK #defines? > > Totally, I'll remove this. Actually this was needed, on win7 sdk builds the code in this block still gets pulled in, and there's no error due to a lack of elevation for NTDDI_VERSION. With this - #undef NTDDI_VERSION #undef _WIN32_IE #define _WIN32_IE _WIN32_IE_IE70 #define NTDDI_VERSION NTDDI_WIN7 the gesture stuff gets defined twice.

Comment on attachment 402505 [details] [diff] [review] mime patch v.5 >+ if (mParameters.Length() < parameterIndex) That should be a <=. Please add a test for this. r=bzbarsky with that.

pushed to try for a final test run.

fully updated to comments and tested via the browser bits.

Can there be a different jump list depending on whether the program is running? If so having an option to run the program in safe mode would be good. But having that option shown while the program is running would be bad.

(In reply to comment #53) > Can there be a different jump list depending on whether the program is running? > If so having an option to run the program in safe mode would be good. But > having that option shown while the program is running would be bad. I have a placeholder task for opening the browser in safe mode in the browser bits. But that depends on us getting our -private cmd line stuff working right. I was planning on keeping it in there at all times. If you're not in safe mode selecting it would reset into safe mode (or if possible, open a new window in safe mode), and if you're in safe mode, the jump list is empty except tasks (new window, new tab, exit safe mode). This all depends on how some of the blocking bug work we still have to do ends up working.

(In reply to comment #52) > Created an attachment (id=402751) [details] > widget v.4 > > fully updated to comments and tested via the browser bits. Rob, ignore the the minor mix ups in comments between nsIWinTaskbar and nsIJumpListBuilder, I've cleaned that up after looking it over here. Also, I've kept my available prop for now so I don't have to include a chunk of your code for the ready property, we can sort that out later.

+ * permitted. Callers should initialize the creation of a new jump list using + * initListBuild, "initialize" -> "begin" + * build calls, make sure and check for errors on each individual step. "make sure to" + * Users may remove items from jump lists after they are commited. The system "committed" + /** + * Set or get the uri for this link item. + */ + attribute ACString uriSpec; Why do you need this? + const short JUMPLIST_ITEM_UNK = 0; // Empty list item "UNK"? couldn't we use a better name here? +[scriptable, uuid(180F1D93-A94D-4D8A-AE37-A388372269B3)] +interface nsIWinTaskbar : nsISupports +{ + /** + * Retrieve the taskbar jump list builder interface. See nsIJumpListBuilder for + * more information. + */ + readonly attribute nsIJumpListBuilder jumpListBuilder; +}; Are we going to add more stuff here? This interface doesn't seem justified right now. Otherwise looks pretty good.

(In reply to comment #56) > + /** > + * Set or get the uri for this link item. > + */ > + attribute ACString uriSpec; > > Why do you need this? Added a comment to this: For tracking items that are removed from jump lists across sessions. A hash is useful way to store the uri in a non human readable format. I looked for a scripting interface to this routine but couldn't find one, so I dropped it in. > > + const short JUMPLIST_ITEM_UNK = 0; // Empty list item > > "UNK"? couldn't we use a better name here? Hmm, EMPTY, UNDEF, NONE. UNDEF seems pretty good. > > +[scriptable, uuid(180F1D93-A94D-4D8A-AE37-A388372269B3)] > +interface nsIWinTaskbar : nsISupports > +{ > + /** > + * Retrieve the taskbar jump list builder interface. See nsIJumpListBuilder > for > + * more information. > + */ > + readonly attribute nsIJumpListBuilder jumpListBuilder; > +}; > > Are we going to add more stuff here? This interface doesn't seem justified > right now. Yes, more is coming. Generally this is the gateway to a lot of other interfaces & functionality and will be pretty sparse.

- updated to roc's comments. (went with JUMPLIST_ITEM_EMPTY) - updated and added more tests - fixed a bug where the second param on addBuildList should have been optional.

(In reply to comment #57) > Added a comment to this: For tracking items that are removed from jump lists > across sessions. A hash is useful way to store the uri in a non human readable > format. > > I looked for a scripting interface to this routine but couldn't find one, so I > dropped it in. I don't get it. nsIURI has a "spec" attribute. So why do you need uriSpec here?

(In reply to comment #56) > + /** > + * Set or get the uri for this link item. > + */ > + attribute ACString uriSpec; > > Why do you need this? > (In reply to comment #57) > > Added a comment to this: For tracking items that are removed from jump lists > > across sessions. A hash is useful way to store the uri in a non human readable > > format. > > > > I looked for a scripting interface to this routine but couldn't find one, so I > > dropped it in. > > I don't get it. nsIURI has a "spec" attribute. So why do you need uriSpec here? Sorry was looking at the wrong property.. I guess for simplicity: uri = createinstance nsIURI uri.spec = "string" builder.uri = uri vs. builder.uriSpec = "string" Overkill?

I think so, yes. Right now it seems ambigious whether builder.uriSpec creates a new nsIURI or sets the spec in the existing nsIURI. That ambiguity seems like a good enough reason to get rid of it...

Looks like my hashing will need an update - NS_SecurityHashURI doesn't hash the whole spec, just the scheme, port, and domain for http. But it looks like I can just pilfer the crt hash code from it to get my own unique hashes for uri. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1467 1484 nsCAutoString spec; 1485 PRUint32 specHash = baseURI->GetSpec(spec); 1486 if (NS_SUCCEEDED(specHash)) 1487 specHash = nsCRT::HashCode(spec.get()); 1488 return specHash;

So... what do we use that hash for? The IDL claims it's "unique", but there's no such guarantee from nsCRT::HashCode (and in fact the current hashcode it generates is extremely collision-prone; see bug 290032). What is this hashcode used for, exactly?

(In reply to comment #64) > So... what do we use that hash for? The IDL claims it's "unique", but there's > no such guarantee from nsCRT::HashCode (and in fact the current hashcode it > generates is extremely collision-prone; see bug 290032). What is this hashcode > used for, exactly? Hashes would be used in tracking a small list of items (uri) across sessions. A typical example - a user's favorites list is populated by an application with uri from history. Over a set of sessions, the user selectively removes some uri that show up in that list (four or five uri). Each time the application refreshes the favorites, it receives a list of uri removed. The hashes of these uri could be used to track removed items across sessions by the app. Rare collisions in cases where you are hashing thousands of uri for a cache can be a major issue, but I'm not sure those types of collisions are such a major issue in this use case.

> Rare collisions It's trivial to produce collisions with the current hashing algorithm. Any two filenames that are identical except for two chars 4 apart (e.g. common URIs that have numbers in them for the filename, like images in a photo album) will generally collide.

Ok, at bz's suggestion I've updated this to use nsICryptoHash instead of the nsCRT has code. Added a few more tests as well for the hashing.

Sorry for the spam, that was the wrong patch.

Comment on attachment 403846 [details] [diff] [review] widget v.6 >diff --git a/widget/public/nsIJumpListBuilder.idl b/widget/public/nsIJumpListBuilder.idl >+ /** >+ * Indicates whether jump list taskbar features are supported by the current >+ * host. >+ */ >+ readonly attribute short available; I don't think we need this because... >+[scriptable, uuid(180F1D93-A94D-4D8A-AE37-A388372269B3)] >+interface nsIWinTaskbar : nsISupports >+{ >+ /** >+ * Retrieve the taskbar jump list builder interface. See nsIJumpListBuilder for >+ * more information. >+ */ >+ readonly attribute nsIJumpListBuilder jumpListBuilder; >+}; ...we have this which can be null to indicate being unavailable. >+/* readonly attribute boolean isListCommitted; */ >+NS_IMETHODIMP JumpListBuilder::GetIsListCommitted(PRBool *aCommit) >+{ >+ *aCommit = PR_FALSE; >+ >+ if (mHasCommit) >+ *aCommit = PR_TRUE; *aCommit = mHasCommit; >+/* readonly attribute short maxItems; */ >+NS_IMETHODIMP JumpListBuilder::GetMaxListItems(PRInt16 *aMaxItems) >+{ >+ if (!mJumpListMgr) >+ return NS_ERROR_NOT_AVAILABLE; >+ >+ *aMaxItems = 0; >+ >+ if (mBuildingList) { >+ *aMaxItems = mMaxItems; >+ return NS_OK; >+ } >+ >+ IObjectArray *objArray; >+ if (SUCCEEDED(mJumpListMgr->BeginList(&mMaxItems, IID_PPV_ARGS(&objArray)))) { >+ *aMaxItems = mMaxItems; >+ >+ if (objArray) >+ objArray->Release(); >+ >+ mJumpListMgr->AbortList(); >+ } >+ >+ return NS_OK; >+} How does this method not interfere with the rest of the methods on this interface? AbortList looks destructive. >+// static >+PRBool WinTaskbar::SetAppUserModelID() >+{ >+ SetCurrentProcessExplicitAppUserModelIDPtr funcAppUserModelID = nsnull; >+ PRBool retVal = PR_FALSE; >+ >+ HMODULE hDLL = ::LoadLibraryW(kShellLibraryName); >+ >+ funcAppUserModelID = (SetCurrentProcessExplicitAppUserModelIDPtr) >+ GetProcAddress(hDLL, "SetCurrentProcessExplicitAppUserModelID"); >+ >+ if (funcAppUserModelID && SUCCEEDED(funcAppUserModelID(gMozillaJumpListIDGeneric))) >+ retVal = PR_TRUE; >+ >+ if (hDLL) >+ ::FreeLibrary(hDLL); >+ >+ return retVal; >+} We only call this once, right? Might be nice to assert that. >+function run_test() >+{ >+ var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes); >+ if (!isWindows) >+ return; >+ >+ // Win7 and up only >+ try { >+ var sysInfo = Cc["@mozilla.org/system-info;1"]. >+ getService(Ci.nsIPropertyBag2); >+ var ver = parseFloat(sysInfo.getProperty("version")); >+ if (ver < 6.1) >+ return; >+ } catch (ex) { } Add a test here to check that jumplist availability matches our platform expectations?

(In reply to comment #69) > (From update of attachment 403846 [details] [diff] [review]) > >diff --git a/widget/public/nsIJumpListBuilder.idl b/widget/public/nsIJumpListBuilder.idl > >+ /** > >+ * Indicates whether jump list taskbar features are supported by the current > >+ * host. > >+ */ > >+ readonly attribute short available; > > I don't think we need this because... > > >+[scriptable, uuid(180F1D93-A94D-4D8A-AE37-A388372269B3)] > >+interface nsIWinTaskbar : nsISupports > >+{ > >+ /** > >+ * Retrieve the taskbar jump list builder interface. See nsIJumpListBuilder for > >+ * more information. > >+ */ > >+ readonly attribute nsIJumpListBuilder jumpListBuilder; > >+}; > > ...we have this which can be null to indicate being unavailable. I like exposing something like the avail prop because it clearly indicates the purpose, where as checking a result on jumpListBuilder isn't as clear cut. The prop really doesn't take up much code either, a get on a bool. > > >+/* readonly attribute short maxItems; */ > >+NS_IMETHODIMP JumpListBuilder::GetMaxListItems(PRInt16 *aMaxItems) > >+{ > >+ if (!mJumpListMgr) > >+ return NS_ERROR_NOT_AVAILABLE; > >+ > >+ *aMaxItems = 0; > >+ > >+ if (mBuildingList) { > >+ *aMaxItems = mMaxItems; > >+ return NS_OK; > >+ } > >+ > >+ IObjectArray *objArray; > >+ if (SUCCEEDED(mJumpListMgr->BeginList(&mMaxItems, IID_PPV_ARGS(&objArray)))) { > >+ *aMaxItems = mMaxItems; > >+ > >+ if (objArray) > >+ objArray->Release(); > >+ > >+ mJumpListMgr->AbortList(); > >+ } > >+ > >+ return NS_OK; > >+} > > How does this method not interfere with the rest of the methods on this > interface? AbortList looks destructive. > >+ if (mBuildingList) { > >+ *aMaxItems = mMaxItems; > >+ return NS_OK; > >+ } If a list is in the process of being built, we return the value returned by initListBuild here. > > >+// static > >+PRBool WinTaskbar::SetAppUserModelID() > >+{ > >+ SetCurrentProcessExplicitAppUserModelIDPtr funcAppUserModelID = nsnull; > >+ PRBool retVal = PR_FALSE; > >+ > >+ HMODULE hDLL = ::LoadLibraryW(kShellLibraryName); > >+ > >+ funcAppUserModelID = (SetCurrentProcessExplicitAppUserModelIDPtr) > >+ GetProcAddress(hDLL, "SetCurrentProcessExplicitAppUserModelID"); > >+ > >+ if (funcAppUserModelID && SUCCEEDED(funcAppUserModelID(gMozillaJumpListIDGeneric))) > >+ retVal = PR_TRUE; > >+ > >+ if (hDLL) > >+ ::FreeLibrary(hDLL); > >+ > >+ return retVal; > >+} > > We only call this once, right? Might be nice to assert that. Hmm, I can add as we do only call it once in platform, although technically you can call this multiple times to reset it. > > >+function run_test() > >+{ > >+ var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes); > >+ if (!isWindows) > >+ return; > >+ > >+ // Win7 and up only > >+ try { > >+ var sysInfo = Cc["@mozilla.org/system-info;1"]. > >+ getService(Ci.nsIPropertyBag2); > >+ var ver = parseFloat(sysInfo.getProperty("version")); > >+ if (ver < 6.1) > >+ return; > >+ } catch (ex) { } > > Add a test here to check that jumplist availability matches our platform > expectations? Will do.

(In reply to comment #69) > >+ var ver = parseFloat(sysInfo.getProperty("version")); > >+ if (ver < 6.1) > >+ return; > >+ } catch (ex) { } > > Add a test here to check that jumplist availability matches our platform > expectations? Actually you know what, one thing I could do here is enable certain tests on jump list items even if win7 isn't present so they get run on our test infrastructure. I think I'll add that as well.

changes: - better test coverage, virtually all the tests can run on platforms other than win7. - updated nits. I skipped the assert since it would require tracking calls, and there doesn't appear to be any restriction in calling this again as long as you do it earlier enough.

Just remembered I'm going to need an ifdef for lesser sdks on those tests, so I've added that. I'm going to push a rev of this to try which is still using the older vista sdk to confirm. Then later today when we flip the sdk switch to the win7 sdk, I'll push it again to confirm.

We're on the new sdk - https://bugzilla.mozilla.org/show_bug.cgi?id=505504#c43

+ boolean addListBuild(in short aCatType, [optional] in nsIArray items, [optional] in AString catName); I think addToListBuild would be slightly clearer. You're not adding a new "list build", you're adding a new list to the current build. I still don't understand why you need uriHash... why do you need something "non human readable"? Is this a privacy feature? There's only one global nsIJumpListBuilder, right? Why is that? That means we risk the possibility of collisions where someone starts a jump list build and someone else clobbers it. Could nsIWinTaskbar::jumpListBuilder be a method createJumpListBuilder so we can support independent jump list transactions?

(In reply to comment #75) > + boolean addListBuild(in short aCatType, [optional] in nsIArray items, > [optional] in AString catName); > > I think addToListBuild would be slightly clearer. You're not adding a new "list > build", you're adding a new list to the current build. How about addListToBuild? initListBuild addListToBuild abortListBuild commitListBuild That seems to work pretty well. > I still don't understand why you need uriHash... why do you need something "non > human readable"? Is this a privacy feature? They are a convenience helper. Developers can generate these on their own if they wanted to. They're used in tracking removed items across sessions in visible data stores like prefs. For example: init list (removed items = 0) create a list w/ url1, url2, url3 commit the list user removes url1 (time to refresh list) init list (removed items = url1) url1 hash -> prefs based removed list create a list w/ (skip url1), url2, url3, url4 commit the list (at this point, windows forgets that url1 was removed.) (time to refresh list) init list (removed items = 0) create a list w/ (skip url1), url2, url3, url4 commit the list > There's only one global nsIJumpListBuilder, right? Why is that? That means we > risk the possibility of collisions where someone starts a jump list build and > someone else clobbers it. Could nsIWinTaskbar::jumpListBuilder be a method > createJumpListBuilder so we can support independent jump list transactions? No, maybe my use of a property is confusing. Some background, windows builds a list in a series of steps through a com interface/object. I can instantiate as many of those objects as I want, but in the end, the last one that calls its commitList applies. The jump list builder acts the same way, you create nsIJumpListBuilder which instantiates the com object, build your list (which calls into the com interface on each step), and then commit it. If a developer writing an app created two copies of nsIJumpListBuilder, and both built lists and called commit, the last call to commit applies. An alternative that was suggested was to build the list in memory in nsIJumpListBuilder and build all at once on commit, so developers could hold multiple lists, but the com interfaces already does that, so there's no need to add code bloat in widget to accomplish the same. The only advantage would be that you could edit the in memory version, which I didn't see as a value add down in c++. The same could be done up in script with less effort. Plus, the removed items list would become state while it was held. > Could nsIWinTaskbar::jumpListBuilder be a method > createJumpListBuilder so we can support independent jump list transactions? It already is, just as a property. Every call creates a new instance of nsIJumpListBuilder. Maybe that's confusing and I should change it to a method like you suggest, however, I wanted to discourage against this. The call to initListBuild returns removed items which can become state if you hold a copy of nsIJumpListBuilder around. Generally speaking the idea is that developers would create one copy of nsIJumpListBuilder and only work with that.

(In reply to comment #76) > How about addListToBuild? Sounds good. > > I still don't understand why you need uriHash... why do you need something > > "non human readable"? Is this a privacy feature? > > They are a convenience helper. Developers can generate these on their own if > they wanted to. They're used in tracking removed items across sessions in > visible data stores like prefs. For example: > > init list (removed items = 0) > create a list w/ url1, url2, url3 > commit the list > > user removes url1 > > (time to refresh list) > init list (removed items = url1) > url1 hash -> prefs based removed list > create a list w/ (skip url1), url2, url3, url4 > commit the list > (at this point, windows forgets that url1 was removed.) > > (time to refresh list) > init list (removed items = 0) > create a list w/ (skip url1), url2, url3, url4 > commit the list OK but why can't you do all this using the actual URI spec, instead of a hash? How about making that property into a method, createJumpListBuilder, which fails if there is an existing builder which hasn't been committed yet?

can a extension developer add items to JumpList 1. using a XUL Overlay 2. or any other mechanism

(In reply to comment #80) > can a extension developer add items to JumpList > 1. using a XUL Overlay > 2. or any other mechanism The can take over management responsibility by disabling default behavior in apps like Fx and manage what's displayed in the lists through the jump list builder api (or their own component). Windows handles the display so there's no way to use mozilla conventions like xul overlays for their display.

(In reply to comment #81) > apps like Fx and manage what's displayed in the lists through the jump list > builder api (or their own component). Windows handles the display so there's This jump list builder api is it now accessible JavaScript ? (ie, may be thru a XPCOM service)

(In reply to comment #82) > (In reply to comment #81) > > apps like Fx and manage what's displayed in the lists through the jump list > > builder api (or their own component). Windows handles the display so there's > > This jump list builder api is it now accessible JavaScript ? > (ie, may be thru a XPCOM service) Yes, we have a number of new apis becoming available for the win7 taskbar: main interface: http://mxr.mozilla.org/mozilla-central/source/widget/public/nsIWinTaskbar.idl progress overlay: http://mxr.mozilla.org/mozilla-central/source/widget/public/nsITaskbarProgress.idl previews: http://mxr.mozilla.org/mozilla-central/source/widget/public/nsITaskbarTabPreview.idl http://mxr.mozilla.org/mozilla-central/source/widget/public/nsITaskbarTabPreview.idl http://mxr.mozilla.org/mozilla-central/source/widget/public/nsITaskbarWindowPreview.idl jump lists: http://mxr.mozilla.org/mozilla-central/source/widget/public/nsIJumpListBuilder.idl http://mxr.mozilla.org/mozilla-central/source/widget/public/nsIJumpListItem.idl

bug 523121 is a regression from this change and should be considered when determining branch readiness.

I hate doing this but is this going to make 3.6? I see the patch has been waiting on approval for 1.9.2 checkin since 10/06 and this has already missed beta 1.

(In reply to comment #86) > I hate doing this but is this going to make 3.6? I see the patch has been > waiting on approval for 1.9.2 checkin since 10/06 and this has already missed > beta 1. No it will not. 1.9.3 is the current target for feature complete jump lists.