Comment 11 for bug 1040313

Revision history for this message
In , Roc-ocallahan (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).