Last Comment Bug 676054 - Provide AUTHOR_SHEET type for registering with nsIStyleSheetService
: Provide AUTHOR_SHEET type for registering with nsIStyleSheetService
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P2 enhancement (vote)
: mozilla19
Assigned To: Gabor Krizsanits [:krizsa :gabor]
:
: Jet Villegas (:jet)
Mentors:
https://developer.mozilla.org/en/XPCO...
: 773602 (view as bug list)
Depends on: 631527 1228542
Blocks: 755606 795877 804120
  Show dependency treegraph
 
Reported: 2011-08-02 12:34 PDT by Stanimir Stamenkov
Modified: 2015-11-27 20:36 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a start... (17.84 KB, patch)
2012-10-04 10:30 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: feedback+
Details | Diff | Splinter Review
Provide AUTHOR_SHEET type for nsIStyleSheetService (18.06 KB, patch)
2012-10-10 03:44 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
part1: Cleanup after the additional doc sheets patch. (1.11 KB, patch)
2012-10-11 04:33 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review+
Details | Diff | Splinter Review
part2: Provide AUTHOR_SHEET type for registering with nsIStyleSheetService (18.04 KB, patch)
2012-10-11 04:34 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review+
Details | Diff | Splinter Review
part3: nsStyleSheetService::GetInstance (4.94 KB, patch)
2012-10-11 04:38 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review+
Details | Diff | Splinter Review

Description Stanimir Stamenkov 2011-08-02 12:34:55 PDT
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/
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-08-02 13:08:58 PDT
David, any concerns here?  Or just do this?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 13:03:01 PDT
Going to do this once bug 631527 lands so I don't conflict with it.
Comment 3 Matteo Ferretti [:zer0] [:matteo] 2012-07-13 11:15:41 PDT
*** Bug 773602 has been marked as a duplicate of this bug. ***
Comment 4 Dave Townsend [:mossop] 2012-07-16 16:43:36 PDT
Is this still something you're working on or could it use some assistance?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-07-16 19:57:07 PDT
I'm not actively working on this.  Feel free to steal...
Comment 6 Gabor Krizsanits [:krizsa :gabor] 2012-07-30 07:54:27 PDT
(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?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-07-30 08:35:21 PDT
> 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.
Comment 8 Matteo Ferretti [:zer0] [:matteo] 2012-07-30 08:58:27 PDT
(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.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-07-30 09:30:11 PDT
Yes, exactly.
Comment 10 Gabor Krizsanits [:krizsa :gabor] 2012-10-04 10:30:30 PDT
Created attachment 668061 [details] [diff] [review]
a start...

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.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-10-05 19:58:37 PDT
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.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-10-05 19:59:21 PDT
Oh, and other than the above this looks great!
Comment 13 Gabor Krizsanits [:krizsa :gabor] 2012-10-10 03:44:02 PDT
Created attachment 669911 [details] [diff] [review]
Provide AUTHOR_SHEET type for nsIStyleSheetService

(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...
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-10-10 09:10:55 PDT
> 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.
Comment 15 Gabor Krizsanits [:krizsa :gabor] 2012-10-10 10:59:14 PDT
(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).
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-10-10 11:06:32 PDT
> 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?
Comment 17 Gabor Krizsanits [:krizsa :gabor] 2012-10-10 12:30:29 PDT
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...
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-10-10 12:33:51 PDT
> 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 19 Gabor Krizsanits [:krizsa :gabor] 2012-10-10 12:48:35 PDT
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...
Comment 20 Gabor Krizsanits [:krizsa :gabor] 2012-10-11 04:33:48 PDT
Created attachment 670340 [details] [diff] [review]
part1: Cleanup after the additional doc sheets patch.

Removing some unnecessary bits from nsDocument::GetIndexOfStyleSheet.
Comment 21 Gabor Krizsanits [:krizsa :gabor] 2012-10-11 04:34:55 PDT
Created attachment 670341 [details] [diff] [review]
part2: Provide AUTHOR_SHEET type for registering with nsIStyleSheetService

I think this should be it.
Comment 22 Gabor Krizsanits [:krizsa :gabor] 2012-10-11 04:38:08 PDT
Created attachment 670342 [details] [diff] [review]
part3: nsStyleSheetService::GetInstance

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.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 12:22:55 PDT
Comment on attachment 670340 [details] [diff] [review]
part1: Cleanup after the additional doc sheets patch.

r=me
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 12:27:58 PDT
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.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 12:30:04 PDT
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....
Comment 26 Nicholas Nethercote [:njn] 2012-10-14 16:50:15 PDT
> 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!
Comment 27 Gabor Krizsanits [:krizsa :gabor] 2012-10-15 01:44:00 PDT
(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.
Comment 28 Gabor Krizsanits [:krizsa :gabor] 2012-10-16 12:54:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5386e4e6f54
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab2a9c94c91
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c10557ef1b5

So I forgot to remove the listeners... and found another typo in the patch too, I fixed those and updated according to bug 800187. It's green on try.
https://tbpl.mozilla.org/?tree=Try&rev=43a74f73b46c
Comment 30 Nicholas Nethercote [:njn] 2012-10-17 15:34:07 PDT
> -  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|.
Comment 31 Gabor Krizsanits [:krizsa :gabor] 2012-10-17 16:37:11 PDT
(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?
Comment 32 Nicholas Nethercote [:njn] 2012-10-17 17:05:31 PDT
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 :)

Note You need to log in before you can comment on or make changes to this bug.