[Bug 1040313] Re: Need access to native handle for tabs
Bug Watch Updater
1040313 at bugs.launchpad.net
Wed Aug 22 22:15:14 UTC 2012
Launchpad has imported 20 comments from the remote bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=760802.
If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.
------------------------------------------------------------------------
On 2012-06-02T13:27:32+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120424151726
Steps to reproduce:
It would be very convenient, for applications or addons interacting with
the desktop or window manager, if nsIBaseWindow could expose its handle
to the native window, in the form of an attribute in JS. This handle
could be passed on to js-ctypes for instance.
It should be an number representing a pointer to the native window (like
a *GdkWindow or a HWND). I guess it should be whatever
nsWindow::GetNativeData(NS_NATIVE_WINDOW) returns but in JS;
This news thread shows a practical use case:
https://groups.google.com/d/msg/mozilla.dev.extensions/JXgOCHSK0ZU/bR5A2ZCZV3sJ
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/0
------------------------------------------------------------------------
On 2012-06-02T13:32:24+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
I'd be happy to work on this one.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/1
------------------------------------------------------------------------
On 2012-06-07T19:12:50+00:00 Roc-ocallahan wrote:
The basic idea sounds reasonable to me. You'll have to cope with the
possibilty of this method returning null. That will certainly be true
for all non-root windows.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/2
------------------------------------------------------------------------
On 2012-06-13T21:09:50+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Created attachment 632872
basic implementation
See attachment for a basic/proof-of-concept implementation: nativeHandle
returns the actual address of the native window as a jsval (not a
pointer as one could expecte).
However, I begin to have doubts about the usefulness of such a property:
1. getting a handle is relatively easy with ctypes. Extensions do that
well already: see
http://forums.mozillazine.org/viewtopic.php?f=19&t=2449775 (windows) or
https://addons.mozilla.org/en/firefox/addon/firetray/ (linux).
2. I can't seem to be able to make use of the memory address provided by
nativeHandle: I can build a ctypes object out of it (gdk.GdkWindow.ptr),
but it does not behave properly when I try to manipulate it, for example
when fetching the GtkWindow stored in the user_data of the GdkWindow.
Here is how I re-construct the ctypes window object:
let baseWin = win.QueryInterface(Ci.nsIInterfaceRequestor)
.getInterface(Ci.nsIWebNavigation)
.QueryInterface(Ci.nsIDocShellTreeItem)
.treeOwner
.QueryInterface(Ci.nsIInterfaceRequestor)
.nsIBaseWindow;
let nativeHandle = baseWin.nativeHandle;
let gdkwPtr = ctypes.uintptr_t(nativeHandle).address();
let gdkWindow = ctypes.cast(gdkwPtr, gdk.GdkWindow.ptr);
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/3
------------------------------------------------------------------------
On 2012-06-21T18:55:24+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Created attachment 635402
basic implementation as String instead of Number
chatting with Yoric, it turned out JS Numbers can only store 32 bits
numbers (hence ctypes.UInt64!). So here is another basic implementation
which returns a String that can easily be parsed by ctypes, like this:
let nativeHandle = baseWin.nativeHandle;
let gdkw = new gdk.GdkWindow.ptr(ctypes.UInt64(nativeHandle));
Note this version returns the protected pointer to the actual native
window (HWND, GdkWindow*, ...), instead of the actual address of the
window as previously implemented.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/4
------------------------------------------------------------------------
On 2012-06-21T19:42:53+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
The problems mentioned in comment 3 point 2. are gone, and it's
relatively easy to get the toplevel GtkWindow, with a
gdk_window_get_user_data() followed by a gtk_widget_get_toplevel().
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/5
------------------------------------------------------------------------
On 2012-06-29T13:31:04+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Created attachment 637885
Adds JS nativeHandler attribute for nsIBaseWindow
tested on Try: https://tbpl.mozilla.org/?tree=Try&rev=7f3510e83597 (I
hope I didn't miss anything)
Few things to note:
* nativeHandle only implemented for nsXULWindow, not for embedding nsWebBrowser, not for nsDocShell (is this needed ?)
* if nsXULWindow->mWindow is unlikely to be void, shouldn't nativeHandle throw NS_ERROR_UNEXPECTED instead returning of JSVAL_NULL ? If not, how to test a nsXULWindow without a mWindow ?
* jsapi.h included in nsIBAseWindow.idl for jsval. It this the proper way ?
Thank you
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/6
------------------------------------------------------------------------
On 2012-06-30T02:22:08+00:00 Roc-ocallahan wrote:
Comment on attachment 637885
Adds JS nativeHandler attribute for nsIBaseWindow
Review of attachment 637885:
-----------------------------------------------------------------
::: widget/nsIBaseWindow.idl
@@ +163,5 @@
> +
> + @throws NS_ERROR_NOT_IMPLEMENTED for non-toplevel widgets, or
> + NS_ERROR_UNEXPECTED if conversion fails
> + */
> + [implicit_jscontext] readonly attribute jsval nativeHandle;
Why not declare it as a DOMString instead of a jsval?
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/7
------------------------------------------------------------------------
On 2012-06-30T22:17:29+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Created attachment 638153
attribute as DOMString
There is no reason to declare the attribute as a jsval, as long as the
string is available to JS. I guess DOMString is even better.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/8
------------------------------------------------------------------------
On 2012-07-01T10:30:49+00:00 Roc-ocallahan wrote:
Comment on attachment 638153
attribute as DOMString
Review of attachment 638153:
-----------------------------------------------------------------
::: widget/nsIBaseWindow.idl
@@ +156,5 @@
> + This is the handle (HWND, GdkWindow*, ...) to the native window of the
> + control, exposed as a DOMString.
> +
> + @return DOMString in hex format with "0x" prepended, or null if mainWidget
> + undefined
It seems you don't actually return null, but an empty string if there is
no native window.
It might be better to simply return an empty string for all failure
conditions. I'm not sure it's worth trying to distinguish different
failure conditions.
::: widget/tests/test_bug760802.html
@@ +57,5 @@
> + "nativeHandle should not be implemented for nsDocShell"
> +);
> +
> +SimpleTest.ok(typeof(nativeHandle) === "string", "nativeHandle should be a string");
> +SimpleTest.ok(nativeHandle.match(/^0x[0-9a-f]+/), "nativeHandle should have a memory address format");
You don't need to prefix ok() and is() with SimpleTest, do you?
Have you tested this on Windows? Visual C++ produces capital hex digits
for %p. Maybe you should put a tolower method call somewhere so only
lower-case digits are returned by this API.
::: xpfe/appshell/src/nsXULWindow.cpp
@@ +753,5 @@
> + |Number| for instance) */
> + char* addrStr = PR_smprintf("0x%p", nativeWindowPtr);
> + if (!addrStr)
> + return NS_ERROR_UNEXPECTED;
> + aNativeHandle = NS_ConvertASCIItoUTF16(addrStr);
You could use nsPrintfCString instead for slightly simpler code (and
avoiding an error path).
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/9
------------------------------------------------------------------------
On 2012-07-01T21:06:09+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Created attachment 638225
revised with nsPrintfCString
> It might be better to simply return an empty string for all failure
conditions.
Do you mean it should not even throw NS_NOT_IMPLEMENTED for non-
XULWindows (i.e. nsDocShell and nsWebBrowser) and return an empty string
instead ?
> You don't need to prefix ok() and is() with SimpleTest, do you?
Changed applied.
> Have you tested this on Windows? Visual C++ produces capital hex digits for %p.
> Maybe you should put a tolower method call somewhere so only lower-case digits
are returned by this API.
I haven't tested on Windows myself, but my last Try
(https://tbpl.mozilla.org/?tree=Try&rev=ad3559d462f6) and this test
ok(nativeHandle.match(/^0x[0-9a-f]+$/), "nativeHandle should have a
memory address format");
tend to show that a tolower method call is not useful. nsPrintfCString
uses PR_vsnprintf, which converts using lowercase digits.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/10
------------------------------------------------------------------------
On 2012-07-01T22:57:25+00:00 Roc-ocallahan wrote:
> > It might be better to simply return an empty string for all failure conditions.
>
> Do you mean it should not even throw NS_NOT_IMPLEMENTED for non-XULWindows
> (i.e. nsDocShell and nsWebBrowser) and return an empty string instead ?
Maybe. It doesn't really matter I guess. Leave it.
> tend to show that a tolower method call is not useful. nsPrintfCString uses
> PR_vsnprintf, which converts using lowercase digits.
OK!
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/11
------------------------------------------------------------------------
On 2012-07-20T11:57:34+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Comment on attachment 638225
revised with nsPrintfCString
roc, are you able to commit the patch as I don't have authorization yet
? Or should I add the checkin-needed keyword ? Is there anything I can
do to help having the patch checked in ?
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/12
------------------------------------------------------------------------
On 2012-07-20T12:26:45+00:00 Patrick Cloke wrote:
Drive by review comments:
- The spacing in embedding/browser/webBrowser/nsWebBrowser.cpp seems wrong (it seems to use 3 spaces, the added code uses 2).
- The comment in widget/nsIBaseWindow.idl has awful whitespace (some tabs, some two spaces).
Also, setting this to assigned...since it doesn't seem to be unconfirmed
any longer.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/13
------------------------------------------------------------------------
On 2012-07-20T16:44:58+00:00 Roc-ocallahan wrote:
Comment on attachment 638225
revised with nsPrintfCString
Review of attachment 638225:
-----------------------------------------------------------------
Please address those drive-by review comments, then reattach your patch
with "checkin?" (no assignee) and someone will check it in :-)
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/14
------------------------------------------------------------------------
On 2012-07-21T08:50:40+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Created attachment 644612
with spacing fixes
Thank you Patrick for your careful review. I completely missed the
spacing issue. It looks like some files have incorrect indentation and
emacs/vim mode lines (see Bug 369322 for instance). I guess I'll need to
file specific bug reports to correct some.
I also took the opportunity to remove this correction
https://bugzilla.mozilla.org/attachment.cgi?id=638225&action=diff#a/testing/mochitest/static/xul.template.txt_sec1
which I believe belongs to a separate bug.
https://tbpl.mozilla.org/?tree=Try&rev=0cd412803918
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/15
------------------------------------------------------------------------
On 2012-07-21T12:04:38+00:00 Foudil-newbie+bugzilla-mozilla-org wrote:
Comment on attachment 644612
with spacing fixes
Re-reading Roc's comment 14, I removed the review? flag and set
checkin?.
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/16
------------------------------------------------------------------------
On 2012-07-25T01:32:27+00:00 Ryanvm wrote:
Comment on attachment 644612
with spacing fixes
You can just use checkin-needed. Also, please follow the directions below for future patches. It makes life much easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/17
------------------------------------------------------------------------
On 2012-07-25T02:07:00+00:00 Ryanvm wrote:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dcd7470dc0b
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/18
------------------------------------------------------------------------
On 2012-07-25T15:10:34+00:00 Bmo-edmorley wrote:
https://hg.mozilla.org/mozilla-central/rev/9dcd7470dc0b
Reply at: https://bugs.launchpad.net/firefox/+bug/1040313/comments/19
** Changed in: firefox
Status: Unknown => Fix Released
** Changed in: firefox
Importance: Unknown => Medium
--
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla.
https://bugs.launchpad.net/bugs/1040313
Title:
Need access to native handle for tabs
To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1040313/+subscriptions
More information about the Ubuntu-mozillateam-bugs
mailing list