518666 - Add Firefox support for the new win7 jump lists

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Jim Mathies [:jimm]]

Splitting this off from bug 473045 to get the browser bits out of there.

Comment 1

[Jim Mathies [:jimm]]

Attachment: browser ui

Comment 2

[Jim Mathies [:jimm]]

Attachment: browser v.1

Generally, we refresh the jump list per session and when new windows open. I didn't feel doing it more often was needed as it's a fairly intensive process for the os. The lists (per the screen shot) show up in two place, a pinned shortcut on the taskbar and a pinned shortcut in the start menu. There are a number of prefs that control things, most are self explanatory.

+pref("browser.taskbar.lists.enabled", true);

enable/disable the functionality. There are some extensions out there that do this in different ways so an on/off switch seemed like a good idea.

+pref("browser.taskbar.lists.frequent.enabled", true);

enable/disable the frequently visited sites list.

+pref("browser.taskbar.lists.recent.enabled", false);

enable/disable the recently visited sites list.

+pref("browser.taskbar.lists.maxListItemCount", 5);

a general pref for limiting the number of items per list, although windows limits these lists internally based on screen dimensions so it's not absolute.
 
+pref("browser.taskbar.lists.maxHistoryDigDepth", 10);

when digging into history, limit the depth we did. improves performance.

+pref("browser.taskbar.lists.tasks.enabled", true);

enable/disable the tasks list. 

Note, this code relies on the widget bits in bug 473045.

Comment 3

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

Perhaps dao could do a first-pass review? I'm not sure I'll be able to get to this in the near future.

Comment 4

[Jim Mathies [:jimm]]

Note, the location of this code will depend on where the aero peek stuff ends up. (comment 29 in bug 474056)

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 402754 [details] [diff] [review]
browser v.1

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+#ifdef XP_WIN
>+  Cu.import("resource://gre/modules/taskbar/nsWinJumpList.jsm")
>+  WinTaskbarJumpList.startup();
>+#endif

>+#ifdef XP_WIN
>+  WinTaskbarJumpList.shutdown();
>+#endif

This will call startup and shutdown each time a window is opened/closed within a session. Is that what you want?

>+  /**
>+   * Task configuration options: title, description, args, iconidx
>+   *
>+   * title       - Task title displayed in the list. (strings in the table are temp fillers.)
>+   * description - Tooltip description on the list item.
>+   * args        - Command line args to invoke the task.
>+   * iconidx     - Optional win icon index into the main application for the
>+   *               list item.
>+   */ 
>+
>+  _tasks: 
>+    [
>+      ["Open new Tab", "description", "-new-tab about:blank", 0],
>+      ["Open new Window", "description", "-browser", 0],
>+      // ["Private Browsing", "description", "-private", 0], XXX not supported yet (bug 471997)
>+    ],

Why not:

>_tasks:
>  [
>    {title: "open a new tab", description: "foo", args: "-new-tab about:blank", iconIndex: 0},
>    {...}
>  ],

>+  _buildList: function (override)

>+    if (this._startBuild()) {

return early here

>+      var success = false;
>+
>+      if (this._showTasks)
>+        success = this._buildTasks();
>+      if (!success)
>+        return false;

Do this instead:

if (this._showTasks && !this._buildTasks())
  return false;

>+  _buildTasks: function ()
>+  {

I'd generally prefer if you put function () { on one line.

>+    for (var idx = 0; idx < this._tasks.length; idx++) {
>+      var item = this._getHandlerAppItem(this._tasks[idx][0], this._tasks[idx][1],
>+                      this._tasks[idx][2], this._tasks[idx][3]);
>+      items.appendElement(item, false);
>+    }

Use this pattern instead:

this._tasks.forEach(function (task) {
  var item = this._getHandlerAppItem(task ...
});

>+  _buildFrequent: function ()

>+    if (list == null || list.length == 0)
>+      return true;

nit: !list instead of list == null

>+    // track frequent items so that we don't add them to
>+    // the recent list.
>+    this._frequentHashList = new Array();

nit: [] instead of new Array()

>+    let count = 0;
>+    for (var idx = 0; idx < list.length; idx++) {
>+      let shortcut = this._getHandlerAppItem(list[idx].title, list[idx].title, list[idx].uri, 1);

>+  _buildRecent: function ()

>+    for (let navIdx = 0; navIdx < list.length; navIdx++) {
>+      let shortcut = this._getHandlerAppItem(list[navIdx].title, list[navIdx].title, list[navIdx].uri, 1);

Again, use list.forEach(function (item) { ... });

>+            if (skip) continue;

nit: line break after if (skip)

Note that this needs to be 'return' instead of 'continue' when you use forEach().

>+  _getHandlerAppItem: function (name, description, args, icon)
>+  {
>+    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>+                 createInstance(Ci.nsIProperties);
>+    var file = dirSvc.get("XCurProcD", Components.interfaces.nsILocalFile);

use Ci

>+  _rdb_trackListStart: function ()
>+  {
>+    this._trackList = new Array();
>+  },

[]

>+  _rdb_updateTrackList: function (hash)
>+  {
>+    for (let idx = 0; idx < this._trackList.length; idx++) {
>+      if (this._trackList[idx] == hash) {
>+        return;
>+      }
>+    }
>+    this._trackList.push(hash);

if (this._trackList.indexOf(hash) == -1)
  this._trackList.push(hash);

>+  _rdb_trackListEnd: function ()
>+  {
>+    // commit the new removed items list in prefs.
>+    let list = "";
>+    for (let idx = 0; idx < this._trackList.length; idx++) {
>+      list += this._trackList[idx];
>+      list += ",";
>+    }

this._trackList.join(",");

>+  _rdb_hasLinkBeenRemoved: function (specHash)
>+  {

>+      if (list[idx] == specHash) {
>+        return true;
>+      }

>+  _rdb_isRemovedStartListItem: function (specHash)
>+  {

>+    if (!this._removedItems) {
>+      return false;
>+    }

drop the curly brackets

>+    var enum = this._removedItems.enumerate();
>+    while (enum.hasMoreElements()) {
>+      var oldItem = enum.getNext().QueryInterface(Ci.nsIJumpListShortcut);
>+      if (oldItem) {
>+        var uriSpec = oldItem.app.getParameter(0);
>+        // make sure it's a uri
>+        if (specHash == this._getURIHash(uriSpec)) {
>+          return true;
>+        }

Drop the curly brackets around single lines and use 'let' in nested blocks.

>+  _getNavService: function ()
>+  {
>+    if (!this._historyService)
>+    this._historyService = Components.classes["@mozilla.org/browser/nav-history-service;1"]
>+                                   .getService(Components.interfaces.nsINavHistoryService);
>+    return this._historyService;
>+  },
>+

This could use a lazy getter.

>+  _getNavFrequent: function ()
>+  {
>+    try {

>+  _getNavRecent: function ()
>+  {
>+    try {

What exceptions are these going to catch?

>+      var list = new Array();

nit: [] instead of new Array();

>+      var rootNode = result.root;
>+      rootNode.containerOpen = true;
>+
>+      // iterate over the immediate children of this folder and dump to console
>+      for (var idx = 0; idx < rootNode.childCount; idx++) {
>+        var node = rootNode.getChild(idx);
>+        list.push({'uri':node.uri, 'title':node.title});
>+      }

nit: space after colons

>+  _pref: function (pref)
>+  {
>+    return PREF_TASKBAR_BRANCH + "." + pref;
>+  },

This isn't the traditional way to do this, nor the easiest one ... see below.

>+  _refreshPrefs: function ()
>+  {
>+    delete this._prefBranch;
>+    this._prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                       getService(Ci.nsIPrefBranch2);

Why do you redefine _prefBranch if it's already there?

Consider adding this to the global scope:

__defineGetter__("_prefs", function () {
  delete this._prefs;
  return this._prefs =
         Cc["@mozilla.org/preferences-service;1"]
           .getService(Ci.nsIPrefService)
           .getBranch(PREF_TASKBAR_BRANCH)
           .QueryInterface(Ci.nsIPrefBranch2);
});

And then use e.g.:

_prefs.getBoolPref(PREF_TASKBAR_ENABLED)

instead of:

this._prefBranch.getBoolPref(this._pref(PREF_TASKBAR_ENABLED))

>+    this._enabled = true;
>+    this._showFrequent = true;
>+    this._showRecent = true;
>+    this._showTasks = true;
>+    this._maxItemCount = 5;
>+    this._maxDigCount = 10;

This isn't needed, see below.

>+    try {
>+      this._enabled = 
>+        this._prefBranch.getBoolPref(this._pref(PREF_TASKBAR_ENABLED));
>+    } catch (e) { }

Don't try/catch as you've added the defaults to firefox.js.

>+  _initObs: function ()
>+  {
>+    try {
>+      var os = Cc["@mozilla.org/observer-service;1"].
>+               getService(Ci.nsIObserverService);
>+      os.addObserver(this, "private-browsing", false);
>+      this._prefBranch.addObserver(PREF_TASKBAR_BRANCH, this, false);
>+    } catch (err) { }
>+  },
>+ 
>+  _freeObs: function ()
>+  {
>+    try {
>+      var os = Cc["@mozilla.org/observer-service;1"].
>+               getService(Ci.nsIObserverService);
>+      os.removeObserver(this, "private-browsing");
>+      this._prefBranch.removeObserver(PREF_TASKBAR_BRANCH, this);
>+    } catch (err) { }
>+  },

Why do you try/catch?

Also consider adding a getter to the global scope:

__defineGetter__("_observerService", function () {
  delete this._observerService;
  return this._observerService =
         Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
});

>+  observe: function (aSubject, aTopic, aData) {
>+    switch(aTopic) {

nit: add a space between switch and (

>+            if (!this._inPrivateBrowsing) {
>+              this._deleteActiveJumpList();
>+            }

nit: drop the curly brackets

>+  _loadStringBundle: function ()
>+  {
>+    var stringSvc = Components.classes["@mozilla.org/intl/stringbundle;1"].
>+                           getService(Components.interfaces.nsIStringBundleService);
>+    this._stringBundle = stringSvc.createBundle("chrome://browser/locale/taskbar.properties");
>+  },
>+
>+  _getString: function (name)
>+  {
>+    if (!this._stringBundle)
>+      this._loadStringBundle();
>+    return this._stringBundle.GetStringFromName(name);
>+  },

As above, consider a lazy getter for _stringBundle.

Also use Cc and Ci.

>+var EXPORTED_SYMBOLS = ["WinTaskbarJumpList"];

This makes more sense at the top of the file, I think.

>--- a/browser/locales/jar.mn
>+++ b/browser/locales/jar.mn

>+    locale/browser/taskbar.properties              (%chrome/browser/taskbar.properties)

That file appears to be missing from the patch.

Comment 6

[Dão Gottwald [:dao]]

It would be nice if someone from the Places team could review the bookmarks and history code.

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #5)
> (From update of attachment 402754 [details] [diff] [review])
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> 
> >+#ifdef XP_WIN
> >+  Cu.import("resource://gre/modules/taskbar/nsWinJumpList.jsm")
> >+  WinTaskbarJumpList.startup();
> >+#endif
> 
> >+#ifdef XP_WIN
> >+  WinTaskbarJumpList.shutdown();
> >+#endif
> 
> This will call startup and shutdown each time a window is opened/closed within
> a session. Is that what you want?

I wanted to update the lists a fairly regular basis, but not every time history data changed. Delay load on opening a window seemed like a good time to do it. Better suggestions?

I'll update the nits here and post a new patch shortly.

Comment 8

[Dão Gottwald [:dao]]

How does shutdown() relate to these updates?

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> How does shutdown() relate to these updates?

It doesn't really, it ties to shutting down allocated resources.

If there was a way to init/shutdown a single object per browser instance, and call an update() that did the build per window, that would work better and use less cpu. Is there a simple way to do that through the code in browser.js?

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> (In reply to comment #8)
> > How does shutdown() relate to these updates?
> 
> It doesn't really, it ties to shutting down allocated resources.

It stops observing private-browser for instance -- this doesn't seem right if another window is still open. Maybe you want to observe quit-application-granted rather than calling shutdown() in browser.js?

> If there was a way to init/shutdown a single object per browser instance, and
> call an update() that did the build per window, that would work better and use
> less cpu. Is there a simple way to do that through the code in browser.js?

As far as I can see, browser.js could call update() and the module could do all the rest.

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > How does shutdown() relate to these updates?
> > 
> > It doesn't really, it ties to shutting down allocated resources.
> 
> It stops observing private-browser for instance -- this doesn't seem right if
> another window is still open. Maybe you want to observe
> quit-application-granted rather than calling shutdown() in browser.js?
> 
> > If there was a way to init/shutdown a single object per browser instance, and
> > call an update() that did the build per window, that would work better and use
> > less cpu. Is there a simple way to do that through the code in browser.js?
> 
> As far as I can see, browser.js could call update() and the module could do all
> the rest.

Thanks, will update.

Comment 12

[Jim Mathies [:jimm]]

Attachment: browser v.2

Updated, and moved to the right place. Instead of using window opens I used a timer instead to do the refresh. One issue I ran into, those forEach calls prevented me from referencing 'this', so I had to use the global object. Is there a trick there I'm missing Dao?

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 403944 [details] [diff] [review]
browser v.2


Shawn can you review the history db related code?

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #12)
> One issue I ran into, those forEach calls
> prevented me from referencing 'this', so I had to use the global object. Is
> there a trick there I'm missing Dao?

Yep, there's another parameter: array.forEach(callback[, thisObject]);

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 403944 [details] [diff] [review]
browser v.2

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+  try {
>+    Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm")
>+    if (WinTaskbarJumpList.startup()) {
>+      WinTaskbarJumpList.update();

Can you move this to nsBrowserGlue.js? This way it won't be executed for each browser window.

>+      setTimeout(TaskbarUpdate, 1000*60*2); // Update every two minutes.

This should be part of the module, using nsITimer (or nsIIdleService?).

>+++ b/browser/modules/taskbar/src/Makefile.in

Rob uses browser/modules/wintaskbar/. Please settle on one path.

>+__defineGetter__("_stringBundle", function () {
>+  delete this._stringSvc;
>+  delete this._stringBundle;
>+  this._stringSvc =
>+          Cc["@mozilla.org/intl/stringbundle;1"]
>+          .getService(Ci.nsIStringBundleService);
>+  return this._stringBundle =
>+         this._stringSvc
>+         .createBundle("chrome://browser/locale/taskbar.properties");
>+});

stringSvc should be a variable, no need to have it outside of that function.

>+      if (list[idx] == specHash) {
>+        return true;
>+      }

more superfluous brackets

>+    for (var idx = 0; idx < rootNode.childCount; idx++) {
>+      var node = rootNode.getChild(idx);
>+      list.push({'uri': node.uri, 'title': node.title});
>+    }

use let for idx and node. ' around uri and title could be dropped.

>+  _initObs: function () {
>+    _observerService.addObserver(this, "private-browsing", false);
>+    _observerService.addObserver(this, "quit-application-granted", false);
>+    Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2)
>+                                            .addObserver(PREF_TASKBAR_BRANCH, this, false);
>+  },
>+ 
>+  _freeObs: function () {
>+    _observerService.removeObserver(this, "private-browsing");
>+    _observerService.removeObserver(this, "quit-application-granted");
>+    Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2)
>+                                            .removeObserver(PREF_TASKBAR_BRANCH, this);
>+  },

I think _prefs.add/removeObserver("", ...) would work here.

Comment 16

[Jim Mathies [:jimm]]

(In reply to comment #15)
> >+  _freeObs: function () {
> >+    _observerService.removeObserver(this, "private-browsing");
> >+    _observerService.removeObserver(this, "quit-application-granted");
> >+    Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2)
> >+                                            .removeObserver(PREF_TASKBAR_BRANCH, this);
> >+  },
> 
> I think _prefs.add/removeObserver("", ...) would work here.

Tried that, something about calling add/remove on the nsIPrefBranch2 of the specific branch. I'll dig around in our prefs code a bit to try and figure out why it does like that.

Will address rest of the comments and repost here in a bit.

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #15)
> >+++ b/browser/modules/taskbar/src/Makefile.in
> 
> Rob uses browser/modules/wintaskbar/. Please settle on one path.

There are some bugs filed specific to doc related features, although nothing at this point that might require a jsm up in browser.

I ended up going with something more generic thinking that at some point code for other platforms taskbars/docs could end up here.

Comment 18

[Jim Mathies [:jimm]]

Attachment: browser v.3

> I think _prefs.add/removeObserver("", ...) would work here.

empty quotes FTW!

Updated per comments.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 404056 [details] [diff] [review]
browser v.3

>-DIRS = base components locales themes fuel app
>+DIRS = base components locales themes fuel app modules

Connor said we shouldn't create modules/...

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js

>+#ifdef XP_WIN
>+Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm")
>+#endif

>+#ifdef XP_WIN
>+    try {
>+      // For windows seven, initialize the jump list module
>+      if (WinTaskbarJumpList.startup())
>+        WinTaskbarJumpList.update(true);
>+    } catch(err) { }
>+#endif

Why doesn't startup call update?

WinTaskbarJumpList doesn't need to be global, just use it locally:

let temp = {};
Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm", temp);
temp.WinTaskbarJumpList.startup();

>+  _free: function () {
>+    this._freeObs();
>+    delete this._taskbar;
>+    delete this.timer;

_timer

Comment 20

[Jim Mathies [:jimm]]

Attachment: browser v.4

updated.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 404102 [details] [diff] [review]
browser v.4

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js

>+#ifdef XP_WIN
>+Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm")
>+#endif

You forgot to remove this.


>+#ifdef XP_WIN
>+    try {
>+      // For windows seven, initialize the jump list module
>+      let temp = {};
>+      Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm", temp);
>+      temp.WinTaskbarJumpList.startup();
>+    } catch(err) { }
>+#endif

Why try/catch? There are a few others (actually, most of your try/catch blocks) where I'm not sure they make sense...

>+++ b/browser/components/taskbar/src/WinTaskbarJumpList.jsm

>+const Cr = Components.results;
>+const Cu = Components.utils;

These are unused.

>+  startup: function () {
>+    // exit if this isn't win7 or higher.
>+    if (!this._initTaskbar())
>+      return false;
>+
>+    // retrieve taskbar related prefs.
>+    this._refreshPrefs();
>+
>+    // load tasks table strings
>+    this._initStrings();
>+
>+    // observer for private browsing and our prefs branch
>+    this._initObs();
>+
>+    // jump list refresh timer
>+    this._initTimer();
>+
>+    // build the list
>+    this.update(true);
>+
>+    return true;
>+  },

The return value is now unused.

>+  _buildList: function (shutdown) {
>+    // anything to build?
>+    if (!this._showFrequent && !this._showRecent && !this._showTasks) {
>+      this._deleteActiveJumpList();
>+      return true;
>+    }
>+
>+    if (!this._startBuild())
>+      return false;
>+
>+    if (this._showTasks && !this._buildTasks(shutdown))
>+      return false;
>+
>+    if (this._showFrequent && !this._buildFrequent())
>+      return false;
>+
>+    if (this._showRecent && !this._buildRecent())
>+      return false;
>+
>+    if (!this._commitBuild())
>+      return false;
>+
>+    return true;
>+  },

ditto

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)
> (From update of attachment 404102 [details] [diff] [review])
> >--- a/browser/components/nsBrowserGlue.js
> >+++ b/browser/components/nsBrowserGlue.js
> 
> >+#ifdef XP_WIN
> >+Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm")
> >+#endif
> 
> You forgot to remove this.
> 

Whoops, apologies, will remove this.

> 
> >+#ifdef XP_WIN
> >+    try {
> >+      // For windows seven, initialize the jump list module
> >+      let temp = {};
> >+      Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm", temp);
> >+      temp.WinTaskbarJumpList.startup();
> >+    } catch(err) { }
> >+#endif
> 
> Why try/catch? There are a few others (actually, most of your try/catch blocks)

Being paranoid basically. Let me look through the code paths in startup and update to be sure anything that could fail is caught, and I'll remove it.

> where I'm not sure they make sense...
> 
> >+++ b/browser/components/taskbar/src/WinTaskbarJumpList.jsm
> 
> >+const Cr = Components.results;
> >+const Cu = Components.utils;
> 
> These are unused.
> 
> >+  startup: function () {
> >+    // exit if this isn't win7 or higher.
> >+    if (!this._initTaskbar())
> >+      return false;
> >+
> >+    // retrieve taskbar related prefs.
> >+    this._refreshPrefs();
> >+
> >+    // load tasks table strings
> >+    this._initStrings();
> >+
> >+    // observer for private browsing and our prefs branch
> >+    this._initObs();
> >+
> >+    // jump list refresh timer
> >+    this._initTimer();
> >+
> >+    // build the list
> >+    this.update(true);
> >+
> >+    return true;
> >+  },
> 
> The return value is now unused.
> 
> >+  _buildList: function (shutdown) {
> >+    // anything to build?
> >+    if (!this._showFrequent && !this._showRecent && !this._showTasks) {
> >+      this._deleteActiveJumpList();
> >+      return true;
> >+    }
> >+
> >+    if (!this._startBuild())
> >+      return false;
> >+
> >+    if (this._showTasks && !this._buildTasks(shutdown))
> >+      return false;
> >+
> >+    if (this._showFrequent && !this._buildFrequent())
> >+      return false;
> >+
> >+    if (this._showRecent && !this._buildRecent())
> >+      return false;
> >+
> >+    if (!this._commitBuild())
> >+      return false;
> >+
> >+    return true;
> >+  },
> 
> ditto

Is that really such a bad thing? I'll remove them but at some point knowing if startup or _buildList fail might be useful.

Comment 23

[Jim Mathies [:jimm]]

Attachment: browser v.5

Comment 24

[Jim Mathies [:jimm]]

Attachment: browser v.5

- removed the debug output code.
- addressed return values.
- removed exception trap on main startup call.

Comment 25

[Jim Mathies [:jimm]]

Attachment: browser v.5

changes:
- minor commenting updates
- removed unused consts

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 404181 [details] [diff] [review]
browser v.5

Adding Shawn back in as an r? on the two history routines.

Comment 27

[Dão Gottwald [:dao]]

Jim, I'm not sure which patches I need to apply for this to work. Would you mind pushing the whole set to the tryserver for me?

Comment 28

[Jim Mathies [:jimm]]

Attachment: browser v.5 (unbitrot)

Sent, will post back with builds in a bit.

Comment 29

[Jim Mathies [:jimm]]

there's going to be a minor delay on these builds due to an sdk rollout issue.

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 404181 [details] [diff] [review]
browser v.5

For more detailed review comments, please see http://reviews.visophyte.org/r/404181/

on file: browser/app/profile/firefox.js line 904
> // Win7 taskbar jump list display prefs 
> #ifdef XP_WIN

don't we also want this ifndef WINCE?
same with this code elsewhere.


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 56
> __defineGetter__("_taskbarService", function () {
>   delete this._taskbarService;
>   return this._taskbarService =
>          Cc["@mozilla.org/windows-taskbar;1"]
>            .getService(Ci.nsIWinTaskbar);
> });

XPCOMUtils.defineLazyServiceGetter


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 63
> __defineGetter__("_prefs", function () {
>   delete this._prefs;
>   return this._prefs =
>          Cc["@mozilla.org/preferences-service;1"]
>            .getService(Ci.nsIPrefService)
>            .getBranch(PREF_TASKBAR_BRANCH)
>            .QueryInterface(Ci.nsIPrefBranch2);
> });

XPCOMUtils.defineLazyGetter


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 72
> __defineGetter__("_navService", function () {
>   delete this._navService;
>   return this._navService =
>           Cc["@mozilla.org/browser/nav-history-service;1"]
>           .getService(Ci.nsINavHistoryService);
> });

XPCOMUtils.defineLazyServiceGetter


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 79
> __defineGetter__("_observerService", function () {
>   delete this._observerService;
>   return this._observerService =
>           Cc["@mozilla.org/observer-service;1"]
>           .getService(Ci.nsIObserverService);
> });

XPCOMUtils.defineLazyServiceGetter


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 86
> __defineGetter__("_stringBundle", function () {
>   delete this._stringBundle;
>   var stringSvc =
>           Cc["@mozilla.org/intl/stringbundle;1"]
>           .getService(Ci.nsIStringBundleService);
>   return this._stringBundle =
>          stringSvc
>          .createBundle("chrome://browser/locale/taskbar.properties");
> });

XPCOMUtils.defineLazyGetter


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 98
>   _taskbar: null,
> 
>   /**
>    * Task configuration options: title, description, args, iconidx
>    *
>    * title       - Task title displayed in the list. (strings in the table are temp fillers.)
>    * description - Tooltip description on the list item.
>    * args        - Command line args to invoke the task.
>    * iconidx     - Optional win icon index into the main application for the
>    *               list item.
>    * close       - Indicates if the command should be visible after the browser closes.
>    */
> 
>   _tasks:
>   [
>     {title: "Open new tab", description: "", args: "-new-tab about:blank", iconIndex: 0, close: false},
>     {title: "Open new window", description: "", args: "-browser", iconIndex: 0, close: false},
>   ],

Please pull these into either the global scope or a new object like I had sid
do in the download manager patch.  We don't want people to get into private
state or private functions inside the jsm.  The applies to all private
functions/variables.


on file: browser/components/taskbar/src/WinTaskbarJumpList.jsm line 121
>   startup: function () {

global nit: use a named function please


r=sdwilsh with these things addressed.

Comment 31

[Jim Mathies [:jimm]]

Attachment: browser v.6

Comment 32

[Jim Mathies [:jimm]]

Finally, some builds!

http://build.mozilla.org/tryserver-builds/[e-mail]-jumplists5

Comment 33

[Jim Mathies [:jimm]]

Attachment: browser v.6

(changed a var to a let in the config option init call.)

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 404519 [details] [diff] [review]
browser v.6

>+XPCOMUtils.defineLazyServiceGetter(this, "_navService",
>+                                   "@mozilla.org/browser/nav-history-service;1",
>+                                   "nsINavHistoryService");

I'd prefer _historyService or even _navHistoryService.

>+/////////////////////////////////////////////////////////////////////////////
>+// Default task list configuration data object. Callers can create their
>+// own as needed. These defaults apply to Fx. 'config' is the table
>+// WinTaskbarJumpList makes a copy of.
>+
>+var defaultTasksCfg =

This is browser/, so expecting anything that doesn't apply to Fx doesn't seem to make sense.

>+  config : [
>+    // Open new window
>+    {
>+      title:        "taskbar.tasks.one.label",
>+      description:  "taskbar.tasks.one.description",
>+      args:         "-new-tab about:blank",
>+      iconIndex:    0, // Fx app icon
>+      open:         true,
>+      close:        false, // The jump list already has an app launch icon
>+    },
>+
>+    // Open new tab
>+    {
>+      title:        "taskbar.tasks.two.label",
>+      description:  "taskbar.tasks.two.description",

newWindown / newTab instead of one / two.

>+  /**
>+   * init: Called by WinTaskbarJumpList before config task data is saved
>+   * for use in building tasks.
>+   */
>+
>+  init: function CFG_init() {
>+    var bundle = Cc["@mozilla.org/intl/stringbundle;1"]
>+                 .getService(Ci.nsIStringBundleService)
>+                 .createBundle("chrome://browser/locale/taskbar.properties");
>+    for (let idx = 0; idx < this.config.length; idx++) {
>+      this.config[idx].title = bundle.GetStringFromName(this.config[idx].title);
>+      this.config[idx].description = bundle.GetStringFromName(this.config[idx].description);
>+    }
>+  },

I'd prefer title and description to be smart getters and avoid the init method. Also use _stringBundle.

>+  _shutdown: function WTBJL__shutdown() {
>+    this.update(true);

Can you add a _shuttingDown property instead of passing that argument through various methods?

>+    if (this._showTasks && !this._buildTasks(shutdown))
>+      return;
>+
>+    // Space for frequent items takes priority over recent when
>+    // adding items to lists when maxListItemCount is larger than
>+    // the number of items the desktop permits. 
>+    
>+    if (this._showFrequent && !this._buildFrequent())
>+      return;
>+
>+    if (this._showRecent && !this._buildRecent())
>+      return;

Do these things actually depend on each other, or why are returning? Also, in which cases will these methods return false, i.e. what kind of exceptions do they catch?

>+    if (!this._commitBuild())
>+      return;
>+  },

You're at the end of that function, no need to return explicitly.

>+    this._enabled = 
>+      _prefs.getBoolPref(PREF_TASKBAR_ENABLED);
>+
>+    this._showFrequent =
>+      _prefs.getBoolPref(PREF_TASKBAR_FREQUENT);
>+
>+    this._showRecent =
>+      _prefs.getBoolPref(PREF_TASKBAR_RECENT);
>+
>+    this._showTasks =
>+      _prefs.getBoolPref(PREF_TASKBAR_TASKS);
>+
>+    this._maxItemCount =
>+      _prefs.getIntPref(PREF_TASKBAR_ITEMCOUNT);
>+
>+    this._maxDigCount =
>+      _prefs.getIntPref(PREF_TASKBAR_DIGCOUNT);

nit: drop the line breaks after '='.

>+taskbar.tasks.one.description=Open a new tab in the current window.

There isn't necessarily a current window. For instance, start firefox, open the download manager, close the browser window.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 404519 [details] [diff] [review]
browser v.6

>+   * iconidx     - Optional win icon index into the main application for the
>+   *               list item.

s/iconidx/iconIndex/

>+    // The windows removed item list is reset on ever commit.
>+    // so we track these removed items and store them in prefs.

s/ever commit./every commit,/

Comment 36

[Jim Mathies [:jimm]]

> >+    // Space for frequent items takes priority over recent when
> >+    // adding items to lists when maxListItemCount is larger than
> >+    // the number of items the desktop permits. 
> >+    
> >+    if (this._showFrequent && !this._buildFrequent())
> >+      return;
> >+
> >+    if (this._showRecent && !this._buildRecent())
> >+      return;
> 
> Do these things actually depend on each other, or why are returning? Also, in
> which cases will these methods return false, i.e. what kind of exceptions do
> they catch?

My preference was that if any of the build steps fail, the entire list building process should fail. If we go that route, you keep the last complete stale list that built successfully. The other route would be to not fail on individual lists, which would result in new lists with missing categories.

Individual list building failures are usually the result of the code adding items that have been removed by the user, or adding duplicate items that have already been added to another list successfully. (The underlying windows apis refuse to allow this, the widget interface throws on these.) The removed items and dupe items code in here prevents that. 

Neither route presents major issues really. Failures are going to be the result of bugs in the code and are not expected.

Comment 37

[Dão Gottwald [:dao]]

I think we should only catch expected exceptions.

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #37)
> I think we should only catch expected exceptions.

Ok, I can pull the traps out. I think if there are issues, it'll be related to what windows considered a unique uri and what we consider a unique uri. We use hashes for the purpose of identifying a unique uri, but windows may do some coalescing of different urls in it's calculations.

I'd like to add tests for this, but generating the list of uri to test with is somewhat problematic since windows api treatment is un-documented, and I have no history to work with.

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #38)
> (In reply to comment #37)
> > I think we should only catch expected exceptions.
> 
> Ok, I can pull the traps out. I think if there are issues, it'll be related to
> what windows considered a unique uri and what we consider a unique uri. We use
> hashes for the purpose of identifying a unique uri, but windows may do some
> coalescing of different urls in it's calculations.
> 
> I'd like to add tests for this, but generating the list of uri to test with is
> somewhat problematic since windows api treatment is un-documented, and I have
> no history to work with.

I should add, I do have basic uri uniqueness testing in widget tests already.

Comment 40

[Jim Mathies [:jimm]]

Attachment: browser v.7

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 404681 [details] [diff] [review]
browser v.7

>+    // For windows seven, initialize the jump list module.
>+    let temp = {};
>+    Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm", temp);
>+    temp.WinTaskbarJumpList.startup(temp.defaultTasksCfg);

defaultTasksCfg doesn't need to be exported. startup should just use the default configuration if no argument was provided. Although I'm not sure it needs to accept an argument at all...

>+var defaultTasksCfg =
>+{
>+  /**
>+   * Task configuration options: title, description, args, iconIndex, open, close.
>+   *
>+   * title       - Task title displayed in the list. (strings in the table are temp fillers.)
>+   * description - Tooltip description on the list item.
>+   * args        - Command line args to invoke the task.
>+   * iconIndex   - Optional win icon index into the main application for the
>+   *               list item.
>+   * open        - Boolean indicates if the command should be visible after the browser opens.
>+   * close       - Boolean indicates if the command should be visible after the browser closes.
>+   */
>+
>+  config : [
>+    // Open new window
>+    {

use this pattern:

var defaultTasksCfg = [
  { ... },
  { ... }
];

instead of:

var defaultTasksCfg = {
  config: [
    { ... },
    { ... }
  ]
};

>+      get title()       { return _stringBundle.GetStringFromName("taskbar.tasks.newTab.label"); },

This can be simplified:

get title()   _stringBundle.GetStringFromName("taskbar.tasks.newTab.label"),

And if you make the _getString method a global function, it can be simplified further:

get title()   _getString("taskbar.tasks.newTab.label"),

>+  observe: function WTBJL_observe(aSubject, aTopic, aData) {
>+    switch(aTopic) {

add a space after 'switch'

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #41)
> (From update of attachment 404681 [details] [diff] [review])
> >+    // For windows seven, initialize the jump list module.
> >+    let temp = {};
> >+    Cu.import("resource://gre/modules/WinTaskbarJumpList.jsm", temp);
> >+    temp.WinTaskbarJumpList.startup(temp.defaultTasksCfg);
> 
> defaultTasksCfg doesn't need to be exported. startup should just use the
> default configuration if no argument was provided. Although I'm not sure it
> needs to accept an argument at all...

Maybe I misread sdwilsh's comments, but I thought he wanted this data isolated from the main jsm. (Which I thought was nice in that it made it easy to swap out config objects.)

Anyway, will update. Thanks for the detailed script review comments, notes taken. ;)

Comment 43

[Dão Gottwald [:dao]]

Shawn just wanted it to be isolated from the exported object, so that the exported object isn't polluted with private stuff like WinTaskbarJumpList._tasks.

Comment 44

[Jim Mathies [:jimm]]

Attachment: browser v.8

updated.

Comment 45

[Jim Mathies [:jimm]]

Comment on attachment 404848 [details] [diff] [review]
browser v.8

sr?

Comment 46

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

Comment on attachment 404848 [details] [diff] [review]
browser v.8

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

>+  _getHandlerAppItem: function WTBJL__getHandlerAppItem(name, description, args, icon) {
>+    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>+                 createInstance(Ci.nsIProperties);
>+    var file = dirSvc.get("XCurProcD", Ci.nsILocalFile);
>+
>+    // XXX where can we grab this from in the build? Do we need to?
>+    file.append("firefox.exe");

This is frequently called, and retrieving the directory service on each invocation is inefficient. I'm also worried that we need to use that path at all. Can't the items that use this be nsIJumpListLinks rather than nsIJumpListShortcut, at least for entries that aren't "tasks"? Do these get persisted for more than the current session?

>+  _getURIHash: function WTBJL__getURIHash(spec) {

>+      var uri = Cc["@mozilla.org/network/simple-uri;1"].
>+                createInstance(Ci.nsIURI);

You shouldn't create URIs by contract ID - call nsIIOService.newURI() instead.

This whole method is kind of gross, though, particularly since it's called in loops. I'd almost prefer duplicating JumpListLink::HashURI here just to avoid all the XPCOM back and forth and object creation. Is that possible?

Stepping even further back, though, do we really need to be persisting these removals beyond the current session? I'm kind of wary of keeping even hashes of URIs around indefinitely, and it'd be simpler to just avoid persisting the removals, which doesn't seem that bad (you can always clear the history item, right?).

I don't really understand nsIJumpListBuilder, but I presume the complexity there is required by the relevant Windows APIs?

Sorry for the potentially stupid questions, but I haven't been following this bug or bug 473045 very closely. I'm also not actually a super-reviewer, so if you were really looking for an SR-stamp rather than just additional review, you'll have to ask someone who is (I don't think it's strictly required, but it probably wouldn't hurt).

Comment 47

[Jim Mathies [:jimm]]

(In reply to comment #46)
> (From update of attachment 404848 [details] [diff] [review])
> >diff --git a/browser/components/wintaskbar/winJumpLists.jsm b/browser/components/wintaskbar/winJumpLists.jsm
> 
> >+  _getHandlerAppItem: function WTBJL__getHandlerAppItem(name, description, args, icon) {
> >+    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> >+                 createInstance(Ci.nsIProperties);
> >+    var file = dirSvc.get("XCurProcD", Ci.nsILocalFile);
> >+
> >+    // XXX where can we grab this from in the build? Do we need to?
> >+    file.append("firefox.exe");
> 
> This is frequently called, and retrieving the directory service on each
> invocation is inefficient.

I can address that through a lazy getter.

> I'm also worried that we need to use that path at
> all. Can't the items that use this be nsIJumpListLinks rather than
> nsIJumpListShortcut, at least for entries that aren't "tasks"?

Originally that was the idea, however there were some problems. Internally nsIJumpListLinks are stored as shell items which have properties windows populates using it's own data stores. We however don't inject any of the information windows needs into windows, so links show up for example sans titles and fave icons. I have an open bug on this, potentially it might be something we take to the european commission folks. Another problem manifested itself as well - to add links, you must be the default protocol handler for the link's protocol. This would cause problems in side by side installs of different versions where jump list items wouldn't show up unless the browser was registered.

AFAICT, other browsers seem to be doing the same thing we are here, chrome for example uses shortcuts rather than links for uri.

> Do these get
> persisted for more than the current session?

Yes, on the taskbar jump list if you've pinned fx to it.

> >+  _getURIHash: function WTBJL__getURIHash(spec) {
> 
> >+      var uri = Cc["@mozilla.org/network/simple-uri;1"].
> >+                createInstance(Ci.nsIURI);
> 
> You shouldn't create URIs by contract ID - call nsIIOService.newURI() instead.
> 
> This whole method is kind of gross, though, particularly since it's called in
> loops. I'd almost prefer duplicating JumpListLink::HashURI here just to avoid
> all the XPCOM back and forth and object creation. Is that possible?

Shortcuts are generic command line apps. Our links are a command line app with the first param as the url. I really can't put a "HashFirstCommandLineParam" property on them. A uri property maybe, but it really doesn't fit.


> 
> Stepping even further back, though, do we really need to be persisting these
> removals beyond the current session? I'm kind of wary of keeping even hashes of
> URIs around indefinitely, and it'd be simpler to just avoid persisting the
> removals, which doesn't seem that bad (you can always clear the history item,
> right?).

If a user removes an item, and we refresh the list, we have to track the items they remove because windows forgets what's been removed as soon as we refresh. Here is an 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

(Hash usage is for general privacy reasons.)  

> 
> I don't really understand nsIJumpListBuilder, but I presume the complexity
> there is required by the relevant Windows APIs?

Well, there are three calls, init, add a list of items with a title, and commit. That's pretty straight forward. Jump lists are made up of multiple smaller lists, tasks, favorites, or whatever else you might want. The widget api breaks the building down into steps because any one step might fail based on the contents of the list you're adding (for example adding a removed item you were informed about in init). The underlying windows com object takes care of data storage as you build it.

> 
> Sorry for the potentially stupid questions, but I haven't been following this
> bug or bug 473045 very closely. I'm also not actually a super-reviewer, so if
> you were really looking for an SR-stamp rather than just additional review,
> you'll have to ask someone who is (I don't think it's strictly required, but it probably wouldn't hurt).

All opinions welcome. :) I seem to be running out of sr candidates though, mconnor didn't have the bandwidth.

Comment 48

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

(In reply to comment #47)
> Shortcuts are generic command line apps. Our links are a command line app with
> the first param as the url. I really can't put a "HashFirstCommandLineParam"
> property on them. A uri property maybe, but it really doesn't fit.

I don't really understand this. My point was that _getURIHash creates nsIURI and nsIJumpListLink objects just so that it can call JumpListLink::HashURI to hash the spec it's given and return the hash. It seems like it would be simpler to just hash the passed-in spec directly here, using the same code as in JumpListLink::HashURI, and avoid the temporary objects and xpcom overhead.

> If a user removes an item, and we refresh the list, we have to track the items
> they remove because windows forgets what's been removed as soon as we refresh.

Right, but do we need to track them for longer than the current session?

Comment 49

[James Ross]

(In reply to comment #47)
> If a user removes an item, and we refresh the list, we have to track the items
> they remove because windows forgets what's been removed as soon as we refresh.

MSDN remarks that "It is strongly recommended that an application clear any destination tracking data when the user elects to remove that destination. If the user accesses that destination again in the future, it may be re-added to the Jump List and can again accumulate data." <http://msdn.microsoft.com/en-gb/library/dd378401>

I would interpret that as meaning that the corresponding history entry (or whatever is the source of the item) ought to be removed. Has this been considered and would that not be desirable for some reason?

Comment 50

[Jim Mathies [:jimm]]

(In reply to comment #48)
> (In reply to comment #47)
> > Shortcuts are generic command line apps. Our links are a command line app with
> > the first param as the url. I really can't put a "HashFirstCommandLineParam"
> > property on them. A uri property maybe, but it really doesn't fit.
> 
> I don't really understand this. My point was that _getURIHash creates nsIURI
> and nsIJumpListLink objects just so that it can call JumpListLink::HashURI to
> hash the spec it's given and return the hash. It seems like it would be simpler
> to just hash the passed-in spec directly here, using the same code as in
> JumpListLink::HashURI, and avoid the temporary objects and xpcom overhead.

Gotcha, I can replace this with direct use of the crypto hash object.

> If a user removes an item, and we refresh the list, we have to track the items
> they remove because windows forgets what's been removed as soon as we refresh.
> 
> Right, but do we need to track them for longer than the current session?

Yes. 

1) surf to a pron url
2) pron url shows up in your recent list
3) remove the porn url from recent list
4) close / reopen
5) pron url shows back up in recent list

That's what I was trying to avoid with the cross session tracking code.

Comment 51

[Jim Mathies [:jimm]]

(In reply to comment #49)
> (In reply to comment #47)
> > If a user removes an item, and we refresh the list, we have to track the items
> > they remove because windows forgets what's been removed as soon as we refresh.
> 
> MSDN remarks that "It is strongly recommended that an application clear any
> destination tracking data when the user elects to remove that destination. If
> the user accesses that destination again in the future, it may be re-added to
> the Jump List and can again accumulate data."
> <http://msdn.microsoft.com/en-gb/library/dd378401>
> 
> I would interpret that as meaning that the corresponding history entry (or
> whatever is the source of the item) ought to be removed. Has this been
> considered and would that not be desirable for some reason?

That's an option, but really, I didn't like this personally. Lets say I'm simply trying to get certain uri out of my favorites jump list. I may still the site in bookmarks and I still want the awesome bar to work for the site. I've done this harmlessly in using the feature, the minefield startup page for example tends to show up. If I delete that from the jump list, fx will add a new item in its place just down the list.

Comment 52

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

What do other browsers do? Removing the "visit"/history entry makes sense to me, and also would simplify things considerably. We don't have to touch bookmarks.

Comment 53

[A.]

5 items in Fx jumplist sames  a bit in the low , maybe you guy should consider 10 items list?

Comment 54

[Jim Mathies [:jimm]]

(In reply to comment #52)
> What do other browsers do? Removing the "visit"/history entry makes sense to
> me, and also would simplify things considerably. We don't have to touch
> bookmarks.

IE seems to follow the msdn convention. Chrome has this available in the dev channel. Unfortunatley while testing it, I think I broke their implementation by removing some items. The items I removed are still in history, but not sure if that is intended.

I have no problems with implementing the delete history functionality if that's what people prefer. It would simplify this code quit a bit.

Comment 55

[Jim Mathies [:jimm]]

(In reply to comment #53)
> 5 items in Fx jumplist sames  a bit in the low , maybe you guy should consider
> 10 items list?

(You can up it via prefs.) I didn't want to overload the ui with a bunch of junk. I think though this is something we can tweak down the road as people start to use this, so the feedback is appreciated.

Comment 56

[Jim Mathies [:jimm]]

Attachment: browser v.8

Ok, updated to remove removed item tracking and added clear removed items from history functionality.

Comment 57

[Dão Gottwald [:dao]]

Comment on attachment 404984 [details] [diff] [review]
browser v.8

> MODULE = wintaskbar
> 
> _WINTASKBAR_FILES = \
> 	preview-per-tab.jsm \
>+	winJumpLists.jsm \

Since this is in wintaskbar, remove the win prefix from the jsm file name.

>+/////////////////////////////////////////////////////////////////////////////

I must have missed it when you added this, please get rid of it again.
Use this style if you want to keep the headings:

/**
 * Foo
 */

>+const PREF_TASKBAR_REMLINKS  = "removedLinks";

This is now unused.

>+function getString(name) {
>+  return _stringBundle.GetStringFromName(name);
>+};

This is not a smart getter. Seems like you need a new heading. ;)
For consistency, you should probably call this function _getString. Also remove the stray semicolon at the end.

>+  _clearHistory: function WTBJL__clearHistory(items) {
>+    if (!items)
>+      return false;

Just return, no return value.

>+    try { // in case we get a bad uri
>+      var enum = items.enumerate();
>+      while (enum.hasMoreElements()) {
>+        let oldItem = enum.getNext().QueryInterface(Ci.nsIJumpListShortcut);
>+        if (oldItem) {
>+          let uriSpec = oldItem.app.getParameter(0);
>+          _browserHistoryService.removePage(
>+            _ioService.newURI(uriSpec, null, null));
>+        }
>+      }
>+    } catch (err) { }

You should only try/catch where you expect the exception, i.e. around _ioService.newURI. I'm not sure why you expect bad URIs, though, so maybe this isn't right at all.

You can spare the null arguments for newURI.

Use _navHistoryService.QueryInterface(Ci.nsIBrowserHistory) instead of _browserHistoryService (and remove the _browserHistoryService getter).

>+  /**
>+   * Misc. utils
>+   */
>+
>+  /*
>+  _debug: function (msg) {
>+    var aConsoleService = Cc["@mozilla.org/consoleservice;1"].
>+         getService(Ci.nsIConsoleService);
>+    aConsoleService.logStringMessage(msg);
>+  },
>+
>+  _error: function (err) {
>+    this._debug("jumplists error: " + err);
>+  },
>+  */

Any reason to still keep this around?

Comment 58

[Jim Mathies [:jimm]]

> >+    try { // in case we get a bad uri
> >+      var enum = items.enumerate();
> >+      while (enum.hasMoreElements()) {
> >+        let oldItem = enum.getNext().QueryInterface(Ci.nsIJumpListShortcut);
> >+        if (oldItem) {
> >+          let uriSpec = oldItem.app.getParameter(0);
> >+          _browserHistoryService.removePage(
> >+            _ioService.newURI(uriSpec, null, null));
> >+        }
> >+      }
> >+    } catch (err) { }
> 
> You should only try/catch where you expect the exception, i.e. around
> _ioService.newURI. I'm not sure why you expect bad URIs, though, so maybe this
> isn't right at all.

No you're right, I changed this to catch inside "if (oldItem) {" to prevent break outs in cases where we try to delete uri that are invalid. Extensions will be able to modify our jump list, so there's no way of knowing what getParameter(0) will return, but we want to try and complete so that _commitBuild can succeed.
 
> Use _navHistoryService.QueryInterface(Ci.nsIBrowserHistory) instead of
> _browserHistoryService (and remove the _browserHistoryService getter).
> 
> >+  /**
> >+   * Misc. utils
> >+   */
> >+
> >+  /*
> >+  _debug: function (msg) {
> >+    var aConsoleService = Cc["@mozilla.org/consoleservice;1"].
> >+         getService(Ci.nsIConsoleService);
> >+    aConsoleService.logStringMessage(msg);
> >+  },
> >+
> >+  _error: function (err) {
> >+    this._debug("jumplists error: " + err);
> >+  },
> >+  */
> 
> Any reason to still keep this around?

after an r+, it's gone. ;)

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #58)
> Extensions will be able to modify our jump list

Does that mean using data that's intentionally not a URI? Otherwise silently eating the exception doesn't seem useful, as the extension author will want to know about it.

Comment 60

[Dão Gottwald [:dao]]

Comment on attachment 404984 [details] [diff] [review]
browser v.8

>+  _buildRecent: function WTBJL__buildRecent() {

>+    let count = 0;
>+    try {
>+      list.forEach(function (entry) {
>+        let shortcut = this._getHandlerAppItem(entry.title, entry.title, entry.uri, 1);
>+        if (count > this._maxItemCount)
>+          throw "";
>+        // do not add items to recent that have already been added
>+        // to frequent.
>+        if (this._frequentHashList &&
>+            this._frequentHashList.indexOf(entry.uri) != -1)
>+          return;
>+        items.appendElement(shortcut, false);
>+        count++;
>+      }, this);
>+    } catch (err) { }

Ugh, I missed that one. Please make this a for (let i = 0;...) loop and break instead of throwing. (And continue instead of returning, consequentially.)

Comment 61

[Jim Mathies [:jimm]]

(In reply to comment #59)
> (In reply to comment #58)
> > Extensions will be able to modify our jump list
> 
> Does that mean using data that's intentionally not a URI? Otherwise silently
> eating the exception doesn't seem useful, as the extension author will want to
> know about it.

It's about clearing any valid uri, while also protecting against a situation where there's something else in the list unanticipated. Say for example after an extension is uninstalled and we need to start managing the list again.

Comment 62

[Jim Mathies [:jimm]]

Attachment: [checked-in] browser v.9

Comment 63

[Jim Mathies [:jimm]]

Comment on attachment 405014 [details] [diff] [review]
[checked-in] browser v.9

http://hg.mozilla.org/mozilla-central/rev/c21bf74e694b

Comment 64

[Jim Mathies [:jimm]]

rats, I left a return in the for( ... I need to update this landing.

Comment 65

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/4d5760a4f96f

Comment 66

[Mike Beltzner [:beltzner, not reading bugmail]]

Ts regressions are being examined in bug 520837 - not sure if that's right or not, as that's about Aero Peek in general, not specifically jumplists.

Comment 67

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

This does not seem to be working in today's nightly build.
No errors I see in Console2

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091007042048

Comment 68

[Marco Bonardo [:mak]]

i filed bug 521008 bug jumplists Ts regression, does not look deeply related to Aeropeek.

Comment 69

[Jim Mathies [:jimm]]

(In reply to comment #67)
> This does not seem to be working in today's nightly build.
> No errors I see in Console2
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091007
> Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091007042048

I have two problems here, one I can address here (import is importing winJumpLists.jsp, not jumpLists.jsm.) The other is that for some reason our installer isn't baking in the new win7 jsms (probably will be addressed in bug 520801.)

Comment 70

[Jim Mathies [:jimm]]

Attachment: [m-c checkin] browser glue touchup

Comment 71

[Jim Mathies [:jimm]]

Comment on attachment 405065 [details] [diff] [review]
[m-c checkin] browser glue touchup

Sorry dao, should have waited to update this today after more sleep.

Comment 72

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/bea32394861e

Comment 73

[Joe Drew (not getting mail)]

Uh, holy crap. That checkin fixed Ts.

Comment 74

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, I'd like to understand that a little bit better.

Comment 75

[Jim Mathies [:jimm]]

Attachment: jump list init perf improvement

Same as aero peek, testing in bug 520837 showed this shaves a couple msec off init times as it avoids the import on non-win7 desktops.

Comment 76

[Dão Gottwald [:dao]]

Comment on attachment 405217 [details] [diff] [review]
jump list init perf improvement

>+    if (WINTASKBAR_CONTRACTID in Cc &&
>+      Cc[WINTASKBAR_CONTRACTID].getService(Ci.nsIWinTaskbar).available) {

Indent the second line two spaces further.

Comment 77

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 405065 [details] [diff] [review]
[m-c checkin] browser glue touchup

a192=beltzner for this as per bug 520837 comment 12

Comment 78

[Dão Gottwald [:dao]]

(In reply to comment #77)
> (From update of attachment 405065 [details] [diff] [review])
> a192=beltzner for this as per bug 520837 comment 12

That code doesn't exist on 192.

Comment 79

[Jim Mathies [:jimm]]

Attachment: [m-c checkin] jumplist init perf improvement

updated patch for trunk.

Comment 80

[Jim Mathies [:jimm]]

Comment on attachment 405316 [details] [diff] [review]
[m-c checkin] jumplist init perf improvement

http://hg.mozilla.org/mozilla-central/rev/9df8361fa9ef

Comment 81

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 405065 [details] [diff] [review]
[m-c checkin] browser glue touchup

Oh, I must have misread Jim, I thought he said he wanted to land this on branch to help with Aero Peek, as well.

Comment 82

[Rimas Kudelis [use needinfo to ping]]

Jim: could you please add appropriate l10n notes to taskbar.dtd? Cause I had to find this  bug and see the screenshot to actually understand how to translate "Frequent" and "Recent". I think the note would spare a few minutes for other localizers.

Comment 83

[Rimas Kudelis [use needinfo to ping]]

Another thing: items in jumplist keep switching their positions, and that's totally not helpful. Should I file a new bug about it, or is it already being dealt with, or should I reopen this?

Comment 84

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #83)
> Another thing: items in jumplist keep switching their positions, and that's
> totally not helpful. Should I file a new bug about it, or is it already being
> dealt with, or should I reopen this?

Ouch, it's not actually the jumplist, but the tab list. I'd better go comment some other bug...

Comment 85

[Jim Mathies [:jimm]]

(In reply to comment #84)
> (In reply to comment #83)
> > Another thing: items in jumplist keep switching their positions, and that's
> > totally not helpful. Should I file a new bug about it, or is it already being
> > dealt with, or should I reopen this?
> 
> Ouch, it's not actually the jumplist, but the tab list. I'd better go comment
> some other bug...

bug 474056, or spin off a new one and link it to that.

Comment 86

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we want to get this in on 192 as well? It'd be late-l10n, I suppose :( We should have considered this for the set of additional strings that Axel said we could get.

Axel: have you already closed for strings on 192, or were you doing that on Monday?

Comment 87

[Axel Hecht [:Pike]]

I have announced the string freeze for past Friday.

Comment 88

[Jim Mathies [:jimm]]

(In reply to comment #86)
> Do we want to get this in on 192 as well? It'd be late-l10n, I suppose :( We
> should have considered this for the set of additional strings that Axel said we
> could get.
> 
> Axel: have you already closed for strings on 192, or were you doing that on
> Monday?

I really don't think we have the developer bandwidth to get this done for 3.6. We have unfinished regression work for tab previews on 1.9.2, unresolved issues for jump lists related to installer links, icons, and unrelated work for higher priorities like electrolysis and top crasher work. I would suggest we hold off on 3.6 and instead release in 3.7.