Related to bug 474054 where we show a preview of the tabs, but this is to actually show the full size contents of the actual tab (incase another tab is actually focused in that window). This is supposed to be just a preview, so we probably don't want to actually switch tabs for the user when the hover leaves the tab preview without selecting a tab.

It looks like ITaskbar3::RegisterTab wants a native window for every tab. I don't think we currently do this.

So we can't just show the window (native window?) containing the tab and internally switch the current tab to be the desired tab?

In my opinion it should be possible to deactivate this. I hate this feature in IE because I don't want to click on the taskbar item first, look for my last used tab and click again on it. I just want to click on the icon and have the Firefox window in front of me! Maybe it would be possible to list the opened tabs in the jump list?

(In reply to comment #3) > In my opinion it should be possible to deactivate this. I hate this feature in > IE because I don't want to click on the taskbar item first, look for my last > used tab and click again on it. I just want to click on the icon and have the > Firefox window in front of me! From my limited usage of IE8 on Windows 7, I agree. Tab switching should be left to the application or at least be optional. However, I believe your comment would be more suitable for bug 474054.

That's why it can be disabled via the internet options for IE and I would do that if I use it. But I think it would be good to have that option for those who like it.

Changing the name to better reflect the intention to create a front end feature (bug 501490 adds the backend support).

First RC. There is still a bug when activating a minimized window's tabs but I'm not sure where the bug is for that (could be in the backend).

Rob, maybe we should change the aeropeek component to something generic like taskbar? I'm no longer in need of something in mozapps, but now I'll need a place up in browser for jump lists.

How does wintaskbar sound? It's consistent with the interface name.

(In reply to comment #10) > How does wintaskbar sound? It's consistent with the interface name. WFM!

Dao may be interested in reviewing this.

Why are you selecting the tab to preview it? I don't think we should do that, given the tabbrowser.xml hacks it implies.

Is the expected UI documented somewhere, btw? I'm having problems getting any previews from IE8 on Win7 RC1.

With IE it should look like http://res1.windows.microsoft.com/resbox/en/Windows%207/Main/3/8/385c8434-d335-4ce5-8925-7d2357ba9ca4/385c8434-d335-4ce5-8925-7d2357ba9ca4.jpg You may not see the previews if you don't have Aero turned on (which can happen if you're running Win7 in a vm where there is no hardware acceleration).

Right, this is a VM :(

(In reply to comment #13) > Why are you selecting the tab to preview it? I don't think we should do that, > given the tabbrowser.xml hacks it implies. Rob and I discussed this. I think it's the only way to achieve the affect that we want. In particular, we wanted most of the browser UI to be displayed as though the tab was selected (back/forward state, selected tab in the tab bar, URL bar value, etc.). Alternate suggestions would be welcome, though.

Ok, now that I've watched a video on aero peek, I understand the desired effect. I guess we could live with maintaining that mode in tabbrowser. The TabSelect shouldn't be dispatched, though. Otherwise you would have to update the code that's listening to that event, and extensions would have to do that as well.

Yeah, that makes sense.

I didn't know if extensions might want to update their UI depending on the selected tab. I think we'd want to preview that.

Perhaps I'm mis-reading, but what good is the Aero-tab-Preview, if clicking on the tab doesn't select the highlighted tab ? Some may not want to inconvience of the extra click, or clicking on the task-bar, but there are those that will use the feature. Just displaying a panel of tabs however, and not being able to select that tab is a waste of coding efforts IMO. If the intent is to only display the tab, then there needs to be a way to 'kill' the feature as its useless.

(In reply to comment #20) > I didn't know if extensions might want to update their UI depending on the > selected tab. I think we'd want to preview that. I think we can live with not previewing that, for the sake of not breaking other stuff. There are definitely TabSelect-dependent actions that should not happen in this case, comparable to the stuff that you made mInPreview-dependent.

(In reply to comment #21) > Perhaps I'm mis-reading, but what good is the Aero-tab-Preview, if clicking on > the tab doesn't select the highlighted tab ? Clicking on the tab will select the tab. Previewing a tab will not leave that tab selected however. This feature operates the same way that it does for IE8 with exception that we update the browser chrome when previewing a tab. > If the intent is to only display the tab, then there needs to be a way to > 'kill' the feature as its useless. There is a hidden pref, aeropeek.enable, to turn the feature off. No restart required either. (In reply to comment #22) > I think we can live with not previewing that, for the sake of not breaking > other stuff. There are definitely TabSelect-dependent actions that should not > happen in this case, comparable to the stuff that you made > mInPreview-dependent. Ok, sounds fine then.

I find it very interesting bug 465076 to implement Ctrl+Tab view is almost the same functionality built directly into the browser.

I'd recommend mentioning the fake tab selections in the dev docs (even if you don't send TabSelect events, some extensions may get confused by this).

Now with more comments and (hopefully) better style. Moved from browser/components/aeropeek to browser/components/wintaskbar. Simplified some code (thumbnails are now drawn from the previews) and other minor polish work.

Needed for Windows 7 parity with Chrome, IE and Safari

Comment on attachment 393028 [details] [diff] [review] v1.5 No tests yet, but with the upcoming code freeze I'd like some review so that this might make it in for beta.

Comment on attachment 393028 [details] [diff] [review] v1.5 Note that I cannot test this, since I have Win 7 only on a VM. So I don't feel comfortable being the only reviewer. You're putting modules in browser/components/. You should probably use browser/modules/ instead. >+ <field name="mInPreview"> >+ false >+ </field> >+ >+ <property name="previewmode"> >+ <getter> >+ return this.mInPreview; >+ </getter> >+ <setter> >+ <![CDATA[ >+ this.mInPreview = val; >+ return val; >+ ]]> >+ </setter> >+ </property> Just make this one public field, previewMode. No need for a private field and a public property. >+//// WinTaskbarHelper >+ >+/* >+ * This object is for helping clients of the nsIWinTaskbar initialize >+ * themselves. The taskbar service is not initially available - it requires >+ * Windows to notify us when it's ready to accept API calls. Are we going to have multiple nsIWinTaskbar clients? >+var WinTaskbarHelper = { >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]), I don't think you need to implement QueryInterface here. >+++ b/browser/components/wintaskbar/preview-per-tab.jsm >+// Pref to enable/disable preview-per-tab >+const kTogglePrefName = "aeropeek.enable"; constants should be uppercase without the k prefix, e.g. TOGGLE_PREF_NAME >+//// Various utility functions >+var Util = { >+ get ioSvc () { >+ delete this.ioSvc; >+ return this.ioSvc = Cc["@mozilla.org/network/io-service;1"] >+ .getService(Ci.nsIIOService); >+ }, [...] I'm not sure that the Util wrapper is useful, you could just dump this stuff in the global scope. E.g. XPCOMUtils.defineLazyServiceGetter(this, "_ioService", "@mozilla.org/network/io-service;1", "nsIIOService"); ... function _imageFromURI(uri) { ... } >+var PreviewController = function (win, tab) { >+ this.win = win.QueryInterface(Ci.nsIDOMWindowInternal);; >+ this.tab = tab; >+ let linkedBrowser = tab.linkedBrowser; >+ this.tabbrowser = linkedBrowser.getTabBrowser(); >+ this.shell = win.gBrowser.docShell; Use win.gBrowser instead of linkedBrowser.getTabBrowser(). >+ this.canvasPreview = >+ doc.createElementNS("http://www.w3.org/1999/xhtml", "html:canvas"); Just "canvas", without "html:". >+ get wrappedJSObject() { >+ return this; >+ }, I'm not sure I understand your use of wrappedJSObject. Can you just get rid of it? >+ onTabPaint: function (rect) { >+ // Ignore spurious dirty rects >+ if (!rect.width || !rect.height) return; \n before return >+ onLocationChange: function (aBrowser, webProgress, request, location) { >+ }, >+ onProgressChange: function (aBrowser, webProgress, request, curSelfProgress, >+ maxSelfProgress, curTotalProgress, >+ maxTotalProgress) { >+ }, >+ onSecurityChange: function (aBrowser, aWebProgress, aRequest, aState) { >+ }, >+ onStateChange: function (aBrowser, aWebProgress, aRequest, >+ aStateFlags, aStatus) { >+ }, >+ onStatusChange: function (aBrowser, aWebProgress, aRequest, >+ aStatus, aMessage) { >+ }, You can spare the argument lists. >+ onRefreshAttempted: function (aBrowser, webProgress, aRefreshURI, >+ aMillis, aSameURI) { >+ return true; >+ }, You don't need to implement onRefreshAttempted. >+TabWindow.prototype = { >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMEventListener]), Also not needed. >+ // nsIDOMEventListener >+ handleEvent: function (evt) { >+ switch(evt.type) { >+ case "TabOpen": >+ case "TabClose": >+ case "TabSelect": >+ case "TabMove": >+ let tab, preview; >+ tab = evt.originalTarget; >+ preview = AeroPeek.previews[tab]; Move this up and reduce the two switches to one? >+ case "TabMove": >+ let controller = preview.controller.wrappedJSObject; >+ let idx = Array.indexOf(controller.tabbrowser.mTabs, tab); >+ let next = null >+ // Check if there is a next preview >+ if (idx+1 < controller.tabbrowser.mTabs.length) { >+ next = AeroPeek.previews[controller.tabbrowser.mTabs[idx+1]]; >+ } use tab.nextSibling instead of controller.tabbrowser.mTabs[idx+1] >+var EXPORTED_SYMBOLS = ["AeroPeek"] I think this belongs to the top of the file. Also add a semicolon.

Comment on attachment 393028 [details] [diff] [review] v1.5 >+function TabWindow(win) { >+ for each (let evtName in this.events) { >+ tabbrowser.addEventListener(evtName, this, false); add the event listeners on tabbrowser.tabContainer Also please drop the curly brackets around single lines...

Notable changes since last patch: Review comments addressed (dao's and others from IRC/email). init.jsm removed due to underlying API changes in bug 501490's latest patch. Large block of module documentation in preview-per-tab.jsm. Feature changes: Time-based tab content cache expiration. Magic number based auto-disabling (see large comment in preview-per-tab.jsm). Implementation changes: Removed global mapping of <tab> to nsITaskbarPreview. There is now a mapping in each TabWindow of <tab> to nsITaskbarPreview via the tab position. The ChromeWindow to TabWindow mapping is now done via property of the ChromeWindow (used only in the case of a closing window). Lots more lazy getters to avoid new window/tab performance regressions. Dirty regions are now clipped to the canvas area to avoid some spurious rectangles causing drawWindow to throw. Tab ordering refreshed for all tabs in a window after move/open/close operations. General code cleanup and simplification. Tests now included. Probably a few other things I've forgotten to mention here.

Comment on attachment 404560 [details] [diff] [review] v2.0 Switching review to Rob Strong per conversation with Dão on IRC.

Rob, we need to sync up on where this code lands. I've currently got: components/taskbar with make clipping out the taskbar dir in browser/components/Makefile.in. The install location I have is dist/bin/modules/(jsm files). Any issues with that?

Comment on attachment 404560 [details] [diff] [review] v2.0 >+ <field name="previewmode"> nit: previewMode >+DIRS += wintaskbar Jim uses "taskbar"... Please make sure that you're on the same page. >+// Pref to enable/disable preview-per-tab >+const TOGGLE_PREF_NAME = "aeropeek.enable"; >+// Pref to determine the magic auto-disable threshold >+const DISABLE_THRESHOLD_PREF_NAME = "aeropeek.maxpreviews"; >+// Pref to control the time in seconds that tab contents live in the cache >+const CACHE_EXPIRATION_TIME_PREF_NAME = "aeropeek.cachetime"; Did you intentionally not add default values for these prefs? It seems like you should (and then don't try/catch when you read them). +function PreviewController(win, tab) { + win.tabbrowser.addTabsProgressListener(this); Can you do this only once per window? >+ updateCanvasPreview: function () { >+ let win = this.linkedBrowser.contentWindow.QueryInterface(Ci.nsIDOMWindowInternal); You don't seem to be using nsIDOMWindowInternal. >+ case "DOMTitleChanged": >+ // The tab's label is sometimes empty when dragging tabs between windows >+ let htmldoc = evt.originalTarget.defaultView.top.document; >+ let title = htmldoc.title || '(Untitled)'; You shouldn't hardcode '(Untitled)'. Perhaps calling tabbrowser.setTabTitle(tab) fixes the issue? And add a comment with a bug number for the empty label bug. >+ onLinkIconAvailable: function (aBrowser) { >+ if (aBrowser != this.tab.linkedBrowser) >+ return; >+ let img = getFaviconAsImage(aBrowser.mIconURL); >+ this.preview.icon = img add a semicolon >+XPCOMUtils.defineLazyGetter(PreviewController.prototype, "canvasPreviewFlags", >+ function () { let canvasInterface = Ci.nsIDOMCanvasRenderingContext2D; >+ return canvasInterface.DRAWWINDOW_DRAW_VIEW >+ | canvasInterface.DRAWWINDOW_DRAW_CARET >+ | canvasInterface.DRAWWINDOW_DO_NOT_FLUSH; >+}); This is only used once, and you have a context there, so you could probably just do this: let flags = ctx.DRAWWINDOW_DRAW_VIEW | ctx.DRAWWINDOW_DRAW_CARET | ctx.DRAWWINDOW_DO_NOT_FLUSH; instead of: let flags = this.canvasPreviewFlags; >+ handleEvent: function (evt) { >+ let tab = evt.originalTarget; >+ switch(evt.type) { nit: add a space after 'switch' >+function TabWindow(win) { >+ this.win = win.QueryInterface(Ci.nsIDOMWindowInternal); Another useless QueryInterface? Otherwise looks good to me code-wise, but I can't test this, so you should definitely try to get a second review.

Comment on attachment 404560 [details] [diff] [review] v2.0 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+#ifdef AERO_PEEK >+ Cu.import("resource://gre/modules/wintaskbar/preview-per-tab.jsm"); >+ >+ AeroPeek.onOpenWindow(window); >+#endif > } > > function BrowserShutdown() > { >+#ifdef AERO_PEEK >+ AeroPeek.onCloseWindow(window); >+#endif I'd like to see on(Open|Close)Window wrapped in something that can handle the .available logic and not bother importing the module if we're on an unsupported windows version. >diff --git a/browser/components/wintaskbar/preview-per-tab.jsm b/browser/components/wintaskbar/preview-per-tab.jsm >+ * This module implements the front end behavior for AeroPeek. Starting in >+ * Windows Vista, the taskbar begin showing live thumbanil previews of windows nit: began/thumbnail >+var EXPORTED_SYMBOLS = ["AeroPeek"] nit: missing semicolon >+Cu.import("resource://gre/modules/NetUtil.jsm"); >+//// Various utility properties >+XPCOMUtils.defineLazyServiceGetter(this, "ioSvc", >+ "@mozilla.org/network/io-service;1", >+ "nsIIOService"); Purely followup fodder: NetUtil now has an ioService property, but it's only on trunk. >+function _imageFromURI(uri) { >+ let channel = ioSvc.newChannelFromURI(uri); >+ >+ let out_img = { value: null }; >+ let inputStream = channel.open(); >+ try { >+ imgTools.decodeImageData(inputStream, channel.contentType, out_img); I noticed a couple sites (BBC, f.e.) that trip this up by serving their favicon with a non-image-y MIME type. Maybe we want to attempt (in a followup) overriding the server's type with image/x-icon or falling back to asking the favicon service. >+TabWindow.prototype = { >+ _enabled: false, >+ events: ["TabOpen", "TabClose", "TabSelect", "TabMove"], Probably want to listen for and invalidate the taskbar previews on window resizes as well. When I was testing this, only the active tab preview was invalidated, leaving differently sized thumbnails that changed size on hover. Looks good!

Updated per Dão and Ryan's comments.

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/a949266351ca Pushed to mozilla-1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7fe92f6dc3dc

This fixes the mochitests when run on machines that aren't Windows 7. Wasn't caught by earlier run through try server because previous versions of the patch always had AeroPeek defined.

i've pushed this additional changeset on central to fix persistent timeout http://hg.mozilla.org/mozilla-central/rev/d6214cab6ad5 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d889630eac9d

While I was playing around with the jump lists patch, I noticed this error show up in the error console: Error: win is not defined Source File: file:///C:/Mozilla/Firefox/473045/mozilla/BUILD-DEBUG/dist/bin/modules/wintaskbar/preview-per-tab.jsm Line: 399

(In reply to comment #40) > While I was playing around with the jump lists patch, I noticed this error show > up in the error console: > > Error: win is not defined > Source File: > file:///C:/Mozilla/Firefox/473045/mozilla/BUILD-DEBUG/dist/bin/modules/wintaskbar/preview-per-tab.jsm > Line: 399 and this one: Error: preview is undefined Source File: file:///C:/Mozilla/Firefox/473045/mozilla/BUILD-DEBUG/dist/bin/modules/wintaskbar/preview-per-tab.jsm Line: 432 This error seems to happen if you have other windows besides the main browser window open and you close the browser window. I haven't been able to reproduce the first yet.

running W7 RTM and I do not get taskbar thumbnail previews for individual tabs. And btw, Aeropeek is the ability in W7 to hover over the box at the very right of the taskbar to make all windows transparent in order to see the desktop. It has nothing to do with taskbar thumbnail previews.

Both errors fixed - this should have been caught before checkin.

(In reply to comment #42) > running W7 RTM and I do not get taskbar thumbnail previews for individual tabs. How many tabs do you have? If you have more than 20, the preview-per-tab feature is disabled to avoid cluttering your UI. Change aeropeek.maxpreviews to modify the threshold. > And btw, Aeropeek is the ability in W7 to hover over the box at the very right > of the taskbar to make all windows transparent in order to see the desktop. It > has nothing to do with taskbar thumbnail previews. The Engineering Windows 7 blog does indeed refer to the live preview as Aero Peek (see http://blogs.msdn.com/e7/archive/2009/02/26/some-changes-since-beta.aspx). The button the taskbar is also part of the Aero Peek feature.

It doesn't make a difference how many tabs I have. I only see the current tab. Let's make sure I got this fix right. If I have two tabs in FF and I hover over the Windows taskbar I should see thumbnails of both tabs, just like I do when I have multiple FF windows opened, correct? The term Aero peek has different meanings depending on where you look. The option for turning AP on is described as "Use Aero Peek to preview the desktop". I like to call it thumbnail previews just as it's always been. I suppose the added ability to hover over the thumbnail and see it fullscreen is sort of a "Peek". Anyways, I can't get it working if I understand it the way I stated.

(In reply to comment #45) > It doesn't make a difference how many tabs I have. I only see the current tab. > > Let's make sure I got this fix right. If I have two tabs in FF and I hover > over the Windows taskbar I should see thumbnails of both tabs, just like I do > when I have multiple FF windows opened, correct? It works for me on the nightly trunk build from this morning. What build are you running? Do you see any errors in the Error Console?

i'm on branch which states the fix is there. Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b1pre) Gecko/20091006 Namoroka/3.6b1pre Firefox/3.5.3 - Build ID: 20091006045110 nothing in the error console

Comment on attachment 404861 [details] [diff] [review] Small bug fixes that should have been caught before checkin > for (let i = 0; i < tabs.length; i++) >- this.removeTab(tabs[i]); >+ this.removeTab(tabs[i], true); Set this._destroying instead of adding the dontSplice argument.

OK, I must be confusing this fix with https://bugzilla.mozilla.org/show_bug.cgi?id=501490 which did come in with last nights trunk.

Tried Trunk and tab previews works! Only thing I found is that when first hovering over a preview thumbnail the screen flickers.

(In reply to comment #50) > Tried Trunk and tab previews works! Only thing I found is that when first > hovering over a preview thumbnail the screen flickers. I have the same problem, so I tried with IE8. With IE8 when you hover a tab preview, only the content (and the window caption) changes.

The message "tab is undefined" keeps showing up in the error console and the previews just show the standard win7 loading icon. Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre

(In reply to comment #52) > The message "tab is undefined" keeps showing up in the error console and the > previews just show the standard win7 loading icon. > > > Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008 > Minefield/3.7a1pre See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=521216 which has a fix pending approval. Not sure if this fix will fix the issue of the loading icon showing up or not, may be another bug.

This only partly works in 3.6pre. Out of the ten tabs that I have, only the last one currently has a preview, all others just display a progress circle. New tabs get a preview though. Could it be that the previews are never generated for startup tabs or something?

Previews are generated on demand. Is there anything in your error console? Also, if you wait a minute or two (without having clicked the taskbar icon), do the progress circles resolve into a preview? Please followup to bug 522262.

Just wondering: both Aero Peek and Jumplists seem to have been disabled in latest mozilla-1.9.2 nightlies. Is that the case, or has something just broke?

(In reply to comment #56) > Just wondering: both Aero Peek and Jumplists seem to have been disabled in > latest mozilla-1.9.2 nightlies. Is that the case, or has something just broke? Jumplists are not yet landed on 1.9.2; see bug 473045, bug 518666 comment 81 and bug 521304 comment 9. Aero peek for tab previews are dependent on about:config preferences browser.taskbar.* and the width of your display and the number of tabs you open. For example if the using the default, you might find 8 previews at 1024x768, adding more moves it the list view upto the max, then disappears all together after that, no list, no previews. Loading a ton of bookmarks via "open all in tabs" > max list only seems to preview the first tab or runs the window preview again vs tab previews. Also stuck aero peek tab previews is still open in bug 522506 on both trunk and branch.

(In reply to comment #57) > (In reply to comment #56) > > Is that the case, or has something just broke? > > Also stuck aero peek tab previews is still open in bug 522506 on both trunk and > branch. Oops.. that should be bug 522262, and bug 522506 is actually for aero peek previews resolving into generic thumbnail icons. What was fixed recently was bug 522416, which resolved loading http requests as async vs sync so we don't see all aero peek previews running together which caused the browser bugs like comment 54 and caused the browser to eventually hang.