Closed
Bug 289263
Opened 19 years ago
Closed 19 years ago
document.all.tags no longer working
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.8beta3
People
(Reporter: iamawalrus, Assigned: jst)
References
()
Details
Attachments
(1 file)
1.16 KB,
patch
|
bzbarsky
:
review+
brendan
:
superreview+
dveditz
:
approval-aviary1.0.8-
asa
:
approval1.7.11-
dveditz
:
approval1.7.13-
brendan
:
approval1.8b3+
|
Details | Diff | Splinter Review |
bug 248549 implemented document.all.tags. But after patch for bug 256640 was checked in, it does not work anymore. I found there's some difference between the patch posted on bug 256640 and the actual one being checked in. The patch posted on the bug does not break the function. The affected part is: @@ -5713,20 +5875,29 @@ nsHTMLDocumentSH::CallToGetPropMapper(JS return JS_FALSE; } // Convert all types to string. JSString *str = ::JS_ValueToString(cx, argv[0]); if (!str) { return JS_FALSE; } - return ::JS_GetUCProperty(cx, JSVAL_TO_OBJECT(argv[-2]), - ::JS_GetStringChars(str), + JSObject *self = JSVAL_TO_OBJECT(argv[-2]); + + if (::JS_ObjectIsFunction(cx, self)) { + // If self (aka this) is a function object we're called through + // document.all.item(), or something similar. In such a case, the + // this object is the parent of argv[-2]. + + self = ::JS_GetParent(cx, self); + } + + return ::JS_GetUCProperty(cx, self, ::JS_GetStringChars(str), ::JS_GetStringLength(str), rval); } static inline JSObject * GetDocumentAllHelper(JSContext *cx, JSObject *obj) { while (obj && JS_GET_CLASS(cx, obj) != &sHTMLDocumentAllHelperClass) { obj = ::JS_GetPrototype(cx, obj);
Comment 1•19 years ago
|
||
And we landed that on the branches too... fun. In any case, the problem is that document.all.tags() is indeed something that looks like a function call, and it's _not_ happening on an object of class sHTMLDocumentAllClass.
Flags: blocking1.8b3?
Comment 2•19 years ago
|
||
Or perhaps the problem is that it actually is happening on an sHTMLDocumentAllClass but we want to be taking the other branch (the argv[-2] one)?
Assignee | ||
Comment 3•19 years ago
|
||
Anyone got a testcase for this laying around?
Comment 4•19 years ago
|
||
I wrote a testcase for bug 253408.
Updated•19 years ago
|
Comment 5•19 years ago
|
||
On that testcase, the |str| passed to nsHTMLDocumentSH::CallToGetPropMapper is "p", and |obj| is of class sHTMLDocumentAllClass. We want to be ending up in the argv[-2] branch, and we aren't.
Assignee | ||
Comment 6•19 years ago
|
||
This fixes the incorrect logic where we determine if 'this' is passed as argv[-2] or as obj. If argv[-2] is a function, use obj, if not, use argv[-2]. With this patch the following cases work: document.all.foo (=== .all['foo']) document.all('foo') document.all.item('foo') document.all.tags.p (=== .tags['p']) document.all.tags('p') and I think that's all we support.
Attachment #185082 -
Flags: superreview?(brendan)
Attachment #185082 -
Flags: review?(bzbarsky)
Updated•19 years ago
|
Attachment #185082 -
Flags: review?(bzbarsky) → review+
Comment 7•19 years ago
|
||
Comment on attachment 185082 [details] [diff] [review] Fix broken logic sr+a=me for 1.8b3. /be
Attachment #185082 -
Flags: superreview?(brendan)
Attachment #185082 -
Flags: superreview+
Attachment #185082 -
Flags: approval1.8b3+
Updated•19 years ago
|
Assignee: general → jst
Flags: blocking1.8b3? → blocking1.8b3+
Target Milestone: --- → mozilla1.8beta3
Updated•19 years ago
|
Summary: document.all.tags no longer works → document.all.tags no longer work
Comment 8•19 years ago
|
||
Technutz, please don't make the Summary ungrammatical again. The subject of that sentence is singular even though it ends in .tags. /be
Summary: document.all.tags no longer work → document.all.tags no longer working
Assignee | ||
Updated•19 years ago
|
Attachment #185082 -
Flags: approval-aviary1.0.5?
Assignee | ||
Comment 9•19 years ago
|
||
Fixed on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Attachment #185082 -
Flags: approval1.7.9?
Comment 10•19 years ago
|
||
Is this by any chance what caused some blogger problems: http://reporter.mozilla.org/app/query/?host_hostname=www.blogger.com&submit_query=true
Comment 11•19 years ago
|
||
Comment on attachment 185082 [details] [diff] [review] Fix broken logic 1.0.5 and 1.7.9 have already shipped; unsetting approval requests.
Attachment #185082 -
Flags: approval1.7.9?
Attachment #185082 -
Flags: approval-aviary1.0.5?
Comment 12•19 years ago
|
||
Comment on attachment 185082 [details] [diff] [review] Fix broken logic Asa, we still need to take this on branch, since this is still broken there. Given that we've managed to ship the release this really should have been fixed in without even looking at the approval flags, perhaps we should approve it for the _next_ release, eh?
Attachment #185082 -
Flags: approval1.7.11?
Attachment #185082 -
Flags: approval-aviary1.0.7?
Updated•19 years ago
|
Attachment #185082 -
Flags: approval1.7.11? → approval1.7.11-
Updated•19 years ago
|
Flags: blocking1.7.12+
Updated•19 years ago
|
Attachment #185082 -
Flags: approval1.7.12?
At this point we're better off just fixing this in 1.5. No point tweaking our API for Web developers yet again.
Flags: blocking1.7.13-
Flags: blocking1.7.13+
Flags: blocking1.7.12-
Comment 14•19 years ago
|
||
Comment on attachment 185082 [details] [diff] [review] Fix broken logic branch approval denied per comment 13
Attachment #185082 -
Flags: approval1.7.13?
Attachment #185082 -
Flags: approval1.7.13-
Attachment #185082 -
Flags: approval-aviary1.0.8?
Attachment #185082 -
Flags: approval-aviary1.0.8-
You need to log in
before you can comment on or make changes to this bug.
Description
•