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.

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.

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?

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...

(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.

(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 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.

(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?

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...

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.

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.

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...?

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

(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 on attachment 730296 [details] [diff] [review] part 1 - factor out StyleSheet{Added,Removed} notifications into separate nsDocument methods r=me

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 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?

> 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 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.

(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.

> 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?

(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 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 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. :(

(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?

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

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.

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

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

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

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?

bz, see comment 30.

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

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...

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

(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.

(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.

(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.

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?

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...)

(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.

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

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

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

Enable style sheet change events in the test.

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

Send StyleSheetApplicableStateChange events.

Don't want to undo part 6...

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.

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

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 on attachment 750075 [details] [diff] [review] Part 6: Only dispatch style sheet change events when enabled. r=me

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 on attachment 750078 [details] [diff] [review] Part 8: Use Document.styleSheetChangeEventsEnabled in test. r=me

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 on attachment 750081 [details] [diff] [review] Part 10: Add StyleSheetApplicableStateChange event. r=me

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.

> 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.

(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.

(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.

(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?

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

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

(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.

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