504220 - document.body and window should contain onhashchange attribute

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Justin Lebar (not reading bugmail)]

We want |"onhashchange" in document.body| and |"onhashchange" in window| to return true so a webpage can detect that the browser supports the hashchange event.

Additionally, changes to document.body.onhashchange should be reflected in window.onhashchange, and vice versa.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Proposed patch

Comment 2

[Nochum Sossonko [:Natch]]

1) You needn't set *_retval afaik if you're returning an error.
2) You need to set *_retval if you're returning success.

This patch breaks XPCOM rules iiuc.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 388732 [details] [diff] [review]
Proposed patch

Nitpicks only:

>diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp
>+NS_IMETHODIMP
>+nsHTMLBodyElementSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext
>+                                *cx, JSObject *obj, jsval id, PRUint32 flags,
>+                                JSObject **objp, PRBool *_retval)
>+{
>+    if (!(::JS_ValueToId(cx, id, &interned_id) &&
>+          JS_DefinePropertyById(cx, obj, interned_id, JSVAL_VOID,
>+                                nsnull, nsnull, JSPROP_ENUMERATE)))

I'd De Morgan's law this to !JS_ValueToId(...) || !JS_DefineProperty. Also, please be consistent about whether you use :: before JS_.

Also, please follow the K&R bracing style, with the first brace on the same line as the condition.

>+    {
...
>+NS_IMETHODIMP
>+nsHTMLBodyElementSH::GetProperty(nsIXPConnectWrappedNative *wrapper,
>+                                 JSContext *cx, JSObject *obj, jsval id,
>+                                 jsval *vp, PRBool *_retval)
>+{
>+    if (!(::JS_ValueToId(cx, id, &interned_id) &&
>+          JS_GetPropertyById(cx, JS_GetGlobalForObject(cx, obj),
>+                             interned_id, vp)))

Ditto.

>+    {
...
>+NS_IMETHODIMP
>+nsHTMLBodyElementSH::SetProperty(nsIXPConnectWrappedNative *wrapper,
>+                                 JSContext *cx, JSObject *obj,
>+                                 jsval id, jsval *vp, PRBool *_retval)
>+{
>+    if (!(::JS_ValueToId(cx, id, &interned_id) &&
>+          JS_SetPropertyById(cx, JS_GetGlobalForObject(cx, obj),
>+                             interned_id, vp)))

Ibid.

>+    {
...
>@@ -6378,16 +6449,32 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
>+    if (!(::JS_ValueToId(cx, id, &interned_id) &&
>+          JS_DefinePropertyById(cx, obj, interned_id, JSVAL_VOID,
>+                                nsnull, nsnull, JSPROP_ENUMERATE)))

...

>+    {

r+sr=mrbkap with those nits picked.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> This patch breaks XPCOM rules iiuc.

The functions that Justin is implementing are part of the nsIXPCScriptable interface. They *look* like XPCOM things, but they're actually not. _retval and the failure return code are sort of unrelated (he could return NS_OK there too).

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch for checkin

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: hg exported patch for checkin

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Really hg exported patch this time. Promise

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 388794 [details] [diff] [review]
Really hg exported patch this time.  Promise

Forwarding review from earlier patch.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: hg exported patch with checkin comment

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Now names reviewer. I think this is the last one.

Comment 11

[Justin Lebar (not reading bugmail)]

Fixed in changeset 88f2dae8a5ff:
http://hg.mozilla.org/mozilla-central/rev/88f2dae8a5ff

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

This patch apparently reduced TXul by 7% on Windows? Not quite sure how that might have happened ..:

-----
Improvement: Txul decrease 7.33% on XP Firefox

Previous results: 99.1053 from build 20090715162751 of revision ebea850caba8 at 2009-07-15 16:27:00 on talos-rev2-xp04 run # 0

New results: 91.8421 from build 20090715170408 of revision 88f2dae8a5ff at 2009-07-15 17:04:00 on qm-pxp-fast03 run # 0

Suspected checkin range: from revision ebea850caba8 to revision 88f2dae8a5ff
-----

Comment 13

[Justin Lebar (not reading bugmail)]

re comment(In reply to comment #12)
> This patch apparently reduced TXul by 7% on Windows? Not quite sure how that
> might have happened ..:

Looks like this is probably incorrect, in some way or another.  Nothing we did should have improved the speed of the test.

Comment 14

[Fabian (Crash) Grutschus]

https://developer.mozilla.org/en/DOM/window.onhashchange