Implement optional taskbar preview-per-tab

RESOLVED FIXED in seamonkey2.21

Status

--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ch.ey, Assigned: neil)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.21
x86
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 712264 [details] [diff] [review]
Patches to make SeaMonkey use Win7 taskbar previews.

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.
(Reporter)

Updated

6 years ago
Blocks: 566138

Comment 2

6 years ago
[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...

Comment 3

6 years ago
> +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"];

Comment 4

6 years ago
> +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.
(Reporter)

Comment 5

6 years ago
(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.

Comment 6

6 years ago
> 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.

Updated

6 years ago
Attachment #712264 - Flags: feedback?(neil)
(Assignee)

Comment 7

6 years ago
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...
(Assignee)

Comment 8

6 years ago
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]
Attachment #712264 - Flags: feedback?(neil) → feedback-
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 722551 [details] [diff] [review]
Proposed patch

OK, I think I've got it working to my satisfaction :-)

Hopefully all of my requestees run Windows 7 or later!
Assignee: ch.ey → neil
Attachment #712264 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #722551 - Flags: review?(bugzilla)
Attachment #722551 - Flags: feedback?(philip.chee)
Attachment #722551 - Flags: feedback?(ch.ey)
(Reporter)

Comment 13

6 years ago
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.
Attachment #722551 - Flags: feedback?(ch.ey) → feedback-
(Assignee)

Comment 14

6 years ago
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)
Attachment #722551 - Flags: feedback?(ch.ey)
(Reporter)

Comment 15

6 years ago
Comment on attachment 722551 [details] [diff] [review]
Proposed patch

Yep, that does the trick. Thanks.
Attachment #722551 - Flags: feedback?(ch.ey) → feedback+
(Assignee)

Comment 16

6 years ago
Created attachment 727856 [details] [diff] [review]
Updated for bitrot and feedback
Attachment #722551 - Attachment is obsolete: true
Attachment #722551 - Flags: review?(bugzilla)
Attachment #722551 - Flags: feedback?(philip.chee)
Attachment #727856 - Flags: review?(bugzilla)
Attachment #727856 - Flags: feedback?(philip.chee)

Comment 17

6 years ago
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?
Attachment #727856 - Flags: feedback?(philip.chee) → feedback+
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).

Comment 19

6 years ago
> How does one test this feature?
You need more than one tab.
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
Created attachment 730999 [details] [diff] [review]
Updated for feedback
Attachment #727856 - Attachment is obsolete: true
Attachment #727856 - Flags: review?(bugzilla)
Attachment #730999 - Flags: review?(bugzilla)
(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];
(Assignee)

Comment 24

6 years ago
(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.
(Assignee)

Comment 25

6 years ago
(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.
(Assignee)

Comment 26

6 years ago
Created attachment 747306 [details] [diff] [review]
Fixed patch
Attachment #730999 - Attachment is obsolete: true
Attachment #730999 - Flags: review?(bugzilla)
Attachment #747306 - Flags: review?(bugzilla)

Comment 27

6 years ago
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

Comment 28

6 years ago
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.
Attachment #747306 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 30

5 years ago
Pushed comm-central changeset 0c3b85a1c1f7.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.21
(Assignee)

Updated

5 years ago
Depends on: 898781

Comment 31

5 years ago
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.

Comment 32

5 years ago
Work done in this bug also appears to be causing bug 998171. Setting  browser.taskbar.previews.enable=false also fixes the issue.
You need to log in before you can comment on or make changes to this bug.