839103 - Provide notifications for style sheet added and removed to chrome JS

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Heather Arthur [:harth]]

For the devtools' Style Editor tool, we need to be notified when a stylesheet is added to the page or removed from the page.

Actually if it's easy to add some other of the events in nsIDocumentObserver (http://dxr.mozilla.org/mozilla-central/content/base/public/nsIDocumentObserver.h.html) that would be grand as well. Would want: StyleSheetAdded, StyleSheetRemoved, StyleRuleAdded, StyleRuleRemoved, StyleRuleChanged, and StyleSheetApplicableStateChanged. But StyleSheetAdded and StyleSheetRemoved are the most important.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that devtools doesn't need to be notified sync, so the concerns about "can't call JS now" are not that relevant: we can just notify off an event.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: add chrome-only JS events for style sheet/rule changes

I think this does the trick; it just matters that we know what document
the event happens on; we don't need to know about individual sheets or rules,
right?

Comment 3

[Boris Zbarsky [:bzbarsky]]

If we're not sending any information about _what_ changed, should we just coalesce the events?

That is, if someone inserts 50 rules, seems like we shouldn't send 50 "hey, rule inserted, you figure out which" events...

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> If we're not sending any information about _what_ changed, should we just
> coalesce the events?
> 
> That is, if someone inserts 50 rules, seems like we shouldn't send 50 "hey,
> rule inserted, you figure out which" events...

I dunno, it doesn't seem that friendly to say that a rule is added or removed without saying *which* rule, either.

Heather, what would be best for devtools?

a) One event per addition/removal/change, no information about the sheet/rule;
b) Just like a) but with information about the sheet/rule;
c) Coalesced events on a per-event basis.  So if two stylesheets were added and one was removed, you'd get a StyleSheetAdded and a StyleSheetRemoved event.  No information about sheets/rules; or
d) Something else.

Comment 5

[Heather Arthur [:harth]]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Heather, what would be best for devtools?
> 
> a) One event per addition/removal/change, no information about the
> sheet/rule;
> b) Just like a) but with information about the sheet/rule;
> c) Coalesced events on a per-event basis.  So if two stylesheets were added
> and one was removed, you'd get a StyleSheetAdded and a StyleSheetRemoved
> event.  No information about sheets/rules; or
> d) Something else.

b) would be best.

What would this look like in JS?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 725081 [details] [diff] [review]
add chrome-only JS events for style sheet/rule changes

Review of attachment 725081 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like we want a richer event to be sent, which will require some more work.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Heather Arthur [:harth] from comment #5)
> (In reply to Nathan Froyd (:froydnj) from comment #4)
> > a) One event per addition/removal/change, no information about the
> > sheet/rule;
> > b) Just like a) but with information about the sheet/rule;
> > c) Coalesced events on a per-event basis.  So if two stylesheets were added
> > and one was removed, you'd get a StyleSheetAdded and a StyleSheetRemoved
> > event.  No information about sheets/rules; or
> > d) Something else.
> 
> b) would be best.
> 
> What would this look like in JS?

For instance, a (Moz)StyleSheetAdded event:

- .target is the document
- .stylesheet is the stylesheet that was added
- .isDocumentSheet is whether .stylesheet is a document sheet or not

The general idea is that the event will have properties corresponding to the arguments passed to the events in nsIDocumentObserver.h.

bz, does that sound sane?  Do we want to keep these events chrome-only?

Comment 8

[Boris Zbarsky [:bzbarsky]]

That sounds sane in general, yes.  The events should probably be chrome-only for now, indeed.

We should also consider optimizing away dispatching them the same way we do mutation events...

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - factor out StyleSheet{Added,Removed} notifications into separate nsDocument methods

OK, attempt #2 at this.  This series is just for StyleSheet{Added,Removed};
I haven't thought about how to make the StyleRule bits efficient.

This part is a basic refactoring so we have a common place to dispatch the
DOM events.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 2 - enable chrome dispatching of nsIDOMEvents in nsAsyncDOMEvent

Since we need to dispatch events async to chrome only--events that have
extra JS-visible parts, nsAsyncDOMEvent needs to be taught how to do so.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 3 - send StyleSheet{Added,Removed} chrome notifications when stylesheets are added/removed

This part adds the definitions for the events themselves and the bits to
send events when appropriate.  test_interfaces.html changes are so that
test will actually pass.  Is there a better way to do this...?

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add test for StyleSheet{Added,Removed} events

...and tests.  Not entirely sure if we can add UA stylesheets easily,
so testing for .documentSheet == false is punted on.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> This part adds the definitions for the events themselves and the bits to
> send events when appropriate.

Comments/bikeshedding welcome on the names of the attributes themselves...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730296 [details] [diff] [review]
part 1 - factor out StyleSheet{Added,Removed} notifications into separate nsDocument methods

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730298 [details] [diff] [review]
part 2 - enable chrome dispatching of nsIDOMEvents in nsAsyncDOMEvent

>+      mEvent->GetIsTrusted(&isTrusted);

  MOZ_ASSERT(mEvent->InternalDOMEvent()->IsTrusted());

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730300 [details] [diff] [review]
part 3 - send StyleSheet{Added,Removed} chrome notifications when stylesheets are added/removed

This seems to be missing the implementation files for the new event objects.

Is there a reason these objects aren't just using WebIDL bindings?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Not entirely sure if we can add UA stylesheets easily

We can (see nsIStyleSheetService), but those would't show up in this code anyway, I would think.

The obvious documentSheet == false case here would be someone loading a sheet via nsIDOMWindowUtils.loadSheet if you want to add a test for that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730302 [details] [diff] [review]
part 4 - add test for StyleSheet{Added,Removed} events

HTML_NS is unused.

>+  gBrowser.contentWindow.setTimeout(continueTest, 0);
>+  gBrowser.contentWindow.setTimeout(concludeTest, 0);

Can we not do executeSoon in this stuff?  I'd hope we can...

>+function concludeTest() {

Please set gLinkElement to null in here.

r=me with the nits.

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #16)
> This seems to be missing the implementation files for the new event objects.

Those would be nsIDOMStyleSheet{Added,Removed}.idl; the event generation stuff (event_impl_conf.gen.in) takes care of everything we need for them.

> Is there a reason these objects aren't just using WebIDL bindings?

(a) laziness, 'cause the event generation stuff takes care of everything; and (b) ignorance/laziness on writing new WebIDL bits.

But I'm happy to forego the event generation stuff and/or think harder about how to convert the event generation stuff to WebIDL if you'd prefer that.

(In reply to Boris Zbarsky (:bz) from comment #18)
> HTML_NS is unused.

Gah, that's what I get for starting from another file.  Will remove.

> >+  gBrowser.contentWindow.setTimeout(continueTest, 0);
> >+  gBrowser.contentWindow.setTimeout(concludeTest, 0);
> 
> Can we not do executeSoon in this stuff?  I'd hope we can...

I think we can.  I was trying to be conscious of bug 715376, but I suppose I can just fix this test when that bug lands.  Will convert.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> the event generation stuff (event_impl_conf.gen.in) takes care of everything we need for
> them.

Oh, nice.  Where are we on that with WebIDL events, Olli?  What would you prefer Nathan does here?

> I was trying to be conscious of bug 715376,

Hmm...  But does it matter which event queue that event goes in, really?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #20)
> > the event generation stuff (event_impl_conf.gen.in) takes care of everything we need for
> > them.
> 
> Oh, nice.  Where are we on that with WebIDL events, Olli?

generated events use still xpidl interfaces for few days (I hope only few days).
I'll convert everything to use webidl at once.

  What would you
> prefer Nathan does here?
Use event code generator as it is now.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 730300 [details] [diff] [review]
part 3 - send StyleSheet{Added,Removed} chrome notifications when stylesheets are added/removed

Review of attachment 730300 [details] [diff] [review]:
-----------------------------------------------------------------

Resetting review for bz based on smaug's comments.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730300 [details] [diff] [review]
part 3 - send StyleSheet{Added,Removed} chrome notifications when stylesheets are added/removed

r=me, though the fact that this is exposing these on the global makes me really sad.  :(

Comment 24

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #23)
> r=me, though the fact that this is exposing these on the global makes me
> really sad.  :(

WebIDL fixes that issue, right?

Comment 25

[Boris Zbarsky [:bzbarsky]]

It can, if the WebIDL event interfaces are marked [NoInterfaceObject], yes.

Comment 26

[Nathan Froyd [:froydnj]]

Pushed with requested changes:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ad493f2d4f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7910dcaf0574
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4274e7cde9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e53576c1bbc6

Marking as [leave open] for adding the style rule and StyleSheetApplicableState notifications.

Comment 27

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out because of a build error on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ed59633c05

https://tbpl.mozilla.org/php/getParsedLog.php?id=21347202&tree=Mozilla-Inbound#error0

Comment 28

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

The build errors were in debug builds. Opt builds also had mochitest/crashtest/reftest crashes too.

https://tbpl.mozilla.org/php/getParsedLog.php?id=21348227&tree=Mozilla-Inbound

08:39:36     INFO -  7956 INFO TEST-START | /tests/content/base/test/test_bug330925.xhtml
08:39:37  WARNING -  TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug330925.xhtml | Exited with code 11 during test run
08:39:37     INFO -  INFO | automation.py | Application ran for: 0:01:19.538922
08:39:37     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpZYjtZspidlog
08:39:37     INFO -  ==> process 2130 launched child process 2178
08:39:37     INFO -  INFO | zombiecheck | Checking for orphan process with PID: 2178
08:39:37     INFO -  mozcrash INFO | Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux/1364914072/firefox-23.0a1.en-US.linux-i686.crashreporter-symbols.zip
08:39:37     INFO -  Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux/1364914072/firefox-23.0a1.en-US.linux-i686.crashreporter-symbols.zip
08:40:06  WARNING -  PROCESS-CRASH | /tests/content/base/test/test_bug330925.xhtml | application crashed [@ nsDocument::NotifyStyleSheetAdded(nsIStyleSheet*, bool)]
08:40:06     INFO -  Crash dump filename: /tmp/tmpBQJCu3/minidumps/3d287571-1956-e888-6697c1c2-03c20fe7.dmp
08:40:06     INFO -  Operating system: Linux
08:40:06     INFO -                    0.0.0 Linux 3.2.0-23-generic-pae #36-Ubuntu SMP Tue Apr 10 22:19:09 UTC 2012 i686
08:40:06     INFO -  CPU: x86
08:40:06     INFO -       GenuineIntel family 6 model 45 stepping 7
08:40:06     INFO -       1 CPU
08:40:06     INFO -  Crash reason:  SIGSEGV
08:40:06     INFO -  Crash address: 0x8
08:40:06     INFO -  Thread 0 (crashed)
08:40:06     INFO -   0  libxul.so!nsDocument::NotifyStyleSheetAdded(nsIStyleSheet*, bool) [nsDocument.cpp:b0e27a5ae2b6 : 11368 + 0x0]
08:40:06     INFO -      eip = 0xb568c23e   esp = 0xbfe72eb0   ebp = 0xbfe72f28   ebx = 0xb6ea0130
08:40:06     INFO -      esi = 0xa1c70000   edi = 0xbfe72ef4   eax = 0x00000000   ecx = 0xa301504c
08:40:06     INFO -      edx = 0xa1c701cc   efl = 0x00010286
08:40:06     INFO -      Found by: given as instruction pointer in context
08:40:06     INFO -   1  libxul.so!mozilla::css::Loader::InsertSheetInDoc(nsCSSStyleSheet*, nsIContent*, nsIDocument*) [Loader.cpp:b0e27a5ae2b6 : 1311 + 0xf]
08:40:06     INFO -      eip = 0xb5586b6e   esp = 0xbfe72f30   ebp = 0xbfe72f88
08:40:06     INFO -      Found by: previous frame's frame pointer
08:40:06     INFO -   2  libxul.so!mozilla::css::Loader::LoadStyleLink(nsIContent*, nsIURI*, nsAString_internal const&, nsAString_internal const&, bool, mozilla::CORSMode, nsICSSLoaderObserver*, bool*) [Loader.cpp:b0e27a5ae2b6 : 1903 + 0x11]
08:40:06     INFO -      eip = 0xb5589bb6   esp = 0xbfe72f90   ebp = 0xbfe73008
08:40:06     INFO -      Found by: previous frame's frame pointer

Comment 29

[Nathan Froyd [:froydnj]]

Trying again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8f05e03872
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbef35abf2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c602d8475a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e889e95154

Comment 30

[Nathan Froyd [:froydnj]]

Turns out this regresses Dromaeo a couple percent (Win 64, but wouldn't be totally surprised if I started getting emails for more platforms):

http://mzl.la/12hVmgN

Is it worth hiding this behind a pref?  Nothing suitable jumps out from about:config, so I guess we'd have to invent a new one.  Heather, is that workable for devtools?  bz?

Comment 31

[Nathan Froyd [:froydnj]]

bz, see comment 30.

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/0d8f05e03872
https://hg.mozilla.org/mozilla-central/rev/7dbef35abf2a
https://hg.mozilla.org/mozilla-central/rev/90c602d8475a
https://hg.mozilla.org/mozilla-central/rev/c0e889e95154

Comment 33

[Boris Zbarsky [:bzbarsky]]

Have you looked at the per-test breakdown?  Are there particular tests that regressed a lot, or was it more of an across-the-board thing?  Because I wouldn't have expected this to necessarily regress dromaeo...

Comment 34

[Boris Zbarsky [:bzbarsky]]

Also, it's possible this regressed Tp5.  Not clear so far.

Comment 35

[Heather Arthur [:harth]]

(In reply to Nathan Froyd (:froydnj) from comment #30)
> Is it worth hiding this behind a pref?  Nothing suitable jumps out from
> about:config, so I guess we'd have to invent a new one.  Heather, is that
> workable for devtools?  bz?

So you wouldn't receive these events unless you flipped a pref in about:config? Does it take effect immediately? Then we could just turn it on when the devtools window is open.

Comment 36

[Nathan Froyd [:froydnj]]

(In reply to Heather Arthur [:harth] from comment #35)
> (In reply to Nathan Froyd (:froydnj) from comment #30)
> > Is it worth hiding this behind a pref?  Nothing suitable jumps out from
> > about:config, so I guess we'd have to invent a new one.  Heather, is that
> > workable for devtools?  bz?
> 
> So you wouldn't receive these events unless you flipped a pref in
> about:config? Does it take effect immediately? Then we could just turn it on
> when the devtools window is open.

Yeah, the events wouldn't be sent unless the pref was flipped; pref flipping would take effect immediately.  So if the pref was flipped appropriately when the devtools window came up, we'd start sending these events.

Comment 37

[Heather Arthur [:harth]]

(In reply to Nathan Froyd (:froydnj) from comment #36)
> Yeah, the events wouldn't be sent unless the pref was flipped; pref flipping
> would take effect immediately.  So if the pref was flipped appropriately
> when the devtools window came up, we'd start sending these events.

Great, that would work for devtools then.

Comment 38

[Cameron McCormack (:heycam)]

I'm thinking that a pref is not the right mechanism to use, since it doesn't make sense for the user to be able to set it from about:config.  Boris suggested making this per-Document, and having a flag on the Document to indicate whether style sheet change event should be dispatched.  Does that work too Heather?

Comment 39

[Cameron McCormack (:heycam)]

I think we should merge nsIDOMStyleSheetAddedEvent and nsIDOMStyleSheetRemovedEvent into a single nsIDOMStyleSheet<Something>Event, since they have the same information on them.  Just use the .type to distinguish them.  (Is there a suitable <Something> to use there?  Something that means either adding or removing?  "Change" is a bit too broad...)

Comment 40

[Heather Arthur [:harth]]

(In reply to Cameron McCormack (:heycam) from comment #38)
> I'm thinking that a pref is not the right mechanism to use, since it doesn't
> make sense for the user to be able to set it from about:config.  Boris
> suggested making this per-Document, and having a flag on the Document to
> indicate whether style sheet change event should be dispatched.  Does that
> work too Heather?

Makes sense about the pref. That would work as long as we could change the flag from JS somehow.

Comment 41

[Cameron McCormack (:heycam)]

Attachment: Part 5: Add a state bit on documents to control whether style sheet change events are dispatched.

Define a flag on documents to control whether style sheet change events are dispatched.

Comment 42

[Cameron McCormack (:heycam)]

Attachment: Part 6: Only dispatch style sheet change events when enabled.

Put in a check for dispatching the StyleSheet{Add,Remove} events only if that bit is set on the document.

Comment 43

[Cameron McCormack (:heycam)]

Attachment: Part 7: Allow style sheet change event enabling to be controlled through a chrome-only attribute on Documents.

Stick a [ChromeOnly] styleSheetChangeEventsEnabled property on Document objects to control the flag.

Comment 44

[Cameron McCormack (:heycam)]

Attachment: Part 8: Use Document.styleSheetChangeEventsEnabled in test.

Enable style sheet change events in the test.

Comment 45

[Cameron McCormack (:heycam)]

Attachment: Part 9: Unify nsIDOMStyleSheet{Added,Removed}Event.

Use a single interface for StyleSheet{Addded,Removed} events, since they record the same information.

Comment 46

[Cameron McCormack (:heycam)]

Attachment: Part 10: Add StyleSheetApplicableStateChange event.

Send StyleSheetApplicableStateChange events.

Comment 47

[Cameron McCormack (:heycam)]

Attachment: Part 9: Unify nsIDOMStyleSheet{Added,Removed}Event. (v1.1)

Don't want to undo part 6...

Comment 48

[Cameron McCormack (:heycam)]

Attachment: Part 11: Add StyleRule{Added,Removed,Changed} events.

Now it turns out these style rule notifications aren't always dispatched when you might think they should be.  For example, if you do

  myRule.declaration.cssText = "{ color: green }";

then there is a StyleRuleChanged notification, but if you do a

  myRule.selectorText = "body";

then there isn't.  Also, doing

  styleElement.cssText = "...";

doesn't seem to get you any notifications.

How about we resolve any issues with notifications not being dispatched in separate bugs, and leave this one just to map the nsIDocumentObserver notifications to events.

Comment 49

[Cameron McCormack (:heycam)]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=6d06c5c5fcfa

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750073 [details] [diff] [review]
Part 5: Add a state bit on documents to control whether style sheet change events are dispatched.

Just put a bool member on nsIDocument; we have a ton of those already.  We don't have that many free nsINode bits left...

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750075 [details] [diff] [review]
Part 6: Only dispatch style sheet change events when enabled.

r=me

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750077 [details] [diff] [review]
Part 7: Allow style sheet change event enabling to be controlled through a chrome-only attribute on Documents.

Could just have inline nsIDocument methods that change that boolean member I suggested we add.

r=me with that.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750078 [details] [diff] [review]
Part 8: Use Document.styleSheetChangeEventsEnabled in test.

r=me

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750097 [details] [diff] [review]
Part 9: Unify nsIDOMStyleSheet{Added,Removed}Event. (v1.1)

There should be some evt.type checks in the tests, right?

r=me with that

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750081 [details] [diff] [review]
Part 10: Add StyleSheetApplicableStateChange event.

r=me

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750108 [details] [diff] [review]
Part 11: Add StyleRule{Added,Removed,Changed} events.

The oldRule bit makes no sense.  In particular, while nsIStyleRule objects are immutable, DOM rules are not, so newRule->GetDOMRule() == oldRule->GetDOMRule().  Just pass in newRule->GetDOMRule(), drop the oldRule stuff, and undo the changes to StyleRule::GetDOMRule and nsCSSStyleSheet::DeleteRule, please.

r=me with that.

Comment 57

[Boris Zbarsky [:bzbarsky]]

>  myRule.selectorText = "body";

The selectorText setter in Gecko is a no-op, so there better not be any events there.  ;)

>  styleElement.cssText = "...";

There is no rule in this case; just an attribute and a declaration.  So no rule to change.

Comment 58

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #57)
> >  styleElement.cssText = "...";
> 
> There is no rule in this case; just an attribute and a declaration.  So no
> rule to change.

Oops, I meant to write textContent.  I just tried this:

  <style>p { color: green; }</style>

  styleElement.textContent = "";

and I was expecting a StyleRuleRemoved notification to be dispatched, but I got StyleSheetRemoved/StyleSheetAdded, which is fine actually.

Comment 59

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #56)
> The oldRule bit makes no sense.  In particular, while nsIStyleRule objects
> are immutable, DOM rules are not, so newRule->GetDOMRule() ==
> oldRule->GetDOMRule().  Just pass in newRule->GetDOMRule(), drop the oldRule
> stuff, and undo the changes to StyleRule::GetDOMRule and
> nsCSSStyleSheet::DeleteRule, please.

Ah, OK.  It makes sense of course that the actual DOM Rule object doesn't change.

Comment 60

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #56)
> Comment on attachment 750108 [details] [diff] [review]
> Part 11: Add StyleRule{Added,Removed,Changed} events.
> 
> The oldRule bit makes no sense.  In particular, while nsIStyleRule objects
> are immutable, DOM rules are not, so newRule->GetDOMRule() ==
> oldRule->GetDOMRule().  Just pass in newRule->GetDOMRule(), drop the oldRule
> stuff, and undo the changes to StyleRule::GetDOMRule and
> nsCSSStyleSheet::DeleteRule, please.

I think I still need the StyleRule::GetDOMRule and nsCSSStyleSheet::DeleteRule bits, otherwise when I call newRule->GetDOMRule() in nsDocument::StyleRuleRemoved, I get null back.  GetDOMRule() needs to be called at some point before the rule is removed from the sheet.  WDYT?

Comment 61

[Boris Zbarsky [:bzbarsky]]

Oh, good point.  I forgot about DeleteRule.  OK, leave those bits be.

Comment 62

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/537f5deb09cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d2652d5d29
https://hg.mozilla.org/integration/mozilla-inbound/rev/26399c87e7b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/57072c988f1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b4aa5f253e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf13d8dac8d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2d807f7fb4

Comment 63

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #25)
> It can, if the WebIDL event interfaces are marked [NoInterfaceObject], yes.

Filed bug 872934 for that.

Comment 64

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/537f5deb09cd
https://hg.mozilla.org/mozilla-central/rev/04d2652d5d29
https://hg.mozilla.org/mozilla-central/rev/26399c87e7b5
https://hg.mozilla.org/mozilla-central/rev/57072c988f1f
https://hg.mozilla.org/mozilla-central/rev/e7b4aa5f253e
https://hg.mozilla.org/mozilla-central/rev/cf13d8dac8d9
https://hg.mozilla.org/mozilla-central/rev/4b2d807f7fb4