Last Comment Bug 804120 - Offer a way to apply author stylesheet on a given document
: Offer a way to apply author stylesheet on a given document
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla19
Assigned To: Gabor Krizsanits [:krizsa :gabor]
:
Mentors:
Depends on: 676054 737003
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-22 05:41 PDT by Matteo Ferretti [:zer0] [:matteo]
Modified: 2012-10-29 13:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Offer a way to apply author stylesheet on a given document (21.27 KB, patch)
2012-10-25 06:15 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
Offer a way to apply author stylesheet on a given document (24.17 KB, patch)
2012-10-26 07:06 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review+
Details | Diff | Splinter Review

Description Matteo Ferretti [:zer0] [:matteo] 2012-10-22 05:41:31 PDT
With bug 737003 and bug 676054 fixed, this is a natural consequences.

We should be able to use the new method provided by bug 737003 with the new AUTHOR style type.
That will permit to us to register author stylesheet to specific document that won't result in the DOM of the page itself, and will give to us the capability to override the author stylesheet of the page, and solving all the issues that we currently have in Add-on SDK with `contentStyle`, described here:

https://github.com/mozilla/addon-sdk/wiki/contentStyle-issues

Notice that the AUTHOR sheet registered in this way should be applied always after all the AUTHOR sheet defined in the page (at loading time and created dynamically) in order to always override them.

Plus, if a global AUTHOR sheet is registered, the AUTHOR sheet per document should be able to override it.
Comment 1 Gabor Krizsanits [:krizsa :gabor] 2012-10-25 06:15:05 PDT
Created attachment 675093 [details] [diff] [review]
Offer a way to apply author stylesheet on a given document

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #0)
> Plus, if a global AUTHOR sheet is registered, the AUTHOR sheet per document
> should be able to override it.

To meet this requirement, I really needed those tricks with INT32_MAX... so AddDocStyleSheet got bit more complex than it was... Shall I use some #defines for these special indexes? This is the reason for changing PresShell::AddAuthorSheet which belongs to the StyleSheetManager based AUTHOR_SHEETS which can now be overridden by document specific AUTHOR_SHEETS (added via DOMWinowUtils...).

I realized that it's easier to use the SheetTypeCount for defining the length of mAdditionalSheets with it than later assert it, if they are the same.
Comment 2 Boris Zbarsky [:bz] (TPAC) 2012-10-25 06:33:21 PDT
Comment on attachment 675093 [details] [diff] [review]
Offer a way to apply author stylesheet on a given document

The only reason you need the extra complexity is because you're reusing AddDocStyleSheet in PresShell::AddAuthorSheet, right?

I think it would be better to just have that explicitly ask the document which sheet to insert before in the list (so just ask it for the pointer to the first additional stylesheet) and add an API on style set to insert a stylesheet before a given other stylesheet.
Comment 3 Gabor Krizsanits [:krizsa :gabor] 2012-10-25 07:32:14 PDT
(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 675093 [details] [diff] [review]
> I think it would be better to just have that explicitly ask the document
> which sheet to insert before in the list (so just ask it for the pointer to
> the first additional stylesheet) and add an API on style set to insert a
> stylesheet before a given other stylesheet.

I like that. Thanks for the idea! I'll file another patch with that change.
Comment 4 Gabor Krizsanits [:krizsa :gabor] 2012-10-26 07:06:29 PDT
Created attachment 675545 [details] [diff] [review]
Offer a way to apply author stylesheet on a given document
Comment 5 Boris Zbarsky [:bz] (TPAC) 2012-10-26 21:59:26 PDT
Comment on attachment 675545 [details] [diff] [review]
Offer a way to apply author stylesheet on a given document

Do you still need the change to nsDocument::GetIndexOfStyleSheet?  Seems like it might be better to remove it and make the sheetDocIndex < 0 case in AddDocStyleSheet check whether "sheet" is the first additional sheet.  That would make the common case of no weird sheets fast...

>+  if (!mAdditionalSheets[eAuthorSheet].Count())
>+    return nullptr;
>+
>+  return mAdditionalSheets[eAuthorSheet][0];

  return mAdditionalSheets[eAuthorSheet].SafeObjectAt(0);

>+  // added with the StyleSheetManager

You mean StyleSheetService?

>+nsStyleSet::InsertBeforeStyleSheet(sheetType aType, nsIStyleSheet *aNewSheet,

How about InsertStyleSheetBefore?

You have a stray whitespace change in AddDocStyleSheet.

r=me with those fixed.
Comment 6 Gabor Krizsanits [:krizsa :gabor] 2012-10-29 04:26:07 PDT
Thanks for all the help.

https://tbpl.mozilla.org/?tree=Try&rev=432cbb0af161

https://hg.mozilla.org/integration/mozilla-inbound/rev/169e189085e2
Comment 7 Phil Ringnalda (:philor) 2012-10-29 13:51:41 PDT
https://hg.mozilla.org/mozilla-central/rev/169e189085e2

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