Closed Bug 676054 Opened 13 years ago Closed 12 years ago

Provide AUTHOR_SHEET type for registering with nsIStyleSheetService

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: stanio, Assigned: gkrizsanits)

References

()

Details

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20110731 SeaMonkey/2.3
Build ID: 20110731194239

Steps to reproduce:

Discussion about the original use case have taken place in the mozilla.dev.tech.layout group, see the "User style sheets - how useful they are? (cascading)" thread:

http://groups.google.com/group/mozilla.dev.tech.layout/browse_frm/thread/0d743888116e688c

Basically employing user style sheets to customize the presentation of specific sites is not as useful, regarding cascading rules/order [1], as having a style sheet added to the existing author styles, having the same "author origin".  Such custom user style sheets will act as overlay over the existing author styles (placed after all author specified ones).  All this should presumably make customizing the existing presentation of specific sites much easier (employed by extensions like Stylish [2]).

[1] http://www.w3.org/TR/CSS2/cascade.html#cascading-order
[2] https://addons.mozilla.org/addon/stylish/
Severity: normal → enhancement
OS: Windows 7 → All
Hardware: x86_64 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
David, any concerns here?  Or just do this?
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
Going to do this once bug 631527 lands so I don't conflict with it.
Assignee: nobody → bzbarsky
Depends on: 631527
Priority: -- → P2
Is this still something you're working on or could it use some assistance?
I'm not actively working on this.  Feel free to steal...
Assignee: bzbarsky → gkrizsanits
(In reply to Stanimir Stamenkov from comment #0)
> existing author styles (placed after all author specified ones).

nsStyleSet::sheetType are not entirely clear for me. Which represents the author style sheets from this enum? (I guess eDocSheet, and StyleAttrSheet is for the inlined ones) So the question is if I should add a brand new type or should I just use one of the existing types? Also in case of the document adds some new styles dynamically, we need to be careful that those new ones should not be placed after the ones registered in the StyleSheetService, right?
> Which represents the author style sheets from this enum?

eDocSheet.

> So the question is if I should add a brand new type or should I just use one of the
> existing types?

This bug report is about specifically adding stylesheets to the end of the eDocSheet list.  Using another type would not work for the use case this bug is about, for the same reason that just making all the rules be !important user rules doesn't work.

> Also in case of the document adds some new styles dynamically, we need to be careful

Yes.
(In reply to Boris Zbarsky (:bz) from comment #7)

> This bug report is about specifically adding stylesheets to the end of the
> eDocSheet list.  Using another type would not work for the use case this bug
> is about, for the same reason that just making all the rules be !important
> user rules doesn't work.

> > Also in case of the document adds some new styles dynamically, we need to be careful

> Yes.

Basically we have to be sure that the eDocSheet added in this way are always at the end of the list (assuming the author stylesheet are applied following the list order), I guess.
Yes, exactly.
Attached patch a start... (obsolete) — Splinter Review
Ok I'll just start this somewhere... It seems to work but I'm sure I'm missing a few things. And in general I'm really missing the big picture here about a few things. I am really not sure about the current PresShell::AddAuthorSheet, and it feels like nsStyleSet::AddDocStyleSheet is getting too complex. So I'm going to try to understand the relationship between all these sheet sets and sheet arrays, to be sure I'm doing the right thing. But in the meanwhile if you can point out something I do totally wrong or forgetting about something please let me know.
Attachment #668061 - Flags: feedback?(bzbarsky)
Comment on attachment 668061 [details] [diff] [review]
a start...

PresShell::AddAuthorSheet seems fine.

I don't see why the newDocIndex bit is needed in AddDocStyleSheet.  None of your sheets should be arguments to that function, should they?

If you do your GetService in FillStyleSet() before you call AddDocStyleSheet, then I think you can simply assert that gInstance is non-null in AddDocStyleSheet.

Other than that, just formatting nits:

1)  Please add -p to your diff flags (or showfunc=1 in your hgrc).
2)  I'd really prefer things like "||" and "&&" come at ends of lines, not
    starts of lines.
Attachment #668061 - Flags: feedback?(bzbarsky) → feedback+
Oh, and other than the above this looks great!
(In reply to Boris Zbarsky (:bz) from comment #11)
> I don't see why the newDocIndex bit is needed in AddDocStyleSheet.  None of
> your sheets should be arguments to that function, should they?

Ah yeah... so this is the part I got confused. I'm not sure how do additional sheets end up in this nsStyleSet::mSheets[eDocSheet]; when they have a different sheet type? And after some late night debugging I got confused to the point that I thought that through AddDocStyleSheet... Which is not the case, so those bits are not needed, but I'm still a bit confused here...

> 
> If you do your GetService in FillStyleSet() before you call
> AddDocStyleSheet, then I think you can simply assert that gInstance is
> non-null in AddDocStyleSheet.

I did that. My original plan was to make a utility function for getting the gInstance (making sure that the dummy is inited only once) and using that from everywhere. Would that make sense?

> 
> 1)  Please add -p to your diff flags (or showfunc=1 in your hgrc).

Uhhh... sorry I thought my hgrc [defaults] section are taking care of it... I fixed it on all my repos and thanks for pointing it out...

> 2)  I'd really prefer things like "||" and "&&" come at ends of lines, not
>     starts of lines.

At some point in the future I'm sure I will get used to it and not make this mistake again and again...
Attachment #668061 - Attachment is obsolete: true
Attachment #669911 - Flags: review?(bzbarsky)
> but I'm still a bit confused here...

OK, what's confusing?

> My original plan was to make a utility function for getting the gInstance 

Sounds fine to me.
(In reply to Boris Zbarsky (:bz) from comment #14)
> > but I'm still a bit confused here...
> 
> OK, what's confusing?

How does the additional sheets end up in nsStyleSet::mSheets[eDocSheet] ? If not via AddDocStyleSheet? And also not via AppendStyleSheet since we call that function with a different type (eUserSheet or eAgentSheet).
> How does the additional sheets end up in nsStyleSet::mSheets[eDocSheet] ?

You mean the ones you're adding?  The patch has styleSet->AppendStyleSheet(nsStyleSet::eDocSheet, aSheet); in AppendAuthorSheet.  Or are you talking about some other additional stylesheets?
I mean the additional sheets I added in my previous patch https://bug737003.bugzilla.mozilla.org/attachment.cgi?id=656410 

In that patch. I changed nsDocument::GetIndexOfStyleSheet to return intmax for those "additional" sheets, because of the ordering in AddDocStyleSheet (if a regular doc sheet is added dynamically after the "additional" sheet). But since those "additional" sheets are either user or agent type sheets, they should not even end up in that array (nsStyleSet::mSheets[eDocSheet]) on the first place, or at least it's not clear to me how they ended up there.

Now I feel quite stupid that it turns out I don't get a part of my previous patch it seems, but before I grok this part better I just take back my r? for now...
> I mean the additional sheets I added in my previous patch 
> https://bug737003.bugzilla.mozilla.org/attachment.cgi?id=656410 

Those don't end up in mSheets[eDocSheet].  I'm not sure why I didn't catch that the change to GetIndexOfStyleSheet was unnecessary, but I think it was unnecessary.
Comment on attachment 669911 [details] [diff] [review]
Provide AUTHOR_SHEET type for nsIStyleSheetService

Review of attachment 669911 [details] [diff] [review]:
-----------------------------------------------------------------

Alright I'll just file a clean up patch for that here and then a clean version of this one too tomorrow. I think I just got confused a few times during writing those tests the last time...
Attachment #669911 - Flags: review?(bzbarsky)
Removing some unnecessary bits from nsDocument::GetIndexOfStyleSheet.
Attachment #670340 - Flags: review?(bzbarsky)
I think this should be it.
Attachment #669911 - Attachment is obsolete: true
Attachment #670341 - Flags: review?(bzbarsky)
The utility function I mentioned earlier. This ofc only works if services cannot just get out of scope and their singleton de-initialized, but if I understood correctly that is the way GetServices works.
Attachment #670342 - Flags: review?(bzbarsky)
Comment on attachment 670340 [details] [diff] [review]
part1: Cleanup after the additional doc sheets patch.

r=me
Attachment #670340 - Flags: review?(bzbarsky) → review+
Comment on attachment 670341 [details] [diff] [review]
part2: Provide AUTHOR_SHEET type for registering with nsIStyleSheetService

This is still doing AddDocStyleSheet calls before the getService call, so the assert seems wrong on its face.

On the other hand, you're still null-checking sheetService after that.  Why?

Watch out for merging with bug 800187: if it lands first you will want to add your new array to the memory reporter.

r=me modulo those bits.
Attachment #670341 - Flags: review?(bzbarsky) → review+
Comment on attachment 670342 [details] [diff] [review]
part3: nsStyleSheetService::GetInstance

r=me

I guess you do need the null checks in the previous patch, but should drop the assert....
Attachment #670342 - Flags: review?(bzbarsky) → review+
> Watch out for merging with bug 800187: if it lands first you will want to
> add your new array to the memory reporter.

I just landed bug 800187, so please update nsStyleSheetService::SizeOfIncludingThisHelper() accordingly.  Thanks!
(In reply to Nicholas Nethercote [:njn] from comment #26)
> I just landed bug 800187, so please update
> nsStyleSheetService::SizeOfIncludingThisHelper() accordingly.  Thanks!

Alright. My patch is failing on try so I need some more time anyway.
> -  nsCOMArray<nsIStyleSheet> mSheets[2];
> +  nsCOMArray<nsIStyleSheet> mSheets[3];

In cases like this it's nice to define a "limit" value, e.g. |LIMIT_SHEET = AUTHOR_SHEET|, and then do |mSheets[LIMIT_SHEET + 1]| instead of hardcoding |3|.
(In reply to Nicholas Nethercote [:njn] from comment #30)
> > -  nsCOMArray<nsIStyleSheet> mSheets[2];
> > +  nsCOMArray<nsIStyleSheet> mSheets[3];
> 
> In cases like this it's nice to define a "limit" value, e.g. |LIMIT_SHEET =
> AUTHOR_SHEET|, and then do |mSheets[LIMIT_SHEET + 1]| instead of hardcoding
> |3|.

If there were an enum for these types and not just defined in idl I would have done that. This way however if anyone would swap two types in the idl (which is VERY unlikely ofc) this would brake. That being said if there is a demand for it I can file a follow up patch, or maybe I could add another define to the idl like NUMBER_OF_SHEET_TYPES. I would actually prefer that... What do you think?
Up to you if you want to do a follow-up;  I'm not that fussed.

http://mxr.mozilla.org/mozilla-central/source/js/src/gc/Heap.h#44 is an example of what I'm talking about -- see FINALIZE_LAST and FINALIZE_LIMIT.  I don't know much about IDL so if you say that approach wouldn't quite work I believe you :)
Depends on: 1228542
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: