buster Reporter
	Description • 24 years ago

1. mFrameConstructor is used all over the place without null checks.  I would add:
  NS_ASSERTION(mFrameConstructor, "unexpected null frame constructor");
  if (!mFrameConstructor) { return NS_ERROR_NULL_POINTER; }

2. no pointer arguments are checked for null.  I would at least add assertions.

	buster Reporter
	Comment 1 • 24 years ago

mine

	buster Reporter
	Comment 2 • 24 years ago

code clean-up bug, not a big priority for me right now.  Any takers?

	Kevin McCluskey (gone)
	Comment 3 • 23 years ago

Build reassigning Buster's bugs to Marc.

	David Baron :dbaron:
	Comment 4 • 22 years ago

I think I partly disagree with the suggestions in this bug, but there is
certainly some cleanup to be done.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 22 years ago

(or rather that's a diff)

	David Baron :dbaron:
	Comment 6 • 22 years ago

How about giving this to bz?  No rush, of course.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 22 years ago

Thanks. ;)

So the changes I've made so far are:

1)  Move from nsISupportsArray to nsCOMArray for type-safety, readability, etc
2)  Move from having lots of copies of the same code to having some macros
3)  Change some function signatures.

I seem to recall you mentioning something about looping over the arrays instead
of using Enumerate(); what was the reasoning there?

Oh, and if you think of things to do to this code feel free to dump them in the
bug, of course.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 22 years ago

EnsureRuleWalker should probably return an error/success indicator.

	David Baron :dbaron:
	Comment 9 • 22 years ago

I have some EnsureRuleWalker-related changes in my tree already (see bug
154751), although I'm just checking mRuleWalker for null afterwards, and I'll
probably land those changes soon.

	Brian Ryner (not reading) Assignee
	Comment 10 • 21 years ago

So, I have a patch I've been working on that eliminates nsIStyleSet as an XPCOM
interface, plus some other changes to make things faster/smaller.  The patch is
somewhat large but the changes are pretty straightforward:

- Consolidated style sheet add/remove API to not duplicate code for each type of
style sheet. Switched to an array of |nsCOMArray|s for holding the style sheets.

- Added batching so that updating the rule processor array can be deferred until
after all style sheets are added/removed.

- Stop null-checking the pres shell's style set everywhere.  Init() will fail if
it could not create the style set object; after that it can be assumed that the
style set is non-null.

- Moved ownership of the frame constructor from the style set to the pres shell,
and made an inline getter for the frame constructor on nsIPresShell.  As with
the style set, the frame constructor is guaranteed to be non-null after
PresShell::Init returns success, for the lifetime of the presshell.

- Removed nsStyleSet methods that simply forwarded to the frame constructor;
changed callers to call the frame constructor methods directly.

- Removed XPCOM component manager creation of both the frame constructor and
style set objects.

- Moved layout debugging hooks off of nsStyleSet.  nsIStyleSet::List (which
listed the document style sheets) becomes nsIDocument::ListStyleSheets, and
nsIStyleSet::ListContexts (which printed the style context tree starting at a
given frame) moves onto nsIPresShell.

- Converted nsIStyleRuleSupplier and its implementation in XBL to use a COMArray
enumerator, to stay compatible with the storage in nsStyleSet.

- Cleaned up and condensed some code in editor that was dealing with style sets.
 It now uses the nsIDocument API exclusively for adding and removing document
style sheets (currently it's inconsistent - it uses the document API for adding
a sheet, but the style set API for removing a sheet).

- Moved the override stylesheet hooks onto nsIPresShell.  It's kind of
unfortunate that we need this, but I'd like to defer any changes in that until
later.

- Moved the agent stylesheet hooks that the chrome registry uses onto nsIPresShell.

- Moved PresShell::CloneStyleSet inside #ifdef DEBUG, it's only needed there.

	Brian Ryner (not reading) Assignee
	Comment 11 • 21 years ago

The aforementioned patch

	Brian Ryner (not reading) Assignee
	Comment 13 • 21 years ago

Comment on attachment 138318 [details] [diff] [review]
big patch

dbaron, boris, either of you want to take a look at this?

	David Baron :dbaron:
	Comment 14 • 21 years ago

Here's a review of roughly the first third of the patch (it's a little late to
untangle the nsStyleSet.cpp diffs):

I don't like the |StyleFrameConstruction| name for the accessor.  How
about |FrameConstructor|?  I'd rather that interface name went away and
we didn't have to think about two names for the same thing.
(You can just diff, un-patch, search-replace (with "()"), and re-patch.)
 
 
Index: content/base/public/nsIDocument.h
 
+#ifdef DEBUG
+#include <stdio.h>   // for FILE definition
+#endif
 
Don't |#ifdef| an |#include|.
 
 
Index: content/base/src/nsDocument.cpp
 
+    ((nsIPresShell *)mPresShells.ElementAt(indx))->StyleSet()->
 
NS_STATIC_CAST(nsIPresShell*, mPresShells.ElementAt(indx))->StyleSet()->
 
(and NS_STATIC_CAST as well just below)
 
 
in DocumentViewerImpl::CreateStyleSet(nsIDocument* aDocument, please use
a temporary variable rather than (*aStyleSet) all over the place.  Also,
it would probably be good to return already_AddRefed<nsStyleSet> instead
of using an nsStyleSet**.
 
 
In the nsPrintEngine.cpp change, it would be good to actually handle
out-of-memory.
 
 
nsStyleSet.cpp:
 
+#ifdef DEBUG
+#include "nsIFrame.h"
+#endif
 
Don't |#ifdef| includes.
 
+#ifdef MOZ_PERF_METRICS
+NS_IMPL_ISUPPORTS1(nsStyleSet, nsITimeRecorder)
 #endif
 
This isn't going to work so well -- you need to use
NS_IMPL_QUERYINTERFACE1 and stub out dummy AddRef and Release methods.
 
I'm up to "StyleSetImpl::RecycleArray".

	Brian Ryner (not reading) Assignee
	Comment 15 • 21 years ago

> This isn't going to work so well -- you need to use
> NS_IMPL_QUERYINTERFACE1 and stub out dummy AddRef and Release methods.

I'm not sure what you mean.  The object is refcounted, so it needs to have
working addref/release methods.  Though now that I think about it, I'm not sure
it needs to be refcounted at all.  Is that what you're saying?

	Brian Ryner (not reading) Assignee
	Comment 16 • 21 years ago

> in DocumentViewerImpl::CreateStyleSet(nsIDocument* aDocument, please use
> a temporary variable rather than (*aStyleSet) all over the place.  Also,
> it would probably be good to return already_AddRefed<nsStyleSet> instead
> of using an nsStyleSet**.

Well, I need to be able to return NS_ERROR_OUT_OF_MEMORY from that function...
are you suggesting making the nsresult an out-parameter instead? (or can a
|already_AddRefed| be an out-parameter?)

	Brian Ryner (not reading) Assignee
	Comment 17 • 21 years ago

I think I'm going to just make the object non-refcounted.

As for the whole nsITimeRecorder business, another option is to simply get rid
of nsITimeRecorder.  nsStyleSet is the only implementation, and the only caller
is the pres shell.  Since it looks unlikely that anyone will be making further
use of this interface, my vote is to ditch it and save the trouble of a fake
nsISupports implementation.

	David Baron :dbaron:
	Comment 18 • 21 years ago

Oh, I was thinking it wasn't refcounted since there was no Addref and Release
implementation #ifndef MOZ_PERF_METRICS -- or at least I didn't see it.  (Was it
somewhere else in the file?)

	David Baron :dbaron:
	Comment 19 • 21 years ago

OK, nsIStyleSet** is fine with me.

	Brian Ryner (not reading) Assignee
	Comment 20 • 21 years ago

Just FYI, in my tree I went ahead and removed refcounting from nsStyleSet, and
also removed the style resolution timing stuff (dbaron and I don't think anyone
is using it).  I'll include these changes in the next patch I post, which will
be after I've got all of dbaron's review comments to incorporate.

	Brian Ryner (not reading) Assignee
	Comment 21 • 21 years ago

One more problem I found and fixed in my tree -- in PresShell::Destroy(), it
needs to set mStyleSet to null after deleting it so that Destroy() is not called
a second time from the destructor.

	David Baron :dbaron:
	Comment 22 • 21 years ago

Here's comments on all of the rest of the patch except for the 3 things that I
noted at the end (I'll try to get to those later today -- I just skipped them
since they seemed to require close inspection and I wanted to feel like I was
mostly done):

nsStyleSet.cpp, continued:
 
  In |EndUpdate|, assigning to dirty and batching on separate lines
  would probably be clearer, and what you currently do seems to be
  asking for weird compiler bugs with bitfields to appear.
 
  In |EnableQuirkStyleSheet|, the following code does different things to
  the refcount in the two halves:
  +#ifdef DEBUG
  +      CallQueryInterface(sheet, &cssSheet);
  +      NS_ASSERTION(cssSheet, "Agent sheet must be a CSSStyleSheet");
  +#else
  +      cssSheet = NS_STATIC_CAST(nsICSSStyleSheet*, sheet);
  +#endif
  It would be better to just do:
  NS_ASSERTION(nsCOMPtr<nsICSSStyleSheet>(do_QueryInterface(sheet)) == cssSheet,
              "Agent sheet must be a CSSStyleSheet");
  after assigning to |cssSheet| with the non-DEBUG line above.
 
nsStyleSet.h:
  in definition of |dirty|, comment that there's one dirty bit per |sheetType|
 
 
The |ListStyleSheets| method you added to nsDocument should probably be
on nsPresShell instead.  That would be the equivalent of what it used to
do (rather than getting the document from the pres shell and then iterating
through all pres shells).  It also seems like something that shouldn't
really be on nsDocument.  (Requires changes to viewer and layout-debug.)
 
nsIPresShell.h:
  Don't |#ifdef| includes.
 
  I already mentioned s/StyleFrameConstruction()/FrameConstructor()/, I
  think.
 
nsPresShell.cpp:
  Should BeginUpdate and EndUpdate with UPDATE_STYLE call
  StyleSet()->[Begin|End]Update() ?  I think they should.  You might
  want to double-check with bz, though...
 
How about making nsCSSFrameConstructor::CreateContinuingFrame take only
a pres context argument and not a pres shell argument so you don't have
to use an extra variable every time you call it?
 
Need to go back to nsHTMLEditor.cpp and the stuff in nsPresShell that it
  calls.
Need to go back to removal of NotifyStyleSheetStateChanged.
Need to go back to nsChromeRegistry.cpp (both!) and the two new
  functions in nsPresShell.

	David Baron :dbaron:
	Comment 23 • 21 years ago

Comment on attachment 138318 [details] [diff] [review]
big patch

PresShell::AddOverrideStyleSheet and PresShell::RemoveOverrideStyleSheet both
need to call ReconstructStyleData (see what the old code in nsHTMLEditor did).
And while you're there, you should move the |mStylesHaveChanged = PR_FALSE|
from EndUpdate into ReconstructStyleData itself.

Also, you probably shouldn't remove the styleSheet->SetOwningDocument call
(and comment) in nsHTMLEditor.cpp.

A list of things you should test:
 * alternate stylesheet switching
 * change font size prefs (should take effect immediately)
 * change color prefs (should take effect immediately)
 * Using CSSOM to add and remove style sheets.
 * Adding and removing style / link elements from the document.
 * changing between views in editor
 * adding and removing stylesheets in editor

	David Baron :dbaron:
	Comment 24 • 21 years ago

> * Using CSSOM to add and remove style sheets.

Never mind.  The CSSOM doesn't support that.

	Brian Ryner (not reading) Assignee
	Comment 25 • 21 years ago

addressed all of dbaron's comments.  fixed a bug where enabling/disabling style
sheets didn't cause the appropriate updates to happen (because the rule
processors' RuleCascadeData is no longer valid).  changed rule node gc to not
gc the root node so we don't have to worry about re-creating mRuleWalker.

	Brian Ryner (not reading) Assignee
	Comment 26 • 21 years ago

this should have been in the above patch

	David Baron :dbaron:
	Comment 27 • 21 years ago

Comment on attachment 138558 [details] [diff] [review]
revised patch

r+sr=dbaron as long as you:
 * cvs add nsStyleSet.h :-)
 * cvs remove nsIStyleSet.h (that also isn't in this patch)
 * make the license on nsStyleSet.h match the license on nsIStyleSet.h /
nsStyleSet.cpp rather than being a new license (although feel free to add
yourself to the contributors list), since it's mainly moved code.

	Brian Ryner (not reading) Assignee
	Comment 28 • 21 years ago

-> me

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 21 years ago

I will try to read this tonight or worst-case tomorrow morning and answer at
least the question in comment 22

	Brian Ryner (not reading) Assignee
	Comment 30 • 21 years ago

Checked in.  I went ahead and added the BeginUpdate changes dbaron talked about,
but I'll make any changes that boris requests for that.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 21 years ago

What's the ordering on the style sheet arrays?  nsStyleSet.h should document
this.... nsIStyleSet.h used to. The comment on override sheets makes it sound
like that array is in the opposite order from the other arrays or something? 
But GatherRuleProcessors works the same way for all sheet types... and
AddDocStyleSheet assumes most-significant-first ordering too.

Does nsIDocument need the stdio.h?  I don't see that interface using FILE anywhere.

Why are we calling the style set's BeginUpdate/EndUpdate directly from the
document?  I would have thought the presshell would have been a more appropriate
place to do that, since all the presshells will get notified anyway?  For that
matter, why did similar code for StyleSheetApplicableStateChanged() move from
the presshell to the document?

The document viewer code could be made leak-safer by using nsAutoPtr<nsStyleSet>
in both places where we have a "owning" ref to the style set and using forget()
as needed (at the return point from the function, and after we give the set to
the document).  As things stand, it looks like if the presshell's Init() fails
we will leak.  Also, it would be good to document in nsStyleSet.h that the
presshell owns this beast and that no one else does.

I don't see anyone destroying nsPrintObject's mStyleSet. Did you mean to make
that member an nsAutoPtr?

Add a comment in nsRuleNode::Sweep explaining _why_ the root should not destroy
itself?

If BuildDefaultStyleData fails, don't you need to Destroy the mDefaultStyleData?
 We may have allocated the reset data and failed on the inherited data.  In any
case, the error there should probably be NS_ERROR_OUT_OF_MEMORY.

In nsStyleSet::ReplaceSheets, why not just call AppendObjects() instead?

In nsStyleSet::AddDocStyleSheet, why not 
  |NS_PRECONDITION(aSheet && aDocument, "null arg");| ?

BeginUpdate/EndUpdate have one issue as done in that patch -- updates can nest.
 It may be better (more efficient) to have a counter for mBatching that
BeginUpdate increments, EndUpdate decrements and takes action if it has reached
zero.  The other places can still just check !mBatching to get the info they
need.  Arguably, presshell should use a similar counter for its own style update
batching...  As things stand, a nested update will cause us to stop batching too
early.

In nsStyleSet::EndUpdate, I think parens around "1 << i" would improve readability.

In nsStyleSet::NotifyStyleContextDestroyed we want to assert that !deleted, no?
 As in, the assert will fire every single time with the current code?

Now that you've fixed my XXX comment in nsHTMLEditor::AddOverrideStyleSheet,
mind adjusting the comment in nsDocument::SetStyleSheetApplicableState accordingly?

In nsIPresShell, I'd think [OWNS] would be more accurate than [STRONG].

In nsIStyleFrameConstruction.h, the comment for nsFindFrameHint shouldn't
mention style set.

+  result = nsComponentManager::CreateInstance(kFrameSelectionCID, nsnull,
+                                              NS_GET_IID(nsIFrameSelection),
+                                              getter_AddRefs(mSelection));

As long as you're touching this code:

  mSelection = do_CreateInstance(kFrameSelectionCID, &result);

Why do we no longer need the hack to cause rule processor reinit in
PresShell::SetPreferenceStyleRules?  Because the style set doesn't care about
changes on the rule level (that's what I seem to recall, at least).

Arguably, AddOverrideSheet should prepend, not append -- that way override
sheets that are added later will override those added earlier.

PresShell::VerifyIncrementalReflow leaks the cloned set, no?  I know it's debug
code, but still... nsAutoPtr, please?

I'm sorry it took me so long to get to this.  Holiday travel is not conducive to
reviews... :(

	Brian Ryner (not reading) Assignee
	Comment 32 • 21 years ago

> Why do we no longer need the hack to cause rule processor reinit in
> PresShell::SetPreferenceStyleRules?  Because the style set doesn't care about
> changes on the rule level (that's what I seem to recall, at least).

Hm.  I tested changing the color prefs and it took effect immediately.  However,
the style set does need to know about style sheet enabling/disabling because it
causes the RuleCascadeData on the rule processor to become invalid.  Does
inserting/removing rules here cause the same situation?

	Brian Ryner (not reading) Assignee
	Comment 33 • 21 years ago

> Now that you've fixed my XXX comment in nsHTMLEditor::AddOverrideStyleSheet,
> mind adjusting the comment in nsDocument::SetStyleSheetApplicableState
> accordingly?

Hm, the comment still seems accurate to me.

> Arguably, AddOverrideSheet should prepend, not append -- that way override
> sheets that are added later will override those added earlier.

Perhaps, but I don't want to make this change right now because of the editor
stuff that could break (and editor is the only user of this).

	Brian Ryner (not reading) Assignee
	Comment 34 • 21 years ago

fix bz's review comments as described above and per irc conversation

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 21 years ago

> because it causes the RuleCascadeData on the rule processor to become invalid

Adding a rule certainly changes the RuleCascadeData... See
CSSRuleProcessor::ClearRuleCascades, which is called by rule insertions.  But
the RuleCascadeData seems to be internal state of the rule processor that the
style set doesn't really look at, to me... So removing that code like you did
_should_ be safe.  I think.  David?  Any idea?

> Hm, the comment still seems accurate to me.

Er.. editor no longer calls SetStyleSheetApplicableState directly like it used
to -- you removed that code in your patch.  The more I think about this, the
more I think that calling SetStyleSheetApplicableState() on the document for a
non-document sheet is really bogus.  Would it make sense to move the override
sheets into the documents sheets array a la inline style at some point?

> because of the editor stuff that could break 

Fair enough.   Makes sense.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 21 years ago

Comment on attachment 138606 [details] [diff] [review]
fix bz's comments

>Index: content/base/src/nsDocumentViewer.cpp

>+  nsresult rv = CreateStyleSet(mDocument, getter_Transfers(styleSet));

I'd use an nsAutoPtr in the CreateStyleSet impl too, in case someone decides to
add early error returns there....

>Index: content/base/src/nsStyleSet.cpp

>+    NS_ASSERTION(!deleted, "Root not must not be gc'd");

"root node"

r=bzbarsky with those nits.

	David Baron :dbaron:
	Comment 37 • 21 years ago

> fixed a bug where enabling/disabling style
> sheets didn't cause the appropriate updates to happen (because the rule
> processors' RuleCascadeData is no longer valid).

Could you remind me how you fixed this?  Perhaps a better fix would be to call
ClearRuleCascades() from CSSStyleSheetImpl::SetEnabled.

I'm again convinced (after being convinced and unconvinced) that the removal of
ClearRuleProcessors is safe, thanks to the code in CSSStyleSheetImpl::DidDirty,
except for the case of enabled state changing (see previous paragraph).

	Brian Ryner (not reading) Assignee
	Comment 38 • 21 years ago

I fixed that by causing the style set to empty its rule processor array and
recreate the rule processors when a stylesheet is enabled or disabled.

I think you're right that calling ClearRuleCascades from SetEnabled would do the
trick and we could remove the clearing of the rule processor array, and in fact,
remove the notification of the style set that the change has happened.

	Brian Ryner (not reading) Assignee
	Comment 39 • 21 years ago

This patch reverts back to using a raw pointer for the style set object, but
properly cleaning up in error cases.  It reduces the code size in
nsDocumentViewer.o by 1.1 KB on my machine.

	Brian Ryner (not reading) Assignee
	Comment 40 • 21 years ago

Comment on attachment 138795 [details] [diff] [review]
remove nsAutoPtr usage to reduce bloat

bz, what do you think?

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 21 years ago

Comment on attachment 138795 [details] [diff] [review]
remove nsAutoPtr usage to reduce bloat

r=me I guess, though I'm still confused about the huge size savings... do
nsCOMPtr and nsRefPtr have similar size issues??

	Brian Ryner (not reading) Assignee
	Comment 42 • 21 years ago

Perhaps.  One thing we found in looking at this code is that there will be at
least 2 extra function calls:  getter_Transfers ends up calling |assign(0)|
which calls |delete oldPtr;|.  We will also |delete mRawPtr| on destruction -
mRawPtr will be null since this is after .forget().

From some simple tests, it doesn't look like the compiler is able to optimize
away these calls to delete(NULL).  If we were to change the logic in assign() to:

if (oldPtr)
  delete oldPtr;

then it looks like it can optimize away the code if it can see that oldPtr will
be null.  Of course, that's at the expense of extra code in the cases where it
can't tell that it will be null.  Unfortunately there's no way to indicate to
the compiler what we really want, which is "If you can tell that oldPtr is null
at compile time, then don't call delete, otherwise just generate a call to
delete".  Of course, it seems like that would be smart behavior anyway and not
something we should have to ask for.

(note to self: check whether __builtin_constant_p() does what I just described
for gcc)

	Brian Ryner (not reading) Assignee
	Comment 43 • 21 years ago

> (note to self: check whether __builtin_constant_p() does what I just described
> for gcc)

Doesn't seem to, though I feel like it's a bug in gcc.  If it can determine that
mRawPtr is 0 such that it can optimize away an |if| block, then it seems like it
should return true for __builtin_constant_p().

	Brian Ryner (not reading) Assignee
	Comment 44 • 21 years ago

Actually, the number of added function calls will be higher than 2.  gcc will
inline the destructor for nsAutoPtr at each of the early return points in the
function.  This should be improved when the tree-ssa branch lands for gcc 3.5.

I checked in the bloat-reduction patch.

	Brian Ryner (not reading) Assignee
	Comment 45 • 21 years ago

Per http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13643 , we'll be able to make
gcc generate something more intelligent here hopefully in 3.5.