Without any other evidence of the contrary, the code in nsCSSLoader needs *really* some cleanup : 1) the hash table mSheetMapTable seems to be totally useless but we generate it, keep it updated with all created stylesheets, seek into it, ... 2) the index of insertion in the document for InsertSheetInDoc and in a parent stylesheet for InsertChildSheet is *totally* false !!! That's probably causing the empty stylesheets we see so often in Document Inspector. I'll will propose a rewriting of this part of code.

Comment on attachment 55896 [details] [diff] [review] patch v1.0 r=peterv if everything still works ;). Glad it's gone though, I never understood why we had the sheetmap.

r=rjesup@wgate.com, though I'm not a StyleSheet expert. I'll check jprofs before and after shortly.

jprof'd with 1ms timer going to cnn, abc, espn, cbs, nbc, before and after patch. InsertSheetInDoc took the same time (modulo statistical error) in both, and in both cases all of the time recorded there was used by nsDocument::InsertStyleSheetAt(). No problems noted. Get an sr and drop it in.

Comment on attachment 55896 [details] [diff] [review] patch v1.0 sr=kin@netscape.com

checked in trunk

Did this checkin just cause a large jump in tinderbox leaks?

Lets wait a cycle or two, if it doesn't go back down, we'll back it out.

Waiting for approval from today's build engineer (jj) before I back this out.

Checkin backed out of trunk: mozilla/content/html/style/src/nsCSSLoader.cpp revision 3.96

Just some data for glazou. From inspecting my leak logs after your check-in, it seems that the leaked stylesheets come from nsXULDocument. Comparing before and after check-in show that one RELEASE from nsXULDocument is missing for the leaked sheets (probably from http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULDocument.cpp#503). It looked as though the number of items in mStyleSheets in nsXULDocument was smaller after your check-in. Stylesheets loaded from HTML and XML don't seem to be leaked. Now it probably looks even more confusing ;).

Interesting... without this patch an XUL doc has anywhere from 4 to 11 sheets (makes sense). With this patch it always seems to have 2.... Or at least _think_ it has 2.

Is this map really useless? It seems to be used for determining insertion positions in a document's array of sheets.

Okee. here is the problem with the patch: - if (sheetMap) { - PRInt32 insertIndex = sheetMap->Count(); - PRBool insertedSheet = PR_FALSE; - while (0 <= --insertIndex) { - PRInt32 targetIndex = NS_PTR_TO_INT32(sheetMap->ElementAt(insertIndex)); - if (targetIndex < aDocIndex) { - mDocument->InsertStyleSheetAt(aSheet, insertIndex + 1, aNotify); - sheetMap->InsertElementAt((void*)aDocIndex, insertIndex + 1); - insertedSheet = PR_TRUE; - break; - } - } - if (!insertedSheet) { // didn't insert yet - mDocument->InsertStyleSheetAt(aSheet, 0, aNotify); - sheetMap->InsertElementAt((void*)aDocIndex, 0); - } + mDocument->InsertStyleSheetAt(aSheet, aDocIndex, aNotify); notice that without the patch we insert at "insertIndex+1", not at "targetIndex+1", which is what aDocIndex would be. In fact, I've just done some testing and it seems to be the case that "insertIndex + 1 === aDocIndex - 2" consistently for XUL documents. But that breaks down for documents that use @import, as far as I can tell... In any case, what happened here is that aDocIndex is always "2" with the patch in this bug. So we addref sheets, then replace them in mStyleSheetList and never release them later...

Here we go. Compare how HTML content sink calls LoadStyleLink: http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#4476 with how the XUL content sink calls LoadStyleLink: http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULContentSink.cpp#891 The basic difference is that the HTML sink has a member called mStyleSheetCount which it increments every time it calls LoadStyleLink or LoadInlineStyle. The XUL sink uses the GetNumberOfStyleSheets() method which does not take into account the number of pending loads of sheets. If we decide to eliminate this sheet map, we should go through and check that all callers of all loader methods that create SheetLoadData objects pass in the correct values for the sheet index. The we should test the hell out of it. The scenario I'm thinking is: 1) We fail to load a sheet because of network issues or charset conversion failure or whatever 2) The sink will keep incrementing its count, and we will think we have more sheets than we really do 3) The first entry in the document's mStyleSheets will be null. 4) The first entry of document.styleSheets in js will do odd things. :) 5) We will crash when we unconditionally NS_RELEASE all entries in mStyleSheets on document destruction. It looks to me like the sheet map is an attempt to deal with those issues.... All that aside, should XUL try to keep a more-correct count of sheets?

BTW, while this might be extra code and a bit of bloat, I've never seen a jprof that showed any hits in the SheetMap code, even in a prof of browsing around for an hour. It may make sense to leave it alone and simply comment it so it doesn't confuse the next person coming along.

I don't think this fix got proper reviews. You should have given attinasi, hyatt, dbaron or myself a chance to say something. If I remember correctly, the sheetMapTable is used for documents that insert the same stylesheet multiple times (or something like that). I vote for Invalid.

I've talked to at least a few people who had major problems with CSS in yesterday's builds (the ones that had this fix). Basically, any web page that had multiple <style> elements was displayed incorrectly -- all but one of the <style> elements were ignored. Once this fix was backed out the problems disappeared. I think I agree with Pierre....

I agree. However, I would like for this bug to become a patch to comment this assumption/interaction so the next person going into that code knows why this is there, and what it does. So lets dump the current patch, and please replace it with a small patch to comment the use of SheetMap.

Pierre, I think that your comment shows that commenting this code is needed... Just for the record, I have asked for help about this sheetMap, remember ? This code does not handle multiple insertions of the same stylesheet in the document. It always inserts the sheet into the document, whatever is already present in the sheetMap. Multiple insertions are imho handled elsewhere above InsertSheetInDoc. Boris, thanks for your comments, very useful as always. They helped Peter and me understanding exactly what's going on here : The CSS loader uses mStyleSheetCount to determine the insertion point of the stylesheet in the document. But because of networking or any other reason, the stylesheets might be linked to the document in an order which is not the appearance order in the loader. The sheetMap keeps track of the requested order, and is updated only when a stylesheet is complete ; the document keeps track only of the complete stylesheets, not pending. When a pending stylesheet becomes complete, InsertSheetInDoc inserts it at the right position in the sheetMap and the insertion index IN THE SHEETMAP reflects then the position of insertion IN THE DOCUMENT, **even if other stylesheets are still pending**. That's why |insertIndex+1| is used for both the document and the sheetMap. This bug is INVALID but really deserves a patch containing only comments to the code. Preparing such a patch now.

peterv, glazou and I had a chat about this on irc. We came to the conclusion that the mSheetMap doesn't cover all the cases when stylesheets are dynamically added. For example: A document contains two stylesheets A and B (in that order) 1. Stylesheet A loads, but B doesn't load immediately 2. a third stylesheet, C, is inserted after B 3. To get the index for C we walk back in the document until we find a loaded stylesheet, so we find A 4. Stylesheet C is inserted right after A 5. Stylesheet B finally loads, but gets inserted after C This might be an uncommon case, but I bet there are more common cases. For example if two stylesheets are dynamically inserted at the same time. The basic problem is that mSheetMap doesn't contain all stylesheets, just the ones that are fully loaded. To fix this we should keep all stylesheet-creating nodes (elements and PIs) in the mSheetMap, but have them point to index -1 until they are fully loaded. So the stylesheet get inserter in mSheetMap immediatly when they are found, and when/if it is fully loaded we just update all neccesary indexes. So the mSheetMap would be a list of all potential stylesheets, rather then all actual stylesheets. So the above example would look something like 1. Stylesheet A loads, but B doesn't load immediately 2. a third stylesheet, C, is inserted after B 3. To get the index for C we walk back in the document until we find a stylesheet, so we find B 4. Stylesheet C is inserted after B 5. Stylesheet B finally loads and gets inserted before C This should also make it easier to fix bug 85631

milstone bulk change

so is this bug still about adding a comment or about extending SheetMap ? If it is the former, was there ever another bug filed? At any rate, someone needs to either change the summary to reflect what is this bug suppost to do or invalid this bug and file 1 or 2 more bugs for the related issues.

sorry, should comment too. This bug is about fixing mSheetMap so that it is able to handle dynamically added and removed stylesheets. To do this it must keep track of _all_ stylesheets, not just the onces that are successfully loaded. See irc-log in attachment 56555 [details] for rationale behind this conclusion.

I am confused, bug 34849 was fixed but this was not? Then this shouldn't be blocking that bug. Is this critical for 1.0/nsbeta1? If so, please nominate. (Gotta say I like patches that mostly remove code, heh.)

I think it is a bit late to fix this for mozilla1.0 or nsbeta1. It will probably be a large and risky patch.

The only thing not fixing this bug causes is that stylesheets sometimes, in more complex documents, get added in the wrong order. IMHO we can live with this in 1.0

Ok. I just thought the reviewed patch v1.0 was the fix. If it is not, obsolete so we won't have false hopes ;)

Taking this bug, with Daniel's permission. If you want to see how well the sheetmap handles things currently, check out http://web.mit.edu/bzbarsky/www/testcases/css-loading/ I've got some work-in-progress to do this better, I think... (and fix a few of the related bugs in the process). The basic plan: 1) Introduce the concept of "complete" and "incomplete" stylesheet objects. A sheet is "complete" if it is completely loaded/created. Incomplete sheets should _not_ be present in style sets or available to the DOM. 2) Make EnsureUniqueInner a no-op for incomplete CSSStyleSheetImpl objects so that you can modify all clones of an incomplete sheet at once. 3) Fix the document to not notify for incomplete sheets till they become complete. Those are what I have done so far (patch coming up in a second). From this point on: 4) Make sure that all callers of nsIDocument::GetStyleSheetAt deal with incomplete sheets (ignore them, that is, for the most part) 5) Move all sheet insertion logic into the CSSLoader (basically walk the document's sheets, calling CompareTreePosition() on our content and the content of the sheet in the sheet array until we find the proper insertion point). 6) Create sheets as we kick off the load, not when the load finishes, and immediately insert them in the document (they're incomplete; cloning possibly happens at this stage, which is why we want to be able to parse into a bunch of clones at once). This will automatically make sure that the document's sheet list is correct and up-to-date. Outstanding issue with this plan: The stylesheet manipulation functions on nsIDocument are really weird... Suffice it to say that if you InsertStyleSheetAt(aIndex) and then GetStyleSheetAt(aIndex) you get a _different_ sheet out. I'm very tempted to eliminate the magic that InsertStyleSheetAt does -- the only caller is the CSSLoader and we could just make the CSSLoader smart enough to deal... Thoughts?

Oh, the upshot is that the sheet map will be eliminated.

Implements steps 1--3 of the 6-step plan (except for fixing the already-broken UpdateStyleSheets() impls (plan for tomorrow).

As glazou points out, we could even dispatch DOM events to notify of sheet loads from StyleSheetComplete() if we wanted to.... apparently some people really want this.

Is all this CSS-specific or does it also provide a way for XSL and other sheets to be supported using the same mechanism?

nsCSSLoader is very much CSS specific. And I don't think we should support XSLT transforms on dynamically added/removed XSLT stylesheets btw.

I just realized that we don't need the sheetmap _and_ we don't need the "complete" business, except for one little problem... Link: headers. Everything else we can insert correctly based on the document ordering of the ownerNodes (the <meta> or <link> or <style> or PI). But Link: stylesheets have no ownernodes.... I don't really feel right setting up this complicated system of "complete" vs "not complete" just for sheets attached via Link:. There has to be a better way to do this.

bz: even elements (and Link headers) that have stylesheets that didn't load right should have an entry in the DOM array, no?

Yes, of course. And they will. But that's not really helpful in ordering the sheets... ;) So what I'm looking at right now is ordering sheets that have an ownernode based on the tree positions of the ownernodes and keeping a simplified version of the sheetmap (simplified because every requested load will result in a sheet) around for the sheet without owner nodes. One nice thing is that any sheet without an owner node has to come before any sheet with an ownernode, so the whole thing can work out (module whatever idiocy XBL is doing, but I think that it actually won't be affected).

(yeah, xbl can insert an arbitrary number of sheets at the node level) Anyway, I'll shut up now, since I don't really know what this is about. :-)

OK. Hixie has convinced me that the DOM should be able to affect incomplete sheets in certain ways (eg changing the disabled state). Further, if we stop blocking the parser for sheet loads we will need a disabled state thing anyway. So I'll be going back to my original proposal after all.

for the record, i think this sounds fantastic and will happily review anything you throw at me :) The only question i have is what happens if somebody gets a "clone" of an incomplete stylesheet and then starts modify it? If i understand things correctly we used to give them their own clone whereas now we give them the 'original' stylesheet.

That's correct. The problem is that the parser needs to be modifying the sheet _after_ it has been cloned and we want these modifications to propagate to all the clones.... My current tree throws exceptions if you try to use the DOM functions to change the sheet, but doesn't really protect against using the nsICSSStyleSheet stuff... which I _do_ find troubling.

Even throwing exceptions on modification seems a bit troublesome to me since it introduces timing issues which are always scary. One problem i can see is someone developing code and testing on local files (which get loaded syncronyously?), but once the files are loaded from the networe the code will fail. Unfortuanlty i can't see a way around this. Is any of this a new problem or do we behave in a similar fasion today?

The only sync loads CSSLoader does are for LoadAgentSheet. No document sheet is ever loaded synchronously. Current behavior is that the sheet is not even available in document.styleSheets until it is fully loaded. So the only way this will cause problems is if the developer uses a timeout or something to "wait for the sheet to load"... It would be nice if the CSS WG were to actually specify document.styleSheets a bit more clearly.. ;)

ok, i feel a lot better now, i'm not worried about webdevelopers but only about mozdevelopers. And i don't think they'll ever add a timer to wait for the stylesheet to load :)

Oh, what happens if someone gets a non-clone while the stylesheet is being loaded and then doesn't do anything with it until it is fully loaded (say for example puts a handle to the stylesheet in a global variable in an "inlined" script and then modifies that stylesheet when some button is clicked). We can't really make a clone at that point since we can't change all the references to the uncloned stylesheet to point to the new clone. I think that we might need to always create the clone but pass all reads through to the original until it is fully read. Hmm.. or is that what you do? I've only skimmed the patch so far

This is a _partial_ patch, dude. ;) I'm going to be cloning the sheets before insertion into the document. So the things accessible through document.styleSheets will be distinct objects that happen to share an inner. Up until SetComplete() is called, modifications to any of them will be reflected in the others. After that, modifications will copy mInner (cloning sheets basically makes all the style data COW now; with my change it will be COW-if-complete).

yeah, i know, i just couldn't help myself :) Ok, sounds good, i realized that that might be the idea a while after posting my comment.

This is not reviewable yet; just checkpointing changes. Remaining to be done: 1) Rename GetEnabled/SetEnabled to something like GetAppliable/SetAppliable 2) Add support for deferring alternate sheet loads until after the non-alternates are loaded 3) Fix a few of the XXX comments (in CreateSheet2 when we need to give the caller more state, in InsertSheetInDoc2 when we need to cancel pending sheet loads? This latter one I may leave for now...) 4) Add cycle detection to LoadChildSheet 5) Clear up the rules for notification on sheet load failures (probably want to notify). 6) Remove old code 7) Clean up API to not take those args it no longer needs (like the indices). Fix callers. 8) Clean up LoadAgentSheet definition to say that you _either_ pass in an observer _or_ want a sheet back with the latter doing a synchronous load. 9) Add some NSPR logging to the whole mess to make it debuggable 10) Test the hell out of it.

Oh, I forgot: 11) Fix the alternate stylesheet stuff in the presshell to check whether sheets are complete.

This is basically fully functional. Remaining issues are: 1) <meta> elements (bug 36511 and being put off till after this patch lands) 2) Stopping pending loads for an element. Need to decide what the cleanest way to handle that is. I'm guessing I'll have to enumerate the whole loading table unless I keep a secondary by-element hash. I may do that too -- enumerating on every linked sheet load is slooow. 3) Removing debug cruft 4) Restoring the MOZ_TIMELINE stuff 5) Restoring the error reporting (via NSPR logging instead of fputs) And testing, of course. I've tested this on all the tests at http://www.hixie.ch/tests/evil/css/import/home.html as well as my css-loading tests and the testcases in the bugs blocked by this. I've not been able to write a testcase yet that tickles the <meta> issue, but that's mostly due to a lack of a way to control sheet load times; if I had that, I think I could do it. I'd be very happy for comments at this point (not a full review until I finish the items above, necessarily, but just feedback...) The important changes are in nsIStyleSheet.h, nsCSSStyleSheet.cpp, nsCSSLoader.cpp, nsDocument.cpp. Everything else is just dealing with the fallout (except nsXULDocument.cpp which is more like copying the nsDocument.cpp code). I can attach the entire nsCSSLoader.cpp if people would find that easier to review.

Fixing bug 181808 will reduce the size of this patch significantly (and greatly reduce the number of files it touches).

Oh, in case anyone starts looking at that I have one more important change I made in my build, and that is to walk the sheets backwards in InsertSheetInDoc. In the typical case (pageload) this will require exactly one comparison using CompareTreePosition before the sheet is inserted... so should hopefully not hurt perf too much. I'm paranoid about regressing T* with this change. ;)

What Emperor Palpatine meant to say: "And now, witness the power of this fully operational CSS LOADER!" This is basically done. There are some issues (which are there without the patch too) with live skin switching, but I would be in favor of having a separate bug for that. Bug 36511 is already filed on the <meta> issue and I'll file a separate bug on the cancelling the load if a new one comes in for the same element business. Reviews?

Comment on attachment 107408 [details] [diff] [review] Ready for review I lied. I forgot some comment changes....

Oh, and at timeless' request I tested live theme switching with that patch and it works. In case anyone cared. ;)

Just tried running Ts and Txul tests (jrgm said he'd see about running Tp maybe). Overall results: Ts before: 3083 3496 2619 3005 2792 3126 3203 Avg: 3046 Ts after: 2817 2631 2811 3266 3138 3234 Avg: 2983 Txul before: 813 873 833 831 (these are the "__xulWinOpenTime") Txul after: 807 825 846 830 So looks like this is not obviously hurting perf, at least... (though the data is pretty noisy :( ).

bz: bug 85631 is related (I think you should take it ;-))

Aha. Right. That is "fixed" by this patch since the sheet is now "loaded" immediately... but we should still stop the useless network traffic, yes.

OK. I'm on vacation till Monday morning. Tree closes on Tuesday night. Fun, eh? glazou, sicking, peterv, thanks in advance for your reviews. jrgm, thanks in advance for the pageload testing. dbaron, if you get a chance to look at the nsCSSStyleSheet changes that would be great.... Here's hoping this makes 1.3a; the feedback would be very helpful in ironing out any issues (which I'm not really expecting any of, by the way).

We're not going to hold 1.3a for this. That doesn't mean that the fix won't be included in 1.3a. (The "wanted1.3a" flag has been changed to "blocking1.3a" to more accurately reflect how the flag is used by drivers@mozilla.org).

Comment on attachment 107410 [details] [diff] [review] Here we are I don't like the additional argument to GetNumberOfStyleSheets and GetStyleSheetAt, I would rather see two separate functions for both states. But it's up to you. I wish I had more substantial comments, but this patch is so nice. Hopefully sicking will find nothing serious. Great job! >Index: rdf/chrome/src/nsChromeRegistry.cpp >=================================================================== >@@ -1432,82 +1432,68 @@ >+ // Iterate over the style sheets. >+ PRInt32 i; >+ for (i = 0; i < count; i++) { >+ // Get the style sheet >+ nsCOMPtr<nsIStyleSheet> styleSheet; >+ document->GetStyleSheetAt(i, PR_FALSE, getter_AddRefs(styleSheet)); >+ >+ if (!oldSheets.AppendObject(styleSheet)) >+ return NS_ERROR_OUT_OF_MEMORY; Add braces. >Index: content/base/public/nsIDocument.h >=================================================================== >+ NS_IMETHOD GetNumberOfStyleSheets(PRBool aIncludeSpecialSheets, >+ PRInt32* aCount) = 0; >+ /** Add a newline before the comment. >+ * Get a particular stylesheet >+ * @param aIndex the index the stylesheet lives at. This is zero-based >+ * @param aIncludeSpecialSheets see GetNumberOfStyleSheets. If this >+ * is false, not all sheets will be returnable >+ * @return the stylesheet at aIndex. Null if aIndex is out of range. >+ * @throws no exceptions >+ */ >+ NS_IMETHOD GetStyleSheetAt(PRInt32 aIndex, PRBool aIncludeSpecialSheets, >+ nsIStyleSheet** aSheet) = 0; >+ /** Add a newline before the comment. >+ * Insert a sheet at a particular spot in the stylesheet list (zero-based) >+ * @param aSheet the sheet to insert >+ * @param aIndex the index to insert at. This index will be >+ * adjusted for the "special" sheets. >+ * @throws no exceptions >+ */ >+ NS_IMETHOD InsertStyleSheetAt(nsIStyleSheet* aSheet, >+ PRInt32 aIndex) = 0; >+ /* Add a newline before the comment. And use /**, not /* >+ * Get the index of a particular stylesheet. This will _always_ >+ * consider the "special" sheets as part of the sheet list. >+ * @param aSheet the sheet to get the index of >+ * @return aIndex the index of the sheet in the full list >+ */ > NS_IMETHOD GetIndexOfStyleSheet(nsIStyleSheet* aSheet, PRInt32* aIndex) = 0; >+ >+ /* Add a newline before the comment. And use /**, not /* >+ * Replace the stylesheets in aOldSheets with the stylesheets in >+ * aNewSheets. The two lists must have equal length, and the sheet >+ * at positon J in the first list will be replaced by the sheet at >+ * position J in the second list. Some sheets in the second list >+ * may be null; if so the corresponding sheets in the first list >+ * will simply be removed. >+ */ >+ NS_IMETHOD UpdateStyleSheets(nsCOMArray<nsIStyleSheet>& aOldSheets, >+ nsCOMArray<nsIStyleSheet>& aNewSheets) = 0; >+ >+ /** >+ * Add a stylesheet to the document >+ */ > virtual void AddStyleSheet(nsIStyleSheet* aSheet, PRUint32 aFlags) = 0; >+ /** Add a newline before the comment. >+ * Remove a stylesheet from the document >+ */ > virtual void RemoveStyleSheet(nsIStyleSheet* aSheet) = 0; >- NS_IMETHOD UpdateStyleSheets(nsISupportsArray* aOldSheets, nsISupportsArray* aNewSheets) = 0; >- >- NS_IMETHOD InsertStyleSheetAt(nsIStyleSheet* aSheet, PRInt32 aIndex, PRBool aNotify) = 0; >- virtual void SetStyleSheetDisabledState(nsIStyleSheet* aSheet, >- PRBool aDisabled) = 0; >+ /** Add a newline before the comment. And use /**, not /* >+ * Notify the document that the applicable state of the sheet changed >+ * and that observers should be notified and style sets updated >+ */ >+ virtual void SetStyleSheetApplicableState(nsIStyleSheet* aSheet, >+ PRBool aApplicable) = 0; > > /** > * Set the object from which a document can get a script context. > * This is the context within which all scripts (during document >- * creation and during event handling) will run. >- */ >+ * creation and during event handling) will run. */ Undo this change. >Index: content/base/public/nsIStyleSheet.h >=================================================================== >+ /** >+ * Set the stylesheet to be enabled. This may or may not make it applicable. Move applicable onto its own line. >+ */ > NS_IMETHOD SetEnabled(PRBool aEnabled) = 0; >Index: content/html/document/src/nsHTMLContentSink.cpp >=================================================================== >@@ -5361,36 +5356,48 @@ > } > > NS_IMETHODIMP > HTMLContentSink::StyleSheetAdded(nsIDocument *aDocument, > nsIStyleSheet* aStyleSheet) > { >- // Processing of a new style sheet causes recreation of the frame >- // model. As a result, all contexts should update their notion of >- // how much frame creation has happened. >- UpdateAllContexts(); >+ // We only care when enabled sheets are added s/enabled/applicable/ >+ NS_PRECONDITION(aStyleSheet, "Must have a style sheet!"); >+ PRBool applicable; >+ aStyleSheet->GetApplicable(applicable); > NS_IMETHODIMP > HTMLContentSink::StyleSheetRemoved(nsIDocument *aDocument, > nsIStyleSheet* aStyleSheet) > { >- // Removing a style sheet causes recreation of the frame model. >- // As a result, all contexts should update their notion of how >- // much frame creation has happened. >- UpdateAllContexts(); >+ // We only care when enabled sheets are removed s/enabled/applicable/ >+ NS_PRECONDITION(aStyleSheet, "Must have a style sheet!"); >+ PRBool applicable; >+ aStyleSheet->GetApplicable(applicable); >Index: content/html/style/src/nsCSSLoader.cpp >=================================================================== >+ // mIsAgent is true from the moment we are placed in the loader's s/mIsAgent/mIsLoading/ >+ // "loading datas" table (right after the async channel is opened) >+ // to the moment we are removed from said table (due to the load >+ // completing or being cancelled). >+ PRPackedBool mIsLoading; // Set once the data is in the "loading" table >+ >+CSSLoaderImpl::CreateSheet(nsIURI* aURI, >+ PRUint32 aDefaultNameSpaceID, >+ StyleSheetState& aSheetState, >+ nsICSSStyleSheet** aSheet) > { >+ if (!*aSheet) { >+ aSheetState = eSheetNeedsParser; >+ nsCOMPtr<nsIURI> sheetURI = aURI; >+ // Need to create one now >+ if (!sheetURI) { >+ // inline style... Why are we using the base url here?? >+ mDocument->GetBaseURL(*getter_AddRefs(sheetURI)); Let's use the document url. >+/** >+ * InsertSheetInDoc handles ordering of sheets in the document. Here >+ * we have two types of sheets -- those with linking elements and >+ * those without. The former are loaded by Link: headers. s/former/latter/ >+ * The following constraints are observed: >+ * 1) Any sheet with a linking element comes after all sheets without >+ * linking elements >+ * 2) Sheets without linking elements are inserted in the order in >+ * which the inserting requests come in, since all of these are * s/ *// >+ * inserted during header data processing in the content sink >+ * 3) Sheets with linking elements are ordered based on document order >+ * as determined by CompareTreePosition. This has me worried about performance, but we'll see. >+ */ >+nsresult >+CSSLoaderImpl::InsertSheetInDoc(nsICSSStyleSheet* aSheet, >+ nsIContent* aLinkingElement, >+ nsIDocument* aDocument) >+{ >+ // XXX <meta> elements do not implement nsIStyleSheetLinkingElement; >+ // need to fix this for them to be ordered correctly. Want to file a bug on that? >+/** >+ * InsertChildSheet handles ordering of @import-ed sheet in their >+ * parent sheets. Here we want to just insert based on order of the >+ * @import rules that imported the sheets. In theory we can't just >+ * append to the end because the CSSOM can insert @import rules. In >+ * practice, we get the call to load the child sheet before the CSSOM >+ * has finished inserting the @import rule, so we have no idea where >+ * to put it anyway. So just append for now. >+ */ Is there a bug report about this? >Index: content/xml/document/src/nsXMLContentSink.cpp >=================================================================== >@@ -757,21 +756,19 @@ > rv = NS_NewURI(getter_AddRefs(url), aHref, nsnull, mDocumentBaseURL); > if (NS_FAILED(rv)) { > return NS_OK; // The URL is bad, move along, don't propagate the error (for now) > } > PRBool doneLoading; > rv = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia, kNameSpaceID_Unknown, >- mStyleSheetCount++, > ((!aAlternate) ? mParser : nsnull), > doneLoading, > this); > if (NS_SUCCEEDED(rv) || (rv == NS_ERROR_HTMLPARSER_BLOCK)) { > if (rv == NS_ERROR_HTMLPARSER_BLOCK && mParser) { > mParser->BlockParser(); > } >- mStyleSheetCount++; > } Remove the outer if too >@@ -1822,18 +1818,17 @@ > ((tagAtom == nsHTMLAtoms::link) || > (tagAtom == nsHTMLAtoms::style))) { > nsCOMPtr<nsIStyleSheetLinkingElement> ssle(do_QueryInterface(content)); > > if (ssle) { > ssle->SetEnableUpdates(PR_TRUE); >- result = ssle->UpdateStyleSheet(nsnull, mStyleSheetCount); >+ result = ssle->UpdateStyleSheet(nsnull); > if (NS_SUCCEEDED(result) || (result == NS_ERROR_HTMLPARSER_BLOCK)) { > if (result == NS_ERROR_HTMLPARSER_BLOCK && mParser) { > mParser->BlockParser(); > } >- mStyleSheetCount++; > } Same here >@@ -1981,15 +1976,13 @@ > } > > result = AddContentAsLeaf(node); > > if (ssle) { > ssle->SetEnableUpdates(PR_TRUE); >- result = ssle->UpdateStyleSheet(nsnull, mStyleSheetCount); >- if (NS_SUCCEEDED(result) || (result == NS_ERROR_HTMLPARSER_BLOCK)) >- mStyleSheetCount++; // This count may not reflect the real stylesheet count >+ result = ssle->UpdateStyleSheet(nsnull); > } > > if (NS_FAILED(result)) { > if (result == NS_ERROR_HTMLPARSER_BLOCK && mParser) { > mParser->BlockParser(); > } Please move this block inside the if (ssle) above (I know you didn't change that) >Index: content/xul/document/src/nsXULDocument.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v >retrieving revision 1.531 >diff -u -6 -r1.531 nsXULDocument.cpp >--- content/xul/document/src/nsXULDocument.cpp 19 Nov 2002 18:22:36 -0000 1.531 >+++ content/xul/document/src/nsXULDocument.cpp 26 Nov 2002 01:07:44 -0000 >@@ -1303,21 +1303,45 @@ > { > aCount = (mRootContent != nsnull); > return NS_OK; > } > > NS_IMETHODIMP >-nsXULDocument::GetNumberOfStyleSheets(PRInt32 *aCount) >+nsXULDocument::GetNumberOfStyleSheets(PRBool aIncludeSpecialSheets, >+ PRInt32 *aCount) > { >- *aCount = mStyleSheets.Count(); >- return NS_OK; >+ PRInt32 count = mStyleSheets.Count(); >+ if (aIncludeSpecialSheets) { >+ *aCount = count; >+ return NS_OK; >+ } >+ >+ if (count != 0 && >+ (nsIHTMLCSSStyleSheet*)mInlineStyleSheet == mStyleSheets.ElementAt(count - 1)) { >+ // subtract off for the inline style sheet "substract the inline style sheet" >+ --count; >+ } >+ >+ if ((nsIHTMLStyleSheet*)mAttrStyleSheet == mStyleSheets.ElementAt(0)) { >+ // subtract off the attr sheet "substract the attr sheet" >Index: layout/html/base/src/nsPresShell.cpp >=================================================================== >@@ -1966,30 +1966,34 @@ > > NS_IMETHODIMP > PresShell::SelectAlternateStyleSheet(const nsString& aSheetTitle) > { >+ PRBool complete; >+ sheet->GetComplete(complete); >+ if (complete) { >+ nsAutoString type; >+ sheet->GetType(type); >+ if (PR_FALSE == type.Equals(textHtml)) { >+ nsAutoString title; Make this if !type.Equals() and remove a space after nsAutoString >+ sheet->GetTitle(title); >+ if (0 < title.Length()) { Make this if (!title.IsEmpty())

Comment on attachment 107410 [details] [diff] [review] Here we are >Index: rdf/chrome/src/nsChromeRegistry.cpp >=================================================================== >+ // The document sheets just need to be done once; the document will notify >+ // the presshells and style sets >+ nsCOMPtr<nsIHTMLContentContainer> container = do_QueryInterface(document); >+ nsCOMPtr<nsICSSLoader> cssLoader; >+ rv = container->GetCSSLoader(*getter_AddRefs(cssLoader)); >+ if (NS_FAILED(rv)) return rv; NS_ENSURE_SUCCESS(rv, rv) please. (same in a few more places in the chrome-registry code) >+ // Iterate over our old sheets and kick off a sync load of the new >+ // sheet if and only if it's a chrome URL. Out of scope for this bug, but would it be enough to copy the disabled-flag over to the new stylesheet? Or do we use some other mechanism to disable titled stylesheets? Hmm.. another thing that occured to me: nodes that hold a pointer to thier stylesheet (like <?xml-stylesheet?> and <link> won't be notified of this change. But that is defenetly out of scope for this patch. Maybe a better design would be to walk all stylesheet linking elements and tell them to update themselfs if they link to a chrome stylesheet? And since i'm on the topic of out of scope for this patch :) What happens with @import-ed stylesheets. If they are in the list in nsIDocument then they will be reloaded even though they might not be imported in the new stylesheet. In general this entire approach seems wrong. If you switch to a skin that is missing a stylesheet and then switch back you will have lost that stylesheet-url since it was removed from the "nsIDocument-list". >Index: content/base/public/nsIDocument.h >=================================================================== > /** > * Set the object from which a document can get a script context. > * This is the context within which all scripts (during document >- * creation and during event handling) will run. >- */ >+ * creation and during event handling) will run. */ This looks like a bad change to me. >Index: content/base/src/nsDocument.cpp >=================================================================== > for (i = 0; i < imax; i++) { > nsCOMPtr<nsIStyleSheet> sheet; >- mDocument->GetStyleSheetAt(i, getter_AddRefs(sheet)); >+ mDocument->GetStyleSheetAt(i, PR_FALSE, getter_AddRefs(sheet)); > if (!sheet) > continue; >+ > nsCOMPtr<nsIDOMStyleSheet> domss(do_QueryInterface(sheet)); >- > if (domss) { > count++; > } Kill the first |if|, do_QI is null-safe. Same in nsDOMStyleSheetList::Item Is this even needed, do we have any stylesheets that are not nsIDOMStyleSheet? > NS_IMETHODIMP >-nsDocument::GetStyleSheetAt(PRInt32 aIndex, nsIStyleSheet** aSheet) >+nsDocument::GetNumberOfStyleSheets(PRBool aIncludeSpecialSheets, >+ PRInt32* aCount) >+{ >+ if (aIncludeSpecialSheets) { >+ nsDocument::InternalGetNumberOfStyleSheets(aCount); >+ } else { >+ InternalGetNumberOfStyleSheets(aCount); >+ } >+ return NS_OK; >+} eww, this is not nice. Couldn't you override GetNumberOfStyleSheets in the subclasses instead of overriding InternalGetNumberOfStyleSheets? Or replace the if-part with |aCount = mStylesheets.Count();|. I would prefer the latter. When getting including the special stylesheets you should always be using the entire mStylesheets-array anyway, right? Same for nsDocument::GetStyleSheetAt >-void nsDocument::SetStyleSheetDisabledState(nsIStyleSheet* aSheet, >- PRBool aDisabled) >+void nsDocument::SetStyleSheetApplicableState(nsIStyleSheet* aSheet, >+ PRBool aApplicable) > { > NS_PRECONDITION(aSheet, "null arg"); >- PRInt32 indx = mStyleSheets.IndexOf(aSheet); >+ >+ PRInt32 indx; > PRInt32 count; > // If we're actually in the document style sheet list >- if (-1 != indx) { >+ if (-1 != mStyleSheets.IndexOf(aSheet)) { > count = mPresShells.Count(); > for (indx = 0; indx < count; ++indx) { > nsCOMPtr<nsIPresShell> shell = > (nsIPresShell*)mPresShells.ElementAt(indx); > nsCOMPtr<nsIStyleSet> set; > if (NS_SUCCEEDED(shell->GetStyleSet(getter_AddRefs(set)))) { > if (set) { >- if (aDisabled) { >- set->RemoveDocStyleSheet(aSheet); >+ if (aApplicable) { >+ set->AddDocStyleSheet(aSheet, this); > } > else { >- set->AddDocStyleSheet(aSheet, this); >+ set->RemoveDocStyleSheet(aSheet); > } > } > } > } >- } >+ } call AddStyleSheetToStyleSets/RemoveStyleSheetFromStyleSets. Same in nsXULDocument >Index: content/base/src/nsDocument.h >=================================================================== >+ virtual void InternalGetStyleSheetAt(PRInt32 aIndex, nsIStyleSheet** aSheet); >+ virtual void InternalGetNumberOfStyleSheets(PRInt32* aCount); Make these functions return the result instead of using an out-param >Index: content/base/src/nsStyleLinkElement.cpp >=================================================================== > NS_IMETHODIMP > nsStyleLinkElement::SetStyleSheet(nsIStyleSheet* aStyleSheet) > { > mStyleSheet = aStyleSheet; >- >+ nsCOMPtr<nsICSSStyleSheet> cssSheet = do_QueryInterface(aStyleSheet); >+ if (cssSheet) { >+ nsCOMPtr<nsIDOMNode> node; >+ CallQueryInterface(this, >+ NS_STATIC_CAST(nsIDOMNode**, getter_AddRefs(node))); nsCOMPtr<nsIDOMNode> node = do_QI(this); >Index: content/base/src/nsStyleSet.cpp >=================================================================== > void StyleSetImpl::RemoveOverrideStyleSheet(nsIStyleSheet* aSheet) > { > NS_PRECONDITION(nsnull != aSheet, "null arg"); >- >+#ifdef DEBUG >+ PRBool complete = PR_TRUE; >+ aSheet->GetComplete(complete); >+ NS_ASSERTION(complete, "Incomplete sheet being removed from style set"); >+#endif Why not assert on applicable here too? Same for RemoveDocStyleSheet and RemoveAgentStyleSheet. Index: content/html/document/src/nsHTMLDocument.cpp =================================================================== +nsHTMLDocument::InternalGetNumberOfStyleSheets(PRInt32* aCount) +{ + NS_PRECONDITION(aCount, "null out param"); + *aCount = mStyleSheets.Count(); + if (*aCount != 0 && mStyleAttrStyleSheet == mStyleSheets[(*aCount) - 1]) + --(*aCount); + --(*aCount); // for the attr sheet + NS_ASSERTION(*aCount >= 0, "Why did we end up with a negative count?"); } In nsHTMLDocument::InternalInsertStyleSheetAt you assume that if mStyleAttrStyleSheet is non-null that it is in the mStyleSheets-array, whereas here you explicitly check that it is. >Index: content/html/style/src/nsCSSParser.cpp >=================================================================== >- PRInt32 mChildSheetCount; why remove this? There shouldn't be the same problem here as for while parsing a DOM since the stylesheet can't be modified during load. Or do you simply want to keep the code cleaner and use the same code during parsing as when the CSSOM is modified? >Index: content/html/style/src/nsCSSStyleSheet.cpp >=================================================================== > nsresult > CSSStyleSheetImpl::EnsureUniqueInner(void) > { > if (! mInner) { > return NS_ERROR_NOT_INITIALIZED; > } >- if (1 < mInner->mSheets.Count()) { >+ if (1 < mInner->mSheets.Count() && mInner->mComplete) { > CSSStyleSheetInner* clone = mInner->CloneFor(this); > if (clone) { > mInner->RemoveSheet(this); > mInner = clone; > } > else { I'm pondering if you should return an error if !mInner->mComplete. Looking for example at the code for CSSRuleListImpl::Item it seems like some parenting will fail if you don't have a unique mInner? It would seem like everybody that requests a unique inner should do it for a good reason? > NS_IMETHODIMP > CSSStyleSheetImpl::DeleteRuleFromGroup(nsICSSGroupRule* aGroup, PRUint32 aIndex) > { > NS_ENSURE_ARG_POINTER(aGroup); >- >+ NS_ASSERTION(mInner && mInner->mComplete, >+ "No deleting from an incomplete sheet!"); why only an assertion? Same in CSSStyleSheetImpl::InsertRuleIntoGroup >@@ -3052,38 +3102,44 @@ > this->QueryInterface(NS_GET_IID(nsIDOMCSSStyleSheet), getter_AddRefs(thisSheet)); > > if (thisSheet != ruleSheet) { > return NS_ERROR_INVALID_ARG; > } > >- result = mDocument->BeginUpdate(); >- NS_ENSURE_SUCCESS(result, result); >+ if (mDocument) { >+ result = mDocument->BeginUpdate(); >+ NS_ENSURE_SUCCESS(result, result); >+ } out of curiosity, when would mDocument be null? Could you NS_ENSURE on that in the beginning of the function instead? Same on a few more places in the same file >Index: layout/html/base/src/nsPresShell.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v >retrieving revision 3.582 >diff -u -6 -r3.582 nsPresShell.cpp >--- layout/html/base/src/nsPresShell.cpp 25 Nov 2002 08:33:12 -0000 3.582 >+++ layout/html/base/src/nsPresShell.cpp 26 Nov 2002 01:08:05 -0000 >@@ -1966,30 +1966,34 @@ > > NS_IMETHODIMP > PresShell::SelectAlternateStyleSheet(const nsString& aSheetTitle) > { > if (mDocument && mStyleSet) { > PRInt32 count = 0; >- mDocument->GetNumberOfStyleSheets(&count); >+ mDocument->GetNumberOfStyleSheets(PR_FALSE, &count); > PRInt32 index; > NS_NAMED_LITERAL_STRING(textHtml,"text/html"); > for (index = 0; index < count; index++) { > nsCOMPtr<nsIStyleSheet> sheet; >- mDocument->GetStyleSheetAt(index, getter_AddRefs(sheet)); >+ mDocument->GetStyleSheetAt(index, PR_FALSE, getter_AddRefs(sheet)); > if (sheet) { This should never be null, right? >Index: layout/mathml/base/src/nsMathMLFrame.cpp >=================================================================== >@@ -650,12 +650,14 @@ > NS_ASSERTION(cssSheet && domSheet, "unexpected null pointers"); > // we will keep the sheet orphan as we populate it. This way, > // observers of the document won't be notified and we avoid any troubles > // that may come from reconstructing the frame tree. Our rules only need > // a re-resolve of style data and a reflow, not a reconstruct-all... > sheet->SetOwningDocument(nsnull); >+ // We're about to manipulate the CSSOM, so we better do this >+ sheet->SetComplete(); why is this needed? isn't the mathml stylesheet loaded syncronyously (sp)? Unfortunatly i still have nsCSSLoader to do, but it's just too late :( I'll try to get to it tomorrow asap.

Comment on attachment 107410 [details] [diff] [review] Here we are putting back petervs sr from the midair

I tested attachment 107410 [details] [diff] [review], applied to a trunk build pulled last night, on 500MHz/128MB Linux, and I don't see any significant difference in page load results, either in overall results, or on a URL-by-URL basis. Nice work, bz.

Peterv's comments: > I don't like the additional argument I don't either, but I found separate functions to be even uglier... I'm aiming for eliminating all callers that need the "full" stylesheet array in any case; then I can just remove those args... Good catches on that comments, Peter. Will fix. > Want to file a bug on that? Bug 36511 > Is there a bug report about this? Bug 183038 Sicking's comments: The chrome registry stuff is just all messed. I "fixed" it to work with the new API and cleaned up some _really_ glaring issues, but I feel it should just be rewritten. No, I don't plan to do that in this bug. > Is this even needed, do we have any stylesheets that are not nsIDOMStyleSheet? Well, in the "old" world there are the "special" sheets. Those are not nsIDOMStyleSheet.... In the "new" world, I don't believe we do. But I'd rather not have to change this code if we ever do. > Or replace the if-part with |aCount = mStylesheets.Count();| What does that mean exactly? (You say you would prefer it, so I'd like to know what you prefer. ;) ). Yes, when getting including "special" sheets the whole array should be used. > call AddStyleSheetToStyleSets/RemoveStyleSheetFromStyleSets Sure thing. > Make these functions return the result OK. > nsCOMPtr<nsIDOMNode> node = do_QI(this); That does not compile due to the ambiguous inheritance from nsISupports. I thought we'd gone over this on IRC already? > Why not assert on applicable here too? Because typically a sheet being removed from a style set is being removed because it got set disabled (hence is no longer applicable). In other words, that assertion would never fire. Will look into the issue with mStyleAttrStyleSheet. That should probably become an assertion... > Or do you simply want to keep the code cleaner Yes. That's what this whole patch is about. ;) > It would seem like everybody that requests a unique inner should do it for a > good reason? Nope. For example, every single rule append during parsing will call down into this code... I could check all callers of EnsureUniqueInner and condition the ones I care about on mComplete, but this seemed safer and simpler... If the inner is not complete, you can't get a CSSRuleListImpl, much less call ::Item() on it, so the parenting is not something I'm worried about. > why only an assertion? Because this function should never even be called if the sheet is not complete. If it is, someone fucked up a completeness check up the ladder somewhere. > out of curiosity, when would mDocument be null? UA sheets, eg. The preference stylesheet that presshell creates, possibly. Sheets whose media lists are being set prior to insertion into the document. That sort of thing. So no, I could not NS_ENSURE that. > This should never be null, right? Yes. I was aiming for minimal changes to that code, since I plan to remove it all anyway. > why is this needed? isn't the mathml stylesheet loaded syncronyously (sp)? It's not loaded. It's created using do_CreateInstance, then populated via the CSSOM. It needs to be set complete before being populated, since the constructor sets the completeness status to PR_FALSE. I'll try to post an updated patch tonight....

> The chrome registry stuff is just all messed. I "fixed" it to work with the > new API and cleaned up some _really_ glaring issues, but I feel it should just > be rewritten. No, I don't plan to do that in this bug. Yeah, this is absolutly out of scope for this bug. Just wanted to put down some mindstorming. > > Is this even needed, do we have any stylesheets that are not > > nsIDOMStyleSheet? > Well, in the "old" world there are the "special" sheets. Those are not > nsIDOMStyleSheet.... > In the "new" world, I don't believe we do. But I'd rather not have to change > this code if we ever do. I'd happily see the code go if it's not needed now (it can always be readded if needed), but it's no biggie. > > Or replace the if-part with |aCount = mStylesheets.Count();| > What does that mean exactly? (You say you would prefer it, so I'd like to know > what you prefer. ;) ). Yes, when getting including "special" sheets the whole > array should be used. The logic right now is something like "InternalGetNumberOfStyleSheets gets the number of non-special stylesheets, except in nsDocument where it gets the total number of stylesheets", this is not nice :) I'd prefer something like: nsDocument::GetNumberOfStyleSheets(PRBool aIncludeSpecialSheets, PRInt32* aCount) { if (aIncludeSpecialSheets) { *aCount = mStylesheets.Count(); } else { InternalGetNumberOfStyleSheets(aCount); } return NS_OK; } And the same in nsDocument::GetStyleSheetAt

More on Peterv's comments: > Let's use the document url. I figured out why we need to use the base url. I've updated the comment accordingly (it's so @import from inline style will use the right base). sicking and I talked about the EnsureUniqueInner issue, and moving the completeness check to WillDirty() makes a bit more sense; I will be doing that.

This addresses the issues peterv and sicking raised (still pending sickings review of nsCSSLoader)

> I figured out why we need to use the base url. I've updated the comment > accordingly (it's so @import from inline style will use the right base). Eh? @import should be based on the "base" of the stylesheet's URI, why would it be different for a stylesheet in a document? Does this affect the URI that the DOM inspector gives for inline style?

> @import should be based on the "base" of the stylesheet's URI Yes, but inline stylesheets don't have a URI per se. Hence the issue. > Does this affect the URI that the DOM inspector gives for inline style? Unfortunately, yes. If there's a bug on that already (and I seem to recall there is) assign it to me. Imo stylesheets should have two separate URIs -- the base URI and the sheet URI. Right now they only have the former, really.

Can't the base uri always be derived from the stylesheet uri?

> Can't the base uri always be derived from the stylesheet uri? That's what we're doing right now. But inline sheets should have a stylesheet URI of "null". Hard to derive anything from that...

Filed bug 183131 on some issues that I was going to address as part of this patch and then decided it was already big enough.

-static nsresult MediumEnumFunc(const nsString& aSubString, void* aData) +static nsresult MediumEnumFunc(const nsAString& aSubString, void* aData) { - nsIAtom* medium = NS_NewAtom(aSubString); - if (!medium) return NS_ERROR_OUT_OF_MEMORY; + nsCOMPtr<nsIAtom> medium = do_GetAtom(aSubString); + if (!medium) { + return NS_ERROR_OUT_OF_MEMORY; + } use NS_ENSURE_TRUE You might wanna comment on that CSSLoaderImpl::CreateSheet can return a stylesheet that has bogus media and title (and other things that gets cloned) + if (!mDocument && !aLoadData->mIsAgent) { + // No point starting the load; just release all the data and such. + LOG_WARN((" No document and not agent sheet; pre-dropping load")); + SheetComplete(aLoadData, PR_FALSE); + return NS_OK; + } Can you move this to the top of ::LoadSheet? i wonder if you should flush mPendingDatas in CSSLoaderImpl::DropDocumentReference? That'll ensure that they won't get leaked and there is not really any point in loading them anyway. + PrepareSheet(sheet, NS_LITERAL_STRING(""), aMedia); + NS_ENSURE_SUCCESS(rv, rv); missing |rv =| in LoadChildSheet. + NS_PRECONDITION((!aSheet || !aObserver) && (aSheet || aObserver), + "One or the other please, and at least one"); s/at least/max/ please use NS_ENSURE_TRUE in both NS_NewCSSLoader CSSLoaderImpl::StopLoadingSheet won't remove the stylesheet from mPendingDatas

fixing midair

Comment on attachment 107986 [details] [diff] [review] Patch updated per sicking's comments In addition to addressing the issues raised in comment 79, this fixes a problem in Stop() and StopLoadingSheet() that could lead to a sheet load data being released twice (once from Stop() and once the actual stream completes).

Comment on attachment 107986 [details] [diff] [review] Patch updated per sicking's comments kick ass!! check it in :-)

Comment on attachment 107988 [details] [diff] [review] Fix up nsDOMStyleSheetList::Item too check in this too ;-)

Checked in, but this seems to have caused a perf regression on btek... :( Leaving open for now pending sorting that out. jrgm, is there any way I can get a more detailed breakdown on the data for that test?

This added another compiler warning too: content/base/src/nsDocument.cpp:233 (See build log excerpt) Comparison between signed and unsigned 231 PRInt32 count = 0; 232 mDocument->GetNumberOfStyleSheets(PR_FALSE, &count); --> 233 if (aIndex < count) { 234 nsCOMPtr<nsIStyleSheet> sheet; 235 mDocument->GetStyleSheetAt(aIndex, PR_FALSE, getter_AddRefs(sheet));

OK. I've checked in a fix for the warning, and it looks like btek had just hiccupped at the same time (the time went back down and no other Tp tbox showed any change). So marking fixed. ;)

alecf points out that Ts on comet went up by a few ms in about the right (wrong?) time period... I'm going to wait for caillon to land his CompareDocumentPosition changes and see how that affects things....

another interesting question is, how does UTF8InputStream compare to ConverterInputStream efficiency-wise? We now use the former for UA sheets instead of the latter (which we sorta used to do). I'd be willing to switch that and see what happens...

re: comment #86. Yes, btek has developed a bad hiccup. I've looked at it several times, but at this point it appears to both "real" and just "noise". See bug 175652 comment #5.

This patch caused bug 183165 because UTF8InputStream does not do error recovery... I fixed that by using nsConverterInputStream, but that seems to have raised the Ts on comet another 5ms or so for a total rise of what looks like 10-12 ms out of 1400. I'm going to see what I can do to fix that....