Starting with Windows 7, the taskbar allows an application to expose its tabbed interface in the taskbar by showing thumbnail previews rather than the default window preview. Additionally, when a user hovers over a thumbnail (tab or window), they are shown a live preview of the window (or tab + its containing window). In contrast to Firefox this isn't used by SeaMonkey and since I became accustomed quite fast to this feature I think SeaMonkey should get it.

The code mainly stems from the files implementing this feature for Firefox, i.e. browser/base/content/tabbrowser.xml, browser/base/content/browser.js and browser/modules/WindowsPreviewPerTab.jsm and has been adapted to work with SeaMonkey.

[This is not a review] There are \t tab characters in your patch. Please use spaces only. > - content/navigator/navigator.js > +* content/navigator/navigator.js Please do not pre-process navigator.js. > +XPCOMUtils.defineLazyGetter(window, "Win7Features", function () Just use defineLazyModuleGetter() here. > +XPCOMUtils.defineLazyGetter(window, "Win7Features", function () A bit of a misnomer since this should also work on Windows8. > +{ > +#ifdef XP_WIN You do not need this ifdef. Testing for the existence of: "WINTASKBAR_CONTRACTID in Components.classes" is enough. > + const WINTASKBAR_CONTRACTID = "@mozilla.org/windows-taskbar;1"; I would just inline "@mozilla.org/windows-taskbar;1" instead of creating a const. > + if (WINTASKBAR_CONTRACTID in Components.classes && > + Components.classes[WINTASKBAR_CONTRACTID] > + .getService(Components.interfaces.nsIWinTaskbar) > + .available) { > + let temp = {}; > + Components.utils.import("resource:///modules/WindowsPreviewPerTab.jsm", temp); You seem to be copying an older version of the Firefox code. Please see Firefox Bug 836470 (Clean up JS module imports that use a temporary scope object) for recent changes to this code. > + <method name="previewTab"> There is a tab somewhere on this line. ... to be continued...

> +this.EXPORTED_SYMBOLS = ["AeroPeek"]; This is a B2G specific idiosyncrasy. See Bug 798491. Since this is preffed on only for B2G this change is unnecesary and should be reverted back to: var EXPORTED_SYMBOLS = ["AeroPeek"];

> +this.EXPORTED_SYMBOLS = ["AeroPeek"]; This is a B2G specific idiosyncrasy. See Bug 798491. Since this is preffed on only for B2G this change is unnecesary and should be reverted back to: var EXPORTED_SYMBOLS = ["AeroPeek"]; > + winEvents: ["tabviewshown", "tabviewhidden"], We don't have Panorama (Tab Candy) so all those changes needed to support Panorama can be removed.

(In reply to Philip Chee from comment #2) Thanks for taking a look. I already addressed all other issues you raised except: > > +XPCOMUtils.defineLazyGetter(window, "Win7Features", function () > Just use defineLazyModuleGetter() here. Wouldn't it be overkill creating a new module and importing it for those two small functions onOpenWindow() and onCloseWindow()? > > +XPCOMUtils.defineLazyGetter(window, "Win7Features", function () > A bit of a misnomer since this should also work on Windows8. It does - I'm using Windows 8 for development right now. It's a Win 7 feature that's also existent in Windows 8 and I find it quite handy in favour of a full describing name like WindowsPreviewPerTab.

> Wouldn't it be overkill creating a new module and importing it for those two small > functions onOpenWindow() and onCloseWindow()? You might be right. I'm going to ask Neil for some feedback.

Comment on attachment 712264 [details] [diff] [review] Patches to make SeaMonkey use Win7 taskbar previews. >+XPCOMUtils.defineLazyGetter(window, "Win7Features", function () This doesn't need to be lazy, because we need to call it during startup anyway. I can think of at least three ways you could implement this: 1. Do the availability dance on startup and shutdown. Simplest, but duplicates code. 2. Define a global AeroPeek variable to null, and make an availability check before loading the module into the global scope, assuming this will overwrite the global. Then null-check AeroPeek on startup and shutdown. 3. Define a dummy AeroPeek object with empty onOpenWindow and onCloseWindow functions, which gets overwritten as in option 2, however now you don't need to null-check AeroPeek. >+ let AeroPeek = temp.AeroPeek; >+ return { >+ onOpenWindow: function () { >+ AeroPeek.onOpenWindow(window); >+ }, >+ onCloseWindow: function () { >+ AeroPeek.onCloseWindow(window); >+ } >+ }; This was the worst bit - it should have just returned AeroPeek directly and the caller could have passed the window in. >+ <method name="previewTab"> I can't work out what this is for - updateCanvasPreview already uses the linked browser's content window, so I don't see why it needs to be the current tab. >diff --git a/suite/modules/WindowsPreviewPerTab.jsm b/suite/modules/WindowsPreviewPerTab.jsm To be continued...

Comment on attachment 712264 [details] [diff] [review] Patches to make SeaMonkey use Win7 taskbar previews. I guess this is basically a copy of Firefox's WindowsPreviewPerTab.jsm? >+XPCOMUtils.defineLazyServiceGetter(this, "ioSvc", >+ "@mozilla.org/network/io-service;1", >+ "nsIIOService"); [Should import Services and use Services.io] >+ try { >+ channel.QueryInterface(Ci.nsIPrivateBrowsingChannel); >+ channel.setPrivate(privateMode); >+ } catch (e) { >+ // Ignore channels which do not support nsIPrivateBrowsingChannel >+ } if (channel instanceof Ci.nsIPrivateBrowsingChannel) channel.setPrivate(privateMode); >+ // Cannot perform the lookup during construction. See TabWindow.newTab >+ XPCOMUtils.defineLazyGetter(this, "preview", function () this.win.previewFromTab(this.tab)); Bah, this code is a mess. The controller wants to know the XPCOM preview object associated with it and vice versa. So they've come up with this really funky ownership model where the controller asks the tabwindow for the preview... >+ let rects = []; >+ for (let i = 0; i < rectstream.length; i+= 4) { >+ let r = {x: rectstream[i], >+ y: rectstream[i+1], >+ width: rectstream[i+2], >+ height: rectstream[i+3]}; >+ rects.push(r); >+ } I tried to come up with a nicer way of writing this but the best I could do was var rects = []; while (rectstream.length) { var r = {}; [ r.x, r.y, r.width, r.height ] = rectstream.splice(0, 4); rects.push(r); } >+ let title = this.win.tabbrowser.updateTitlebar(this.linkedBrowser); The plot thickens! Instead of abusing updateTitlebar, it needs to be refactored into a separate method that both updateTitlebar and this can call. >+ case "TabAttrModified": >+ this.updateTitleAndTooltip(); Wrong event, this needs to be done on a DOM title change, like the way the tabbrowser does it in the first place... >+XPCOMUtils.defineLazyGetter(PreviewController.prototype, "canvasPreviewFlags", >+ function () { let canvasInterface = Ci.nsIDOMCanvasRenderingContext2D; >+ return canvasInterface.DRAWWINDOW_DRAW_VIEW >+ | canvasInterface.DRAWWINDOW_DRAW_CARET >+ | canvasInterface.DRAWWINDOW_ASYNC_DECODE_IMAGES >+ | canvasInterface.DRAWWINDOW_DO_NOT_FLUSH; >+}); Well, that was futile. It should be a global constant... >+ winEvents: ["tabviewshown", "tabviewhidden"], Not sure what these events are... >+ set enabled (enable) { >+ this._enabled = enable; >+ // Because making a tab visible requires that the tab it is next to be >+ // visible, it is far simpler to unset the 'next' tab and recreate them all >+ // at once. >+ this.previews.forEach(function (preview) { >+ preview.move(null); >+ preview.visible = enable; >+ }); >+ this.updateTabOrdering(); Hmm, I can't see updateTabOrdering working when the previews aren't enabled... >+ // Since the internal taskbar array has not yet been updated we must force >+ // on it the sorting order of our local array. To do so we must walk >+ // the local array backwards, otherwise we would send move requests in the >+ // wrong order. See bug 522610 for details. >+ for (let i = this.previews.length - 1; i >= 0; i--) { >+ let p = this.previews[i]; >+ let next = i == this.previews.length - 1 ? null : this.previews[i+1]; >+ p.move(next); >+ } Snazzy version using reduceRight: this.previews.reduceRight(function(next, p) { p.move(next); return p; }, null); >+ case "TabMove": >+ let oldPos = evt.detail; >+ let newPos = this.tabbrowser.tabContainer.getIndexOfItem(tab); >+ let preview = this.previews[oldPos]; >+ this.previews.splice(oldPos, 1); >+ this.previews.splice(newPos, 0, preview); Not convinced this works for moving tabs to the right... >+ initialize: function () { >+ if (!(WINTASKBAR_CONTRACTID in Cc)) >+ return; >+ this.taskbar = Cc[WINTASKBAR_CONTRACTID].getService(Ci.nsIWinTaskbar); >+ this.available = this.taskbar.available; >+ if (!this.available) >+ return; Interesting, so your caller doesn't have to check this after all! >+ if (this.previews.length > this.maxpreviews) >+ this.enabled = false; >+ else >+ this.enabled = this._prefenabled; Could write this as this.enabled = this._prefenabled && this.previews.length < this.maxpreviews; >+ case "timer-callback": I learned something new today! >+XPCOMUtils.defineLazyServiceGetter(AeroPeek, "prefs", >+ "@mozilla.org/preferences-service;1", >+ "nsIPrefBranch"); [Services.prefs]

(In reply to comment #8) >(From update of attachment 712264 [details] [diff] [review]) >>+ case "TabMove": >>+ let oldPos = evt.detail; >>+ let newPos = this.tabbrowser.tabContainer.getIndexOfItem(tab); >>+ let preview = this.previews[oldPos]; >>+ this.previews.splice(oldPos, 1); >>+ this.previews.splice(newPos, 0, preview); >Not convinced this works for moving tabs to the right... Don't panic, I'm more awake today and this does work.

(In reply to comment #8) >(From update of attachment 712264 [details] [diff] [review]) >>+ let title = this.win.tabbrowser.updateTitlebar(this.linkedBrowser); >The plot thickens! Instead of abusing updateTitlebar, it needs to be >refactored into a separate method that both updateTitlebar and this can call. I see the Firefox's latest version of WindowsPreviewPerTab.jsm does this now.

(In reply to comment #7) >(From update of attachment 712264 [details] [diff] [review]) >>+ <method name="previewTab"> >I can't work out what this is for - updateCanvasPreview already uses the >linked browser's content window, so I don't see why it needs to be the >current tab. Seems to be so that the tabstrip can be drawn appropriately.

OK, I think I've got it working to my satisfaction :-) Hopefully all of my requestees run Windows 7 or later!

Comment on attachment 722551 [details] [diff] [review] Proposed patch Thanks Neil for taking this. I was able to fiddle through the Firefox code and make it work but almost never seen Mozillas browser code I couldn't do better. I did test your patch now and it works as far as I can see - with one exception. The exception is that toggling browser.taskbar.previews.enable doesn't have an immediate effect on existing tabs like it has on Firefox and I would expect. I.e. previews displayed won't go away when disabled resp. previews won't generated for existing tabs when enabling.

Comment on attachment 722551 [details] [diff] [review] Proposed patch >+ for (let [, win] of windows) I see the problem. This should say: for (let [, win] of this.windows)

Comment on attachment 722551 [details] [diff] [review] Proposed patch Yep, that does the trick. Thanks.

Comment on attachment 727856 [details] [diff] [review] Updated for bitrot and feedback f=me since this works. But patch needs some tweaks. > +++ b/suite/browser/tabbrowser.xml > - this.tabContainer._handleNewTab(t); ... > + this.tabContainer._handleNewTab(t); > + > + t.dispatchEvent(new Event("TabOpen", > + { bubbles: true, cancelable: false })); Why did you move _handleNewTab() from addTab() to restoreTab()? > +++ b/suite/modules/WindowsPreviewPerTab.jsm Three instances of trailing whitespace somewhere in the comments. > const CRC2D = Components.interfaces.nsIDOMCanvasRenderingContext2D; > const canvasPreviewFlags = CRC2D.DRAWWINDOW_DRAW_VIEW | > CRC2D.DRAWWINDOW_DRAW_CARET | > CRC2D.DRAWWINDOW_ASYNC_DECODE_IMAGES | > CRC2D.DRAWWINDOW_DO_NOT_FLUSH; According to: http://mxr.mozilla.org/comm-central/source/mozilla/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl#26 > 29 * used in WindowsPreviewPerTab.jsm and some extensions. The constants can > 30 * be referenced directly via a canvas context 2d rather than this interface, > 31 * and that should be preferred in new code. So probably we could do something like: http://hg.mozilla.org/mozilla-central/annotate/631d57b31bb1/layout/tools/reftest/reftest-content.js#l637 > let out_img = { value: null }; > imgTools.decodeImageData(inputStream, channel.contentType, out_img); decodeImageData is deprecated. Instead use imgTools.decodeImage. out_img = imgTools.decodeImage(inputStream, channel.contentType); Advantage: no need for outparam. I *think* the try/catch block isn't necessary since we always pass a proper iconurl or the default favicon. > _imageFromURI(defaultURI, callback); Doesn't _imageFromURI() take three parameters? I see this comes from http://hg.mozilla.org/mozilla-central/rev/7c3acad5cf39 Bug 788275 - Tab preview should respect the private browsing mode when attempting to load a favicon. I wonder why Ehsan missed this. > rects.push(r); > > } Please remove the blank line above. > // Length of time in seconds that previews are cached > cacheLifespan: 20, s/20/5/ please. > this.cacheTimer.init(this, 1000*this.cacheLifespan, Nit: a space before and after "*". > drawPreview: function (ctx) { > let win = this.win; > let tabbrowser = win.tabbrowser; > tabbrowser.mAeroPeek = true; > let tab = tabbrowser.selectedTab; > tabbrowser.selectedTab = this.tab; > ctx.drawWindow(win.win, 0, 0, win.width, win.height, "transparent"); https://developer.mozilla.org/en-US/docs/DOM/CanvasRenderingContext2D#drawWindow() > If 'rgba(0,0,0,0)' is used for the background color, the drawing will be > transparent wherever the window is transparent. -- Top-level browsed > documents are usually not transparent because the user's background-color > preference is applied, but IFRAMEs are transparent if the page doesn't set a > background. -- If an opaque color is used for the background color, > rendering will be faster because we won't have to compute the window's > transparency. Which is of course why we use |mozOpaque = true;| in canvasPreview() and in other parts of this file we use "white". So do we need to be consistent here or am I just going down a rabbit hole?

How does one test this feature? browser.taskbar.previews.enable is set to true, but I only see the normal window preview in the Windows taskbar (works fine with FF).

> How does one test this feature? You need more than one tab.

(In reply to Philip Chee from comment #17) > (From update of attachment 727856 [details] [diff] [review]) > > - this.tabContainer._handleNewTab(t); > Why did you move _handleNewTab() from addTab() to restoreTab()? I didn't intend to. Fixed. > > +++ b/suite/modules/WindowsPreviewPerTab.jsm > Three instances of trailing whitespace somewhere in the comments. Fixed. (This file was mostly copied from Firefox, and then the worst abuses fixed.) > > const CRC2D = Components.interfaces.nsIDOMCanvasRenderingContext2D; > > const canvasPreviewFlags = CRC2D.DRAWWINDOW_DRAW_VIEW | > > CRC2D.DRAWWINDOW_DRAW_CARET | > > CRC2D.DRAWWINDOW_ASYNC_DECODE_IMAGES | > > CRC2D.DRAWWINDOW_DO_NOT_FLUSH; Fixed. > > let out_img = { value: null }; > > imgTools.decodeImageData(inputStream, channel.contentType, out_img); > decodeImageData is deprecated. Instead use imgTools.decodeImage. Fixed. > > _imageFromURI(defaultURI, callback); > Doesn't _imageFromURI() take three parameters? Fixed. > > rects.push(r); > > > > } > Please remove the blank line above. Fixed. > > // Length of time in seconds that previews are cached > > cacheLifespan: 20, > s/20/5/ please. Fixed. > > this.cacheTimer.init(this, 1000*this.cacheLifespan, > Nit: a space before and after "*". Fixed. > > ctx.drawWindow(win.win, 0, 0, win.width, win.height, "transparent"); > So do we need to be consistent here or am I just going down a rabbit hole? Fixed. I don't think it's possible for the window to be transparent.

(In reply to Philip Chee from comment #19) > > How does one test this feature? > You need more than one tab. I had too many tabs open, that is why it didn't work :)

Comment on attachment 730999 [details] [diff] [review] Updated for feedback Closing a window (or better said: A window with one tab) with the Windows close button in the taskbar (the red X) one tab does not work with this patch. If you open two windows and try to close one of those with said close button, I get this message in the Error Console: Error: TypeError: p is undefined Source File: resource:///modules/WindowsPreviewPerTab.jsm Line: 448 And now that I look at the Firefox version of WindowsPreviewPerTab.jsm it looks like you forgot to copy/add this line: let next = i == this.previews.length - 1 ? null : this.previews[i+1];

(In reply to Frank Wein from comment #23) > Error: TypeError: p is undefined > Source File: resource:///modules/WindowsPreviewPerTab.jsm > Line: 448 > > And now that I look at the Firefox version of WindowsPreviewPerTab.jsm it > looks like you forgot to copy/add this line: > let next = i == this.previews.length - 1 ? null : this.previews[i+1]; Irrelevant, because it's p that's undefined, not next.

(In reply to Frank Wein from comment #23) > Closing a window (or better said: A window with one tab) with the Windows > close button in the taskbar (the red X) one tab does not work with this patch. The close button closes tabs, not windows. But actually I get the exception whenever I close a tab, because I need to skip the tab that's being closed. The Firefox code doesn't need this because it has a separate array of previews where the closed tab has already been removed.

Q.v. Bug 857061 - [HiDPI] Hovered over Taskbar Fx icon thumbnail, chrome contents and window appears smaller https://hg.mozilla.org/mozilla-central/rev/2fbc1a6c7461

q.v. Bug 874635 Intermittent browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js,browser_cached_messages.js | uncaught exception - TypeError: preview is undefined at resource://app/modules/WindowsPreviewPerTab.jsm:371

Comment on attachment 747306 [details] [diff] [review] Fixed patch r+, but check Comment 27 and Comment 28 if those fixes should be included in the final patch, too. I'm still not happy that this feature closes tabs only (and not the whole window when you close the last tab in a window), but that does not prevent r+ for now. With the default setting in SeaMonkey, you cannot close the last tab only, you can only close the whole window. Of course you can change the setting though to display the tabbar when only one tab is open, so that you can close that last tab separately from the window.

Pushed comm-central changeset 0c3b85a1c1f7.

Work done in this bug appears to be causing bugs 921874 and bug 970993. Both of those bugs resolve when setting browser.taskbar.previews.enable=false.

Work done in this bug also appears to be causing bug 998171. Setting browser.taskbar.previews.enable=false also fixes the issue.