415948 - Dock and tray icon need menus

Status: RESOLVED FIXED

(Mozilla Labs :: Prism, defect)

Description

[Matthew Gertner]

It should be possible for the app to specify menus to be attached to the dock or tray icon. It should be easy to use the same menu for both (i.e. on Mac and Windows respectively).

Comment 1

[Matthew Gertner]

Attachment: Patch for Windows

This implements a new API for adding menu items to the tray icon under Windows. It includes Mark's patch from bug 420661. I did some refactoring of the design of the application tile API.

Comment 2

[Matthew Gertner]

Attachment: Example of using the new API

The HostUI.getWindow() might not be the way to go. We might want to add a specific API to HostUI for adding menu items and getting a notification when they are triggered.

Comment 3

[Matthew Gertner]

Attachment: Patch for Windows (added missing files)

Comment 4

[Matthew Gertner]

Attachment: Mac WIP

This is just a patch to adapt the Mac to the new interfaces so it builds.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Initial notes: 
* You have some cruft in the patch from "install-shortcut.xul"
* I would be happier if we could keep nsIPlatformGlueXxx and the nsIDesktopEnvironment (& friends) separate. We want to try to upstream both parts into the Mozilla codebase. My gut says nsIPlatfromXxx will be harder to upstream.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

More:

+NS_IMETHODIMP nsDesktopEnvironment::CreateApplicationTile(nsIApplicationTile** _retval)
+{
+  *_retval = new nsNotificationArea;
+  NS_ENSURE_TRUE(*_retval, NS_ERROR_OUT_OF_MEMORY);
+
+  NS_ADDREF(*_retval);
+  return NS_OK;
+}
+

Why are we recreating this on every call instead of caching?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Still More:

+NS_IMETHODIMP
+nsNotificationArea::AddMenuItem(PRInt32 aId, const nsAString& aLabel)
+{
+  if (!mMenu) {
+    mMenu = ::CreatePopupMenu();
+    NS_ENSURE_TRUE(mMenu, NS_ERROR_FAILURE);
+  }
+
+  ::AppendMenu(mMenu, MF_STRING, aId, NS_ConvertUTF16toUTF8(aLabel).get());
+
+  return NS_OK;
+}

Can't we use ::AppendMenuW and not convert to UTF8. AppendMenu doesn't handle UTF8 the way you think anyway, iirc.

* nsIPlatformGlueMenu.idl seems to be missing from the patch.

Comment 8

[Fredrik Larsson (:nossralf)]

Nice! This shouldn't be too hard to bring over to Linux.

Regarding the comment in nsNotificationArea::Show about using the webapp's icon as a default; since WebAppInstall registers a chrome directory service provider (IconProvider) to feed the main window's icon to XULRunner that way, it should in theory at least be possible to get at without mucking about with the file system (since that provider should be available to all XPCOM consumers).

Comment 9

[Fredrik Larsson (:nossralf)]

For reference before I forget:

http://mxr.mozilla.org/mozilla/source/widget/src/xpwidgets/nsBaseWidget.cpp#874

How to get at the icons the chrome directory way...

Comment 10

[Matthew Gertner]

(In reply to comment #6)
> More:
> 
> +NS_IMETHODIMP nsDesktopEnvironment::CreateApplicationTile(nsIApplicationTile**
> _retval)
> +{
> +  *_retval = new nsNotificationArea;
> +  NS_ENSURE_TRUE(*_retval, NS_ERROR_OUT_OF_MEMORY);
> +
> +  NS_ADDREF(*_retval);
> +  return NS_OK;
> +}
> +
> 
> Why are we recreating this on every call instead of caching?

Well the way I implemented it the PlatformGlue handles the caching but deletes to DesktopEnvironment if it doesn't have a tile yet and needs to create one.

(In reply to comment #7)
> Still More:
> 
> +NS_IMETHODIMP
> +nsNotificationArea::AddMenuItem(PRInt32 aId, const nsAString& aLabel)
> +{
> +  if (!mMenu) {
> +    mMenu = ::CreatePopupMenu();
> +    NS_ENSURE_TRUE(mMenu, NS_ERROR_FAILURE);
> +  }
> +
> +  ::AppendMenu(mMenu, MF_STRING, aId, NS_ConvertUTF16toUTF8(aLabel).get());
> +
> +  return NS_OK;
> +}
> 
> Can't we use ::AppendMenuW and not convert to UTF8. AppendMenu doesn't handle
> UTF8 the way you think anyway, iirc.

Good point.

I'll fix this stuff and get the Mac dock stuff going.

Comment 11

[Matthew Gertner]

Attachment: WIP, implement dock tile menus on OS X

This is an aggregate patch of all my work so far, including Mark's patch for 420661. It implements dock tile menus on Mac. Menu items are defined using the HTML5 <command> tag (see http://www.w3.org/html/wg/html5/#the-command). The "DOMActivate" event is sent to the element when the item is selected.

Part of this patch is to change the "window" variable in webapp.js to the contentWindow of the browser. In this way, menus can be added by the web app itself in normal content or by webapp.js. This makes things a bit trickier since the platform glue has to keep track of the window object if a new page is loaded into the main browser.

The only remaining issue I know about is that I wasn't able to get forwardInvocation to send messages that my new application delegate doesn't handle (it only deal with the dock menu) to the old delegate that Mozilla registers. This means that some potentially important messages won't get through. I'm looking into this.

I haven't yet adapted the patch so that it builds on Windows with all the new changes.

Comment 12

[Matthew Gertner]

Attachment: Fixes message forwarding to old application delegate

Already got a response from cocoa-dev (go cocoa-dev!) about the message forwarding issue. Seems I needed a respondsToSelector method. Problem fixed.

Comment 13

[Matthew Gertner]

Attachment: Add tray menu support on Windows

This is the master patch to fix bug 420661 as well as adding both dock and tray menus. I've excluded deleted files to improve reviewability.

I still need to renamed nsIApplicationTile to nsIApplicationIcon, but I'd rather get this reviewed and committed first since I think this will cause svn havoc otherwise.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 319553 [details] [diff] [review]
Add tray menu support on Windows

>Index: runtime/components/src/windows/nsNotificationArea.cpp
>===================================================================

>+NS_IMETHODIMP
>+nsNotificationArea::AddMenuItem(const nsAString& aId)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIDOMElement> element;
>+  rv = GetElementById(aId, getter_AddRefs(element));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsAutoString label;
>+  rv = element->GetAttribute(NS_LITERAL_STRING("label"), label);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!mMenu) {
>+    mMenu = ::CreatePopupMenu();
>+    NS_ENSURE_TRUE(mMenu, NS_ERROR_FAILURE);
>+  }
>+  
>+  PRUint32 itemId = mLastMenuId++;
>+  
>+  ::AppendMenu(mMenu, MF_STRING, itemId, NS_ConvertUTF16toUTF8(label).get());

Use ::AppendMenuW and keep the unicode string

>Index: runtime/components/src/nsPlatformGlue.js

>+  // nsIWebProgressListener
>+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>+    if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
>+      this._window = aWebProgress.DOMWindow;
>+    }
>+  },

>+  //nsIPlatformGlue
>+  init : function init(aWindow, aWebProgress) {
>+    this._window = aWindow;
>+    aWebProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_NETWORK);
>+  },

Why do we need to use "init"? Can't we just use the docloader webprogress approach, like nsLoginManager.js?
Also, why would we need the "aWindow" param? It is set by the onStateChange callback above.

>Index: runtime/chrome/jar.mn
>===================================================================

> webrunner.jar:
>-% content webrunner %content/
>+% content webrunner %content/ xpcnativewrappers=no

I thought we were _not_ going to turn off wrappers

>Index: runtime/chrome/content/webrunner.js
>===================================================================
>+/* Development of this Contribution was supported by Yahoo! Inc. */

Not in here it isn't

> 
>+      // Call the script's load() function once the page has finished loading
>+      if (WebAppProperties.script.load)
>+        window.addEventListener("DOMContentLoaded", WebAppProperties.script.load, false);
>       this._getBrowser().loadURI(WebAppProperties.uri, null, null);
>     }

I think you want:
          this._getBrowser().addEventListener("DOMContentLoaded", WebAppProperties.script.load, true);

You don't want to hook that into the XUL window, you want the browser.

>   _prepareWebAppScript : function()
>   {
>-   WebAppProperties.script["XMLHttpRequest"] = Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1");
>-   WebAppProperties.script["window"] = window;
>-   WebAppProperties.script["WebAppProperties"] = WebAppProperties;
>-   WebAppProperties.script["host"] = HostUI;
>+    // Initialize the platform glue
>+    var browser = this._getBrowser();
>+    // Use createInstance since we're overloaded the factory to always use a singleton
>+    var platformGlue = Cc["@mozilla.org/platform-web-api;1"].createInstance(Ci.nsIPlatformGlue);
>+    platformGlue.init(browser.contentWindow, browser.webProgress);

We should try to make this go away (as mentioned above)

>+    
>+    WebAppProperties.script["XMLHttpRequest"] = Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1");
>+    WebAppProperties.script["window"] = browser.contentWindow;
>+    WebAppProperties.script["WebAppProperties"] = WebAppProperties;

I don't like the CAPS

 WebAppProperties.script["properties"] = WebAppProperties;

>+    var icon = this._getBrowser().contentWindow.wrappedJSObject.platform.icon();
>+    icon.imageSpec = iconUri.spec;
>+    icon.title = document.title;
>+    icon.show();

Why not using the XPCOM singleton? Why using access to content?

>     if (WebAppProperties.trayicon) {
>-      var desktop = Cc["@mozilla.org/desktop-environment;1"].
>-        getService(Ci.nsIDesktopEnvironment);
>-      desktop.QueryInterface(Ci.nsINotificationArea).hideIcon(WebAppProperties.id);
>+      this._getBrowser().contentWindow.wrappedJSObject.platform.icon().hide();

Why not using the XPCOM singleton? Why using access to content?

r-, but really close

Comment 15

[Matthew Gertner]

Attachment: Incorporated reviewer's comments

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 319827 [details] [diff] [review]
Incorporated reviewer's comments


>Index: runtime/components/public/nsIPlatformGlue.idl
>===================================================================

>+interface nsIPlatformGlue : nsISupports
>+{
>+  /**
>+   * Initialize the platform glue.
>+   * @param   aWindow
>+   *          The current content window.
>+   * @param   aWebProgress
>+   *          Main browser's web progress (to detect when a new document is loaded).
>+   */
>+  void init(in nsIDOMWindow aWindow, in nsIWebProgress aWebProgress);

Remove this?

>Index: runtime/chrome/jar.mn
>===================================================================

> webrunner.jar:
>-% content webrunner %content/
>+% content webrunner %content/ xpcnativewrappers=no

Are you sure about this? For real and true?

r+ with nits

Comment 17

[Matthew Gertner]

(In reply to comment #16)
> >+  void init(in nsIDOMWindow aWindow, in nsIWebProgress aWebProgress);
> 
> Remove this?

Removed.

> >+% content webrunner %content/ xpcnativewrappers=no
> 
> Are you sure about this? For real and true?

Well, webapp.js can't use the platform property without a wrapper unless I do this. I would suggest we leave out wrappers for now and investigate using a sandbox to run the script, which hopefully will make this unnecessary.

> r+ with nits

Committed.

Comment 18

[Fredrik Larsson (:nossralf)]

I mentioned this on IRC but I'll make a note of it here.

runtime/components/public/Makefile.in has a reference to nsIPlatformGlue.idl, a file that doesn't live in the tree just yet (it's from the "content JS API" bug)

runtime/components/src/Makefile.in references nsPlatformGlue.js (same as above, does not yet live in tree) and nsCommandLineHandler.js which lives in runtime/components currently, so that runtime/components/Makefile.in should deal with that file.

A trunk checkout build fails currently, but works by dealing with the above.

Comment 19

[Matthew Gertner]

Sorry, some glitches in my check in. Fixed now.

Comment 20

[Matthew Gertner]

Attachment: Rename nsIApplicationTile to nsIApplicationIcon

Should probably tack this onto this bug as well.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 320177 [details] [diff] [review]
Rename nsIApplicationTile to nsIApplicationIcon

>Index: runtime/components/src/mac/nsDesktopEnvironmentMac.mm
>===================================================================
> 
>-NS_IMETHODIMP nsDesktopEnvironment::GetApplicationTile(nsIDOMWindow* aWindow, nsIApplicationTile** _retval)
>+NS_IMETHODIMP nsDesktopEnvironment::GetApplicationTile(nsIDOMWindow* aWindow, nsIApplicationIcon** _retval)

Whoops. GetApplicationIcon(...)


>Index: runtime/components/src/windows/nsDesktopEnvironmentWin.cpp
>===================================================================
>-NS_IMETHODIMP nsDesktopEnvironment::GetApplicationTile(nsIDOMWindow* aWindow, nsIApplicationTile** _retval)
>+NS_IMETHODIMP nsDesktopEnvironment::GetApplicationTile(nsIDOMWindow* aWindow, nsIApplicationIcon** _retval)

Same.


Also, make sure any new files are added: nsIApplicationIcon.idl, if it's not already in SVN

r+ with changes, and the any cascades to those changes :)

Comment 22

[Matthew Gertner]

Committed, slipped in a fix for the nsDockTile::Show build error on Mac as well.