Dock and tray icon need menus

RESOLVED FIXED

Status

Mozilla Labs
Prism
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Matthew Gertner, Assigned: Matthew Gertner)

Tracking

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Updated

10 years ago
Assignee: nobody → matthew
(Assignee)

Comment 1

10 years ago
Created attachment 309464 [details] [diff] [review]
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.
Attachment #309464 - Flags: review?
(Assignee)

Comment 2

10 years ago
Created attachment 309465 [details]
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.
(Assignee)

Updated

10 years ago
Attachment #309465 - Attachment mime type: application/x-javascript → text/plain
(Assignee)

Comment 3

10 years ago
Created attachment 309467 [details] [diff] [review]
Patch for Windows (added missing files)
Attachment #309464 - Attachment is obsolete: true
Attachment #309464 - Flags: review?
(Assignee)

Comment 4

10 years ago
Created attachment 309843 [details] [diff] [review]
Mac WIP

This is just a patch to adapt the Mac to the new interfaces so it builds.
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.
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?
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.
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).
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...
(Assignee)

Comment 10

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

Comment 11

9 years ago
Created attachment 319019 [details] [diff] [review]
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.
Attachment #309467 - Attachment is obsolete: true
Attachment #309843 - Attachment is obsolete: true
(Assignee)

Comment 12

9 years ago
Created attachment 319022 [details] [diff] [review]
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.
Attachment #319019 - Attachment is obsolete: true
(Assignee)

Comment 13

9 years ago
Created attachment 319553 [details] [diff] [review]
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.
Attachment #319022 - Attachment is obsolete: true
Attachment #319553 - Flags: review?(mark.finkle)
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
Attachment #319553 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 15

9 years ago
Created attachment 319827 [details] [diff] [review]
Incorporated reviewer's comments
Attachment #319553 - Attachment is obsolete: true
Attachment #319827 - Flags: review?(mark.finkle)
(Assignee)

Updated

9 years ago
Attachment #319827 - Attachment is patch: true
Attachment #319827 - Attachment mime type: application/octet-stream → text/plain
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
Attachment #319827 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 17

9 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 19

9 years ago
Sorry, some glitches in my check in. Fixed now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

9 years ago
Created attachment 320177 [details] [diff] [review]
Rename nsIApplicationTile to nsIApplicationIcon

Should probably tack this onto this bug as well.
Attachment #320177 - Flags: review?(mark.finkle)
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 :)
Attachment #320177 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 22

9 years ago
Committed, slipped in a fix for the nsDockTile::Show build error on Mac as well.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Blocks: 420661
You need to log in before you can comment on or make changes to this bug.