http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULContentSink.cpp#895 895 // XXX For now, we don't add the PI to the content model. 896 // We just check for a style sheet PI You can check it out for yourself with domi. Dynamically-added processing instructions show up just fine. A lot of places in the codebase assume there are no PIs in the content model, I'll file bugs on fixing that assumption.

Please mark those bugs as blockers for this bug.

This is a good list to start with: http://lxr.mozilla.org/mozilla/search?string=document.firstChild

The codebase is now clear of bogus document.firstChilds.

So we probably need an nsXULPrototypePI or something...

Someone would need to step up and fix this.

This should do it. Also fixes bug 183505, at least partially.

I went with a simple nsXULPrototypePI storing target/data strings in the cache; PI-specific parsing now happens when building the content model for a document. This will work a tad slower, but since there are few processing instructions in general, the performance hit should be low. On the other hand, this should take slightly less memory. The stylesheets are loaded synchronously, as before. To do that I had to add LoadStyleLinkSync to nsCSSLoader (as hinted in bug 183505). (I tried doing asynchronous loading first, but it was rather hard, and I decided it was not worth the effort.) ----- I'm not sure what's the deal with 'rv |= ...' code, but used the same pattern, even though I don't understand why/if this always leads to meaningful error codes.

I'll try to get to this sometime in the next week. I don't know this code as well as some other parts of Gecko, though, so it'll take a bit of work.

Fixed a few jst-review nits and an issue with nsXULPrototypePI appearing to leak according to refcount balancer (I |delete|d it instead of |Release|ing).

> I'm not sure what's the deal with 'rv |= ...' code, It's used when we just care about success vs fail and not the exact failure. The resulting rv will generally be bogus, but will be failure if any of the things involved failed.

So doing sync loads of general PIs in XUL is not good. Right now we do async ones from the sink, no? Why did you have to switch to sync? In particular, any remote XUL that points to an http: stylesheet will be Very Bad with this code (sync load of things from the web is not good).

In particular: The stylesheets are loaded synchronously, as before. confuses me, since before they are loaded asynchronously.

Thanks for your comments. > > I'm not sure what's the deal with 'rv |= ...' code, > > It's used when we just care about success vs fail and not the exact failure. > The resulting rv will generally be bogus, but will be failure if any of the > things involved failed. Yes, that makes sense, but this means we shouldn't return the rv as is, as we do in nsXULPrototypeDocument::Read, no? > So doing sync loads of general PIs in XUL is not good. Right now we do async > ones from the sink, no? Why did you have to switch to sync? Right now, we load the sheets added via PIs asynchronously from the sink when not fastloading (I was slightly confused and thought we actually loaded them twice in this case - from the sync and in AddPrototypeSheets). When I wrote about loading "synchronously as before", I was thinking about the fastload case. The reasons I didn't go with loading stylesheets async are: 1. Right now the sheets are loaded before building the content model from the prototype starts. I didn't want to change this, because I'm not familiar enough with the system to know the consequences. 2. We can't just load the sheet from the sink, as there's no node for the PI created at that point. 3. Loading stylesheets async from nsXULDocument::PrepareToWalk, suspending the walking process until they're done loading, would be non-trivial and complicate ResumeWalk even further. 4. I didn't think hard enough about remote XUL.

> as we do in nsXULPrototypeDocument::Read, no? I think all the fastload code basically treats return values as a binary success-or-error thing. So this is ok. > 1. Right now the sheets are loaded before building the content model from the > prototype starts. I didn't want to change this, because I'm not familiar > enough with the system to know the consequences. Hmm. I think we do in fact want to change this. Otherwise we have weird behavior differences between the fastload/non-fastload case in terms of onload, etc. In my opinion. > suspending the walking process until they're done loading Would we need to suspend walking? Could we get away with just continuing to walk and using the fact that onload is blocked by those stylesheets? Or would we need to also delay the StartLayout call I guess?

Updated per bz's comments. The sheets now load async with StartLayout (or xul-overlay-merged notification) being held up until all sheets are loaded. The patch has some changes not necessary for this bug, namely - making a few ResumeWalk callers use OnPrototypeLoadDone instead - ensuring that style overlays are loaded w/o differences in the fastload and non-fastload cases. In particular I've made a functional change - non-chrome style overlays are no longer loaded (before they were only loaded in the non-fastload case). I can try to make a patch without those changes, if desired.

Comment on attachment 240837 [details] [diff] [review] patch v2 >Index: content/xul/content/src/nsXULElement.cpp You need to change nsXULPrototypeElement::Deserialize to handle children of type eType_PI, no? Or do we not put them in the DOM? >Index: content/xul/document/public/nsIXULDocument.h Rev the iid? >Index: content/xul/document/public/nsIXULPrototypeDocument.h >+ NS_IMETHOD AddProcessingInstruction(nsXULPrototypePI* aPI) = 0; Document? I know that's not file style; time to change that style. ;) Especially document the ownership model weirdness here (that is, that the caller is supposed to pass in an already-addrefed aPI. In fact, maybe just make the argument be an already_AddRefed<nsXULPrototypePI>. >+ NS_IMETHOD GetProcessingInstructions(nsVoidArray** aResult) = 0; This part I don't like. In particular, the XPCOM ownership model would imply that the caller is getting it's own copy of the array, which is so not what we want here. How about: virtual const nsTArray<nsXULPrototypePI*>& GetProcessingInstructions() const = 0; That would make me much happier, and is likely to make the caller code cleaner. You'd need to make the member an nsTArray<nsXULPrototypePI*>, of course, but I think you should. Though if you make the arg to AddProcessingInstruction already_AddRefed<nsXULPrototypePI>, you could use nsTArray<nsRefPtr<nsXULPrototypePI> > for the member and probably create a typedef for that type to make code more readable. Or leave the existing destructor code, I guess (and use .get() in AddProcessingInstruction to get a raw addrefed pointer). > // CID for factory-based creation, used only for deserialization. > #define NS_XULPROTOTYPEDOCUMENT_CLASSNAME "XUL Prototype Document" > #define NS_XULPROTOTYPEDOCUMENT_CID \ >- {0xa08101ae,0xc0e8,0x4464,{0x99,0x9e,0xe5,0xa4,0xd7,0x09,0xa9,0x28}} >+ {0x9fca71ea,0xc560,0x4ab4,{0xba,0x03,0xc1,0x76,0xfe,0x3d,0x8f,0x36}} I don't think you need to change the CID. >Index: content/xul/document/src/nsXULContentSink.cpp > XULContentSinkImpl::HandleProcessingInstruction(const PRUnichar >+ return mPrototype->AddProcessingInstruction(pi); So we don't put PIs into their correct place in the DOM? >Index: content/xul/document/src/nsXULDocument.cpp >@@ -408,24 +414,16 @@ nsXULDocument::StartDocumentLoad(const c >- if (loaded) { >- rv = AddPrototypeSheets(); >- if (NS_FAILED(rv)) return rv; Why this change? >@@ -489,100 +489,83 @@ nsXULDocument::EndLoad() > nsICSSLoader* cssLoader = CSSLoader(); This seems to be unused now? >- cssLoader->LoadSheetSync(sheetURI, getter_AddRefs(sheet)); Why is it ok to remove this code? What's replacing it? In particular, what happens if useXULCache is false or the sheet is not a chrome: sheet? >+ OnPrototypeLoadDone(); Is this why we no longer need that AddPrototypeSheets() call in StartDocumentLoad()? Is this code called in that case (prototype done in cache)? I got up to the start of nsXULDocument::OnPrototypeLoadDone so far.

Comment on attachment 240837 [details] [diff] [review] patch v2 > Index: content/xul/document/public/nsIXULDocument.h >+ NS_IMETHOD OnPrototypeLoadDone(PRBool aCallerResumesWalk = PR_FALSE) = 0; Please make this a non-optional arg -- optional args make the code a lot harder to follow. And probably reverse the sense (so PR_TRUE means to call ResumeWalk). I think that would be clearer... and certainly fit better with the way you're using this arg when you do pass it in. >Index: content/xul/document/src/nsXULDocument.cpp >+nsXULDocument::OnPrototypeLoadDone(PRBool aCallerResumesWalk) The documentation for this function should indicate whether it applies to cases when we found an already-loaded prototype in the proto cache... I assume this is not called in those cases, right? >@@ -2397,37 +2376,16 @@ nsXULDocument::PrepareToWalk() >@@ -2444,21 +2402,67 @@ nsXULDocument::PrepareToWalk() >+ int piCount = 0; Why not PRInt32? >+ if (mState != eState_Master) { >+ piCount = IndexOf(GetRootContent()); So is this enshrining the lack of comment nodes, doctype nodes, etc in XUL? This seems suboptimal to me. It seems to me like this would work better if we actually had the PI proto nodes at the right places in the DOM... >+ // When code in ResumeWalk needs to get an overlay to load, it takes >+ // the last element of mUnloadedOverlays (for the load order to be sane). >+ // However, we want overlays specified using <?xul-overlay ?> to be >+ // applied in the same order processing instructions are in. To do this, >+ // we insert the overlay URIs (in the document order) at the following index: >+ PRUint32 overlaysCount = mUnloadedOverlays.Count(); I think it would be clearer to say: We insert overlays into mUnloadedOverlays at the same index in document order, so they end up in the reverse of the document order in mUnloadedOverlays. Or something like that. >+ if (protoPI->mTarget.EqualsLiteral("xml-stylesheet")) { >+ rv = ProcessXMLStylesheetPI(protoPI, node, piCount); This will, given the rest of this patch, load stylesheets from PIs that are not in the prolog. That seems wrong. >+ } else if (protoPI->mTarget.EqualsLiteral("xul-overlay")) { >+ rv = ProcessXULOverlayPI(protoPI, node, piCount, overlaysCount); Same issue with PIs not in the prolog. Not sure what's correct here. >+nsXULDocument::ProcessXMLStylesheetPI(const nsXULPrototypePI* aProtoPI, >+ // We're assuming that we were passed a PI node and that the only PI >+ // node that implements nsIStyleSheetLinkingElement is nsXMLStylesheetPI I don't see where we assume the latter. >+ // XXX This duplicates the logic of >+ // nsXMLStylesheetPI::GetStyleSheetInfo/URL >+ nsAutoString type; >+ nsParserUtils::GetQuotedAttributeValue(aProtoPI->mData, nsGkAtoms::type, type); So... why not just use those? That is, why not just put the PI into the DOM, without disabling updates, and let it handle the load? Was there a good reason for that? That is, see what nsXMLContentSink::HandleProcessingInstruction does. You'd have to deal with NS_ERROR_HTMLPARSER_BLOCK meaning success (and in fact meaning you need to increment your pending sheets thing), but that's ok... Though there's the complication that alternate sheets won't return NS_ERROR_HTMLPARSER_BLOCK, so you'd not count them (and use the aWasAlternate arg to StyleSheetLoaded for great justice). Unless we _want_ to block the walk for alternate sheets? >+nsXULDocument::ProcessXULOverlayPI(const nsXULPrototypePI* aProtoPI, >+ nsIContent* aNode, PRUint32 aIndex, >+ PRUint32 overlaysCount) aOverlaysCount ? Or more correctly aOverlayInsertionPoint? >+ // XXX This is wrong, the error message could be out of memory >+ // or something else equally bad, which we should propagate. >+ // Bad URL should probably be "success with info" value. Indeed. Why not just convert NS_ERROR_INVALID_URI to NS_OK? >+ mUnloadedOverlays.InsertObjectAt(uri, overlaysCount); Propagate error if that fails? Same for other places where you insert things into arrays. >+nsXULDocument::DoneWalking() >+#if 0 >+ printf("nsXULDocument::DoneWalking: master=%d current=%d; " >+ "mPendingSheets=%d, mStillWalking=%d\n", >+ mMasterPrototype, mCurrentPrototype, >+ mPendingSheets, mStillWalking); >+#endif Don't check that in? Or make it a relevant PR_LOG message? >Index: content/xul/document/src/nsXULDocument.h >+ // nsICSSLoaderObserver >+ NS_IMETHOD StyleSheetLoaded(nsICSSStyleSheet* aSheet, Put this before the static method? >+ * Since ResumeWalk is interruptible, it's possible that last >+ * stylesheet finishes loading while the PD walk is still in What's "PD"? >+ PRUint16 mPendingSheets; Just make it PRUint32. >+ * Inserts the passed <?xml-stylesheet ?> PI at the specified >+ * index. Loads and applies the associated stylesheet synchronously. You mean "asynchronously"? Document how this interacts with the proto doc walk? >+ * Checks that both the prototype document walk is complete and all >+ * referenced stylesheets finished loading, and if so, performs >+ * a few finishing touches of the load. It doesn't check. It asserts those. So the callers should be checking. Fix the comment? >Index: content/xul/document/src/nsXULPrototypeDocument.cpp >+nsXULPrototypeDocument::AddProcessingInstruction(nsXULPrototypePI* aPI) >+ mProcessingInstructions.AppendElement(aPI); > return NS_OK; Handle OOM? >+nsXULPrototypeDocument::AddStyleSheetReference(nsIURI* aURI) >+ mStyleSheetReferences->AppendElement(aURI); And here. >Index: layout/style/nsICSSLoaderObserver.h Thanks for the comment improvement here! r- because we need to deal better with PIs not in the prolog. But this looks really good in general! The next review should be _much_ faster now that I've read this, esp if the updated patch appears while this stuff is fresh in my mind. ;)

Thanks a lot for the careful review. I never knew that PIs are allowed outside of the prologue. Thanks for reminder. If I understood your comments correctly, you're saying that PIs can be under the document element, but such PIs should not have any effect (i.e. the associated stylesheets should not be loaded and the overlays should not get merged). Correct? PD is short for "prototype document", the tree of nsXULPrototype* things we walk when building the actual document in ResumeWalk. I thought it was a common abbreviation, but I can use the non-shortened version to avoid unnecessary confusion. I'll try to address your comments in the next few days, hopefully. (I have little time to work on moz now..)

Whether a PI has any effect when placed outside of the prologe is entierly dependant on which PI it is. There is no general rule. It can vary from PI to PI.

I'm sorry. What I meant to ask was if the two specific PIs that are handled by XUL document, xml-stylesheet and xul-overlay, should not have effect outside the prologue. From a very brief skimming of http://www.w3.org/TR/xml-stylesheet/ it appears that putting xml-stylesheet outside the prolog is forbidden by the spec. Should we add xml-stylesheet PIs appearing elsewhere to tree but ignore them? Don't add them to the tree at all? Show the yellow screen of death? What about xul-overlay PIs?

> PIs can be under the document element, but such PIs should not have any effect For our purposes, yes. That is, xml-stylesheet and xul-overlay PIs only have an effect in the prolog. Other types of PIs might have an effect anywhere, were we to support them. So for xml-stylesheet, we should put the PIs anywhere they appear in the tree and just not load the sheets for the ones outside the prolog. I believe the xml-stylesheet PIs enforce this themselves when you go through the normal stylesheet-loading codepath -- see <http://lxr.mozilla.org/seamonkey/source/content/xml/content/src/nsXMLStylesheetPI.cpp#192>. I'd say we should do the same thing for xul-overlay. Means we don't have to walk the whole DOM looking for them... ;) > but I can use the non-shortened version to avoid unnecessary confusion. Yeah, in comments I think it's better to just say "prototype document".

Looking at the code for xml-stylesheet, we ignore them if they're outside the prolog: http://lxr.mozilla.org/mozilla/source/content/xml/content/src/nsXMLStylesheetPI.cpp#181 I'd say we should do the same for xml-overlays

(In reply to comment #17) > >+ // XXX This duplicates the logic of > >+ // nsXMLStylesheetPI::GetStyleSheetInfo/URL > >+ nsAutoString type; > >+ nsParserUtils::GetQuotedAttributeValue(aProtoPI->mData, nsGkAtoms::type, type); > > So... why not just use those? That is, why not just put the PI into the DOM, > without disabling updates, and let it handle the load? Was there a good reason > for that? That is, see what nsXMLContentSink::HandleProcessingInstruction > does. You'd have to deal with NS_ERROR_HTMLPARSER_BLOCK meaning success (and > in fact meaning you need to increment your pending sheets thing), but that's > ok... Though there's the complication that alternate sheets won't return > NS_ERROR_HTMLPARSER_BLOCK, so you'd not count them (and use the aWasAlternate > arg to StyleSheetLoaded for great justice). Unless we _want_ to block the walk > for alternate sheets? > There's no good reason for that; the old code had this duplication and letting the stylesheet PI handle the load would require changes to UpdateStyleSheet, as I understand it. This is because unlike its other callers, we don't have a parser around, so have no way to know whether |doneLoading| was true or false in UpdateStyleSheet (we'd decide if we want to increment mPendingSheets based on that). Do you think adding another optional param to nsStyleLinkElement::UpdateStyleSheet is a good idea? NS_IMETHOD UpdateStyleSheet(nsIDocument *aOldDocument = nsnull, nsICSSLoaderObserver* aObserver = nsnull, PRBool aForceUpdate = PR_FALSE + ,PRBool* aDoneLoading = nsnull); Maybe even make it aDoneLoadingOrIsAlternate, since I think alternate sheets shouldn't block the load (not that I ever saw alternate stylesheets used in XUL)? Sorry for the delay, I've been busy with my day job. I'll be working on addressing your suggestions and creating a flowchart to make the complicated setup slightly easier to follow.

> so have no way to know whether |doneLoading| was true or false in > UpdateStyleSheet We should change UpdateStyleSheet to return NS_ERROR_HTMLPARSER_BLOCK even if there's no parser (and document that that's what it does). Imo, of course. There's code in XSLT, for example, that already assumes that that's what it does. Looking at callers that check the rv of UpdateStyleSheet, I see the XSLT code in question and the sinks. Everyone else ignores the return value. > Do you think adding another optional param to > nsStyleLinkElement::UpdateStyleSheet is a good idea? In general, yes, but I don't think we should do it in this bug. In my local tree I do have such a param, and NS_ERROR_HTMLPARSER_BLOCK is gone, But that takes a lot more work to make it happen, I suspect, than just removing the parser null check in UpdateStyleSheet.

Thanks again for your comments. Here's an updated version of the patch. Didn't finish the flowchart, but most of the interesting methods should be already included: http://mozilla.doslash.org/stuff/xul/load/ (as it works before the patch; I made some changes in the patch). > (From update of attachment 240837 [details] [diff] [review] [edit]) > >Index: content/xul/content/src/nsXULElement.cpp > > You need to change nsXULPrototypeElement::Deserialize to handle children of > type eType_PI, no? Or do we not put them in the DOM? > Didn't think about that, fixed now. > >Index: content/xul/document/public/nsIXULDocument.h > > Rev the iid? > Fixed. > >Index: content/xul/document/public/nsIXULPrototypeDocument.h > > >+ NS_IMETHOD AddProcessingInstruction(nsXULPrototypePI* aPI) = 0; > > Document? I know that's not file style; time to change that style. ;) > Especially document the ownership model weirdness here (that is, that the > caller is supposed to pass in an already-addrefed aPI. In fact, maybe just > make the argument be an already_AddRefed<nsXULPrototypePI>. > I added the documentation, but didn't change the param type, as I didn't see any benefits in doing so. (Plus already_AddRefed doesn't even integrate well with nsRefPtr) > >+ NS_IMETHOD GetProcessingInstructions(nsVoidArray** aResult) = 0; > > This part I don't like. In particular, the XPCOM ownership model would imply > that the caller is getting it's own copy of the array, which is so not what we > want here. > > How about: > > virtual const nsTArray<nsXULPrototypePI*>& GetProcessingInstructions() const > = 0; > > That would make me much happier, and is likely to make the caller code cleaner. > You'd need to make the member an nsTArray<nsXULPrototypePI*>, of course, but I > think you should. > Yep, makes more sense. I was/am not quite sure what kind of signatures am I supposed to use on non-IDL nsI* interfaces. > Though if you make the arg to AddProcessingInstruction > already_AddRefed<nsXULPrototypePI>, you could use > nsTArray<nsRefPtr<nsXULPrototypePI> > for the member and probably create a > typedef for that type to make code more readable. Or leave the existing > destructor code, I guess (and use .get() in AddProcessingInstruction to get a > raw addrefed pointer). > You didn't sound very sure about which way is better here, so I used nsTArray<nsXULPrototypePI*> as the member for simplicity. We don't do anything complex with these PIs. > > // CID for factory-based creation, used only for deserialization. > > #define NS_XULPROTOTYPEDOCUMENT_CLASSNAME "XUL Prototype Document" > > #define NS_XULPROTOTYPEDOCUMENT_CID \ > >- {0xa08101ae,0xc0e8,0x4464,{0x99,0x9e,0xe5,0xa4,0xd7,0x09,0xa9,0x28}} > >+ {0x9fca71ea,0xc560,0x4ab4,{0xba,0x03,0xc1,0x76,0xfe,0x3d,0x8f,0x36}} > > I don't think you need to change the CID. > Ah, got confused by the comment. You're right, after all the fastload file version is changed, so I shouldn't worry about the CID being used for deserialization. > >Index: content/xul/document/src/nsXULContentSink.cpp > > XULContentSinkImpl::HandleProcessingInstruction(const PRUnichar > >+ return mPrototype->AddProcessingInstruction(pi); > > So we don't put PIs into their correct place in the DOM? > Yeah, as I said I thought they can only appear in the prologue... Fixed. > >Index: content/xul/document/src/nsXULDocument.cpp > > >@@ -408,24 +414,16 @@ nsXULDocument::StartDocumentLoad(const c > > >- if (loaded) { > >- rv = AddPrototypeSheets(); > >- if (NS_FAILED(rv)) return rv; > > Why this change? > AddPrototypeSheets, along with PrepareToWalk and ResumeWalk was called from a few different places, this was pretty hard to follow. So I changed those callers to call OnPrototypeLoadDone instead. In the proto loaded case, nsXULDocument::CachedChromeStreamListener::OnStopRequest used to do the PrepareToWalk/ResumeWalk dance. Now it simply calls to OnPrototypeLoadDone, which includes the AddPrototypeSheets call. > >@@ -489,100 +489,83 @@ nsXULDocument::EndLoad() > > nsICSSLoader* cssLoader = CSSLoader(); > > This seems to be unused now? > Yes, thanks. Now that I looked at that code, there was an OOM crash and a pointless check (both in old code). > >- cssLoader->LoadSheetSync(sheetURI, getter_AddRefs(sheet)); > > Why is it ok to remove this code? What's replacing it? In particular, what > happens if useXULCache is false or the sheet is not a chrome: sheet? > This loads so called "style overlays", which are now loaded from OnPrototypeLoadDone (more specifically AddPrototypeSheets), just before starting the PD walk. Note that the prototype document is built regardless of whether the XUL cache is enabled. I'm not sure what's problem you see for non-chrome: sheets. > >+ OnPrototypeLoadDone(); > > Is this why we no longer need that AddPrototypeSheets() call in > StartDocumentLoad()? Is this code called in that case (prototype done in > cache)? > No, see above. > > Index: content/xul/document/public/nsIXULDocument.h > > >+ NS_IMETHOD OnPrototypeLoadDone(PRBool aCallerResumesWalk = PR_FALSE) = 0; > > Please make this a non-optional arg -- optional args make the code a lot harder > to follow. And probably reverse the sense (so PR_TRUE means to call > ResumeWalk). I think that would be clearer... and certainly fit better with > the way you're using this arg when you do pass it in. > Done. > >Index: content/xul/document/src/nsXULDocument.cpp > >+nsXULDocument::OnPrototypeLoadDone(PRBool aCallerResumesWalk) > > The documentation for this function should indicate whether it applies to cases > when we found an already-loaded prototype in the proto cache... > > I assume this is not called in those cases, right? > I hope I made the documentation for this function clearer. > >@@ -2397,37 +2376,16 @@ nsXULDocument::PrepareToWalk() > >@@ -2444,21 +2402,67 @@ nsXULDocument::PrepareToWalk() > >+ int piCount = 0; > > Why not PRInt32? > Oops. > >+ if (mState != eState_Master) { > >+ piCount = IndexOf(GetRootContent()); > > So is this enshrining the lack of comment nodes, doctype nodes, etc in XUL? > This seems suboptimal to me. It seems to me like this would work better if we > actually had the PI proto nodes at the right places in the DOM... > Since the prototype tree is not exactly like a DOM tree (e.g. only nsXULPrototypeElements are supposed to have children), I left the prolog-handling without major changes. This means that only PIs will appear in prolog for now. When/if we decide to support other nodes in prolog, Add/GetProcessingInstruction(s) and related code can be changed to handle arbitrary nsXULPrototypeNodes. I didn't want to write generic code, mainly because I don't see benefit in allowing text/comment nodes there, but if you think it's evil, I can make the necessary changes in this patch (without adding the actual support for other kinds of nodes) > >+ // When code in ResumeWalk needs to get an overlay to load, it takes > >+ // the last element of mUnloadedOverlays (for the load order to be sane). > >+ // However, we want overlays specified using <?xul-overlay ?> to be > >+ // applied in the same order processing instructions are in. To do this, > >+ // we insert the overlay URIs (in the document order) at the following index: > >+ PRUint32 overlaysCount = mUnloadedOverlays.Count(); > > I think it would be clearer to say: > > We insert overlays into mUnloadedOverlays at the same index in > document order, so they end up in the reverse of the document > order in mUnloadedOverlays. > > Or something like that. > ok. > >+ if (protoPI->mTarget.EqualsLiteral("xml-stylesheet")) { > >+ rv = ProcessXMLStylesheetPI(protoPI, node, piCount); > > This will, given the rest of this patch, load stylesheets from PIs that are not > in the prolog. That seems wrong. > > >+ } else if (protoPI->mTarget.EqualsLiteral("xul-overlay")) { > >+ rv = ProcessXULOverlayPI(protoPI, node, piCount, > overlaysCount); > > Same issue with PIs not in the prolog. Not sure what's correct here. > Fixed by reusing nsXMLStylesheetPI code and by adding a manual nsContentUtils::InProlog check respectively. > >+nsXULDocument::ProcessXMLStylesheetPI(const nsXULPrototypePI* aProtoPI, > > >+ // We're assuming that we were passed a PI node and that the only PI > >+ // node that implements nsIStyleSheetLinkingElement is nsXMLStylesheetPI > > I don't see where we assume the latter. > We don't. Removed the whole comment. > >+ // XXX This duplicates the logic of > >+ // nsXMLStylesheetPI::GetStyleSheetInfo/URL > >+ nsAutoString type; > >+ nsParserUtils::GetQuotedAttributeValue(aProtoPI->mData, nsGkAtoms::type, type); > > So... why not just use those? Now using that. Thanks for the tip. > >+nsXULDocument::ProcessXULOverlayPI(const nsXULPrototypePI* aProtoPI, > >+ nsIContent* aNode, PRUint32 aIndex, > >+ PRUint32 overlaysCount) > > aOverlaysCount ? Or more correctly aOverlayInsertionPoint? > I got rid of the parameter altogether, instead always inserting the new items at the beginning of the array. > >+ // XXX This is wrong, the error message could be out of memory > >+ // or something else equally bad, which we should propagate. > >+ // Bad URL should probably be "success with info" value. > > Indeed. Why not just convert NS_ERROR_INVALID_URI to NS_OK? > This (or a similar) comment was in the original code and I was lazy to figure out which error code actually means 'bad URI, move along' :) Fixed now. We probably want to report the URI in the case of NS_ERROR_MALFORMED_URI, but there are so many things that should be fixed about this code, so I'd like to leave this to another bug. > >+ mUnloadedOverlays.InsertObjectAt(uri, overlaysCount); > > Propagate error if that fails? > OK. > >+nsXULDocument::DoneWalking() > >+#if 0 > >+ printf("nsXULDocument::DoneWalking: master=%d current=%d; " > >+ "mPendingSheets=%d, mStillWalking=%d\n", > >+ mMasterPrototype, mCurrentPrototype, > >+ mPendingSheets, mStillWalking); > >+#endif > > Don't check that in? Or make it a relevant PR_LOG message? > Oopsie. > >Index: content/xul/document/src/nsXULDocument.h > > >+ // nsICSSLoaderObserver > >+ NS_IMETHOD StyleSheetLoaded(nsICSSStyleSheet* aSheet, > > Put this before the static method? > fixed. > >+ * Since ResumeWalk is interruptible, it's possible that last > >+ * stylesheet finishes loading while the PD walk is still in > > What's "PD"? > Clarified. > >+ PRUint16 mPendingSheets; > > Just make it PRUint32. > OK. > >+ * Inserts the passed <?xml-stylesheet ?> PI at the specified > >+ * index. Loads and applies the associated stylesheet synchronously. > > You mean "asynchronously"? Document how this interacts with the proto doc > walk? > The only thing bad about documentation is that it tends to become out of date. This is a left-over from an earlier version of the patch. Fixed. > >+ * Checks that both the prototype document walk is complete and all > >+ * referenced stylesheets finished loading, and if so, performs > >+ * a few finishing touches of the load. > > It doesn't check. It asserts those. So the callers should be checking. Fix > the comment? > Again, a left-over from an older patch. Fixed. > >Index: content/xul/document/src/nsXULPrototypeDocument.cpp > >+nsXULPrototypeDocument::AddProcessingInstruction(nsXULPrototypePI* aPI) > >+ mProcessingInstructions.AppendElement(aPI); > > return NS_OK; > > Handle OOM? > Apparently it's not the style of XUL code to handle OOM :) I'll fix the code I added, but the rest of code should be audited and fixed in another bug. > >+nsXULPrototypeDocument::AddStyleSheetReference(nsIURI* aURI) > >+ mStyleSheetReferences->AppendElement(aURI); > > And here. > Fixed.

> didn't change the param type, as I didn't see any benefits in doing so. (Plus > already_AddRefed doesn't even integrate well with nsRefPtr) I guess that's OK, though I do think using the other type is clearer. But in fact, already_AddRefed should work well with nsRefPtr. If it doesn't, please file bugs. > This loads so called "style overlays", which are now loaded from > OnPrototypeLoadDone Ah, excellent. That's what I needed to know! > but there are so many things that should be fixed about > this code, so I'd like to leave this to another bug. Oh, absolutely. I'm _so_ not expecting you to fix this whole cess-pit in this bug. ;) > Apparently it's not the style of XUL code to handle OOM :) Verily. ;) And yes, followup bugs are the way to go. Reading diff now.

Comment on attachment 244850 [details] [diff] [review] patch v3 >Index: content/xul/document/public/nsIXULDocument.h >+ * This is invoked whenever the prototype for this document is loaded >+ * and should be walked, whether or not the XUL cache is disabled or >+ * not, whether the prototype was loaded from the cache or created by >+ * parsing the actual XUL source, etc. How about: .... and should be walked, regardless of whether the XUL cache is disabled, whether the protototype was loaded ... >Index: content/xul/document/public/nsIXULPrototypeDocument.h >+ * Access the array of the processing instructions in the prolog. Perhaps: Add a processing instruction to the prolog. instead? And maybe even make it clear that these do not live in the normal prototype node tree for the time being? > #define NS_XULPROTOTYPEDOCUMENT_CID \ >- {0xa08101ae,0xc0e8,0x4464,{0x99,0x9e,0xe5,0xa4,0xd7,0x09,0xa9,0x28}} >+ {0x9fca71ea,0xc560,0x4ab4,{0xba,0x03,0xc1,0x76,0xfe,0x3d,0x8f,0x36}} No need for that change. >Index: content/xul/document/src/nsXULContentSink.cpp > XULContentSinkImpl::HandleProcessingInstruction(const PRUnichar *aTarget, > const PRUnichar *aData) >+ nsRefPtr<nsXULPrototypePI> pi = new nsXULPrototypePI(); Why bother with the nsRefPtr? Not really needed... Might be nice to comment that a brand-new nsXULPrototypePI has a refcount of 1, though. >+ NS_ENSURE_SUCCESS(rv, rv); That leaks the prototype PI, no? Need to manually call Release() on it... >+ return children->AppendElement(pi); Same here if it fails. Speaking of which, doesn't AppendElement return a boolean, not an nsresult? So you want more like: if (!children->AppendElement(pi)) { pi->Release(); return NS_ERROR_OUT_OF_MEMORY; } return NS_OK I think. >Index: content/xul/document/src/nsXULDocument.cpp >+ for (PRUint32 i = 0; i < total; ++i) { >+ rv = CreateAndInsertPI(processingInstructions[i], >+ this, piInsertionPoint); >+ if (NS_FAILED(rv)) return rv; So shouldn't we increment piInsertionPoint at each step? Otherwise stylesheet PIs will get inserted in reverse of document order, which means the sheets will have reversed precedence... Try having two PIs in the document; the first one with * { color: red } and the second with * { color: green } and see which one applies? (We should add that as a regression test in any case.) >+nsXULDocument::CreateAndInsertXMLStylesheetPI( This is really just InsertXMLStylesheetPI, no? >+ nsIContent* aNode) I'd call that aPINode. >+nsXULDocument::CreateAndInsertXULOverlayPI( Again, just InsertXULOverlayPI >+ nsIContent* aNode) And again, aPINode. >+ // We insert overlays into mUnloadedOverlays at the same index in >+ // document order, so they end up in the reverse of the document >+ // order in mUnloadedOverlays. Add back the explanation of why we want reverse order in mUnloadedOverlays? It was useful. >@@ -2508,29 +2593,32 @@ nsXULDocument::AddChromeOverlays() >+ // Same comment as in nsXULDocument::ProcessXULOverlayPI That function's name is different now. Still r- because of the stylesheet ordering thing, but that's the only substantive comment -- the rest are style and comment nits. This is so close I can feel it!

Filed bug 359846 and bug 359849. Thanks for catching that nsRefPtr thing; I don't know what I was thinking. I hope to post an updated patch tonight or tomorrow.

Review comments fixed. Thanks a lot for your patience, I really appreciate it.

You mentioned adding a regression test. I do have a bunch of reftest-like testcases which I need to clean up and have them committed to the tree as well. Unfortunately they don't test much of the possible cases this code handles, and the fact that the code takes different branches for XUL cache enabled/disabled, filled/not filled, chrome/non-chrome URIs poses an additional challenge of testing this code well.

Comment on attachment 244962 [details] [diff] [review] patch v4 Looks great! Let's land it!

Checked this in. Here's hoping Ts and Txul are happy! ;)

Bonus: DOM Inspector now shows the PI's for XUL documents. Nice. :)

Ts and Txul look happy. I think we can call this fixed. ;) Nickolay, I agree that testing all the codepaths here is hard. If we just test the common ones, that's already a good step. Do make sure to land a test for stylesheet ordering, ok? And thanks a ton for doing this!

Hmmmm... I just realized that nsXULDocument::StyleSheetLoaded has two issues I didn't catch: 1) It'll decrease mPendingSheets for alternate sheets. 2) It'll call AddStyleSheet, even if the sheet is already in the document (which will be the case if the sheet was added by a PI, right?). Seems to me that it should only add sheets that have no owner node or something?

And more interestingly, where should the sheets loaded via nsXULDocument::AddPrototypeSheets be sorted wrt each other and PI sheets? Right now with all of them loading async they'll basically race, right?

(And sorry for catching all this now; I just have a patch that was waiting on this to land, and I'm updating it now and running into these questions...)

Last comment for the night, I promise! One way to make sheet completion order not match the order the loads started in might be to use @import in the earlier sheets...

It's possible that this is causing the Mac orange on the Firefox tbox, though I can't tell how...

Thanks for landing. Things in comment 35 sound easy to fix, I'll do it soon. >And more interestingly, where should the sheets loaded via >nsXULDocument::AddPrototypeSheets be sorted wrt each other and PI sheets? >Right now with all of them loading async they'll basically race, right? That's a good catch! I was slightly confused about how sheets from xml-stylesheet were added to the document, will have to look more into doing it right for the style overlays as well. I will make sure to get the tests checked in after I post fixes to the current patch. Not sure about mac orange either. Gavin said he'll try backing it out to see if it fixes the orange.

One thought for the style overlays is to just decide that all style overlay sheets come after all document sheets (this would allow them to override the document sheets, which I think is the way to go). Then don't insert the style overlay sheets into the document's stylesheet array until we're actually doing DoneWalking; at that point put them all at the end... Neil, Mike, what do you think of doing that?

> One thought for the style overlays is to just decide that all style overlay > sheets come after all document sheets (this would allow them to override the > document sheets, which I think is the way to go). Yes, this is what the current patch was supposed to do. There's the question if we care about order the style overlays are loaded in (I'm not sure we do). And we still should wait until the overlay stylesheets are loaded before running DoneWalking.

I'm pretty sure the order of style overlays is undefined... Benjamin, is that true?

Well, I think we do return them in the order we find them in the chrome.manifest files. So we will return xulrunnerchrome -> appchrome -> extensions, but we don't guarantee ordering within those broad categories.

The backout did in fact fix the orange.

The style ordering thingy caught on of my extension, where two style sheets are added via chrome.manifest and one is expected to conditionally overlay the other. As Boris guessed correctly, they raced with this patch, making the result unpredictable. (In reply to comment #42) > There's the question if > we care about order the style overlays are loaded in (I'm not sure we do). Would be nice if you did, since this was true for the past.

See also bug 359971 comment #1 about Sm trunk 20061108 nightly hang while loading.

Just to clarify comment 41, we could add a version of nsICSSLoader::LoadSheet that acts just like the current one but returns a stylesheet object also. The sheet object won't be usable until it's notified on, but that's ok. So we could build up an array of those sheets in the order in which we started the loads, then once it's all done loading stick them at the end of the stylesheet list. I certainly think that if two sheets are listed in the same manifest it's worth preserving their order, and that it's worth preserving the relative ordering of the groups bsmedberg mentions, and at that point we might as well preserve whatever order the chrome registry gives us.

With a fix for the crash reported in bug 359959. The rest of comments are still to be fixed.

- Fix the crash from bug 359959. - log a warning to the Error Console when a PI outside the prolog is found (and ignore it) - fix an assertion in the case of a missing overlay referenced from a xul-overlay PI - add a few XXXs in the code (will file bugs if you agree about these) - preserve chrome stylesheet ordering using the method suggested by bz. Everything appears to work (it even doesn't break seamonkey in an obvious way ;) ), but I'll need to do more testing and fix bug 360119 before landing this.

Comment on attachment 245212 [details] [diff] [review] patch v5 > Index: layout/style/nsICSSLoader.h I'd really rather have a separate method here instead of adding a default arg to the existing method. The separate method would just call InternalLoadNonDocumentSheet, of course, and could be documented as just like the method without the out param, with additional docs for the out param. And it could then assert that the out param is non-null... >Index: content/xul/document/src/nsXULDocument.cpp >+nsXULDocument::InsertXMLStylesheetPI(const nsXULPrototypePI* aProtoPI, >+ } else if (NS_FAILED(rv)) { >+ // XXX check for specific error codes here, like >+ // NS_ERROR_FILE_NOT_FOUND, as the error could indicate a problem >+ // with the specified URI as well as an internal error Actually, the only serious error code you should expect here is NS_ERROR_MALFORMED_URI. Most other errors would be async (NS_ERROR_FILE_NOT_FOUND is not at the moment, but that's a bug that we should fix). But there are also cases when a user error leads to NS_OK here (eg using type="tetx/css" or whatnot). So I'm not sure how much we should really worry about random error returns here. >@@ -2783,31 +2876,36 @@ nsXULDocument::ResumeWalk() >+ // we're in the master document -or -we're in an overlay, and far s/-or -/-or- / ? >+ case nsXULPrototypeNode::eType_PI: { >+ nsXULPrototypePI* piProto = >+ NS_REINTERPRET_CAST(nsXULPrototypePI*, childproto); Why not NS_STATIC_CAST? (And yes, I realize that was in he earlier versions too). >+ if (processingOverlayHookupNodes) { ... How about: nsIContent* parent = processingOverlayHookupNodes ? GetRootContent() : element; And then just have the one CreateAndInsertPI call. On which note, can this code guarantee that GetRootContent returns non-null? I would guess that it in fact cannot, and that that case needs to be handled. r+sr=bzbarsky with these nits

I think I fixed all your nits; also reordered two |if|s in InternalLoadNonDocumentSheet to fix an assertion (null sheet got posted in that case).

Comment on attachment 245329 [details] [diff] [review] patch v5.5 > Same as above, but cannot be used to obtain the not-yet-loaded sheet. How about "Same as above, to be used when the caller doesn't care about the not-yet-loaded sheet"? No need for additional reviews after that change. And good catch on the swap() ordering!

Index: content/xul/document/src/nsXULDocument.cpp + nsIContent* parent = processingOverlayHookupNodes ? + GetRootContent() : element; That kills my (Mac) build with "error: operands to ?: have different types" - bz's suggestion of s/element/element.get()/ fixes it.

With the comment nit from comment 53 and the build fix from comment 54.

Checked in, and Mac is green. Resolving. Did a followup bug on reporting stylesheet loading errors ever get filed?

bz, thanks for dealing with this bug; I was away without a decent internet connection. Sorry about forgetting to bump nsICSSLoader's IID, hope I'll remember to rev the IID next time :) bug 360467 filed about reporting missing stylesheets.

Edited http://developer.mozilla.org/en/docs/Firefox_3_for_developers to mention this change and posted this http://groups-beta.google.com/group/mozilla.dev.tech.xul/browse_frm/thread/1d4be633082f5099/36085ddb5b956a58#36085ddb5b956a58 I still need to post my tests.

sheppy asked me to add this to dev-doc-needed list.

>- NS_PRECONDITION((!aSheet || !aObserver) && (aSheet || aObserver), >- "One or the other please, at most one"); >+ NS_PRECONDITION(aSheet || aObserver, "One or the other please"); I didn't see a comment about this, but either the text of that string needs to be changed or the logic needs to go back to what it was. "One or the other" implies exclusive or, which is what used to be tested. Not sure if it was intentional, and maybe I'm missing something, but I thought I'd point it out.

Didn't think it implied exclusive or. Any suggestions for the correct message for that assertion?

I also don't think that text implies exclusivity (you'll note what I had as the text when I _did_ want exclusivity implied). I suppose you could use "sheet and observer can't both be null" or something?

(In reply to comment #62) > Didn't think it implied exclusive or. Any suggestions for the correct message > for that assertion? I was comparing it to the old behavior, but after taking a closer look it seems as though you changed it so inclusive or makes sense. I do think bz's suggestion would be more clear however.

The assertion text is changed to bz's text in the patch in bug 360863.

A few testcases as promised. Guess it's better than nothing. I run these via reftest by putting them into layout/reftests and editing the Makefile to point to my .list. http://developer.mozilla.org/en/docs/Mozilla_automated_testing#Reftest I don't know how this should be checked in (i.e. where, what .list file to use, etc.), ideas are welcome. We should also ought to run these tests from chrome URIs to test how it works with the xul cache, but it can be done later. (Would be nice if there was a special location in the objdir that chrome://test/* is mapped to.) Guessing which reviewers to pick..

Comment on attachment 245868 [details] some testcases for reftest I think for reftest dbaron is a better bet than I -- I'm still sorting out how reftest works.

Comment on attachment 245868 [details] some testcases for reftest Rob Campbell and Martijn Wargers are taking over reftest stuff from me

Comment on attachment 245868 [details] some testcases for reftest This looks like a good use of reftest. I don't know of any bugs in style precedence in XUL that you'd be depending on here, since all your styles are specified in CSS. So really the only concern is that all these tests are documenting correct behavior. And given that (presumably) they pass, we can always revisit the tests later if we decide the behavior is wrong. So I haven't looked closely, but this looks fine, and is probably better off being checked in sooner rather than later so that it doesn't get lost.

I've gone ahead and checked this in; if there are review comments I'll be happy to address those.

Comment on attachment 245868 [details] some testcases for reftest I'm not really suitable for reviewing. However, I looked at all the testcases, they all look fine, except perhaps these two: test6 doesn't seem right to me, although the end result is green. It seems to me that test006-007.js is only useful for test007. It seems to me that style="background-color: red" should be removed in test006.xul and the !important part should be removed in test006.css to get a good working testcase. I think test010.css needs to have '!important' added.

(In reply to comment #71) > test6 doesn't seem right to me, although the end result is green. It seems to > me that test006-007.js is only useful for test007. > It seems to me that style="background-color: red" should be removed in > test006.xul and the !important part should be removed in test006.css to get a > good working testcase. > Could you elaborate? The script checks that the PI got added to the DOM, why is it not useful in 006? I'm not sure why you think current usage of background-color makes it a bad testcase, either. > I think test010.css needs to have '!important' added. > That's right; also test010.xul should do 'assert(false, e);' instead of just 'assert(false);'

(In reply to comment #72) > Could you elaborate? The script checks that the PI got added to the DOM, why is > it not useful in 006? I'm not sure why you think current usage of > background-color makes it a bad testcase, either. Ah, sorry, never mind, I was incorrect.

If the testcases need changes, post a diff and I'll land it?

(In reply to comment #74) > If the testcases need changes, post a diff and I'll land it? Well, I could land this too.

Comment on attachment 246307 [details] [diff] [review] test10.diff Ok, I checked this in.

This is marked as dev-doc-needed but I'm not clear on exactly what needs writing about. Is it just documentation of new interfaces (which seem to be mostly implementation details at first glance, and probably not critical to write about immediately), or is there more to it than I'm seeing?

Well, the first six dependencies give it away, but in previous versions the document object only contained one child, the document element. Now the document object includes all of the processing instructions. This is best observed by inspecting chrome in DOM Inspector with the View -> Show Processing Instructions menuitem enabled. XUL documents should contain at least one stylesheet processing instruction (to import the theme), and may include overlay processing instructions although dynamic overlays are not included.

In short, any reference to "document.firstChild" will likely break now that's likely to point to a processing instruction rather than the document element.

The "document.firstChild" references should in most cases be replaced by "document.documentElement".

As comment 59 says, this was already partially documented, now I also added it to http://developer.mozilla.org/en/docs/Updating_extensions_for_Firefox_3#Other_changes and mentioned documentElement on http://developer.mozilla.org/en/docs/DOM:document.firstChild This is also mentioned on http://developer.mozilla.org/en/docs/Firefox_3_for_developers#Notable_bugs_fixed