[FIXr]Styles registered with stylesheet service should apply to already loaded documents

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Jason Barnabe (np), Assigned: bz)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9alpha1
x86
All
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
nsIStylesheetService allows you to register user stylesheets. These stylesheets only take effect on documents loaded after the registration. There's no way to have it apply to documents loaded before registration short of reloading them.

To make the interface more useful, when registering a stylesheet, it should also be applied to documents that are already loaded.
Should unregistering a sheet deapply it from already-loaded documents too?
(Reporter)

Comment 2

11 years ago
Yeah, I was just about to mention that.
We defined this as out of scope when we implemented it originally.  :)  I agree that it would be useful, it's probably just beyond my ability to implement it in that way.
Created attachment 220022 [details] [diff] [review]
Proposed patch

Jason, want to test this?
(Reporter)

Comment 5

11 years ago
Created attachment 220094 [details]
testcase (must be loaded as chrome)

I opened the testcase as chrome in one tab and Google in the other. I checked off the box in the testcase which registered a style that should make everything red. Nothing changed. I opened a new tab, again going to Google, and everything was red there. I unchecked the checkbox in the testcase, which unregisters the style, and the red Google tab didn't switch back to normal colors.

Unless I'm missing something, there doesn't seem to be a difference in behaviour with this patch.
Created attachment 220096 [details] [diff] [review]
With feeling

Observer notifications don't work if you don't call AddObserver!  News at 11.

Does this work?  ;)
Attachment #220022 - Attachment is obsolete: true
(Reporter)

Comment 7

11 years ago
Still nothing.
Created attachment 220100 [details] [diff] [review]
And this?

I finally got the testcase to work, and this patch definitely works with that testcase over here.  Then again, so does the previous one, but this one has a change that should make some compilers happier, maybe...
Attachment #220096 - Attachment is obsolete: true
(Reporter)

Comment 9

11 years ago
Created attachment 220219 [details]
test version of stylish

Sorry, I was just not building correctly. It seems to work perfectly fine. Attached is a version of the Firefox extension Stylish. This version solely uses the stylesheet service to apply styles, and with this patch, they are applied and unapplied without requiring a reload.

Performance-wise it takes ~3 seconds to do either action with a handful of tabs open, which is comparable to the time it took previous versions of Stylish to apply or remove a html:link to every open document.

Nit: you've misspelled "successful" in your patch.
Comment on attachment 220100 [details] [diff] [review]
And this?

Yeah, unfortunately if we do this it does require us to reresolve all style in all open things...  Not much I can do about making that faster offhand...

I'll fix the spelling.
Attachment #220100 - Flags: superreview?(dbaron)
Attachment #220100 - Flags: review?(dbaron)

Comment 11

11 years ago
Can it be done later in background tabs?  (I think there's an optimization like that when the window is resized.)
Maybe...  It could easily screw up scripts to do that, though, so doing it right is a lot of work, and since I don't use tabs much myself I have pretty low motivation for that. ;)  At least in this bug.

Comment 13

11 years ago
> Maybe...  It could easily screw up scripts to do that, though, so doing it
> right is a lot of work

Can you elaborate?

> since I don't use tabs much myself I have pretty low motivation for that

Is there an API in Gecko to determine whether a window is completely obscured?
> Can you elaborate?

If script running in a tab asks for the styles for something, it should probably get the current styles for that thing, background tab or no.  Unless you can force that script to be rerun when the tab is shown...

> Is there an API in Gecko to determine whether a window is completely obscured?

See what the code in nsViewManager::SetWindowDimensions does.

Comment 15

11 years ago
> If script running in a tab asks for the styles for something, it should
> probably get the current styles for that thing, background tab or no.  Unless
> you can force that script to be rerun when the tab is shown...

How do we solve that problem for the window-resize optimization?
We perform the resize.  Scripts that care about window size always watch resize events.  There are no equivalent events for style changes, really.
Comment on attachment 220100 [details] [diff] [review]
And this?

You should call mStyleSet's BeginUpdate and EndUpdate methods at the start and end of AddUserSheet -- otherwise it could do a lot of rule processor gathering, no?  (If not, assert that it's batching.)

Have you tested that the ordering stuff that you commented about really works?  I didn't bother following through the twisty maze of Append vs. Prepend calls, or checking that forward iteration was the right way -- but please check that you chose those the right way in AddAgentSheet and AddUserSheet.  It might also be good for PresShell::AddUserSheet to assert that aSheet doesn't get removed but it does get added.  And for both nsStyleSet::mSheets (and nsCSSRuleProcessor::mSheets, probably by reference) to document what the order of the sheets means.

Any idea what the assertion in nsStyleSet::RemoveStyleSheet about completeness is about and whether you could trigger it?
Attachment #220100 - Flags: superreview?(dbaron)
Attachment #220100 - Flags: superreview+
Attachment #220100 - Flags: review?(dbaron)
Attachment #220100 - Flags: review+
> You should call mStyleSet's BeginUpdate and EndUpdate methods 

Good catch.

> but please check that you chose those the right way in AddAgentSheet and
> AddUserSheet.

Urgh.  Good catch.  I was iterating in the wrong direction when prepending... Will fix.

> It might also be good for PresShell::AddUserSheet to assert that aSheet
> doesn't get removed but it does get added.

I can't assert the latter in the face of OOM.  I could assert the former just by checking whether aSheet is in the set before I start removing, I suppose.  It doesn't seem that useful, but let me know if you want me to do this and I will.

> Any idea what the assertion in nsStyleSet::RemoveStyleSheet about completeness
> is about

Yeah.  When I added the idea of incomplete sheets I put asserts in all the style set methods that we're not working with such sheets in the set.  There's no way to trigger it without triggering the asserts on adding incomplete sheets to the set, to my knowledge.
Assignee: dbaron → bzbarsky
Summary: Styles registered with stylesheet service should apply to already loaded documents → [FIXr]Styles registered with stylesheet service should apply to already loaded documents
Target Milestone: --- → mozilla1.9alpha
Created attachment 224821 [details] [diff] [review]
Updated to comments
Attachment #220100 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

11 years ago
Comment on attachment 224821 [details] [diff] [review]
Updated to comments

I'd like this on the 1.8.1 branch because it greatly improves the usefulness of this interface.
Attachment #224821 - Flags: approval-branch-1.8.1?
You want to request the approval from someone...  And note that this is technically an API change,  It does make the API more useful, but still...

I probably should have revved the IID, actually.
(Reporter)

Updated

11 years ago
Attachment #224821 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1?(dbaron)
Do you think it could break any existing callers?  Or is it just that you think new callers may want to test for the new behavior being present?  Technically we're supposed to avoid such API changes...
(Reporter)

Comment 24

11 years ago
Stylish registers sheets with the stylesheet service, but also puts a html:link in every document to make the style apply immediately. It's not a perfect solution because each method applies the sheets with a different level of importance, but it's better than nothing. If the new functionality is present, nothing would break. If I could test for it, though, I could skip the html:link part and get consistent results.
David, I doubt existing callers would really depend on the current behavior, though I wouldn't put anything past them, frankly.  The main reasons to change the IID would be so that callers could test for the old behavior and supply workarounds if desired.

I'm not sure I would be happy taking this change on the 1.8 branch, frankly.
Created attachment 225198 [details] [diff] [review]
UUID change if we decide to allow testing for the changed behavior
As much as I'd like to see this on the 1.8 branch, it definitely changes the contract for this interface.  I don't think we can take it and claim unchanged APIs.
Comment on attachment 224821 [details] [diff] [review]
Updated to comments

Bugzilla users in
general shouldn't make approval requests; they should either (1) make blocking
requests or (2) ask the patch author to make an approval request if the patch
author thinks the patch is appropriate for whatever branch.  One of the
principles of requests like this is that two people have to agree:  the patch
author and one of the author's peers.  Other users requesting approval skips
that step: you get only one person, the peer, and not the author.

If bzbarsky re-requests, I'll reconsider, but he seems slightly against.  I don't care so much about the interface issue...
Attachment #224821 - Flags: approval-branch-1.8.1?(dbaron) → approval-branch-1.8.1-
(Reporter)

Comment 29

11 years ago
What about the IID change?
Since now stylesheet addition and removal are applied immediately, *reloading* a sheet (which I do by removing it and adding it again) now causes 2 of those (slow) updates. Would it be possible to add a ReloadSheet function to nsIStyleSheetService which combines that into only one update to all open pages? Should I file a new bug for that?
Comment on attachment 225198 [details] [diff] [review]
UUID change if we decide to allow testing for the changed behavior

I think we should take the IID change on trunk.
Attachment #225198 - Flags: superreview?(dbaron)
Attachment #225198 - Flags: review?(dbaron)
(In reply to comment #30)
> Since now stylesheet addition and removal are applied immediately

chpe, they're applied off an event; the event is now posted immediately.  There is only one change, if nothing weird is happening.  Are you actually seeing the remove/add pattern take twice as much time now as it used to?  If so, I'd love a testcase (extension, xulrunner app, whatever) so I can debug what's going on.

That's not to say that we shouldn't have a ReloadSheet function, just from the point of view of API nicety.  But performance shouldn't be a driving reason for it.
Attachment #225198 - Flags: superreview?(dbaron)
Attachment #225198 - Flags: superreview+
Attachment #225198 - Flags: review?(dbaron)
Attachment #225198 - Flags: review+
Checked in the iid change on trunk.
Keywords: dev-doc-needed
The article covering nsIStyleSheetService has a note now indicating that Firefox 3 applies the style immediately; I'm marking this as doc complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.