Provide notifications for style sheet added and removed to chrome JS

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: harth, Assigned: heycam)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 2 obsolete attachments)

5.82 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.88 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
1.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.89 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
943 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
19.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Reporter

Description

7 years ago
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?
Attachment #725081 - Flags: review?(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...
(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

6 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 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)
(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)
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)
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)
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)
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)
...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)
(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
Attachment #730296 - Flags: review?(bzbarsky) → review+
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 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-
> 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.
Attachment #730302 - Flags: review?(bzbarsky) → review+
(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?
Flags: needinfo?(bugs)
(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 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 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+
(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.
Whiteboard: [leave open]
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
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)
bz, see comment 30.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(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...
Also, it's possible this regressed Tp5.  Not clear so far.
Reporter

Comment 35

6 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)
(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

6 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

6 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

6 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

6 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

6 years ago
Define a flag on documents to control whether style sheet change events are dispatched.
Attachment #750073 - Flags: review?(bzbarsky)
Assignee

Comment 42

6 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

6 years ago
Stick a [ChromeOnly] styleSheetChangeEventsEnabled property on Document objects to control the flag.
Attachment #750077 - Flags: review?(bzbarsky)
Assignee

Comment 44

6 years ago
Enable style sheet change events in the test.
Attachment #750078 - Flags: review?(bzbarsky)
Assignee

Comment 45

6 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

6 years ago
Send StyleSheetApplicableStateChange events.
Attachment #750081 - Flags: review?(bzbarsky)
Assignee

Comment 47

6 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

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

r=me
Attachment #750078 - Flags: review?(bzbarsky) → review+
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 on attachment 750081 [details] [diff] [review]
Part 10: Add StyleSheetApplicableStateChange event.

r=me
Attachment #750081 - Flags: review?(bzbarsky) → review+
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+
>  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

6 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

6 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

6 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)
Oh, good point.  I forgot about DeleteRule.  OK, leave those bits be.
Flags: needinfo?(bzbarsky)
Assignee

Comment 63

6 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.
You need to log in before you can comment on or make changes to this bug.