Last Comment Bug 311090 - Patch for bug 310351 was mildly bogus
: Patch for bug 310351 was mildly bogus
: fixed1.8, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Depends on: 311151
Blocks: 310351 311067 311094 311104 311107 311119 311171
  Show dependency treegraph
Reported: 2005-10-04 11:09 PDT by Blake Kaplan (:mrbkap)
Modified: 2005-10-05 12:12 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

As described (3.03 KB, patch)
2005-10-04 11:10 PDT, Blake Kaplan (:mrbkap)
jst: review+
brendan: superreview+
mscott: approval1.8b5+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2005-10-04 11:09:26 PDT
My method of getting the length was incorrect for _generic_ arrays, where we
can't make any assumptions about our data. Instead, we should go through
javascript (and then back through XPConnect, ick.
Comment 1 Blake Kaplan (:mrbkap) 2005-10-04 11:10:33 PDT
Created attachment 198474 [details] [diff] [review]
As described
Comment 2 Brendan Eich [:brendan] 2005-10-04 11:20:04 PDT
Comment on attachment 198474 [details] [diff] [review]
As described

This is ok, but you might change the JSVAL_IS_INT fretting to cope with
unsigned 32 bit values that do not fit in the nonnegative half of the signed 31
bit domain of int jsvals.

But that's for later, on the trunk.  For this patch, just change the XXX
comment to say how this can really happen (sparse array delegating to the DOM

Comment 3 Blake Kaplan (:mrbkap) 2005-10-04 11:34:49 PDT
Comment on attachment 198474 [details] [diff] [review]
As described

Prematurely requesting approval, this patch (re-)fixes
Comment 4 Scott MacGregor 2005-10-04 13:22:54 PDT
I e-mailed jst directly this morning asking him to review this ASAP.
Comment 5 Scott MacGregor 2005-10-04 13:30:43 PDT
Comment on attachment 198474 [details] [diff] [review]
As described

I'm going to approve this for the branch. jst, doesn't have internet
connectivity at the moment and we'd rather gets some bits up that we can all
start testing instead of waiting. 

Leaving the review request alone, so we can address any review comments from
jst later.
Comment 6 Blake Kaplan (:mrbkap) 2005-10-04 13:44:40 PDT
Checked in on trunk and MOZILLA_1_8_BRANCH.
Comment 7 Johnny Stenback (:jst, 2005-10-04 14:01:46 PDT
Comment on attachment 198474 [details] [diff] [review]
As described

Comment 8 Peter van der Woude [:Peter6] 2005-10-04 14:24:26 PDT
should Bug 311119 be fixed py this patch too ?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2005-10-04 17:53:35 PDT
Hmm.. This had noticeable (0.5%?) Tp impact on btek.  Anything we can do about that?
Comment 10 Brendan Eich [:brendan] 2005-10-04 19:03:17 PDT
(In reply to comment #9)
> Hmm.. This had noticeable (0.5%?) Tp impact on btek.  Anything we can do about
> that?

Sure -- at the cost of a QI test, restore the lost fast path that resulted in
this bug, and do the full get-property on 'length' if the QI to nsIDOMNodeList


Comment 11 Blake Kaplan (:mrbkap) 2005-10-04 19:20:12 PDT
bug 311151 filed to track the Tp regression.
Comment 12 Peter van der Woude [:Peter6] 2005-10-05 12:12:57 PDT
Blake, Bug 311204 has the same regressionwindow as all others here.
Could it have been caused by bug 310351 but not fixed by this bug ?

Note You need to log in before you can comment on or make changes to this bug.