Closed
Bug 504220
Opened 15 years ago
Closed 15 years ago
document.body and window should contain onhashchange attribute
Categories
(Core :: DOM: Events, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 5 obsolete files)
9.22 KB,
patch
|
justin.lebar+bug
:
review+
justin.lebar+bug
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #388732 -
Flags: superreview?(mrbkap)
Attachment #388732 -
Flags: review?(mrbkap)
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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+
Comment 4•15 years ago
|
||
(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).
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #388732 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #388791 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
Attachment #388793 -
Attachment is obsolete: true
Assignee | ||
Comment 8•15 years ago
|
||
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+
Assignee | ||
Comment 9•15 years ago
|
||
Attachment #388794 -
Attachment is obsolete: true
Attachment #388809 -
Flags: superreview+
Attachment #388809 -
Flags: review+
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #388809 -
Attachment is obsolete: true
Attachment #388812 -
Flags: superreview+
Attachment #388812 -
Flags: review+
Assignee | ||
Comment 11•15 years ago
|
||
Fixed in changeset 88f2dae8a5ff: http://hg.mozilla.org/mozilla-central/rev/88f2dae8a5ff
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 12•15 years ago
|
||
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 -----
Assignee | ||
Comment 13•15 years ago
|
||
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.
Updated•15 years ago
|
Keywords: dev-doc-needed
Updated•15 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•