939784 - Provide a notification when the applicable state of a style sheet changes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 839103 introduced a StyleSheetApplicableStateChange event but that is unfortunately unusable for SessionStore to be notified about page style changes for documents and their subdocuments as those events need to be enabled per document.

For SessionStore we would need a simple observer notification like "style-sheet-applicable-state-changed" with the document as the subject. That would enable us to be notified about state changes efficiently without querying page style data every time or having to enable all style sheet change events.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-939784-Provide-a-notification-when-the-applicabl.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why exactly does sessionstore care about which sheets are enabled for a document?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

If there's no page style set for the main document, we will recurse into child documents to check whether they have a specific set of style sheets enabled:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/PageStyle.jsm#32

This code is quite old and I don't understand it enough to say whether doing this all is necessary but it has been like that for some time.

According to the specification we could/should update those collection routines anyway:

http://dev.w3.org/csswg/cssom/#persisting-the-selected-css-style-sheet-set

Alas, this doesn't really say anything about subframes, etc. I think this assumes that we would just serialize the selectedStyleSheetSet property for every frame? This would mean we need a notification in nsDocument::SetSelectedStyleSheetSet().

The current code also tries to cover cases where the web page doesn't use .selectedStyleSheetSet but the "disable" attribute on style sheets directly. Maybe we could get rid of that?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, does sessionstore restore the selected stylesheet set or something?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Yes :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8333848 [details] [diff] [review]
0001-Bug-939784-Provide-a-notification-when-the-applicabl.patch

OK.  r=me, and let's hope this doesn't regress performance too much.  :(  Make sure whatever you do in that notification is _very_ cheap.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> OK.  r=me, and let's hope this doesn't regress performance too much.  :( 
> Make sure whatever you do in that notification is _very_ cheap.

We don't need to be notified synchronously, would you be happier if this would be using a Runnable? Is that something we do for notifications elsewhere?

Comment 8

[Boris Zbarsky [:bzbarsky]]

If we had a runnable that batched up the notifications, that might be better, yes.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-939784-Provide-a-notification-when-the-applicabl.patch

Using a runnable now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

That doesn't reduce the number of notifications compared to just doing it sync.  We probably want to set a boolean member to suppress creation of new runnables while this one is pending.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-939784-Provide-a-notification-when-the-applicabl.patch

(In reply to Boris Zbarsky [:bz] from comment #10)
> That doesn't reduce the number of notifications compared to just doing it
> sync.  We probably want to set a boolean member to suppress creation of new
> runnables while this one is pending.

Good point.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8334436 [details] [diff] [review]
0001-Bug-939784-Provide-a-notification-when-the-applicabl.patch

>+    NS_DispatchToCurrentThread(notification);
>+    mPendingSSApplicableStateNotification = true;

  if (NS_SUCCEEDED(NS_DispatchToCurrentThread(notification)) {
    mPendingSSApplicableStateNotification = true;
  }

And maybe call the boolean mSSApplicableStateNotificationPending?

>+  bool mPendingSSApplicableStateNotification;

Please put this over with the other boolean members.

r=me wtoth that fixed.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Applied all requested changes.

https://hg.mozilla.org/integration/fx-team/rev/59e1d4236705

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59e1d4236705