Closed
Bug 839103
Opened 12 years ago
Closed 12 years ago
Provide notifications for style sheet added and removed to chrome JS
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: harth, Assigned: heycam)
References
Details
Attachments
(11 files, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
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?
Attachment #725081 -
Flags: review?(bzbarsky)
![]() |
||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
(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.
Flags: needinfo?(fayearthur)
Reporter | ||
Comment 5•12 years ago
|
||
(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?
Flags: needinfo?(fayearthur)
![]() |
||
Comment 6•12 years ago
|
||
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.
Attachment #725081 -
Flags: review?(bzbarsky)
![]() |
||
Comment 7•12 years ago
|
||
(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?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 8•12 years ago
|
||
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...
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 9•12 years ago
|
||
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.
Attachment #725081 -
Attachment is obsolete: true
Attachment #730296 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•12 years ago
|
||
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.
Attachment #730298 -
Flags: review?(bzbarsky)
![]() |
||
Comment 11•12 years ago
|
||
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...?
Attachment #730300 -
Flags: review?(bzbarsky)
![]() |
||
Comment 12•12 years ago
|
||
...and tests. Not entirely sure if we can add UA stylesheets easily, so testing for .documentSheet == false is punted on.
Attachment #730302 -
Flags: review?(bzbarsky)
![]() |
||
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
Comment on attachment 730296 [details] [diff] [review] part 1 - factor out StyleSheet{Added,Removed} notifications into separate nsDocument methods r=me
Attachment #730296 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 15•12 years ago
|
||
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
Attachment #730298 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 16•12 years ago
|
||
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?
Attachment #730300 -
Flags: review?(bzbarsky) → review-
![]() |
||
Comment 17•12 years ago
|
||
> 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•12 years ago
|
||
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.
Attachment #730302 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
> 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?
Flags: needinfo?(bugs)
Comment 21•12 years ago
|
||
(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.
Flags: needinfo?(bugs)
![]() |
||
Comment 22•12 years ago
|
||
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.
Attachment #730300 -
Flags: review- → review?(bzbarsky)
![]() |
||
Comment 23•12 years ago
|
||
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. :(
Attachment #730300 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 24•12 years ago
|
||
(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•12 years ago
|
||
It can, if the WebIDL event interfaces are marked [NoInterfaceObject], yes.
![]() |
||
Comment 26•12 years ago
|
||
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.
Whiteboard: [leave open]
Comment 27•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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?
Flags: needinfo?(fayearthur)
Comment 32•12 years ago
|
||
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
Flags: in-testsuite+
![]() |
||
Updated•12 years ago
|
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 33•12 years ago
|
||
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...
Reporter | ||
Comment 35•12 years ago
|
||
(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.
Flags: needinfo?(fayearthur)
![]() |
||
Comment 36•12 years ago
|
||
(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.
Reporter | ||
Comment 37•12 years ago
|
||
(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.
Assignee | ||
Comment 38•12 years ago
|
||
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?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(fayearthur)
Assignee | ||
Comment 39•12 years ago
|
||
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...)
Reporter | ||
Comment 40•12 years ago
|
||
(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.
Flags: needinfo?(fayearthur)
Assignee | ||
Comment 41•12 years ago
|
||
Define a flag on documents to control whether style sheet change events are dispatched.
Attachment #750073 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 42•12 years ago
|
||
Put in a check for dispatching the StyleSheet{Add,Remove} events only if that bit is set on the document.
Attachment #750075 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 43•12 years ago
|
||
Stick a [ChromeOnly] styleSheetChangeEventsEnabled property on Document objects to control the flag.
Attachment #750077 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 44•12 years ago
|
||
Enable style sheet change events in the test.
Attachment #750078 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 45•12 years ago
|
||
Use a single interface for StyleSheet{Addded,Removed} events, since they record the same information.
Attachment #750079 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 46•12 years ago
|
||
Send StyleSheetApplicableStateChange events.
Attachment #750081 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 47•12 years ago
|
||
Don't want to undo part 6...
Attachment #750079 -
Attachment is obsolete: true
Attachment #750079 -
Flags: review?(bzbarsky)
Attachment #750097 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 48•12 years ago
|
||
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.
Attachment #750108 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 49•12 years ago
|
||
Green try run: https://tbpl.mozilla.org/?tree=Try&rev=6d06c5c5fcfa
![]() |
||
Comment 50•12 years ago
|
||
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...
Attachment #750073 -
Flags: review?(bzbarsky) → review-
![]() |
||
Comment 51•12 years ago
|
||
Comment on attachment 750075 [details] [diff] [review] Part 6: Only dispatch style sheet change events when enabled. r=me
Attachment #750075 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 52•12 years ago
|
||
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.
Attachment #750077 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 53•12 years ago
|
||
Comment on attachment 750078 [details] [diff] [review] Part 8: Use Document.styleSheetChangeEventsEnabled in test. r=me
Attachment #750078 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 54•12 years ago
|
||
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
Attachment #750097 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 55•12 years ago
|
||
Comment on attachment 750081 [details] [diff] [review] Part 10: Add StyleSheetApplicableStateChange event. r=me
Attachment #750081 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 56•12 years ago
|
||
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.
Attachment #750108 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 57•12 years ago
|
||
> 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.
Assignee | ||
Comment 58•12 years ago
|
||
(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.
Assignee | ||
Comment 59•12 years ago
|
||
(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.
Assignee | ||
Comment 60•12 years ago
|
||
(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?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 61•12 years ago
|
||
Oh, good point. I forgot about DeleteRule. OK, leave those bits be.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 62•12 years ago
|
||
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
Whiteboard: [leave open]
Assignee | ||
Comment 63•12 years ago
|
||
(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•12 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•