While working on bug 984880 I noticed that parseStyleSheet re-evaluates @import rules. This causes problems for the style editor and for the as-authored project. Suppose there is an outer style sheet A that @imports an inner sheet B. Open the style editor and edit B. Then, edit A. The edits you've made to B are now lost. I think what we'd like for devtools is for CSSStyleSheet::ParseSheet to try to preserve @imports rather than re-evaluate them. The basic idea would be to keep all the imported sheets when disconnecting, then reconnect a sheet when the appropriate @import is seen. This approach would make it possible for parseStyleSheet to add or delete an @import, while avoiding extra re-evals. I have no idea how feasible this is.

Comment on attachment 8658379 [details] [diff] [review] re-use @imported style sheets from inIDOMUtils.parseStyleSheet This seems to work; though I am not sure if there are any details I have missed.

Some comments without my having had a very close look at the patch. I think the general approach of saving style sheets to re-use should be OK. I guess the expectation is that if you add a new @import to the sheet, then it will get loaded (and maybe cause other loads of its own, due to @import rules), but that if you leave existing @imports in there, with their same URL, those ones get re-used. I'm worried that Loader methods might be called re-entrantly -- maybe one of the @import rules references a data: URL which is loaded synchronously? -- and so having "global" state in Loader::mReusableSheets might not be appropriate. Or would that be fine/expected? If you add a new @import rule, with a new URL, and that newly referenced sheet wants to @import one of the sheets that you had saved for re-use, would you want it using it? Probably not -- I think you only want the top sheet that is being re-parsed to be able to re-use sheets. Another option could instead be passing the sheets to re-use as an argument into nsCSSParser::ParseSheet. Then, in CSSParserImpl::ProcessImport it can maybe pass in the re-usable sheet into Loader::LoadChildSheet. So the CSSParserImpl would be in charge of ensuring each re-usable sheet gets re-used once. Does that sound reasonable?

(In reply to Cameron McCormack (:heycam) from comment #3) > Some comments without my having had a very close look at the patch. Thanks very much. > I'm worried that Loader methods might be called re-entrantly -- maybe one of > the @import rules references a data: URL which is loaded synchronously? -- > and so having "global" state in Loader::mReusableSheets might not be > appropriate. Or would that be fine/expected? If you add a new @import > rule, with a new URL, and that newly referenced sheet wants to @import one > of the sheets that you had saved for re-use, would you want it using it? > Probably not -- I think you only want the top sheet that is being re-parsed > to be able to re-use sheets. Yes, I agree, only the top sheet should be able to reuse the saved sheets. > Another option could instead be passing the sheets to re-use as an argument > into nsCSSParser::ParseSheet. Then, in CSSParserImpl::ProcessImport it can > maybe pass in the re-usable sheet into Loader::LoadChildSheet. So the > CSSParserImpl would be in charge of ensuring each re-usable sheet gets > re-used once. > > Does that sound reasonable? Yes. I'll do that.

Updated per comments.

Comment on attachment 8660038 [details] [diff] [review] re-use @imported style sheets from inIDOMUtils.parseStyleSheet This implements the idea you mentioned. Note that a given sheet can only be reused once due to how FindReusableStyleSheet modifies the list of style sheets when one is found. This messes up the alignment of some parameter names in nsCSSParser. It seemed better this way to me, but I'm happy to change them all to be aligned again if you prefer. I chose to use a default argument to make the patch simpler, but it's easy enough to update all the other callers to pass nullptr if that is what you like.

Comment on attachment 8660038 [details] [diff] [review] re-use @imported style sheets from inIDOMUtils.parseStyleSheet Review of attachment 8660038 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments addressed. ::: layout/inspector/tests/test_parseStyleSheetImport.html @@ +44,5 @@ > + "re-re-re-parsed sheet has @import rule"); > + isnot(sheet.cssRules[0].styleSheet, importedSheet, > + "imported sheet has changed now"); > + > + SimpleTest.finish(); Can you add a sub-test that has multiple @imports, with different URLs, possible added back in different orders, to check that that works as expected? ::: layout/style/CSSStyleSheet.cpp @@ +2306,5 @@ > + importRule->GetStyleSheet(getter_AddRefs(childSheet)); > + > + nsRefPtr<CSSStyleSheet> cssSheet = do_QueryObject(childSheet); > + if (cssSheet) { > + reusableSheets.AddReusableSheet(cssSheet); CSSStyleSheet::GetOriginalURI says it can return null sometimes. Should we check that and skip adding it to the reusable sheets list if so? ::: layout/style/CSSStyleSheet.h @@ +251,5 @@ > > bool UseForPresentation(nsPresContext* aPresContext, > nsMediaQueryResultCacheKey& aKey) const; > > + nsresult ParseSheet(const nsAString& aInput, While you're here, what do you think about renaming ParseSheet to ReparseSheet? ::: layout/style/Loader.cpp @@ +516,5 @@ > + nsRefPtr<CSSStyleSheet>& aResult) > +{ > + for (size_t i = 0; i < mReusableSheets.Length(); ++i) { > + bool sameURI; > + nsresult rv = aURL->Equals(mReusableSheets[i]->GetOriginalURI(), &sameURI); CSSStyleSheet::GetOriginalURL says it can return null sometimes. Can we make this @@ +519,5 @@ > + bool sameURI; > + nsresult rv = aURL->Equals(mReusableSheets[i]->GetOriginalURI(), &sameURI); > + if (!NS_FAILED(rv) && sameURI) { > + aResult = mReusableSheets[i]; > + mReusableSheets.RemoveElementAt(i); Storing mReusableSheets in reverse order would also allow us to stop shuffling elements around so much as in the common case of the @import rules remaining the same, we'll pick the element off the end of the array. ::: layout/style/Loader.h @@ +143,5 @@ > + } > + > + ~LoaderReusableStyleSheets() > + { > + } May as well omit the destructor. @@ +148,5 @@ > + > + /** > + * Look for a reusable sheet (see AddReusableSheet) matching the > + * given URL. If found, set aResult, remove the reused sheet from > + * the internal list, and return true. Otherwise, return false. Mention explicitly that aResult remains untouched if false is returned? @@ +158,5 @@ > + > + /** > + * Indicate that a certain style sheet is available for reuse if its > + * URI matches the URI of an @import. Sheets should be added in the > + * opposite order in which they are intended to be reused. I assume we're adding in reverse order since that's what's easiest for CSSStyleSheet::ParseSheet. To avoid moving the contents of the array each time AddReusableSheet is called, how about appending them to mReusableSheets and then iterating over them in reverse order? ::: layout/style/nsCSSParser.cpp @@ +1178,5 @@ > // Used for @import rules > mozilla::css::Loader* mChildLoader; // not ref counted, it owns us > > + // Any sheets we may reuse when parsing an @import. > + LoaderReusableStyleSheets* mReusableSheets; Please init this to null in the CSSParserImpl constructor. ::: layout/style/nsCSSParser.h @@ +88,5 @@ > nsIURI* aBaseURI, > nsIPrincipal* aSheetPrincipal, > uint32_t aLineNumber, > + bool aAllowUnsafeRules, > + mozilla::css::LoaderReusableStyleSheets* aReusableSheets = nullptr); I do have mixed feelings about identifier alignment like this, but don't mind leaving long types to break the pattern. Though I think more important is to keep to 80 columns. Can you rewrap the "nullptr);" to the next line?

(In reply to Cameron McCormack (:heycam) from comment #7) > Can you add a sub-test that has multiple @imports, with different URLs, > possible added back in different orders, to check that that works as > expected? Thanks, this revealed a bug. > While you're here, what do you think about renaming ParseSheet to > ReparseSheet? Aha, I didn't realize that domutils was the only user of this method. This change let me drop the new enum.

This addresses the review comments. I removed the enum that I added in an earlier version of the patch. Because ReparseSheet is only called from a single place, the new behavior doesn't need to be optional. I changed LoaderReusableStyleSheets to store mReusableSheets as suggested. This required a little oddity in the iteration, to avoid a compiler warning due to comparing "size_t >= 0". I changed it to check for a null GetOriginalURI. I added some assertions in Loader.cpp as well. These should never fire; one because the caller of FindReusableStyleSheet has the same assertion, and the other due to the check done when constructing the list of reusable sheets. The test to reorder the imported sheets revealed that nothing was resetting a CSSStyleSheet's mNext link; causing AppendStyleSheet to create a circular list. This version of the patch clears mNext when clearing the child sheet list.

Forgot to git add the new .css file for the test.

Comment on attachment 8662428 [details] [diff] [review] re-use @imported style sheets from inIDOMUtils.parseStyleSheet I wasn't sure whether this changed enough to warrant re-review (in particular due to the enum removal and mNext resetting), or if I could carry over the r+. So, erring on the side of caution. Comment #9 explains the changes.

Comment on attachment 8662428 [details] [diff] [review] re-use @imported style sheets from inIDOMUtils.parseStyleSheet Review of attachment 8662428 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Can you just add one more subtest (that I just thought of) for adding two @imports with the same URL and ensuring that one gets re-used and the other re-loaded. ::: layout/style/Loader.cpp @@ +516,5 @@ > + nsRefPtr<CSSStyleSheet>& aResult) > +{ > + MOZ_ASSERT(aURL); > + for (size_t i = mReusableSheets.Length(); i > 0; --i) { > + size_t index = i - 1; Yeah, I'm never sure what's the most idiomatic way to loop downwards over an unsigned variable. I've never much liked the too-cute: for (size_t i = n; i-- > 0; ) This isn't bad: for (size_t i = n - 1; i < n; --i) though I think the |i < n| condition takes a little longer to think about than I'd like. Your approach is fine. :)

Add new test.