560846 - Add support for setting taskbar application user models ids per top level window

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

http://msdn.microsoft.com/en-us/library/dd378459%28v=VS.85%29.aspx
http://msdn.microsoft.com/en-us/library/dd378430%28v=VS.85%29.aspx

Seamonkey needs access to this ability to set individual app user model ids per top level application window.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 3

[Jim Mathies [:jimm]]

-#include <propvarutil.h>
-#include <propkey.h>
+  #include <propvarutil.h>
+  #include <propkey.h>

(ignore that ^^)

Comment 4

[Jim Mathies [:jimm]]

Some simple example code that works in the ede javascript console:

var wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
var win = wm.getMostRecentWindow("navigator:browser");
var taskbar = Cc["@mozilla.org/windows-taskbar;1"].getService(Ci.nsIWinTaskbar);

// Split this window out into it's own stack
taskbar.setGroupIdForWindow(win, "myId");

// Restore this window to the bottom of the default stack
taskbar.setDefaultGroupIdForWindow(win);

Comment 5

[Rob Arnold [:robarnold]]

Comment on attachment 443501 [details] [diff] [review]
patch v.1

I'd like to see a slightly different API.

First, it's not clear why the functions return a boolean since this is equivalent to NS_SUCCEEDED (actually you should always write a return value, even in error cases).

Second, the SetDefaultGroupIdFromWindow function just grabs a window-invariant string and then essentially calls SetGroupIdForWindow - how about we expose this window-invariant string as a readonly property instead of having SetDefaultGroupIdFromWindow

nit: no need to have the idl prototype in a comment 2 lines above the C++ definition

GetHWNDFromDOMWindow can reuse a lot of GetHWNDFromDocShell which we already use elsewhere in this file.

>+nsresult
>+SetWindowAppUserModelProp(nsIDOMWindow *aParent, const nsString & aIdentifier)
>+{
>...
>+  PROPVARIANT pv;
>+  if (FAILED(InitPropVariantFromString(aIdentifier.get(), &pv)))
>+    return NS_ERROR_INVALID_ARG;
>+
>+  IPropertyStore* pPropStore;
>+  if (FAILED(SHGetPropertyStoreForWindow(toplevelHWND, IID_PPV_ARGS(&pPropStore))) ||
>+      FAILED(pPropStore->SetValue(PKEY_AppUserModel_ID, pv)) ||
>+      FAILED(pPropStore->Commit()) ||
>+      FAILED(pPropStore->Release())) {
>+    PropVariantClear(&pv);
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  PropVariantClear(&pv);
>+
>+  return NS_OK;
>+}

If SetValue or Commit fail, we leak the property store. Can you wrap it in an nsCOMPtr for sanity? Also, Release returns a ULONG and not an HRESULT so applying FAILED is the wrong thing to do (the return value is for testing purposes only). Also I notice that you call PropVariantClear in both return paths - I don't know if you can combine them somehow but it would be a plus if you could (don't do it if it makes the code harder to read or slower).

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> (From update of attachment 443501 [details] [diff] [review])
> I'd like to see a slightly different API.
> 
> First, it's not clear why the functions return a boolean since this is
> equivalent to NS_SUCCEEDED (actually you should always write a return value,
> even in error cases).

Good point, a try catch and docs on exceptions would suffice.

> Second, the SetDefaultGroupIdFromWindow function just grabs a window-invariant
> string and then essentially calls SetGroupIdForWindow - how about we expose
> this window-invariant string as a readonly property instead of having
> SetDefaultGroupIdFromWindow
> 

This is available through the new taskbar info interface. Creating two interfaces though for this and grabbing a string to set a string.. is cumbersome. The setdefault takes care of this. I'd prefer to keep it.

> nit: no need to have the idl prototype in a comment 2 lines above the C++
> definition

Hmm. I've always added that, although I'm not seeing anything in the style guidelines. Isn't this standard practice?

> 
> GetHWNDFromDOMWindow can reuse a lot of GetHWNDFromDocShell which we already
> use elsewhere in this file.
> 
> >+nsresult
> >+SetWindowAppUserModelProp(nsIDOMWindow *aParent, const nsString & aIdentifier)
> >+{
> >...
> >+  PROPVARIANT pv;
> >+  if (FAILED(InitPropVariantFromString(aIdentifier.get(), &pv)))
> >+    return NS_ERROR_INVALID_ARG;
> >+
> >+  IPropertyStore* pPropStore;
> >+  if (FAILED(SHGetPropertyStoreForWindow(toplevelHWND, IID_PPV_ARGS(&pPropStore))) ||
> >+      FAILED(pPropStore->SetValue(PKEY_AppUserModel_ID, pv)) ||
> >+      FAILED(pPropStore->Commit()) ||
> >+      FAILED(pPropStore->Release())) {
> >+    PropVariantClear(&pv);
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  PropVariantClear(&pv);
> >+
> >+  return NS_OK;
> >+}
> 
> If SetValue or Commit fail, we leak the property store. Can you wrap it in an
> nsCOMPtr for sanity? Also, Release returns a ULONG and not an HRESULT so
> applying FAILED is the wrong thing to do (the return value is for testing
> purposes only). Also I notice that you call PropVariantClear in both return
> paths - I don't know if you can combine them somehow but it would be a plus if
> you could (don't do it if it makes the code harder to read or slower).

Good catch, will clean this up.

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 443911 [details] [diff] [review]
patch v.2

comments addressed.

Comment 9

[Rob Arnold [:robarnold]]

Comment on attachment 443911 [details] [diff] [review]
patch v.2

Would be nice if we didn't have to copy the string into the nsString but it seems that's not possible in this case.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 443911 [details] [diff] [review]
patch v.2

sr? -> roc for interface changes.

Comment 11

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/44dd40341996

Seamonky folks, you're good to go! I posted some sample script lower in the bug.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> Seamonky folks, you're good to go! I posted some sample script lower in the
> bug.

jimm, thanks a lot for the groundwork here, we appreciate that very much!