Closed Bug 504220 Opened 15 years ago Closed 15 years ago

document.body and window should contain onhashchange attribute

Categories

(Core :: DOM: Events, enhancement)

x86
All
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 5 obsolete files)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #388732 - Flags: superreview?(mrbkap)
Attachment #388732 - Flags: review?(mrbkap)
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 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.
Attachment #388732 - Flags: superreview?(mrbkap)
Attachment #388732 - Flags: superreview+
Attachment #388732 - Flags: review?(mrbkap)
Attachment #388732 - Flags: review+
(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).
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #388732 - Attachment is obsolete: true
Attached patch hg exported patch for checkin (obsolete) — Splinter Review
Attachment #388791 - Attachment is obsolete: true
Attachment #388793 - Attachment is obsolete: true
Comment on attachment 388794 [details] [diff] [review]
Really hg exported patch this time.  Promise

Forwarding review from earlier patch.
Attachment #388794 - Flags: superreview+
Attachment #388794 - Flags: review+
Attachment #388794 - Attachment is obsolete: true
Attachment #388809 - Flags: superreview+
Attachment #388809 - Flags: review+
Attachment #388809 - Attachment is obsolete: true
Attachment #388812 - Flags: superreview+
Attachment #388812 - Flags: review+
Fixed in changeset 88f2dae8a5ff:
http://hg.mozilla.org/mozilla-central/rev/88f2dae8a5ff
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
-----
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: