Closed
Bug 493701
Opened 15 years ago
Closed 10 years ago
Use TObserverArray for DocLoader Listeners
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: Swatinem, Assigned: Swatinem)
References
Details
Attachments
(3 files, 3 obsolete files)
4.72 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
21.36 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.17 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Per bug 474369 comment 140: It might be nice to have a macro that takes a flag-set and some code and then basically does the whole "iterate the array, skip infos that don't have these flags, call this code on the others" thing. That should be a followup bug, though. The code in question looks like this: // First notify any listeners of the new state info... nsCOMPtr<nsIWebProgressListener> listener; nsTObserverArray<nsListenerInfo>::EndLimitedIterator iter(mListenerInfoList); while (iter.HasMore()) { nsListenerInfo &info = iter.GetNext(); if (!(info.mNotifyMask & _FLAG_)) { continue; } listener = do_QueryReferent(info.mWeakListener); if (!listener) { // the listener went away. gracefully pull it out of the list. mListenerInfoList.RemoveElementAt(&info - mListenerInfoList.Elements()); continue; } listener->_CODE_; } mListenerInfoList.Compact();
Comment 1•15 years ago
|
||
Arpad, do you want to take this?
Assignee | ||
Comment 2•15 years ago
|
||
Yes, I take this once the original patch has re-landed.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•15 years ago
|
||
This patch also adds the tests suggested in bug 474369 comment 155.
Attachment #382682 -
Flags: review?(bzbarsky)
Updated•15 years ago
|
Attachment #382682 -
Flags: review?(bzbarsky) → review+
Comment 4•15 years ago
|
||
Comment on attachment 382682 [details] [diff] [review] patch [pushed: comment 5] 1) Use PR_BEGIN_MACRO and PR_END_MACRO instead of do/while (it's the same in the end, but clearer as to why it's there). 2) Put parens around _flags in the macro body. With those nits, looks good.
Assignee | ||
Comment 5•15 years ago
|
||
Comment on attachment 382682 [details] [diff] [review] patch [pushed: comment 5] Pushed as http://hg.mozilla.org/mozilla-central/rev/2f2586790f32 with nits fixed.
Attachment #382682 -
Attachment description: patch → patch [pushed: comment 5]
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 6•15 years ago
|
||
Regression: DHTML increase 0.98% on XP Firefox: http://graphs-new.mozilla.org/graph.html#tests=[{%22machine%22:55,%22test%22:18,%22branch%22:1},{%22machine%22:56,%22test%22:18,%22branch%22:1},{%22machine%22:57,%22test%22:18,%22branch%22:1},{%22machine%22:71,%22test%22:18,%22branch%22:1},{%22machine%22:108,%22test%22:18,%22branch%22:1},{%22machine%22:109,%22test%22:18,%22branch%22:1},{%22machine%22:110,%22test%22:18,%22branch%22:1},{%22machine%22:111,%22test%22:18,%22branch%22:1}]&sel=1244728380,1244901180 Previous results: 1083.47 from build 20090612062022 of revision 04ed36482fa9 at 2009-06-12 06:20:00 on qm-pxp-fast03 run # 0 New results: 1094.12 from build 20090612065328 of revision 2f2586790f32 at 2009-06-12 06:53:00 on qm-pxp-trunk01 run # 0 (this and bug 474369 fall into the suspected regression range)
Comment 7•15 years ago
|
||
It's more likely to be the other than this one; this bug shouldn't change what the compiled code looks like.
Assignee | ||
Comment 8•15 years ago
|
||
I backed this out together with bug 474369 for performance regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Summary: Use a macro to reduce redundancy in nsDocLoader.cpp → Use TObserverArray for DocLoader Listeners
Assignee | ||
Comment 9•15 years ago
|
||
I split out the related changes from bug 474369, forwarding r=bz, as you have also reviewed the docloader changes from the other bug. Status report is following in the other bug as well.
Attachment #382682 -
Attachment is obsolete: true
Attachment #385614 -
Flags: review+
Want to try to reland this?
Assignee | ||
Comment 11•15 years ago
|
||
I'm on vacation starting tomorrow so I won't be able to watch the tree. Also I'm not sure what happened to the dhtml regressions.
Assignee | ||
Comment 12•14 years ago
|
||
Long time no progress here. I folded parts of bug 474369 into this one, so the two patches are not dependent any more. Otherwise its just minor changes unbitrotting the patch, I won’t bother you with requesting review again.
Attachment #385614 -
Attachment is obsolete: true
Attachment #466290 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8406816 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•10 years ago
|
||
Wow, lets resurrect some 4 year old zombie. I would feel more comfortable if you could review again :-)
Attachment #466290 -
Attachment is obsolete: true
Attachment #8406817 -
Flags: review?(bzbarsky)
Comment 15•10 years ago
|
||
Comment on attachment 8406816 [details] [diff] [review] part 1: add nsTObserverArray::BackwardIterator r=me
Attachment #8406816 -
Flags: review?(bzbarsky) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8406817 [details] [diff] [review] part2: Use TObserverArray for DocLoader Listeners s/PRBool/bool/, please. I worry a bit about quadratic behavior from RemoveEmptyListeners... maybe it's ok. And maybe call it RemoveDeadListeners? r=me
Attachment #8406817 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #16) > I worry a bit about quadratic behavior from RemoveEmptyListeners... maybe > it's ok. And maybe call it RemoveDeadListeners? Reading my own 5 year old code, the method is only there because BackwardIterator does not expose the current index, and has no Remove() method. How about adding such a method instead?
Comment 18•10 years ago
|
||
Makes sense to me.
Assignee | ||
Comment 19•10 years ago
|
||
Alright, here it is. I will probably fold this into the first 2 patches for landing.
Attachment #8409289 -
Flags: review?(bzbarsky)
Comment 20•10 years ago
|
||
Comment on attachment 8409289 [details] [diff] [review] iteratorremove.patch Please document that calling Remove() won't affect what the ext GetNext() call returns. r=me, and sorry for the lag...
Attachment #8409289 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Comment added. Also, I folded the 3rd patch into the first two, like I said. Thanks. https://hg.mozilla.org/integration/mozilla-inbound/rev/154fb9a9e0bc https://hg.mozilla.org/integration/mozilla-inbound/rev/f615a0532971
Comment 22•10 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/73055a66d95a for across-the-board Cpp bustage, e.g.: https://tbpl.mozilla.org/php/getParsedLog.php?id=38558037&tree=Mozilla-Inbound
Assignee | ||
Comment 23•10 years ago
|
||
static int test27Expected[] = { 1, 8, 2, 7, 4, 9, 3 }; […] - static int test28Expected[] = { 1, 8, 2, 7, 4, 3 }; + static int test28Expected[] = { 1, 2, 7, 4, 9, 3 }; oh boy, since the expected is in iteration order, ofc the 2nd element from the front is removed. I will reland once I get the tests to run locally, having some problem with that.
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Arpad Borsos (Swatinem) from comment #23) > static int test27Expected[] = { 1, 8, 2, 7, 4, 9, 3 }; > […] > - static int test28Expected[] = { 1, 8, 2, 7, 4, 3 }; > + static int test28Expected[] = { 1, 2, 7, 4, 9, 3 }; > > oh boy, since the expected is in iteration order, ofc the 2nd element from > the front is removed. // Removal using Iterator DO_TEST(BackwardIterator, test27Expected, + // when this code runs, |GetNext()| has only been called once, so + // this actually removes the very first element if (count == 1) iter.Remove(); ); - static int test28Expected[] = { 1, 2, 7, 4, 9, 3 }; + static int test28Expected[] = { 8, 2, 7, 4, 9, 3 }; So it is actually the first element, since |GetNext()| is called after _code. Sorry for the churn.
Assignee | ||
Comment 25•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bff4e97827d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/30f8577510ea
https://hg.mozilla.org/mozilla-central/rev/bff4e97827d2 https://hg.mozilla.org/mozilla-central/rev/30f8577510ea
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•