Closed Bug 1415940 Opened 7 years ago Closed 6 years ago

Stylo: insertRule() with newlines can break rules inspector

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: adrian17, Assigned: bradwerth)

References

Details

Attachments

(8 files, 1 obsolete file)

Bug 1415940 Part 7: Change canSetRuleText() to test the sheet for modification by CSSOM.
59 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1415940 Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools.
59 bytes, text/x-review-board-request
tromey
: review+
bzbarsky
: review+
Details
Bug 1415940 Part 1: Expand StyleSheet dirty flag into a bitfield, to allow more types of dirtiness.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1415940 Part 2: Give StyleSheet a dirty flag of MODIFIED_RULES, and a method to test it.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1415940 Part 8: Add a browser test to confirm DOM added rules work with leading newlines.
59 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1415940 Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1415940 Part 4: Generalize a StyleSheet cast in ServoStyleRuleDeclaration::SetCSSDeclaration to prevent a NULL dereference from AsServo().
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1415940 Part 5: Change InspectorUtils::GetRelativeRuleLine to not remap line numbers if it introduces underflow.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
On any site (for example bugzilla) run the following code in console:

  let style = document.createElement('style');
  document.head.appendChild(style);
  style.sheet.insertRule('\nbody { color: red }');

Note the leading newline.

While the rule works correctly, trying to see CSS rules of elements in the Inspector results in a white pane and errors in the browser console (similar to bug 1224121):

    Protocol error (unknownError): couldn't find start of the rule
This is a regression that only occurs in Stylo mode.
Blocks: stylo-devtools
Status: UNCONFIRMED → NEW
status-firefox56: --- → unaffected
status-firefox57: --- → wontfix
status-firefox58: --- → affected
Component: Developer Tools: CSS Rules Inspector → CSS Parsing and Computation
Ever confirmed: true
Priority: -- → P2
Product: Firefox → Core
Summary: insertRule() with newlines can break rules inspector → Stylo: insertRule() with newlines can break rules inspector
Assignee: nobody → bwerth
Hmm, curious... in Stylo mode, DevTools tries to read the `textContent` of the <style> node, which is blank in this case.  But that doesn't happen in Gecko mode...
I kept chasing this a bit out of curiosity...

It seems to relate to the DevTools function `allRulesHaveSource`[1], which itself calls `DOMUtils.getRelativeRuleLine`.

In Gecko mode, `DOMUtils.getRelativeRuleLine` returns 0 for a rule added via CSSOM like this one.
In Stylo mode, `DOMUtils.getRelativeRuleLine` returns an actual line number (it varies depending on how many \n you add).

It's possible we may want to accept having line numbers in Stylo mode here, and instead re-work the DevTools code...?

:tromey, looks like you added `allRulesHaveSource` a while back.  What's the real purpose of this check?  Does it want to check "from CSSOM or not" and line numbers were a way to achieve that back then?

[1]: http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/devtools/server/actors/stylesheets.js#224
Flags: needinfo?(ttromey)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)

> It's possible we may want to accept having line numbers in Stylo mode here,
> and instead re-work the DevTools code...?

Perhaps some cases can be made to work, but unless all uses of CSSOM in Stylo
cause edits to the style sheet text, there will still be cases that can't work.

> :tromey, looks like you added `allRulesHaveSource` a while back.  What's the
> real purpose of this check?  Does it want to check "from CSSOM or not" and
> line numbers were a way to achieve that back then?

Yes, that was the idea.  See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1224121#c8
Flags: needinfo?(ttromey)
Not sure this is something I can move forward. Taking myself off for now.
Assignee: bwerth → nobody
It seems like storing a flag on the sheet that is set to true when any of its rules have been modified by CSSOM might be a nice way forward here.

There is already `mDirty` on the sheet, but that is set true any time `EnsureUniqueInner()` is called, which includes read access to the rules (both by CSSOM and the internal inspector APIs).

Brad, do you think that is something you could investigate?
Flags: needinfo?(bwerth)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Nov 22 - Dec 3) from comment #6)
> It seems like storing a flag on the sheet that is set to true when any of
> its rules have been modified by CSSOM might be a nice way forward here.

After some thought, I think it's more sensible to have Stylo match the Gecko behavior, and assign a line number of "0" to any added rules. Here's why:

1) Any added rules aren't really at the source location that a real line number would suggest. It's unclear what value it provides to report that number in logging messages, etc.
2) Maintaining consistency in line numbers requires the rules after an added/removed rule to be adjusted, making THOSE rules no longer match the actual source location.
3) The line number properties seems to primarily be used for logging messages, and for this "is anything from CSSOM?" check. There doesn't appear to be any usage that requires comparison between line numbers of different rules to determine which comes first, etc. I can't find anything other than an annotation of source location, which is deceptive as already noted.

So unless I hear about a pain point that this will create, I'm going to purse the "always 0" approach.
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #7)

> So unless I hear about a pain point that this will create, I'm going to
> purse the "always 0" approach.

From the devtools side that will certainly work, as it's what gecko historically did.

However, the whole bit about checking for 0 was always just a workaround.
See comment #6 here, and also bug 1412319 -- some kind of "was this ever touched
by cssom" bit would be more efficient and just as good for devtools.

Also see the full CSSOM-handling plan in bug 1196250.
(In reply to Tom Tromey :tromey from comment #8)
> However, the whole bit about checking for 0 was always just a workaround.
> See comment #6 here, and also bug 1412319 -- some kind of "was this ever
> touched
> by cssom" bit would be more efficient and just as good for devtools.

Got it. I'll take this approach then. I will need to find a way to do this that avoids allocating extra memory for every stylesheet.
Assignee: nobody → bwerth
(In reply to Brad Werth [:bradwerth] from comment #9)
> (In reply to Tom Tromey :tromey from comment #8)
> > However, the whole bit about checking for 0 was always just a workaround.
> > See comment #6 here, and also bug 1412319 -- some kind of "was this ever
> > touched
> > by cssom" bit would be more efficient and just as good for devtools.
> 
> Got it. I'll take this approach then. I will need to find a way to do this
> that avoids allocating extra memory for every stylesheet.

Having a more efficient way to know if a sheet was touched by cssom would help fix the performance issues mentioned in Bug 1412319. Is the plan to do this for Stylo only?
(In reply to Julian Descottes [:jdescottes][:julian] from comment #10)
> Having a more efficient way to know if a sheet was touched by cssom would
> help fix the performance issues mentioned in Bug 1412319. Is the plan to do
> this for Stylo only?

My plan now is to do this for gecko and servo stylesheets both. I will post my in-progress patches (all infrastructure, no change to behavior and no tests) so you can see the approach. I am replacing the mDirty bool in StyleSheet.h with a bitfield so we can hold more "types" of dirtiness, including whether or not the sheet has had rules added by CSSOM.
Comment on attachment 8937038 [details]
Bug 1415940 Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools.

https://reviewboard.mozilla.org/r/207770/#review213638

I looked at this a little and made some drive-by comments.  Thanks for doing this, it is looking really great.

::: layout/inspector/inDOMUtils.cpp:428
(Diff revision 1)
> +      return NS_ERROR_INVALID_POINTER;
> +    }
> +  }
> +
> +  MOZ_ASSERT(sheet, "Should have a StyleSheet pointer by now.");
> +  *_retval = sheet->HasRulesFromCSSOM();

I don't know all that much about the css code; but I think the devtools code wants this to return false for basically any CSSOM change to a style sheet.  That's because the devtools uses its check to decide whether to do as-authored editing, and due to how as-authored editing works (making textual edits and then re-evaluating the style sheet in-place), any change via CSSOM should cause devtools to revert to "non as-authored" (not sure we have a name for that).

So, changes inside a rule, rule additions, or rule deletions should all cause this; i.e., should all return true here.
Comment on attachment 8937036 [details]
Bug 1415940 Part 7: Change canSetRuleText() to test the sheet for modification by CSSOM.

https://reviewboard.mozilla.org/r/207766/#review213632

::: layout/style/StyleSheet.cpp:451
(Diff revision 1)
>  void
>  StyleSheet::EnsureUniqueInner()
>  {
>    MOZ_ASSERT(mInner->mSheets.Length() != 0,
>               "unexpected number of outers");
> -  mDirty = true;
> +  mDirtyFlags &= INNER_MODIFIED;

I think this ought to read `|=`.
Comment on attachment 8937037 [details]
Bug 1415940 Part 2: Give StyleSheet a new dirty flag for indicating a rule was added.

https://reviewboard.mozilla.org/r/207768/#review213634

::: layout/style/StyleSheet.cpp:630
(Diff revision 1)
>  
>  void
>  StyleSheet::RuleAdded(css::Rule& aRule)
>  {
>    DidDirty();
> +  mDirtyFlags &= RULE_ADDED;

Here too.
(In reply to Tom Tromey :tromey from comment #15)

> I don't know all that much about the css code; but I think the devtools code
> wants this to return false for basically any CSSOM change to a style sheet. 
...
> So, changes inside a rule, rule additions, or rule deletions should all
> cause this; i.e., should all return true here.

I mixed up true and false here, but hopefully the idea comes through: basically
any CSSOM change should be reported to devtools.
(In reply to Tom Tromey :tromey from comment #18)
> (In reply to Tom Tromey :tromey from comment #15)
> 
> > I don't know all that much about the css code; but I think the devtools code
> > wants this to return false for basically any CSSOM change to a style sheet. 
> ...
> > So, changes inside a rule, rule additions, or rule deletions should all
> > cause this; i.e., should all return true here.
> 
> I mixed up true and false here, but hopefully the idea comes through:
> basically
> any CSSOM change should be reported to devtools.

Then there shouldn't be a need for the separate flag, which is great :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #19)
> (In reply to Tom Tromey :tromey from comment #18)
> > (In reply to Tom Tromey :tromey from comment #15)
> > I mixed up true and false here, but hopefully the idea comes through:
> > basically
> > any CSSOM change should be reported to devtools.
> 
> Then there shouldn't be a need for the separate flag, which is great :)

Agreed. If this is what's needed, then the patch will become much simpler and will mostly be a new method inDOMUtils::isModifiedByCSSOM which will return the state of the existing mDirty boolean.
Attachment #8937037 - Attachment is obsolete: true
(In reply to Brad Werth [:bradwerth] from comment #20)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #19)
> > (In reply to Tom Tromey :tromey from comment #18)
> > > (In reply to Tom Tromey :tromey from comment #15)
> > > I mixed up true and false here, but hopefully the idea comes through:
> > > basically
> > > any CSSOM change should be reported to devtools.
> > 
> > Then there shouldn't be a need for the separate flag, which is great :)
> 
> Agreed. If this is what's needed, then the patch will become much simpler
> and will mostly be a new method inDOMUtils::isModifiedByCSSOM which will
> return the state of the existing mDirty boolean.

New thinking: since the existing dirty flag is set AS SOON AS the rules are exposed (i.e. StyleSheet::GetCSSRules leads to StyleSheet::EnsureUniqueInner, which sets mDirty), then we can't use that flag to mean "rules have changed." I'll need to use some version of the original proposed patch with the bitfield replacement of mDirty. Patience.
Attachment #8939994 - Flags: review?(bzbarsky)
Attachment #8939995 - Flags: review?(bzbarsky)
Attachment #8937038 - Flags: review?(ttromey)
Attachment #8937036 - Flags: review?(ttromey)
Attachment #8939996 - Flags: review?(ttromey)
Comment on attachment 8939994 [details]
Bug 1415940 Part 1: Expand StyleSheet dirty flag into a bitfield, to allow more types of dirtiness.

https://reviewboard.mozilla.org/r/210266/#review216076

::: layout/style/StyleSheet.h:139
(Diff revision 1)
>    virtual already_AddRefed<StyleSheet> Clone(StyleSheet* aCloneParent,
>                                               dom::CSSImportRule* aCloneOwnerRule,
>                                               nsIDocument* aCloneDocument,
>                                               nsINode* aCloneOwningNode) const = 0;
>  
> -  bool IsModified() const { return mDirty; }
> +  bool IsModified() const { return mDirtyFlags & INNER_MADE_UNIQUE; }

Hmm.  This IsModified() method is slightly bonkers, since that's not what mDirty means.  But that predates your patch.

However that _does_ mean that using IsModified() in the various tests where mDirty was tested is wrong.  Those locations don't care whether the sheet was modified.  They care whether there's a forced unique inner.  I'd be happy to rename from "dirty" to some other name for this concept, and update the assertion messages, etc, but "modified" is not the right thing.
Attachment #8939994 - Flags: review?(bzbarsky) → review-
Comment on attachment 8939995 [details]
Bug 1415940 Part 2: Give StyleSheet a dirty flag of MODIFIED_RULES, and a method to test it.

https://reviewboard.mozilla.org/r/210268/#review216078
Attachment #8939995 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939996 [details]
Bug 1415940 Part 8: Add a browser test to confirm DOM added rules work with leading newlines.

https://reviewboard.mozilla.org/r/210270/#review216218

Thank you.  This looks good to me.
Attachment #8939996 - Flags: review?(ttromey) → review+
Comment on attachment 8937038 [details]
Bug 1415940 Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools.

https://reviewboard.mozilla.org/r/207770/#review216222

::: layout/inspector/inDOMUtils.cpp:418
(Diff revision 3)
> +    RefPtr<CSSStyleSheet> geckoSheet = do_QueryObject(aSheet);
> +    if (geckoSheet) {
> +      sheet = geckoSheet;

It seems to me that the refptr will go out of scope, but this code expects "sheet" to still be valid.  While it might be safe in this case (I don't know), it would be more obviously safe to just hoist the refptr declaration out of the "if".
Comment on attachment 8937038 [details]
Bug 1415940 Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools.

https://reviewboard.mozilla.org/r/207770/#review216236

One issue with adding stuff to domutils is that it's being removed in bug 1427419.  Please coordinate with heycam.

On the bright side, in webidl CSSStyleSheet will just map to StyleSheet directly and you won't need the QI games.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #37)
> Comment on attachment 8937038 [details]
> Bug 1415940 Part 3: Create an inDOMUtils::HasRulesModifiedByCSSOM for use by
> devtools.
> 
> https://reviewboard.mozilla.org/r/207770/#review216236
> 
> One issue with adding stuff to domutils is that it's being removed in bug
> 1427419.  Please coordinate with heycam.

Thank you for notifying me of this. I think the simplest solution is for this bug to depend on bug 1427419, which seems like it'll land soon.
Depends on: 1427419
Comment on attachment 8939994 [details]
Bug 1415940 Part 1: Expand StyleSheet dirty flag into a bitfield, to allow more types of dirtiness.

https://reviewboard.mozilla.org/r/210266/#review216076

> Hmm.  This IsModified() method is slightly bonkers, since that's not what mDirty means.  But that predates your patch.
> 
> However that _does_ mean that using IsModified() in the various tests where mDirty was tested is wrong.  Those locations don't care whether the sheet was modified.  They care whether there's a forced unique inner.  I'd be happy to rename from "dirty" to some other name for this concept, and update the assertion messages, etc, but "modified" is not the right thing.

I agree. I'll change the method to HasForcedUniqueInner() and rename the flag value to match that more closely.
Comment on attachment 8939994 [details]
Bug 1415940 Part 1: Expand StyleSheet dirty flag into a bitfield, to allow more types of dirtiness.

https://reviewboard.mozilla.org/r/210266/#review216368

::: dom/base/nsTreeSanitizer.cpp
(Diff revision 2)
>      rv = parser.ParseSheet(aOriginal, aDocument->GetDocumentURI(), aBaseURI,
>                             aDocument->NodePrincipal(), 0);
>    }
>    NS_ENSURE_SUCCESS(rv, true);
>    // Mark the sheet as complete.
> -  MOZ_ASSERT(!sheet->IsModified(),

Why are we just removing this assertion?  This should check for forced unique inners instead.

::: layout/style/Loader.cpp:1011
(Diff revision 2)
>  
> -      // Make sure it hasn't been modified; if it has, we can't use it
> -      if (sheet->IsModified()) {
> +      // Make sure it hasn't been forced to have a unique inner;
> +      // that is an indication that its rules have been exposed to
> +      // CSSOM and so we can't use it.
> +      if (sheet->HasForcedUniqueInner()) {
>          LOG(("  Not cloning completed sheet %p because it's been modified",

Fix the log message?

::: layout/style/Loader.cpp:1062
(Diff revision 2)
>          }
>        }
>      }
>  
>      if (sheet) {
>        // The sheet we have now should be either incomplete or unmodified

Comment doesn't match the code.
Attachment #8939994 - Flags: review?(bzbarsky) → review+
Comment on attachment 8937036 [details]
Bug 1415940 Part 7: Change canSetRuleText() to test the sheet for modification by CSSOM.

https://reviewboard.mozilla.org/r/207766/#review216376

Thanks.  This is much nicer than what's currently in the tree :-)
Attachment #8937036 - Flags: review?(ttromey) → review+
Comment on attachment 8937038 [details]
Bug 1415940 Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools.

https://reviewboard.mozilla.org/r/207770/#review216382

This looks good to me, but I think all changes to webidl require a DOM peer's approval as well.
Attachment #8937038 - Flags: review?(ttromey) → review+
I think this approach needs a further thinking-through.(In reply to Tom Tromey :tromey from comment #18)
> I mixed up true and false here, but hopefully the idea comes through:
> basically
> any CSSOM change should be reported to devtools.

I'm sorry to say that this bug needs more thinking-through before I can land it. The patch creates failures in tests that reveal (I think) fundamental problems with the approach. devtools/client/styleeditor/test/browser_styleeditor_sync.js fails because it uses the Rule View to disable two rules, in succession. The first one succeeds but sets the flag that the sheet has been modified, so the second one fails.

One possible solution to this problem would be add another dirty bit. We would keep MODIFIED_RULES and add a new bit for ADDED_OR_REMOVED_RULES and canSetRuleText would check this new bit instead of the more general MODIFIED_RULES bit. Is this good? It feels like this solution would still fail in this use case:

1) Add a new rule to an existing selector (this would set the ADDED_OR_REMOVED_RULES).
2) Disable the newly added rule (would fail on the canSetRuleText check).

I worry that we're overarchitecting a solution for this, and the solution doesn't even work. What was the original bad behavior we were avoiding with canSetRuleText failing for sheets with added rules? Can we fix this bug with the approach I proposed in comment 7 (added rules get source line 0)? Should we transform this proposed patch with extra dirty bits into patches for bug 1412319 and/or bug 1196250?
Flags: needinfo?(ttromey)
(In reply to Brad Werth [:bradwerth] from comment #48)

> I'm sorry to say that this bug needs more thinking-through before I can land
> it. The patch creates failures in tests that reveal (I think) fundamental
> problems with the approach.
> devtools/client/styleeditor/test/browser_styleeditor_sync.js fails because
> it uses the Rule View to disable two rules, in succession. The first one
> succeeds but sets the flag that the sheet has been modified, so the second
> one fails.
[...]
> I worry that we're overarchitecting a solution for this, and the solution
> doesn't even work. What was the original bad behavior we were avoiding with
> canSetRuleText failing for sheets with added rules? Can we fix this bug with
> the approach I proposed in comment 7 (added rules get source line 0)? Should
> we transform this proposed patch with extra dirty bits into patches for bug
> 1412319 and/or bug 1196250?


The way I believe this should work -- you may want to double-check it -- is that
there are fundamentally two cases for style editing, the "as-authored" case and the
other (nameless) case.

The as-authored case happens when the style comes from a style sheet.  Changes to
the rule are done by applying edits to the stylesheet and then re-evaluating the
entire sheet in place, using a special inIDOMUtils method.  This happens here:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/stylesheets.js#543

The nameless case used to be used by element styles (used to be -- I think maybe
these use the authored approach now); and also when CSSOM was used.  In the CSSOM
case, as-authored would not work because there has already been divergence between
the source text and the actual rules as applied.  That is, re-evaluating the edited text
would cause other changes unrelated to the edit at hand -- namely, reverting things
done by CSSOM.

I think what should happen for the test case in question is that DOMUtils.parseStyleSheet should
clear the "CSSOM modified" bit, leaving devtools happy.  It sounds like this isn't happening?

I think any approach that preserves the desired properties is fine, though I suppose
we were excited that this patch would be faster than the status quo, so it would be
nice to preserve that if possible.  I think fixing bug 1196250 would be super but it
is somewhat more involved; basically the idea there is to be able to detect CSSOM changes
and edit them back into the original text, removing the limitation that leads to the
"nameless" case.

Please don't hesitate to ping me again if this didn't answer your questions.
Flags: needinfo?(ttromey)
Comment on attachment 8939994 [details]
Bug 1415940 Part 1: Expand StyleSheet dirty flag into a bitfield, to allow more types of dirtiness.

https://reviewboard.mozilla.org/r/210266/#review216368

> Why are we just removing this assertion?  This should check for forced unique inners instead.

I have restored the assert. Initially, I had removed it because it is duplicated in the implementation of StyleSheet::SetComplete. It felt redundant. But if it has semantic meaning here, then I agree it should stay.
Getting closer. The browser_styleeditor_sync.js test is still failing. The problem now is that after the first call to StyleSheet::ReparseSheet, the ServoCSSRuleDeclaration after the changed rule loses its stylesheet pointer, which makes the second rule modification fail. This failure was a NULL dereference crash, but I changed it to a silent failure in attachment 8942402 [details]. I'll see if I can figure out is going on.
(In reply to Brad Werth [:bradwerth] from comment #58)
> Getting closer. The browser_styleeditor_sync.js test is still failing. The
> problem now is that after the first call to StyleSheet::ReparseSheet, the
> ServoCSSRuleDeclaration after the changed rule loses its stylesheet pointer,
> which makes the second rule modification fail. This failure was a NULL
> dereference crash, but I changed it to a silent failure in attachment
> 8942402 [details]. I'll see if I can figure out is going on.

The problem was I was clearing the new MODIFIED_RULES flag too early in the ::ReparseSheet() method. It has to be done at the end, because each of the new rules calls RuleAdded, which re-dirties the flags. An updated version of the patch fixes this and locally passes the browser_styleeditor_sync.js test.
Attachment #8942401 - Flags: review?(bzbarsky)
Attachment #8942402 - Flags: review?(bzbarsky)
Attachment #8943441 - Flags: review?(bzbarsky)
Comment on attachment 8942401 [details]
Bug 1415940 Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed.

https://reviewboard.mozilla.org/r/212704/#review219584

I don't think this is right, and we should be able to add a test accordingly, like so:

1. Have a page load a sheet from some URL.
2. ReparseSheet that sheet.
3. Have a page load another sheet from the same URL.
4. Remove the first sheet from the DOM.

I would expect the CSS loader to clone the reparsed sheet in step 3, which is not correct (see the IsModified() checks in css::Loader::CreateSheet).
Attachment #8942401 - Flags: review?(bzbarsky) → review-
Comment on attachment 8942402 [details]
Bug 1415940 Part 4: Generalize a StyleSheet cast in ServoStyleRuleDeclaration::SetCSSDeclaration to prevent a NULL dereference from AsServo().

https://reviewboard.mozilla.org/r/212706/#review219586
Attachment #8942402 - Flags: review?(bzbarsky) → review+
Comment on attachment 8943441 [details]
Bug 1415940 Part 5: Change InspectorUtils::GetRelativeRuleLine to not remap line numbers if it introduces underflow.

https://reviewboard.mozilla.org/r/213772/#review219588
Attachment #8943441 - Flags: review?(bzbarsky) → review+
Comment on attachment 8942401 [details]
Bug 1415940 Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed.

https://reviewboard.mozilla.org/r/212704/#review219584

I believe I've seen a test in the tree that does this, though it wouldn't have been included on my try runs for this bug. I'll find the test and confirm it still works, or create a test if I'm mistaken and we aren't already testing this.

But, I don't follow the argument that this patch would break this behavior. The added ClearModifiedRules method only clears the MODIFIED_RULES bit, and leaves the FORCED_UNIQUE_INNER bit alone. css::Loader::CreateSheet is checking HasForcedUniqueInner().
Comment on attachment 8942401 [details]
Bug 1415940 Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed.

https://reviewboard.mozilla.org/r/212704/#review219584

I was thinking of /layout/inspector/tests/test_parseStyleSheetImport.html which doesn't do quite what you proposed. I'll create a new test.
Comment on attachment 8942401 [details]
Bug 1415940 Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed.

https://reviewboard.mozilla.org/r/212704/#review219792
Attachment #8942401 - Flags: review- → review+
Comment on attachment 8942401 [details]
Bug 1415940 Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed.

https://reviewboard.mozilla.org/r/212704/#review219584

Ah, I missed the difference in the two flags.  Given that, looks good.
(In reply to Brad Werth [:bradwerth] from comment #81)
> I was thinking of /layout/inspector/tests/test_parseStyleSheetImport.html
> which doesn't do quite what you proposed. I'll create a new test.

I've convinced myself that this test actually DOES execute the logic in comment 77. I have some tests to fix, but I won't be adding a new test for this logic.
Comment on attachment 8937038 [details]
Bug 1415940 Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools.

https://reviewboard.mozilla.org/r/207770/#review220064
Attachment #8937038 - Flags: review?(bzbarsky) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/307642e5b5f4
Part 1: Expand StyleSheet dirty flag into a bitfield, to allow more types of dirtiness. r=bz
https://hg.mozilla.org/integration/autoland/rev/f76d80e4c6fb
Part 2: Give StyleSheet a dirty flag of MODIFIED_RULES, and a method to test it. r=bz
https://hg.mozilla.org/integration/autoland/rev/e199a8c03768
Part 3: Make StyleSheets clear their MODIFIED_RULES flag when the sheet is reparsed. r=bz
https://hg.mozilla.org/integration/autoland/rev/544075fc4800
Part 4: Generalize a StyleSheet cast in ServoStyleRuleDeclaration::SetCSSDeclaration to prevent a NULL dereference from AsServo(). r=bz
https://hg.mozilla.org/integration/autoland/rev/1d6ebcc1be8f
Part 5: Change InspectorUtils::GetRelativeRuleLine to not remap line numbers if it introduces underflow. r=bz
https://hg.mozilla.org/integration/autoland/rev/fc525a00ff0c
Part 6: Create an InspectorUtils::HasRulesModifiedByCSSOM for use by devtools. r=bz,tromey
https://hg.mozilla.org/integration/autoland/rev/626ed9760dd1
Part 7: Change canSetRuleText() to test the sheet for modification by CSSOM. r=tromey
https://hg.mozilla.org/integration/autoland/rev/9cef0d0769af
Part 8: Add a browser test to confirm DOM added rules work with leading newlines. r=tromey
Depends on: 1432416
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: