727834 - Add an API to (re)parse a style sheet inplace

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cedric Vivier [:cedricv]]

This inIDOMUtils API (layout/inspector) would be used by the Style Editor so that style sheet updates do not need to manipulate DOM nodes (<style> ...).

Comment 1

[Cedric Vivier [:cedricv]]

Attachment: patch v1

patch v1.

Successful Try with this patch and a follow-up to make Style Editor use this API:
https://tbpl.mozilla.org/?tree=Try&rev=b0119ea037d7

(the unique browser_styleeditor_new.js fail/intermittent is not related to this patch)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Do you really want to modify an existing sheet object as opposed to creating a new one and replacing the old one in the page with the new one?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I guess one question is whether you need to be able to replace sheets imported via @import or not.

Comment 4

[Cedric Vivier [:cedricv]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Do you really want to modify an existing sheet object as opposed to creating
> a new one and replacing the old one in the page with the new one?

Yes, keeping the same document.styleSheets[n] object references makes things simpler and less intrusive (wrt to the page) for the API users imho (ie. Style Editor).


(In reply to Boris Zbarsky (:bz) from comment #3)
> I guess one question is whether you need to be able to replace sheets
> imported via @import or not.

That would be a great testcase to add :)
If I'm not mistaken it should "just work" as well by keeping the same reference as it is now (though we'd perhaps need to propagate 'dirty bit' to the parent style sheet?)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Yes, keeping the same document.styleSheets[n] object references makes things simpler and
> less intrusive 

Maybe.  It means that suddenly any rules the page is holding references to are orphaned and trying to modify them will throw, right?

I do agree that it probably simplifies implementation...

Comment 6

[Cedric Vivier [:cedricv]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Maybe.  It means that suddenly any rules the page is holding references to
> are orphaned and trying to modify them will throw, right?

Yup, there is one test for this behavior, please let me know if you think there is need for more.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, I'm questioning the behavior itself...

Comment 8

[Cedric Vivier [:cedricv]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Well, I'm questioning the behavior itself...

This is the same behavior as if the page holds a reference to a rule that has been deleted via styleSheet.deleteRule(ruleIndex).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sure.  I know what the behavior is.  I'm trying to understand the design goals here.  Why is modifying the CSSOM OK but modifying the DOM not OK, for example?  Why is removing all rules from a stylesheet OK but changing the URI of an @import rule not ok?

Comment 10

[Boris Zbarsky [:bzbarsky]]

And the upshot is that I'd like to minimize core code complexity if possible; if we're going to add a bunch of scary C++ goop then I'd like to have a _really_ good idea of what we're trying to accomplish with it and why.

Comment 11

[Cedric Vivier [:cedricv]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Why is modifying the CSSOM OK but modifying the DOM not OK, for example?

We'd rather touch only the CSSOM rather than having to touch both (as touching the DOM might trigger mutation events on the page and so on - notwithstanding DOM tree viewer interactions..).

Also this avoids having to specifically handle every different way the CSSOM and the DOM can bind together (<link> or <style> in HTML, <?xml-stylesheet?> in XML/XUL, ...) - it just works with any document that can be styled with CSS.

Deeper down, this API allows fixing 'elegantly' bug 719552, which would as far as I know otherwise require parsing CSS on the javascript side to find and rewrite URIs on change application OR add a DOM API to force a given base URI on an inline <style> element.


> Why is removing all rules from a stylesheet OK but changing the
> URI of an @import rule not ok?

I'm not sure to understand (the why of) this question, so please don't mind if I'm not answering on what you expected :
I would say that the former is ok because removing all rules from a style sheet is something valid that you can do with JavaScript and DOM alone - not breaking semantics/conventions here.

Indeed, this API provides an efficient/easy version of :
1) while (styleSheet.cssRules.length) styleSheet.deleteRule(0);
2) parse CSS to delimit rules character ranges
3) for each (ruleRange in ruleRanges) styleSheet.insertRule(ruleRange);

With the same end behavior.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> We'd rather touch only the CSSOM rather than having to touch both (as touching the DOM
> might trigger mutation events on the page and so on - notwithstanding DOM tree viewer
> interactions..).

Hmm.  Ok.  That seems fair.

> because removing all rules from a style sheet is something valid that you can do with
> JavaScript and DOM alone

Yes, and you can change the URI of an @import with JS and CSSOM...

Alright.  I guess we do want to do it this way.  Let me take a look at the code.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 598155 [details] [diff] [review]
patch v1

>+++ b/layout/style/nsCSSStyleSheet.cpp
>+NS_IMETHODIMP

I see no reason for this to be an NS_IMETHOD, or for that matter to be virtual.

>+  //-- Security check: Only scripts whose principal subsumes that of the
>+  //   style sheet can modify rule collections.
>+  nsresult rv = SubjectSubsumesInnerPrincipal();
>+  NS_ENSURE_SUCCESS(rv, rv);

Web content can't call this, so yo don't need that.

>+  // Hold strong ref to the CSSLoader in case the document update
>+  // kills the document
>+  nsRefPtr<css::Loader> loader;
>+  if (mDocument) {
>+    loader = mDocument->CSSLoader();
>+    NS_ASSERTION(loader, "Document with no CSS loader!");
>+  }

This will make edits of non-document sheets fail to process @import, right?  Is that OK?  If not, you should get a loader manually if !mDocument.

>+  // detach child sheets
>+  for (nsCSSStyleSheet* child = mInner->mFirstChild;
>+       child; child = child->mNext) {
>+    if (child->mParent == this) {
>+      child->SetOwningDocument(nsnull);

You need to null out child->mParent here too.

>+  rv = parser.ParseSheet(aInput, mInner->mSheetURI, mInner->mBaseURI,
>+                         mInner->mPrincipal, 1, false);

This will also fail for UA sheets because of the false.

In fact, given this line calling this method on a UA sheet would be a disaster (of the "crash the browser" variety)....  Can you please check whether just bailing out early if mInner->mPrincipal is the system principal will catch UA sheets?

>+  mInner->RebuildNameSpaces();

I think that needs to come before the ParseSheet call; otherwise the parse will use the old namespace map.  Could you add a test for this, please?

>+  if (mDocument) {
>+    mDocument->StyleRuleAdded(this, GetOwnerRule());

This looks wrong; nothing says passing null for StyleRuleAdded is ok, but you can easily be doing that here.

Technically, the right thing to do would be to notify about all your rule removals and additions.  An alternative may be to just notify nothing for sheets that are disabled, and for sheets that are enabled disable them first, then do all this, then reenable them...

>+  }
>+  return NS_OK;
>+}
>+
> /* virtual */ nsIURI*
> nsCSSStyleSheet::GetOriginalURI() const
> {
>   return mInner->mOriginalSheetURI;
> }
> 
> nsresult
> NS_NewCSSStyleSheet(nsCSSStyleSheet** aInstancePtrResult)
>diff --git a/layout/style/nsCSSStyleSheet.h b/layout/style/nsCSSStyleSheet.h
>index fa602a9..70a4d21 100644
>--- a/layout/style/nsCSSStyleSheet.h
>+++ b/layout/style/nsCSSStyleSheet.h
>@@ -253,16 +253,18 @@ public:
> 
>   // Append all of this sheet's child sheets to aArray.  Return true
>   // on success and false on allocation failure.
>   bool AppendAllChildSheets(nsTArray<nsCSSStyleSheet*>& aArray);
> 
>   bool UseForPresentation(nsPresContext* aPresContext,
>                             nsMediaQueryResultCacheKey& aKey) const;
> 
>+  NS_IMETHOD ParseSheet(const nsAString& aInput);
>+
>   // nsIDOMStyleSheet interface
>   NS_DECL_NSIDOMSTYLESHEET
> 
>   // nsIDOMCSSStyleSheet interface
>   NS_DECL_NSIDOMCSSSTYLESHEET
> 
>   // Function used as a callback to rebuild our inner's child sheet
>   // list after we clone a unique inner for ourselves.
>-- 
>1.7.6.5
>

Comment 14

[Cedric Vivier [:cedricv]]

Attachment: patch v1.1 - addressed review comments

Comment 15

[Cedric Vivier [:cedricv]]

Try underway: https://tbpl.mozilla.org/?tree=Try&rev=f6aac9cd7b01

Comment 16

[Cedric Vivier [:cedricv]]

Attachment: patch v1.1.1 - fix one typo and one signed/unsigned warning

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 600096 [details] [diff] [review]
patch v1.1.1 - fix one typo and one signed/unsigned warning

>From 9156947313e6cbf28cf6eb54c05bf0bea571697b Mon Sep 17 00:00:00 2001
>From: Cedric Vivier <[e-mail]>
>Date: Thu, 2 Feb 2012 18:55:08 +0800
>Subject: [PATCH 1/5] Bug 727834 - Add an API to (re)parse a style sheet
> inplace. r=bz

The checkin comment here is ... odd looking.  Esp the diffstat bits.

>+++ b/layout/inspector/tests/chrome/test_bug727834.xul
>+  is(rule.parentStyleSheet, testSheet,
>+     "child sheet's parent is not null");

You're not looking at the child sheet's parent.  That might be worth testing too, though.  The test string here should talk about the rule's parent.

>+  is(rule.parentStyleSheet, null,
>+     "detached child sheet's parent is null");

Similar here.

>+++ b/layout/style/nsCSSStyleSheet.cpp
>@@ -1905,23 +1915,17 @@ nsCSSStyleSheet::InsertRuleInternal(const nsAString& aRule,
>+      notify = !IsChildSheetLoaded(cssRule);

That looks backwards.  Did we really have no tests for this?  :(

In any case, please double-check this part, but I'm pretty sure that the '!' should not be there.

>+nsCSSStyleSheet::ParseSheet(const nsAString& aInput)
>+  // detach existing rules (including child sheets via import rules)

I don't believe this detaches the child sheets correctly...

Yes, that means deleteRule doesn't quite do it either, but it leaves the sheets in the list starting mFirstChild.  Whereas you drop them, leaving them with dangling pointers as far as I can tell.

I'm sorry the memory management here is a bit confused; this stuff is really not designed around dynamic modification....

>+  while (mInner->mOrderedRules.Count() != 0) {
>+    nsRefPtr<css::Rule> rule = mInner->mOrderedRules.ObjectAt(0);

It's probably better to remove from the end so you can avoid all the memmoves.

>+  // allow unsafe rules if the style sheet's principal is the system principal
>+  bool allowUnsafeRules = false;
>+  nsIScriptSecurityManager *securityManager = nsContentUtils::GetSecurityManager();
>+  rv = securityManager->IsSystemPrincipal(mInner->mPrincipal, &allowUnsafeRules);
>+  NS_ENSURE_SUCCESS(rv, rv);

  bool allowUnsafeRules = nsContentUtils::IsSystemPrincipal(mInner->mPrincipal);

>+      nsRefPtr<css::Rule> rule = mInner->mOrderedRules.ObjectAt(index);
...
>+      mDocument->StyleRuleAdded(this, mInner->mOrderedRules.ObjectAt(index));

Just pass |rule| as the second arg there?

>+  return rv;

  return NS_OK;

please.

Comment 18

[Cedric Vivier [:cedricv]]

Attachment: patch v1.2

Sorry for the long time to get back on this patch, I think I've addressed all your comments.
"!IsChildSheetLoaded(cssRule)" logic was indeed incorrect and double-checking made that made me realize and fix a race failing the assertion in StyleSheetLoaded().

Comment 19

[Cedric Vivier [:cedricv]]

Attachment: patch v1.2.1 - formatting fix

Comment 20

[Boris Zbarsky [:bzbarsky]]

> "!IsChildSheetLoaded(cssRule)" logic was indeed incorrect

I assume the updated tests would catch it?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 628322 [details] [diff] [review]
patch v1.2.1 - formatting fix

>Subject: [PATCH] Bug 727834 - Add an API to (re)parse a style sheet inplace. r=bz

"in place", not "inplace".  Similar in some other places in the patch.

Again, I assume the diffstat stuff won't be part of the checkin comment when landing?  Nor the "[PATCH]" bit?

>+++ b/layout/inspector/src/inDOMUtils.cpp
>+inDOMUtils::ParseStyleSheet(nsIDOMCSSStyleSheet *aSheet,
>+  NS_ENSURE_ARG_POINTER(aSheet);
>+
>+  nsRefPtr<nsCSSStyleSheet> sheet = do_QueryObject(aSheet);

You want to null-check after the do_QueryObject.  There's no reason sheet will be non-null here even if aSheet is non-null.

Alternately, you could make nsIDOMCSSStyleSheet "builtinclass" and then you don't need the QI: a static cast would do.

>+++ b/layout/style/nsCSSStyleSheet.cpp
>+IsChildSheetLoaded(css::Rule *cssRule)

Ok, I just realized why the boolean check here was being so confusing.

This function is NOT testing whether the sheet has loaded.  It's testing whether there is a sheet at all.  The sheet might be fully loaded.  Or it might still be loading.

Please rename the function accordingly.  Perhaps to "RuleHasChildSheet()"?

>@@ -1870,23 +1880,17 @@ nsCSSStyleSheet::InsertRuleInternal(const nsAString& aRule,
>+      notify = IsChildSheetLoaded(cssRule);

And then here the correct code is:

  notify = !RuleHasChildSheet(cssRule);

(as in, the first patch was correct here, modulo the incorrect naming of the helper method).

>+nsCSSStyleSheet::ParseSheet(const nsAString& aInput)
>+  // nuke child sheets list and current namespace map
>+  for (nsCSSStyleSheet* child = mInner->mFirstChild; child; child = child->mNext) {
>+    if (child->mParent == this) {

Why do you need that check?  child->mParent should definitely be "this" here.

>+        if (!IsChildSheetLoaded(rule)) {
>+          continue; // notify when loaded (see StyleSheetLoaded)

And here you'll want to "continue" if RuleHasChildSheet().  So in fact it was this callsite that was wrong in the original patch, not the other one.

Comment 22

[Cedric Vivier [:cedricv]]

Attachment: patch v1.2.2 - address review comments

(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 628322 [details] [diff] [review]
> patch v1.2.1 - formatting fix
> >Subject: [PATCH] Bug 727834 - Add an API to (re)parse a style sheet inplace. r=bz
> 
> "in place", not "inplace".  Similar in some other places in the patch.

Fixed.


> Again, I assume the diffstat stuff won't be part of the checkin comment when
> landing?  Nor the "[PATCH]" bit?

Yes, they won't be there assuming the person doing the check in 'imports' the patch in any sensible way (eg. hg import), never had this included with other checked in patches.


> >+  nsRefPtr<nsCSSStyleSheet> sheet = do_QueryObject(aSheet);
> 
> You want to null-check after the do_QueryObject.  There's no reason sheet
> will be non-null here even if aSheet is non-null.

Done.

> >+++ b/layout/style/nsCSSStyleSheet.cpp
> >+IsChildSheetLoaded(css::Rule *cssRule)
> 
> Ok, I just realized why the boolean check here was being so confusing.
> 
> This function is NOT testing whether the sheet has loaded.  It's testing
> whether there is a sheet at all.  The sheet might be fully loaded.  Or it
> might still be loading.
> Please rename the function accordingly.  Perhaps to "RuleHasChildSheet()"?

I believe your previous gut in comment #17 is correct. It was wrong but what we really want to test is if the child sheet is complete/loaded, I modified the condition to test what the name IsChildSheetLoaded implies.

> >@@ -1870,23 +1880,17 @@ nsCSSStyleSheet::InsertRuleInternal(const nsAString& aRule,
> >+      notify = IsChildSheetLoaded(cssRule);
> 
> And then here the correct code is:
>   notify = !RuleHasChildSheet(cssRule);
> (as in, the first patch was correct here, modulo the incorrect naming of the
> helper method).
> >+        if (!IsChildSheetLoaded(rule)) {
> >+          continue; // notify when loaded (see StyleSheetLoaded)
> 
> And here you'll want to "continue" if RuleHasChildSheet().  So in fact it
> was this callsite that was wrong in the original patch, not the other one.

I modified both callsites to use the same conditional to make this clearer:
we do NOT want to notify the document immediately when the child sheet is not loaded already, so continue'ing the loop is correct afaics.


> >+nsCSSStyleSheet::ParseSheet(const nsAString& aInput)
> >+  // nuke child sheets list and current namespace map
> >+  for (nsCSSStyleSheet* child = mInner->mFirstChild; child; child = child->mNext) {
> >+    if (child->mParent == this) {
> 
> Why do you need that check?  child->mParent should definitely be "this" here.

This seems necessary according to what's done in the destructor and the comment at http://hg.mozilla.org/mozilla-central/file/f4981b5e1f7a/layout/style/nsCSSStyleSheet.h#l76

Comment 23

[Boris Zbarsky [:bzbarsky]]

> It was wrong but what we really want to test is if the child sheet is complete/loaded

No, what we _really_ want to test is whether we expect to get a StyleSheetLoaded notification for the child sheet of the rule.

And that notification is always async, when it gets sent.  So there are only two possible cases here: rule has child sheet, we'll get a StyleSheetLoaded for it later or rule has no child sheet (e.g. load blocked by security check) and we should notify on the rule now.

> This seems necessary according to what's done in the destructor

The destructor has to deal with the case when the inner of the sheet being destroyed is shared with other sheets.  That's also what the comment is about.

But in your case, you've already called WillDirty(), so you have ensured that you have your own inner, so everything in mInner should be parented to you.  Adding an assert to that effect might be good, of course.

Comment 24

[Cedric Vivier [:cedricv]]

Attachment: patch v1.2.3 - rename IsChildSheetLoaded, add assertion

(In reply to Boris Zbarsky (:bz) from comment #23)
> > It was wrong but what we really want to test is if the child sheet is complete/loaded
> 
> No, what we _really_ want to test is whether we expect to get a
> StyleSheetLoaded notification for the child sheet of the rule.
> And that notification is always async, when it gets sent. (...)

Ok got it. Loader.cpp confused me a bit :)


> But in your case, you've already called WillDirty(), so you have ensured
> that you have your own inner, so everything in mInner should be parented to
> you.  Adding an assert to that effect might be good, of course.

Ok, replaced the condional block with an assertion instead.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Ok got it. Loader.cpp confused me a bit :)

Yeah, it's ... overcomplicated.  :(  In fact, complicated enough that I'm confused too.

In fact, what happens right now, without your patch, is that if the sheet is not complete after the rule is parsed we end up notifying StyleRuleAdded twice, because we skip notifying only when there is no child sheet, and in the common case there is one.

It looks like the loader code thinks that in the IsComplete() case the sheet will handle notifying.

So maybe we should name the helper something like "RuleHasPendingChildSheet()" and return true if the sheet is not null and is not complete.  And then skip notifying if RuleHasPendingChildSheet()?

I'm really sorry about the back-and-forth on this.  :(

Comment 26

[Cedric Vivier [:cedricv]]

(In reply to Boris Zbarsky (:bz) from comment #25)
> So maybe we should name the helper something like
> "RuleHasPendingChildSheet()" and return true if the sheet is not null and is
> not complete.  And then skip notifying if RuleHasPendingChildSheet()?

Ok, so if I understand correctly let's use the same check as earlier patch v1.2.2 but with this new clearer name right?

> I'm really sorry about the back-and-forth on this.  :(

Heh, it's alright, it's getting clearer and clearer at least.

Comment 27

[Cedric Vivier [:cedricv]]

Attachment: patch v1.2.4 - RuleHasPendingChildSheets with completeness check

Comment 28

[Boris Zbarsky [:bzbarsky]]

> let's use the same check as earlier patch v1.2.2

Not quite.  The check should be:

  RuleHasPendingSheet(...) {
    return cssSheet && !cssSheet->IsComplete();
  }

(note the '!' there).

And then in the loops:

  if (type == css::Rule::IMPORT_RULE && RuleHasPendingSheet(cssRule)) {
    continue;
  }

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 628792 [details] [diff] [review]
patch v1.2.4 - RuleHasPendingChildSheets with completeness check

Ah, which is what you did!

r=me.  Thank you for putting up with this stuff!

Comment 30

[Sebastian Zartner [:sebo]]

Sounds like this could also be useful for Firebug.

Sebastian

Comment 31

[Cedric Vivier [:cedricv]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ddbc87ee85e4

Comment 32

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ddbc87ee85e4

Comment 33

[Florian Scholz (Open Web Docs)]

Added the new method to https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/inIDOMUtils