501490 - Enable Taskbar Previews for Windows 7

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P2)

Description

[Rob Arnold [:robarnold]]

We need some native widgetry to enable us to create and control the previews.

Comment 1

[Rob Arnold [:robarnold]]

Attachment: v1.0

This should have all the needed widgetry to enable taskbar previews.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 387260 [details] [diff] [review]
v1.0

+XPIDLSRCS	+= nsIPrintSettingsWin.idl nsIWinTaskbar.idl nsITaskbarPreview.idl nsITaskbarPreviewController.idl nsITaskbarPreviewButton.idl

nit - shouldn't this be on multiple lines?

+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2000
+ * the Initial Developer. All Rights Reserved.

nit - out of date header.

+using namespace mozilla::widget;

New namespace usage? I really haven't heard much on this. 

+  WinTaskbar.cpp \
+  TaskbarPreview.cpp \
+  TaskbarPreviewButton.cpp \

nit - spaces vs. tabs, lets tighten this up.

+ * The Original Code is mozilla.org code.

nit - no copyright info in a couple of your headers.

+UINT      nsToolkit::sTaskbarButtonCreatedMsg = 0;

I didn't see anyplace where this gets initialized, do we need to set it with a call to RegisterWindowMessage or similar, or is this happening someplace else? AFAICT it's always zero.

Comment 3

[Rob Arnold [:robarnold]]

Attachment: v1.1

Nits addressed (there was a call to RegisterWindowMessage at one point - I don' t know how it disappeared). Also, it helps to include new files in the patch.

Comment 4

[Jim Mathies [:jimm]]

+ WinTaskbar.cpp

nsWinTaskbar.cpp/.p fits better with our current naming convention in src/windows.

+ // Defined in dwmapi in a header than needs a higher numbered _WINNT #define

nit, than -> that

+ sTaskbarButtonCreatedMsg = ::RegisterWindowMessageW(L"TaskbarButtonCreated");

Assert on this if it fails.

Before landing, could we better comment the interfaces? Especially nsITaskbarPreview since we'll be adding a number of new functionality blocks to this. 

Also, still missing some copyrights in src file headers? Not sure if that was intentional or not.

I'm also getting compile errors on the namespace stuff in nsWinWidgetFactory.cpp on a standard build that I just check out this morning. Looks like this depends on a number of other bugs though.. have you built up a try build with all the required patches? Maybe do that and post it here so we can play with it.

Generally though the core preview functionality in this looks good.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Looks like this depends on a number of other bugs though.. have you built up a
> try build with all the required patches? Maybe do that and post it here so we
> can play with it.

Err, guess that's not going to do much good for testing purposes. :) But definitely run it through try to be sure it builds properly.

Comment 6

[Rob Arnold [:robarnold]]

(In reply to comment #4)
> + WinTaskbar.cpp
> 
> nsWinTaskbar.cpp/.p fits better with our current naming convention in
> src/windows.

Not going to change this as discussed over IRC (part of the new namespace style rules).

> + // Defined in dwmapi in a header than needs a higher numbered _WINNT #define
> 
> nit, than -> that

Fixed.


> + sTaskbarButtonCreatedMsg = ::RegisterWindowMessageW(L"TaskbarButtonCreated");
> 
> Assert on this if it fails.

Ok, done. Not sure how this can fail :)

> Before landing, could we better comment the interfaces? Especially
> nsITaskbarPreview since we'll be adding a number of new functionality blocks to
> this.

Yes, I will do that.

> 
> Also, still missing some copyrights in src file headers? Not sure if that was
> intentional or not.

Missed the src headers. Fixed.

> I'm also getting compile errors on the namespace stuff in
> nsWinWidgetFactory.cpp on a standard build that I just check out this morning.

I've fixed the namespace issue - you probably weren't compiling with the Windows 7 SDK.

Comment 7

[Rob Arnold [:robarnold]]

Attachment: v1.2

Made all the changes above, did the documentation.

Decided to remove the clip method because we don't use it and it wasn't clear what it actually did in the native code (which caused me lots of woe attempting to use it).

Comment 8

[Jim Mathies [:jimm]]

I'm going to have to pull a comment nazi and ask that your interface comments follow java-doc style commenting. Take a look at the jl widget patch in bug 473045 for an example.

Also,

+  void Initialize();

+  readonly attribute boolean ready;

On these two, can we tag them so that people know they are associated with the preview code? I've got a general "available" property in the jl patch that indicates win7 support.

Digging into the rest of this now.. will post back any additional review comments here in a bit.

Comment 9

[Jim Mathies [:jimm]]

+const PRUnichar *const kWindowClass = L"TaskbarPreviewClass";
+  ::CreateWindowW(kWindowClass, L"TaskbarPreviewWindow",

"MozillaTaskbarPreviewClass" to match the stuff in nsWindow.

+  if (!aController)
+    return NS_ERROR_INVALID_ARG;

Might as well use NS_ENSURE_ARG_POINTER in cases like this.

+NS_IMETHODIMP
+TaskbarPreview::SetTitle(const nsAString &aTitle) {
+  mTitle = aTitle;
+  return IsEnabled() ? UpdateTitle() : NS_OK;
+}

If we're going to throw on all these for failures we should have that in the header comments.

+  nsresult rv = NS_OK;
+  nsresult rvUpdate;
+  rvUpdate = UpdateTooltip();
+  if (NS_FAILED(rvUpdate))
+    rv = rvUpdate;
+
+  rvUpdate = UpdateTitle();
+  if (NS_FAILED(rvUpdate))
+    rv = rvUpdate;
+
+  rvUpdate = UpdateNext();
+  if (NS_FAILED(rvUpdate))
+    rv = rvUpdate;
+
+  rvUpdate = UpdateIcon();
+  if (NS_FAILED(rvUpdate))
+    rv = rvUpdate;

Looks like if any one of these fails, we return a failure code from Enable(), but that isn't necessarily the case?

+    observerService->NotifyObservers(this, "windows-taskbar", L"ready");

Is ITaskbarList3 specific to win7? In which case we can key off of your ready prop. If not though (might it show up on other platforms with a service pack or what not) we should use something else to indicate win7 features are present.

Comment 10

[Joe Drew (not getting mail)]

We almost certainly want this to block the 1.9.2 branch.

Comment 11

[Rob Arnold [:robarnold]]

Attachment: v1.3

Comments touched up. As discussed over IRC, Initialize and ready are generic for anything involving the taskbar (not just previews) so they don't need to be marked for taskbar previews.

Comment 12

[Rain [:rain1] (inactive)]

Comment on attachment 390552 [details] [diff] [review]
v1.3

> 
> #ifndef WINCE
>+    sTaskbarButtonCreatedMsg = ::RegisterWindowMessageW(L"TaskbarButtonCreated");
>+    NS_ASSERTION(sTaskbarButtonCreatedMsg, "Could not register taskbar button creation message");
>     nsUXThemeData::Initialize();
> #endif
> }

...

> void
>+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7
>+      if (msg == nsToolkit::sTaskbarButtonCreatedMsg) {
>+        nsCOMPtr<nsIWinTaskbar> taskbarService = do_GetService("@mozilla.org/windows-taskbar;1");
>+
>+        taskbarService->Initialize();
>+      }
>+#endif // WIN7

I haven't run with this patch yet, but the TaskbarButtonCreated message is per-taskbar button, right? Wouldn't we initialize multiple times here in case multiple windows are opened and windows is set to either combine when the taskbar is full, or not combine?

Comment 13

[Rain [:rain1] (inactive)]

Also, builds with this patch fail because TBATFLAG isn't defined anywhere. I grepped my Win7 SDK's include dir, and couldn't find TBATFLAG anywhere.

<http://msdn.microsoft.com/en-us/library/dd391699%28VS.85%29.aspx> is the call where TBATFLAG is used, and it looks like it now accepts just a plain DWORD. I suspect the interface changed between the beta and RC SDKs...

Comment 14

[Joe Drew (not getting mail)]

Rob says this shouldn't block the alpha, but _should_ block 1.9.2 final.

Comment 15

[Rob Arnold [:robarnold]]

Attachment: v1.4

Changes:
Moved registration of taskbar button message to nsAppShell (nsToolkit will disappear).
We now wait to call any methods on the taskbar until we get said message.
The hack to wait for a window to be shown before adding buttons is removed now that we wait.

Note that buttons are still flaky. They like to disappear without any apparent reason or API call on our behalf.

Comment 16

[Rob Arnold [:robarnold]]

Attachment: v1.4.5

Sorry for the spam. Found a typo and a fix for the focus problem when restoring from a minimized window.

Comment 17

[Jim Mathies [:jimm]]

+ * Creating a taskbar preview will find the window
+ * associated with the controller's nsIDocShell and remove it from the taskbar.
+ * There is no way to avoid this behavior - it is how the native APIs work.
+ *
+ * We do not have a way to get the default preview for a given window. However
+ * it is not terribly difficult to imitate this behavior.

This is a confusing comment, can you clarify a bit? Does this have to do with the default window preview you normally see in Vista and Win7?

Overall though didn't see any issues.

Comment 18

[Rob Arnold [:robarnold]]

Comment on attachment 392558 [details] [diff] [review]
v1.4.5

Buttons don't seem to be stable for tab previews but they are still a feature we'll want for window previews. Going to redesign the API to allow button use for window previews.

Comment 19

[Rob Arnold [:robarnold]]

Attachment: New proposed API

Here's my proposed revised API for taskbar previews. It is a bit more complicated than before. I would like some feedback before I go ahead and start refactoring the patch into this.

Comment 20

[Jim Mathies [:jimm]]

1) I think you wanted this to be a button, not a controller?

interface nsITaskbarWindowPreviewController : nsISupports {
  /**
   * Invoked when one of the buttons on the preview is pressed
...

There seems to be some missing functionality for buttons in your doc beyond this, but maybe you just didn't go that deep?

2) nsITaskbarPreview gives the app control over how previews are displayed. The default for any app is a window preview. The window preview can be replaced with a tab preview by calling methods on nsITaskbarPreview. Hence the createTaskbarTabPreview vs. getTaskbarWindowPreview here?  (Is window a default that is always created?)

I like the relationships you've set up. I'd suggest getting to a v1.0 though which might not include buttons or anything else that has remaining issues. A solid implementation of the taskbar tab preview functionality is really all we need for 3.6.

Comment 21

[Rob Arnold [:robarnold]]

(In reply to comment #20)
> 1) I think you wanted this to be a button, not a controller?
> 
> interface nsITaskbarWindowPreviewController : nsISupports {
>   /**
>    * Invoked when one of the buttons on the preview is pressed
> ...
> 
> There seems to be some missing functionality for buttons in your doc beyond
> this, but maybe you just didn't go that deep?

Yep, I forgot buttons. They are unchanged from before though.

> 2) nsITaskbarPreview gives the app control over how previews are displayed. The
> default for any app is a window preview. The window preview can be replaced
> with a tab preview by calling methods on nsITaskbarPreview. Hence the
> createTaskbarTabPreview vs. getTaskbarWindowPreview here?  (Is window a default
> that is always created?)

Yes, but by the system. We create our object that represents it lazily.

> I like the relationships you've set up. I'd suggest getting to a v1.0 though
> which might not include buttons or anything else that has remaining issues. A
> solid implementation of the taskbar tab preview functionality is really all we
> need for 3.6.

Yeah, I may just end up doing that. I know that Songbird will want the window preview functionality. I've since merged the controllers too - with custom window previews, I need pretty much the same features of the controller (minus onClose and onActivate) and the only addition is the onClick button handler.

Does that sound good to you?

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)

> Does that sound good to you?

Yes. Song bird aside, we need to make sure previews get into 3.6. So I would make that the top priority right now. If we can get what song bird needs too without risking anything for Fx, that's great too.

Comment 23

[Rob Arnold [:robarnold]]

Attachment: v2.0

Now with the new API (more or less) and various bug fixes that came about while I was moving code around.

Window previews (with and without custom drawing) appear to work as well though the buttons are still flaky as before. That should be fixed in a separate bug because they are not needed for the preview-per-tab functionality.

Comment 24

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

Needed for Windows 7 parity with Chrome, Safari and IE.
Vlad: we can haz sr?

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Just had a long discussion with Rob about tests for this bug -- he's going to comment in here with his plan for at least "logic testing" of the peek stuff, if not HWND-peeking awesomeness to make sure that the previews render correctly.  (Until we get various issues with build hosts and SDK versions resolved, the tests he comes up with won't be run automatically, but people working on widget stuff can run them directly, and once the configuration deps fall down we'll have them ready.)

Comment 26

[Rob Arnold [:robarnold]]

Attachment: v2.5

Slight API revisions:
nsITaskbarPreview.makeActive replaced by attribute boolean active;
nsIWinTaskbar.getWindowPreview no longer requires a controller - a default one is provided instead

Misc improvements:
Tests are included.
GC-triggered crash is now resolved.
nsITaskbarPreview.invalidate no longer throws when composition isn't enabled.
Fixed an impossible requirement of visible = true that mNext had to be registered if non-null - it will now force mNext to register itself in this situation.
Misc code cleanup, simplification and documentation.

Comment 27

[Jim Mathies [:jimm]]

first block..

+++ b/widget/public/nsIWinTaskbar.idl

+  /**
+   * Creates a taskbar preview. The docshell is used to find the toplevel window.
+   * See the documentation for nsITaskbarTabPreview for more information.
+   */
+  nsITaskbarTabPreview createTaskbarTabPreview(in nsIDocShell shell,

Creates a taskbar tab preview.

+   * Gets the taskbar preview for a window. The docshell is used to find the
+   * toplevel window. See the documentation for nsITaskbarTabPreview for more
+   * information.

See the docs in nsITaskbarWindowPreview?

nsITaskbarTabPreview doesn't appear to have any docs in it. (maybe nsITaskbarPreview?)


+++ b/widget/src/windows/WinTaskbar.cpp

+NS_IMETHODIMP
+DefaultController::GetWidth(PRUint32 *aWidth)
+{
+  RECT r;
+  ::GetClientRect(mWnd, &r);
+  *aWidth = r.right - r.left + 1;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+DefaultController::GetHeight(PRUint32 *aHeight)
+{
+  RECT r;
+  ::GetClientRect(mWnd, &r);
+  *aHeight = r.bottom - r.top + 1;
+  return NS_OK;
+}

This always returns r.right + 1 since r.left is zero (intended?). Also right & bottom are exclusive, so they return width/height + 1. Are we getting the dimensions we want here? If not, looks like it causes uneeded aspect calculations in WM_DWMSENDICONICTHUMBNAIL. (depending on how much we rely on the default controller.)

+  //::AllocConsole();
+  //*stdout = *_fdopen(::_open_osfhandle((intptr_t)GetStdHandle(STD_OUTPUT_HANDLE), 0), "w");
+  //*stderr = *_fdopen(::_open_osfhandle((intptr_t)GetStdHandle(STD_ERROR_HANDLE), 0), "w");

(nix the cruft or wrap it in a debug ifdef.)


+++ b/widget/src/windows/TaskbarPreview.cpp

+nsresult
+TaskbarPreview::UpdateTaskbarProperties() {
+  return NS_OK;
+  nsresult rv = UpdateTooltip();
+

Has this been tested? :)


+++ b/widget/src/windows/TaskbarPreview.h

+  // Gets a references to the window used to handle the preview messages
s/references/reference


+  // Returns whether or not the taskbar icon has been created for mWnd
+  PRBool CanMakeTaskbarCalls();

There's some what of a disconnect here in naming/comments. Maybe just add some commenting here explaining why we wait for windows to message us back. 

+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7
+  PRBool HasTaskbarIconBeenCreated() {
+    return mHasTaskbarIconBeenCreated;
+  }
+  void SetHasTaskbarIconBeenCreated(PRBool created = PR_TRUE) {
+    mHasTaskbarIconBeenCreated = created;
+  }
+  already_AddRefed<nsITaskbarWindowPreview> GetTaskbarPreview()
+  {
+    nsCOMPtr<nsITaskbarWindowPreview> preview(do_QueryReferent(mTaskbarPreview));
+    return preview.forget();
+  }
+  void                    SetTaskbarPreview(nsITaskbarWindowPreview *preview) {
+    mTaskbarPreview = do_GetWeakReference(preview);
+  }
+#endif
+

Let's clean up the tabs and place this in a commented section of the header, or better yet, put this in the cpp.


+++ b/widget/public/nsITaskbarPreview.idl

+   * Whether or not the preview is visible.
+   *
+   * Changing this option is not cheap but also not terribly expensive.
+   * Toggling this option will destroy/create the proxy window and its
+   * registration with the taskbar. If any step of that fails, an exception
+   * will be thrown.

It's either cheap, expensive, or there's no need for this comment. :) (Sounds expensive to me!)

Comment 28

[Jim Mathies [:jimm]]

+  /**
+   * Max 7 buttons per preview per the Windows Taskbar API
+   */
+  const long MAX_TOOLBAR_BUTTONS = 7;

+   * Gets the nth button for the preview image. By default, a preview has no
+   * buttons. Enabling a button will add the button toolbar below the preview.
+   *
+   * @param index The index into the button array. Must be >= 0 and <
+   *              MAX_TOOLBAR_BUTTONS.
+   */
+  nsITaskbarPreviewButton getButton(in unsigned long index);

+++ b/widget/src/windows/TaskbarWindowPreview.cpp

+  // Windows button format
+  THUMBBUTTON             mThumbButtons[nsITaskbarWindowPreview::MAX_TOOLBAR_BUTTONS];
+  // Pointers to our button class (cached instances)
+  nsWeakPtr               mWeakButtons[nsITaskbarWindowPreview::MAX_TOOLBAR_BUTTONS];

+  memset(mThumbButtons, 0, sizeof mThumbButtons);
+  for (PRInt32 i = 0; i < nsITaskbarWindowPreview::MAX_TOOLBAR_BUTTONS; i++) {
+    mThumbButtons[i].dwMask = THB_FLAGS | THB_ICON | THB_TOOLTIP;
+    mThumbButtons[i].iId = i;
+    mThumbButtons[i].dwFlags = THBF_HIDDEN;
+  }

+nsresult
+TaskbarWindowPreview::GetButton(PRUint32 index, nsITaskbarPreviewButton **_retVal) {
+  if (index > nsITaskbarWindowPreview::MAX_TOOLBAR_BUTTONS)
+    return NS_ERROR_INVALID_ARG;

+nsresult
+TaskbarWindowPreview::UpdateButton(PRUint32 index) {
+  if (index > nsITaskbarWindowPreview::MAX_TOOLBAR_BUTTONS)
+    return NS_ERROR_INVALID_ARG;

>= max number of buttons.

(Add a test for this if possible.)

+  if (aEnable == mCustomDrawing) {
+    return NS_OK;
+  }

silly nit - nix the braces.

+  /**
+   * Whether or not the button is shown. Buttons that are invisible do not
+   * participate the layout of buttons underneath the preview.
+   *
+   * Default: false
+   */
+  attribute boolean visible;

nit - participate in the

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+ * We do not have a way to get the default preview for a given window. However
+ * it is not terribly difficult to imitate this behavior.

It's not clear what this means.

+   * participate the layout of buttons underneath the preview.

participate in

+  /**
+   * The width of the preview image
+   */
+  readonly attribute unsigned long width;
+
+  /**
+   * The height of the preview image
+   */
+  readonly attribute unsigned long height;
+
+  /**
+   * The aspect ratio of the thumbnail - this does not need to match the ratio
+   * of the preview.
+   */
+  readonly attribute float thumbnailAspectRatio;

When is the client allowed to change these? Please document if these can change during the lifetime of the controller, and if they can change, what (if anything) the client needs to do to make sure everything's updated appropriately.

+interface nsITaskbarTabPreview : nsITaskbarPreview

+interface nsITaskbarWindowPreview : nsITaskbarPreview

+interface nsIWinTaskbar : nsISupports

These interfaces need documentation. You should definitely say explicitly that nsIWinTaskbar is a service and mention what its service name is. And you need to explain what the difference between a tab preview and a window preview is.

I wonder if it would make sense to pass the controller in when we create a Preview object, and not have it as an attribute? That would be a little simpler and guarantee the controller doesn't change, which seems like a good idea. Oh, you do pass the controller into createTaskbarTabPreview, but not getTaskbarWindowPreview ... why? Is it because there is only one window preview and you get the same one back, whereas createTaskbarTabPreview can create as many as you like?

Perhaps we could make getTaskbarWindowPreview into createTaskbarWindowPreview and have it fail if there already is one? You don't want extensions and other code stomping on your WindowPreview anyway. If we do that, we could make enableCustomDrawing just be a parameter to createTaskbarWindowPreview?

How do createTaskbarTabPreview and getTaskbarWindowPreview interact? Can you have tab previews and a window preview for the same window?

What do those methods do if the taskbar service is not available? I wonder if it would make more sense for do_GetService to simply fail?

Can taskbar availability change over time or is it constant for the lifetime of the app?

What happens if you make a window preview invisible? That doesn't make a whole lot of sense to me.

Only WindowPreviews have buttons, right? And they always have 7 buttons, but the buttons all start off invisible by default? This could be explained more clearly. In particular MAX_TOOLBAR_BUTTONS should be called NUM_TOOLBAR_BUTTONS. And this comment is very confusing:
+   * Gets the nth button for the preview image. By default, a preview has no
+   * buttons. Enabling a button will add the button toolbar below the preview.

I think it sould say "By default, all buttons are invisible."

+   * Enables/disables custom drawing of thumbnails and previews

What is the default drawing behaviour?

Your test should use NUM_TOOLBAR_BUTTONS+1 instead of hardcoding 8.

Comment 30

[Rob Arnold [:robarnold]]

Attachment: v2.6

(In reply to comment #29)
> + * We do not have a way to get the default preview for a given window. However
> + * it is not terribly difficult to imitate this behavior.
> 
> It's not clear what this means.

Replaced with a more complete explanation.

> +  /**
> +   * The width of the preview image
> +   */
> +  readonly attribute unsigned long width;
> +
> +  /**
> +   * The height of the preview image
> +   */
> +  readonly attribute unsigned long height;
> +
> +  /**
> +   * The aspect ratio of the thumbnail - this does not need to match the ratio
> +   * of the preview.
> +   */
> +  readonly attribute float thumbnailAspectRatio;
> 
> When is the client allowed to change these? Please document if these can change
> during the lifetime of the controller, and if they can change, what (if
> anything) the client needs to do to make sure everything's updated
> appropriately.

Documented. These can change any time.

> 
> +interface nsITaskbarTabPreview : nsITaskbarPreview
> 
> +interface nsITaskbarWindowPreview : nsITaskbarPreview
> 
> +interface nsIWinTaskbar : nsISupports
> 
> These interfaces need documentation. You should definitely say explicitly that
> nsIWinTaskbar is a service and mention what its service name is. And you need
> to explain what the difference between a tab preview and a window preview is.

It is a service, where should the service name be provided. Is there precedent for that?

> I wonder if it would make sense to pass the controller in when we create a
> Preview object, and not have it as an attribute? That would be a little simpler
> and guarantee the controller doesn't change, which seems like a good idea. Oh,
> you do pass the controller into createTaskbarTabPreview, but not
> getTaskbarWindowPreview ... why? Is it because there is only one window preview
> and you get the same one back, whereas createTaskbarTabPreview can create as
> many as you like?

Yes.

> Perhaps we could make getTaskbarWindowPreview into createTaskbarWindowPreview
> and have it fail if there already is one? You don't want extensions and other
> code stomping on your WindowPreview anyway. If we do that, we could make
> enableCustomDrawing just be a parameter to createTaskbarWindowPreview?

I don't like the word create because the preview essentially already exists (though our object representing it does not).
Multiple extensions should play nicely with each other's controllers (this shouldn't be hard to do from JS).

> How do createTaskbarTabPreview and getTaskbarWindowPreview interact? Can you
> have tab previews and a window preview for the same window?

Now documented. Visible tab previews override window previews.

> What do those methods do if the taskbar service is not available? I wonder if
> it would make more sense for do_GetService to simply fail?

Gracefully return an error. Not sure if failing is the better option since that involves breaking into the nice abstraction that the current macros give us. If we do want to go down that route, we could not register at all if the services is unavailable.

> Can taskbar availability change over time or is it constant for the lifetime of
> the app?

Constant. Documented.

> What happens if you make a window preview invisible? That doesn't make a whole
> lot of sense to me.

It disappears from the list of windows in the taskbar until you make it visible again.

> Only WindowPreviews have buttons, right? And they always have 7 buttons, but
> the buttons all start off invisible by default? This could be explained more
> clearly. In particular MAX_TOOLBAR_BUTTONS should be called
> NUM_TOOLBAR_BUTTONS. And this comment is very confusing:
> +   * Gets the nth button for the preview image. By default, a preview has no
> +   * buttons. Enabling a button will add the button toolbar below the preview.
> 
> I think it sould say "By default, all buttons are invisible."

Done.

> +   * Enables/disables custom drawing of thumbnails and previews
> 
> What is the default drawing behaviour?

Now documented. Windows will draw the client area for the thumbnail and the preview.

> 
> Your test should use NUM_TOOLBAR_BUTTONS+1 instead of hardcoding 8.

Changed to MAX_TOOLBAR_BUTTONS to catch edge case reported by Jim.

(In reply to comment #29)
> + * We do not have a way to get the default preview for a given window. However
> + * it is not terribly difficult to imitate this behavior.
> 
> It's not clear what this means.

Replaced with a more complete explanation.

> +  /**
> +   * The width of the preview image
> +   */
> +  readonly attribute unsigned long width;
> +
> +  /**
> +   * The height of the preview image
> +   */
> +  readonly attribute unsigned long height;
> +
> +  /**
> +   * The aspect ratio of the thumbnail - this does not need to match the ratio
> +   * of the preview.
> +   */
> +  readonly attribute float thumbnailAspectRatio;
> 
> When is the client allowed to change these? Please document if these can change
> during the lifetime of the controller, and if they can change, what (if
> anything) the client needs to do to make sure everything's updated
> appropriately.

Documented. These can change any time.

> 
> +interface nsITaskbarTabPreview : nsITaskbarPreview
> 
> +interface nsITaskbarWindowPreview : nsITaskbarPreview
> 
> +interface nsIWinTaskbar : nsISupports
> 
> These interfaces need documentation. You should definitely say explicitly that
> nsIWinTaskbar is a service and mention what its service name is. And you need
> to explain what the difference between a tab preview and a window preview is.

It is a service, where should the service name be provided. Is there precedent for that?

> I wonder if it would make sense to pass the controller in when we create a
> Preview object, and not have it as an attribute? That would be a little simpler
> and guarantee the controller doesn't change, which seems like a good idea. Oh,
> you do pass the controller into createTaskbarTabPreview, but not
> getTaskbarWindowPreview ... why? Is it because there is only one window preview
> and you get the same one back, whereas createTaskbarTabPreview can create as
> many as you like?

Yes.

> Perhaps we could make getTaskbarWindowPreview into createTaskbarWindowPreview
> and have it fail if there already is one? You don't want extensions and other
> code stomping on your WindowPreview anyway. If we do that, we could make
> enableCustomDrawing just be a parameter to createTaskbarWindowPreview?

I don't like the word create because the preview essentially already exists (though our object representing it does not).
Multiple extensions should play nicely with each other's controllers (this shouldn't be hard to do from JS).

> How do createTaskbarTabPreview and getTaskbarWindowPreview interact? Can you
> have tab previews and a window preview for the same window?

Now documented. Visible tab previews override window previews.

> What do those methods do if the taskbar service is not available? I wonder if
> it would make more sense for do_GetService to simply fail?

Gracefully return an error. Not sure if failing is the better option since that involves breaking into the nice abstraction that the current macros give us. If we do want to go down that route, we could not register at all if the services is unavailable.

> Can taskbar availability change over time or is it constant for the lifetime of
> the app?

Constant. Documented.

> What happens if you make a window preview invisible? That doesn't make a whole
> lot of sense to me.

It disappears from the list of windows in the taskbar until you make it visible again.

> Only WindowPreviews have buttons, right? And they always have 7 buttons, but
> the buttons all start off invisible by default? This could be explained more
> clearly. In particular MAX_TOOLBAR_BUTTONS should be called
> NUM_TOOLBAR_BUTTONS. And this comment is very confusing:
> +   * Gets the nth button for the preview image. By default, a preview has no
> +   * buttons. Enabling a button will add the button toolbar below the preview.
> 
> I think it sould say "By default, all buttons are invisible."

Done.

> +   * Enables/disables custom drawing of thumbnails and previews
> 
> What is the default drawing behaviour?

Now documented. Windows will draw the client area for the thumbnail and the preview.

> Your test should use NUM_TOOLBAR_BUTTONS+1 instead of hardcoding 8.

Changed to MAX_TOOLBAR_BUTTONS to catch edge case reported by Jim.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> It is a service, where should the service name be provided. Is there precedent
> for that?

Oh, you're only using the CID. Never mind I guess.

> > Perhaps we could make getTaskbarWindowPreview into createTaskbarWindowPreview
> > and have it fail if there already is one? You don't want extensions and other
> > code stomping on your WindowPreview anyway. If we do that, we could make
> > enableCustomDrawing just be a parameter to createTaskbarWindowPreview?
> 
> I don't like the word create because the preview essentially already exists
> (though our object representing it does not).

createCustomTaskbarWindowPreview?

> Multiple extensions should play nicely with each other's controllers (this
> shouldn't be hard to do from JS).

Actually I think it will be hard. You can install your own controller and delegate to the previous controller but if that other controller tries to uninstall itself things will go badly wrong.

> > What do those methods do if the taskbar service is not available? I wonder
> > if it would make more sense for do_GetService to simply fail?
> 
> Gracefully return an error. Not sure if failing is the better option since
> that involves breaking into the nice abstraction that the current macros give
> us. If we do want to go down that route, we could not register at all if the
> services is unavailable.

The latter is what I meant. But what you have now is acceptable.

> > What happens if you make a window preview invisible? That doesn't make a
> > whole lot of sense to me.
> 
> It disappears from the list of windows in the taskbar until you make it
> visible again.

Hmm, OK that could be useful. Great.

> > Only WindowPreviews have buttons, right? And they always have 7 buttons, but
> > the buttons all start off invisible by default? This could be explained more
> > clearly. In particular MAX_TOOLBAR_BUTTONS should be called
> > NUM_TOOLBAR_BUTTONS. And this comment is very confusing:
> > +   * Gets the nth button for the preview image. By default, a preview has no
> > +   * buttons. Enabling a button will add the button toolbar below the preview.
> > 
> > I think it sould say "By default, all buttons are invisible."
> 
> Done.

You still have MAX_TOOLBAR_BUTTONS, which I think is misleading. It makes the user look for a createToolbarButton method or something similar.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 404744 [details] [diff] [review]
v2.6

sr+ with the MAX->NUM change

Comment 33

[Rob Arnold [:robarnold]]

Attachment: v2.6.1

As discussed on IRC, getTaskbarWindowPreview says because it modifies the default preview.

Changed MAX_* to NUM_*.

Comment 34

[Rob Arnold [:robarnold]]

Attachment: Bustage fix

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/5d4df2ddcc20
Bustage fix (didn't rename everything):
http://hg.mozilla.org/mozilla-central/rev/4fabad3d0522

Comment 35

[Rob Arnold [:robarnold]]

Pushed to mozilla-1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18fd0e804075

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

NS_IMETHODIMP
TaskbarPreview::SetTooltip(const nsAString &aTooltip) {
  return NS_OK;
  mTooltip = aTooltip;
  return CanMakeTaskbarCalls() ? UpdateTooltip() : NS_OK;
}

This looks strange.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Rob, the patch has landed on trunk (1.9.3a1) before it was checked-in on 1.9.2 branch. The TM is always the most recent version which is 1.9.3a1 at the moment. Branch check-ins are tracked by the status1.9.2 or status1.9.1 fields. Sorry for not giving any comment with the last change.

Comment 38

[Eric Shepherd [:sheppy]]

I'm starting to document this and have a question: in nsITaskbarPreviewController, the IDL comments say that width, height, and thumbnailAspectRatio may change at any time, yet they're read-only. Does this mean the OS may change these behind the back of the interface, or something else?

Comment 39

[Rob Arnold [:robarnold]]

(In reply to comment #38)
> I'm starting to document this and have a question: in
> nsITaskbarPreviewController, the IDL comments say that width, height, and
> thumbnailAspectRatio may change at any time, yet they're read-only. Does this
> mean the OS may change these behind the back of the interface, or something
> else?

The nsITaskbarPreviewController is intended to be implemented by the client (see browser/components/wintaskbar/WindowsPreviewPerTab.jsm:PreviewController for an example). It's used by the nsITaskbar*Preview implementations to provide behavior for the previews. The properties are readonly to indicate that they will be read and not written by the previews; readonly does not mean const here (hence the comment about mutability).

Comment 40

[Eric Shepherd [:sheppy]]

I have reference documentation for the interfaces involved here:

https://developer.mozilla.org/en/nsITaskbarPreview
https://developer.mozilla.org/en/nsITaskbarPreviewButton
https://developer.mozilla.org/en/nsITaskbarPreviewController
https://developer.mozilla.org/en/nsITaskbarTabPreview
https://developer.mozilla.org/en/nsITaskbarWindowPreview
https://developer.mozilla.org/en/nsIWinTaskbar

I'm preparing to install Windows 7 in a VM so I can actually play with this stuff and put together some code. Once I've done that, I'll do some refinement on these docs, and will be writing an article called "Working with the Windows taskbar" that will be featured on the Firefox 3.6 for developers page.

Please feel free to review and comment upon the docs listed above for now, however.

Comment 41

[Rain [:rain1] (inactive)]

> I'm preparing to install Windows 7 in a VM so I can actually play with this
> stuff and put together some code.

Please keep in mind that this works best with Aero enabled. I think VMware Fusion 3 and Workstation 7/Player 3 both support Aero, though.

Comment 42

[Eric Shepherd [:sheppy]]

I've already upgraded both my Parallels and my VMWare Fusion to versions that support Aero.

Comment 43

[Eric Shepherd [:sheppy]]

The onClick() method in the controller receives an nsITaskbarPreviewButton as
input, but I don't see any way to identify which button it actually is; there's
no ID or label field in the button object itself. How are you supposed to
detect which of the buttons has been clicked?

Also, I can't find any code anywhere that actually uses the icon field in the buttons. How do you create the image to put there? I'm trying to write some code to build a window preview button as an example, and so far am not having luck getting it to work.

Comment 44

[Eric Shepherd [:sheppy]]

Should this stuff work from an extension? I've written one, but even though the code looks right, it doesn't seem to be able to successfully install a button onto the window preview.