581916 - Support fetching mouse cursor from window through DOM window utils API

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[jkk]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060309 Ubuntu/8.04 (hardy) Firefox/3.0.11
Build Identifier: 

Due to fennec process separation there is need for the implementation for fetching
mouse cursor type from some usable JS API like DOMWindowUtils. Custom native components cannot be loaded anymore in content.js so this kind of approach is needed.

Reproducible: Always

Comment 1

[jkk]

Attachment: Patch for adding cursor fetch in DOM window utils API

Comment 2

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

I don't think this is the right way to go. I think PuppetWidget::SetCursor should communicate with the chrome process to set the cursor for the content window.

Comment 3

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

OK, Oleg says this is for an extension, not the browser itself. That makes more sense.

+   * Get current cursor type from this window
+   */
+  short getCursorTypeFromWindow(in nsIDOMWindow aWindow);

Why pass in aWindow? You should just return the cursor type for the window the DOMWindowUtils is associated with.

Are you sure you don't need to be able to get the contents of custom cursors?

Comment 4

[jkk]

Attachment: Patch 2 for adding cursor fetch in DOM window utils API

Changed code to use DOM Utils related window.

Comment 5

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

Need to rev IID on nsIDOMWindowUtils.

So one big problem with this approach is that post bug-130078, you'll find that the widget is the widget of the toplevel window. So if the mouse is over chrome, that's the cursor you'll get, not the cursor for the content window you asked for.

So what I think you need to do is record somewhere, probably in a global variable in nsEventStateManager, the document for the last frame that was used in nsEventStateManager::SetCursor. When someone calls getCursorType, you need to check whether the document that last set SetCursor is (or is a descendant of) the document for the DOMWindowUtils. If it's not, probably just return 0 or some other value to indicate that the mouse is not currently over this window.

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 464780 [details] [diff] [review]
Patch 2 for adding cursor fetch in DOM window utils API

Juha, please update patch according to comment #5

Comment 7

[jkk]

Attachment: Proposal patch 3

Is this something like you had in mind roc?

Comment 8

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

+    if (nsEventStateManager::sPointerFocusedDocument != doc.get())
+        return eCursor_none;

Close, but you need to check the document hierarchy to see if the document with the cursor is a descendant of the document you care about --- e.g. if the cursor is over an IFRAME of the document you care about, you should still return its cursor. Use GetParentDocument to walk the document hierarchy.

:smaug should be the reviewer here by the way.

Comment 9

[jkk]

Attachment: Proposal patch 4

Comment 10

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

Comment on attachment 475801 [details] [diff] [review]
Proposal patch 4

>@@ -3406,6 +3407,13 @@ nsEventStateManager::SetCursor(PRInt32 a
>                                float aHotspotX, float aHotspotY,
>                                nsIWidget* aWidget, PRBool aLockCursor)
> {
>+  EnsureDocument(mPresContext);
>+  NS_ENSURE_TRUE(mDocument, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsPIDOMWindow> window(mDocument->GetWindow());
>+  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(window->GetExtantDocument()));
>+  sMouseOverDocument = doc.get();

I don't understand this. Why you need to get the document from window, and not just use mDocument?


>+nsDOMWindowUtils::GetWidgetForWindow(nsIDOMWindow *aWindow, nsIWidget** aWidget)
>+{
>+  NS_ENSURE_ARG_POINTER(aWindow);
>+  NS_ENSURE_ARG_POINTER(aWidget);
>+
>+  nsresult rv;
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
>+  NS_ENSURE_TRUE(docShell, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIPresShell> presShell;
>+  rv = docShell->GetPresShell(getter_AddRefs(presShell));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIViewManager> viewManager = presShell->GetViewManager();
>+  NS_ENSURE_TRUE(viewManager, NS_ERROR_FAILURE);
>+
>+  return viewManager->GetRootWidget(aWidget);
>+}

Why this and why not use GetWidget() ?

>+nsDOMWindowUtils::GetCursorType(PRInt16 *aCursor)
>+{
>+  NS_ENSURE_ARG_POINTER(aCursor);
>+
>+  PRBool isSameDoc = PR_FALSE;
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(mWindow->GetExtantDocument()));
>+
>+  do {
>+    if (nsEventStateManager::sMouseOverDocument == doc.get()) {
>+      isSameDoc = PR_TRUE;
>+      break;
>+    }
>+  } while ((doc = doc->GetParentDocument()));
>+
>+  if (!isSameDoc)
>+    return eCursor_none;
The method returns nsresult, not some cursor value.


>@@ -83,6 +83,12 @@ interface nsIDOMWindowUtils : nsISupport
You should update the iid.

>   /**
>+   * Get current cursor type from this window
>+   * @return the current calue of nsCursor
value

This needs some tests (which would have captured the wrong return value bug).

Comment 11

[jkk]

Attachment: Proposal patch 5

Next attempt. Good points on previous review taken into account.

Comment 12

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

Comment on attachment 475841 [details] [diff] [review]
Proposal patch 5

>+nsIDocument* nsEventStateManager::sMouseOverDocument = nsnull;
So where is sMouseOverDocument cleared after it is set?
nsEventStateManager dtor should probably check if 
mDocument == sMouseOverDocument and set
sMouseOverDocument = nsnull in that case.


>+nsDOMWindowUtils::GetCursorType(PRInt16 *aCursor)
>+{
>+  NS_ENSURE_ARG_POINTER(aCursor);
>+
>+  PRBool isSameDoc = PR_FALSE;
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(mWindow->GetExtantDocument()));
>+
>+  do {
>+    if (nsEventStateManager::sMouseOverDocument == doc.get()) {
>+      isSameDoc = PR_TRUE;
>+      break;
>+    }
>+  } while ((doc = doc->GetParentDocument()));
You crash here if mWindow->GetExtantDocument() returned null.

Please use -U 8 when creating patches.

Comment 13

[Antti Järvelin]

Attachment: Proposal patch 6

I'm updating the patch on behalf of jkk, because he is away for over a week.  So this version addresses the issues found from the previous proposal 5 (see comment 12).

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 476201 [details] [diff] [review]
Proposal patch 6

This patch is needed for meego extensions development

Comment 15

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/50a9c7362b66

Comment 16

[Daniel C]

I tweaked the summary to fix a typo and because it didn't read very well. Let me know if I've got it wrong.