Closed Bug 254600 Opened 21 years ago Closed 19 years ago

nsIXULTemplateBuilder.refresh() shouldn't call .rebuild()

Categories

(Core :: XUL, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: bernhard.hermann)

Details

Attachments

(2 files)

From an IRC conversation, nsIXULTemplateBuilder.refresh() shouldn't call .rebuild(), because we're normally loading the datasource async from the network, and the data has not yet updated when we try to rebuild, which can cause odd problems. <bsmedberg> we could try just removing the .rebuild() call from the .refresh() impl <bsmedberg> and making callers call .rebuild() if they really need ti <telpochyaotl> bsmedberg: if that worked that would be perfect * bsmedberg looks for r=Pike sr=Neil <Pike> granted <Neil> bsmedberg: sorry, lost you a sec... where's .refresh()? <bsmedberg> nsIXULTemplateBuilder I think * bsmedberg added it before 1.6 to allow remote chrome to update cached RDF datasources <bsmedberg> Neil: I just want to remove this line: http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULTemplateBuilder.cpp#265 <bsmedberg> and return NS_OK instead <telpochyaotl> bsmedberg: once that's done, how do i check it out? (i just have an apt-getted mozilla) <Pike> bsmedberg: if we'd want much, we could QI to nsIRDFXMLSink and add an observer, which, after being called enough, would call rebuild() <Neil> bsmedberg: refreshing won't actually cause notifications, will it? <bsmedberg> Neil: yes, it does <bsmedberg> Neil: we use a mark and sweep algorithm <Pike> I'm not talking about an nsIRDFObserver, btw, but an nsIRDFXMLSinkObserver <Neil> bsmedberg: ah, so the sweep clears out any dead assertions? <bsmedberg> right <Neil> bsmedberg: great. sr=me
Attached patch Don't .rebuild()Splinter Review
I have committed attachment 155367 [details] [diff] [review]. I'm going to leave this bug open for Pike's suggestion to install an nsIRDFXMLSink observer.
Assignee: benjamin → axel
This is an implementation of the suggestion by bsmedberg/Pike to do a Rebuild() after an async Refresh() via an nsIRDFXMLSink. Works smoothly with seamonkey-1.0.2-sources. Unfortunately I wasn't able to test against the CVS sources, because I can't get them to compile. The patch is against the current CVS though, and I strongly suppose it to work.
Assignee: axel → bernhard.hermann
Attachment #228804 - Flags: review?(enndeakin)
#define @ asm { NOP; }
Severity: normal → enhancement
(In reply to comment #4) > #define @ asm { NOP; } > uh... like.. oops.
The template builder should be notified of changes while the datasource is loading already, ingoring bugs in handling certain cases. I'm not convinced that refresh should always rebuild afterwards.
Status: NEW → ASSIGNED
(In reply to comment #6) > The template builder should be notified of changes while the datasource is > loading already, ingoring bugs in handling certain cases. I'm not convinced > that refresh should always rebuild afterwards. > We were thinking it doesn't work in JS, that's why I wrote this "enhancement". But apparently {Pike,bsmedberg}'s suggestion isn't needed. Shall I resolve the bug? I would add a boolean to Refresh(), so it becomes something like Refresh(boolean rebuildAfterwards). But it would require a change to the header of the method, which is rather inconvenient, isn't it?
no responses, bug closed to reduce clutter
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #228804 - Flags: review?(enndeakin)
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: