nsIStyleContext doesn't need to be an XPCOM interface -- it should really be accessed only within the layout library. vtune information showed that one of the places with the highest overhead for virtual function calls was nsStyleContext::GetStyleData Also see bug 107100.

This depends on bug 107100 and may also depend on bug 104346, although probably not.

cc'ing myself

This includes all of dbaron's changes plus several more. A couple of points: - I wonder if we should consider making a (non-inline) method on nsIFrame to check style properties such as visibility. That would avoid the long-winded code pattern necessary to check the computed style. - I'm not sure what the best way is to remove the dependency in embedding/browser/webBrowser/nsContextMenuInfo.cpp, where it actually traverses the style context tree. Would this be equivalent to checking the computed style on each ancestor element?

Comment on attachment 109882 [details] [diff] [review] more dependency reduction Hmmm. So one of the reasons I never checked in my changes was because I didn't really like them all that much. I'm not sure what the rationale for the changes to inspector / nsInspectorCSSUtils is. The idea of the latter was to be as small as possible while still preventing linkage problems -- are there still such problems? (But, actually, won't decomifying introduce a whole new series of problems here?) Do you think this is the best way to go or do you think we're better off with a virtual method on nsIFrame (in addition to nsIFrame::GetStyleData)?

Oh, after reading your comment again, I'd agree with you about adding a method to nsIFrame. I think the right solution to nsContextMenu.cpp is to scrap the code and make it call the code that was *copied and pasted* to create that code (code from nsCSSRendering.cpp), despite my objections at the time.

We could certainly add nsIFrame::GetRuleNode and avoid the extra code in InspectorCSSUtils. Could I just use GetStyleData instead of doing the GetComputedStyle thing? I wasn't quite sure why you didn't do that in your original patch.

Actually, nevermind, that wouldn't work because the method is inlined. We could do something like: NS_IMETHOD GetStyleDataExternal(...) = 0; virtual nsRuleNode* GetRuleNode() = 0; Or we could #ifdef the GetStyleData declaration depending on whether you're in the layout code (for example, make it an inline call to GetStyleDataExternal from outside layout). Regarding the background image code, I couldn't find any code like that in nsCSSRendering. But, if we made the additions to nsIFrame I suggested above, it could walk up the frame tree and check the background style on each one without adding all the extra code needed for the computed style stuff.

... except that the inspector code has that hack for table frames where it needs to walk up the style context tree one node more. So I don't know that this can be solved entirely by using a GetRuleNode method on nsIFrame, unless we can assume something about the rule tree (e.g. that both style contexts would point to the same rule node).

Comment on attachment 109882 [details] [diff] [review] more dependency reduction Just thought I'd note that this introduces a major regression in editor - nsEditor::IsPreformatted fails because text nodes don't QI to nsIDOMElement.

nsIFrame::GetStyleDataExternal (virtual rather than inline) seems like a reasonable solution. When you ask me why I didn't do something in the first patch, remember that I said I didn't like that patch.

I got rid of the DOM computed style stuff in favor of an externally callable version of GetStyleData and a couple of new methods on nsIPresContext. This eliminates _all_ use of nsIStyleContext from outside content and layout.

Comment on attachment 110117 [details] [diff] [review] another try somehow i got two review requests here...

Comment on attachment 110117 [details] [diff] [review] another try ResolveStyleDataFor needs a much uglier name, since we don't want anyone to use it. How about ResolveStyleContextAndGetStyleData, plus a big ugly XXX comment telling people how silly it is? I don't see any callers for nsFrame::GetRuleNode. Could you remove it? In nsContextMenuInfo.cpp, I like to write the global GetStyleData as "::GetStyleData(...)" to show that it's a global function rather than a member function. (There was also one thing I thought you did because you wanted to avoid refcounting the style context outside of layout --- but there's really nothing wrong with passing around nsStyleContext* as an opaque pointer outside of layout, since when we deCOMify it, we'll stop having things (except for frames / the undisplayed map) refcount the style context. Right? But I can't find that anymore, so don't worry about it.) Did you check that there aren't any |GetStyleData| calls on frames in the content directory? Other than that, r+sr=dbaron.

> ResolveStyleDataFor needs a much uglier name, since we don't want anyone to use > it. How about ResolveStyleContextAndGetStyleData, plus a big ugly XXX comment > telling people how silly it is? Sure. I thought about just using the computed style API here, but I figured it might slow things down. Any thoughts on that? >I don't see any callers for nsFrame::GetRuleNode. Could you remove it? Done. At one point I had that as a method on nsIFrame to be used from inspector instead of adding GetRuleNodeForContent to nsInspectorCSSUtils. > (There was also one thing I thought you did because you wanted to avoid > refcounting the style context outside of layout --- but there's really nothing > wrong with passing around nsStyleContext* as an opaque pointer outside of > layout, since when we deCOMify it, we'll stop having things (except for frames > / the undisplayed map) refcount the style context. Right? But I can't find > that anymore, so don't worry about it.) Right, we should be able to pass it as an opaque pointer outside of layout, except that nsIFrame.h will have to #include nsStyleContext.h and we'll have to export it, because of the inline functions that use mStyleContext. Unless we remove those methods from nsIFrame, put them on nsFrame, and pass those around inside layout (which I think would be cleaner). Also, we should change all callers of nsIFrame::GetStyleContext in layout at that point to explicitly reference the style context if they need to (I really can't imagine anyone needs to keep a reference to it besides the frame). > Did you check that there aren't any |GetStyleData| calls on frames in the > content directory? There are, but what can we do about it until we merge content and layout? If any of these calls are performance-critical I guess I could hack in a #define _IMPL_NS_LAYOUT so that the calls continue to be inlined, but it would just be a temporary measure until content and layout are merged.

> > Did you check that there aren't any |GetStyleData| calls on frames in the > > content directory? > There are, but what can we do about it until we merge content and layout? If > any of these calls are performance-critical I guess I could hack in a #define > _IMPL_NS_LAYOUT so that the calls continue to be inlined, but it would just be a > temporary measure until content and layout are merged. Er, um, but nsStyleContext.cpp is right now in content, no? Will this cause problems on some platforms? Other than that, the previous comment is fine with me.

This fixes all the things dbaron mentioned, and also adds #defines to the prefix files for Mac CFM.

Comment on attachment 110374 [details] [diff] [review] final patch (hopefully) carrying over r=dbaron, requesting sr=bz

I'll take this since I think I have a handle on how to finish it up.

Comment on attachment 110374 [details] [diff] [review] final patch (hopefully) Change things like: > + if (NS_FAILED(frame->GetStyleData(eStyleStruct_Visibility, > + (const nsStyleStruct*) vis)) || to use ::GetStyleData classinfo could use computed style -- GetCSSPropertyValue("-moz-binding") followed by GetStringValue will give you the URL... in webshell/tests/viewer/nsBrowserWindow.cpp > + if (NS_SUCCEEDED(root->QueryInterface(NS_GET_IID(nsIFrameDebug), > + (void**) &fdbg))) { CallQueryInterface, please.

addesses bz's comments, except that I didn't change the nsDOMClassInfo thing because caillon says we get a 2% Txul win by not using the DOM computed style interface.

This patch was checked in. Further work depends on bug 106161.

This patch finishes the job. A couple of notes: - StyleContexts are still refcounted, but only when necessary (hopefully). Most functions no longer return an addref'd style context. nsStyleSet::Resolve* and ProbePseudoStyleFor do return a reference, since they may create a new style context and return the only reference to it. The corresponding functions on nsIPresContext pass through the owning reference to the caller. - This patch depends on nsRefPtr being enabled (see bug 104346). - I removed nsIFrame::GetStyle entirely; it had no callers except for one in commented-out code (which I changed, just to be thorough). And, a few things that I'm not fully decided on yet: - I avoided using the NS_ADDREF and NS_RELEASE macros for nsStyleContext raw pointers, but there's no reason they wouldn't work. Should I switch to using them? - Where should nsStyleContext.h live? - I'd like to remove any methods that take or return an nsStyleContext from public interfaces (such as nsIFrame) since nsStyleContext is now private to layout. I think the right way to do that would be to move the functionality into nsFrame, and make layout and content use nsFrame pointers instead of nsIFrame pointers. That's a major change though, and I'd like to keep it separate from this patch.

Comment on attachment 112119 [details] [diff] [review] remove nsIStyleContext this crashes on the pageload test. i'll attach a new patch once i've figured out the problem.

fixes the crash I mentioned. The only change is ensuring that |parentContext| isn't used uninitialized in nsFrameManager::ReResolveStyleContext().

Despite the vtune data mentioned at the beginning of this bug, this actually provides no measurable performance gain in the pageload test on Mac, Linux, or Windows. Sigh. Maybe this is no longer a hotspot? (I'd still like to get this in, of course)

I started to review attachment 112258 [details] [diff] [review]. Below are the comments I wrote. However, the majority of the comments are about a single issue, and I think it might make sense to decide what to do about that issue before I continue reviewing the rest of the patch. The issue is that this patch leaves a lot of |nsStyleContext**| out parameters, and some of them are |AddRef|ed pointers and some of them are non-|AddRef|ed pointers. I think the ideal would be never to use out parameters, and instead always use |nsStyleContext*| or |already_AddRefed<nsStyleContext>| return values. Slightly worse, but perhaps tolerable, would be to use out parameters to either always mean AddRefed (more consistent with current code) or never mean AddRefed (probably easier to do). However, I think having a mix as currently exists will be very confusing and lead to errors when maintaining this code later on. Anyway, the review comments I wrote on the first chunk of the patch (before writing the comment above) are: nsStyleContext::FindChildWithRules should return nsStyleContext* rather than nsresult. Actually, it would probably make more sense to return already_AddRefed<nsStyleContext>, since both callers either call this or NS_NewStyleContext, which does addref. (And, actually, it might make sense to change NS_NewStyleContext to return already_AddRefed<nsStyleContext> to clarify the model and avoid any functions taking out parameters that do AddRef.) |nsStyleContext::GetStyle| and |nsIFrame::GetStyle| can just be removed. There aren't any callers. |nsStyleContext::GetBorderPaddingFor| and |nsStyleContext::SetStyle| should return |void|. |nsStyleContext::CalcStyleDifference| should return |nsChangeHint| (there's one caller, and it initializes the parameter to |NS_STYLE_HINT_NONE|). Removing the ADDREF in StyleSetImpl::ReParentStyleContext seems wrong. The other codepaths AddRef. Perhaps this should return already_AddRefed<nsStyleContext> ? in content/html/style/src/nsComputedDOMStyle.cpp, you should prefer |dont_AddRef| to |getter_AddRefs|. (I don't like the overloading of |getter_AddRefs|.) |nsInspectorCSSUtils::GetStyleContextForContent| looks like it should return already_AddRefed<nsStyleContext>

I got rid of all nsStyleContext out parameters and instead use nsStyleContext* and already_AddRefed<nsStyleContext> return values. In the process I found a leak and a double-release (which shouldn't have compiled; it was being called on a nsRefPtr).

Comment on attachment 113967 [details] [diff] [review] remove nsIStyleContext, part 1 dbaron, can you review this and attachment 113969 [details] [diff] [review]?

Comment on attachment 113969 [details] [diff] [review] remove nsIStyleContext, part 2 This checkin have added 4 "may be used uninitialized" warnings on brad TBox: +layout/html/style/src/nsCSSFrameConstructor.cpp:10646 + `nsStyleContext*styleContext' might be used uninitialized in this function + +layout/html/style/src/nsCSSFrameConstructor.cpp:13388 + `nsStyleContext*styleContext' might be used uninitialized in this function + +layout/html/style/src/nsCSSFrameConstructor.cpp:13687 + `nsStyleContext*styleContext' might be used uninitialized in this function + +layout/html/style/src/nsCSSFrameConstructor.cpp:2869 + `nsStyleContext*styleContext' might be used uninitialized in this function Some of them are just a case of compiler not being smart enough, but some are real issues, including dereferencing an uninitialized pointer in certain cases.

This has been checked in.

> This has been checked in. Yeah, but the warnings are still there!

I went ahead and checked in a patch to initialize all of these to null.