Closed
Bug 229371
Opened 21 years ago
Closed 20 years ago
deCOMtaminate nsIPresContext
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: bryner)
Details
(Keywords: perf)
Attachments
(16 files, 3 obsolete files)
I'm using this as a tracking bug for all nsIPresContext deCOMtamination, though this will happen in several patches.
Assignee | ||
Comment 1•21 years ago
|
||
This moves the image animation mode onto nsIPresContext and inlines the getter. In order to accomodate this, I had to introduce a new boolean that's set by the Print and PrintPreview prescontext subclasses to ensure that the animation mode is never changed from DontAnim. The memory usage here isn't really ideal but I'm planning to clean up some of this later using bitfields (though a few bytes per document don't concern me tremendously).
Assignee | ||
Updated•21 years ago
|
Attachment #137941 -
Flags: superreview?(bz-vacation)
Attachment #137941 -
Flags: review?(bz-vacation)
Comment 2•21 years ago
|
||
Comment on attachment 137941 [details] [diff] [review] patch for Get/SetImageAnimationMode r+sr=bzbarsky.
Attachment #137941 -
Flags: superreview?(bz-vacation)
Attachment #137941 -
Flags: superreview+
Attachment #137941 -
Flags: review?(bz-vacation)
Attachment #137941 -
Flags: review+
Assignee | ||
Comment 3•21 years ago
|
||
Here's what I changed in this patch: GetImageLoadFlags: removed entirely, no one uses this GetLookAndFeel: Made failure to obtain the LookAndFeel service cause failure to be returned from Init(), and don't bother null-checking it after that. Moved the member variable to nsIPresContext and inlined the getter. Changed nsHTMLObjectResizer caller to simply use the service manager rather than jumping through hoops to get a PresContext. Changed nsMenuPopupFrame to obtain L&F from the prescontext instead of using CreateInstance (it's supposed to be a singleton, anyway). GetIOService: Removed from nsIPresContext, moved caching to nsImageFrame, the only user of this. Regarding the LookAndFeel service caching -- I'd love it if we could cache this globally (i.e. a static class variable) instead of per-prescontext. But as it is now, we wouldn't be able to make the getter non-virtual then, since the caller would need to directly reference the global in gklayout (and there are callers outside of gklayout). We could do this if we made callers outside of gklayout go through the service manager. Boris, any thoughts?
Assignee | ||
Comment 4•21 years ago
|
||
Assignee | ||
Comment 5•21 years ago
|
||
Comment on attachment 138021 [details] [diff] [review] patch for GetImageLoadFlags, GetLookAndFeel, GetIOService Boris, if you're just looking at this in your queue, be sure to see my question in comment 3 as well.
Attachment #138021 -
Flags: superreview?(bz-vacation)
Attachment #138021 -
Flags: review?(bz-vacation)
Comment 6•21 years ago
|
||
David, did you have further plans for GetIOService, or is this move to a static cached in nsImageFrame OK with you? As for the look and feel, the only non-gklayout users are native theme stuff in gfx and the resizer in htmleditor. The latter should just use the service manager like you did. The gfx code could be somewhat perf-sensitive (on pages with a lot of form controls or something); perhaps we could just cache the pointer there separately? Ultimately, I'd think that linking gfx into gklayout is the way to go in general... I'll try to get to this review ASAP, but that may not be till after the 15th; I have to get some non-Mozilla work done by that deadline.
None of the callers of nsPresContext::Init check the return value: DocumentViewerImpl::InitInternal DocumentViewerImpl::Show nsPrintEngine::ReflowPrintObject PresShell::VerifyIncrementalReflow
Assignee | ||
Comment 8•21 years ago
|
||
I changed the callers in my tree to return an error if PresContext::Init fails, and more importantly, to not hang onto the pres context, since we've made the assumption elsewhere that it will not be used if Init() fails.
Comment 9•21 years ago
|
||
Comment on attachment 138021 [details] [diff] [review] patch for GetImageLoadFlags, GetLookAndFeel, GetIOService r+sr=bzbarsky with the changes mentioned in comment 8
Attachment #138021 -
Flags: superreview?(bz-vacation)
Attachment #138021 -
Flags: superreview+
Attachment #138021 -
Flags: review?(bz-vacation)
Attachment #138021 -
Flags: review+
Assignee | ||
Comment 10•21 years ago
|
||
Comment on attachment 138021 [details] [diff] [review] patch for GetImageLoadFlags, GetLookAndFeel, GetIOService Checked in.
Assignee | ||
Comment 11•21 years ago
|
||
This patch: - removes GetBaseURL and stops holding onto it. no one accesses this through the pres context. - Renames GetMedium() to Medium() and makes it an inline, non-addreffing getter. - Removes ClearStyleDataAndReflow(). All of the callers are within layout and can call styleSet->ClearStyleData() and then shell->StyleChangeReflow() themselves. - Removes virtual methods ResolveStyleContextFor, ResolveStyleContextForNonElement, ResolvePseudoStyleContextFor, ResolvePseudoStyleWithComparator, and ProbePseudoStyleContextFor. All of the callers are within layout and can call these methods directly on the style set without going through any virtual methods. - Changed NS_IMETHOD to |virtual nsresult| for GetXBLBindingURL. Not much else to be done here, it needs to be able to return a failure code.
Assignee | ||
Updated•21 years ago
|
Attachment #139686 -
Flags: superreview?(dbaron)
Attachment #139686 -
Flags: review?(dbaron)
Assignee | ||
Comment 12•21 years ago
|
||
Oops, I missed the BaseURL changes in this patch...
Assignee | ||
Updated•21 years ago
|
Attachment #139686 -
Flags: superreview?(dbaron)
Attachment #139686 -
Flags: review?(dbaron)
Assignee | ||
Comment 13•21 years ago
|
||
Attachment #139686 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #139689 -
Flags: superreview?(dbaron)
Attachment #139689 -
Flags: review?(dbaron)
I think this would be simplified a bit by doing two things: * adding a public StyleSet() member function to the pres context (which I should have objected to you removing in bug 64116) -- if we want to eliminate the pres shell eventually, why add lots of cx->PresShell()->StyleSet()->... when it could just be cx->StyleSet() ? * adding a private PresContext() member function to the style set that does mRuleTree->GetPresContext(), and eliminating the aPresContext parameter you added to the style resolution functions It also seems like ClearStyleDataAndReflow was called in enough places that it deserves its own function (and, likewise, this change seems like it would make eliminating the pres shell harder). It's worth commenting in nsIPresContext.h that the mMedium pointer is weak and owned by an atom list. In nsCSSFrameConstructor::MaybeRecreateFramesForContent, you may as well use the new GetFrameManager() accessor. What requires that you add webshell to REQUIRES?
Assignee | ||
Comment 15•21 years ago
|
||
* adding a public StyleSet() member function to the pres context (which I should have objected to you removing in bug 64116) -- if we want to eliminate the pres shell eventually, why add lots of cx->PresShell()->StyleSet()->... when it could just be cx->StyleSet() ? I think there may have been an include ordering problem in an early iteration of my 64116 patch that caused me to remove that accessor. So I don't really have a strong case for not having it, other than we'd need to make sure the callers are being efficient (i.e. use shell->StyleSet() if they already have the shell pointer, rather than cx->StyleSet()). > * adding a private PresContext() member function to the style set that does > mRuleTree->GetPresContext(), and eliminating the aPresContext parameter you > added to the style resolution functions I didn't add it, it was already there. > It also seems like ClearStyleDataAndReflow was called in enough places that it > deserves its own function (and, likewise, this change seems like it would make > eliminating the pres shell harder). I can go ahead and add that back. > It's worth commenting in nsIPresContext.h that the mMedium pointer is weak and > owned by an atom list. > In nsCSSFrameConstructor::MaybeRecreateFramesForContent, you may as well use the > new GetFrameManager() accessor. Ok, sure. > What requires that you add webshell to REQUIRES? nsLinkState (nsILinkHandler.h), in nsIStyleRuleProcessor.h included via nsStyleSet.h.
> I didn't add it, it was already there.
It was there on the implementation, but not the callers, since they went through
the pres context wrapper.
Assignee | ||
Updated•21 years ago
|
Attachment #139689 -
Flags: superreview?(dbaron)
Attachment #139689 -
Flags: review?(dbaron)
Assignee | ||
Comment 17•21 years ago
|
||
Let's try this one
Assignee | ||
Updated•21 years ago
|
Attachment #139689 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #139969 -
Flags: superreview?(dbaron)
Attachment #139969 -
Flags: review?(dbaron)
Comment on attachment 139969 [details] [diff] [review] revised patch >Index: layout/html/tests/TestInlineFrame.cpp >+ styleContext = = presContext->StyleSet()->ResolveStyleFo(b, nsnull); >Index: layout/mathml/base/src/nsMathMLmactionFrame.cpp >+ newStyleContext = aPresContext->StyleSet()-> >+ ResolveStyleFor(aContent, parentStyleContext); and the weird indentation here (tabs?) and r+sr=dbaron
Attachment #139969 -
Flags: superreview?(dbaron)
Attachment #139969 -
Flags: superreview+
Attachment #139969 -
Flags: review?(dbaron)
Attachment #139969 -
Flags: review+
Assignee | ||
Comment 19•21 years ago
|
||
This patch moves several member variables (default colors, font scaler, focus ring width) up to nsIPresContext and inlines the getters. I removed ReParentStyleContext and made the few callers call the frame manager directly. I removed several setters that are not used anywhere. And GetDefaultFont now just returns a nsFont pointer (or null if the value is out of range). I purposely skipped some of the boolean getters because I'd like to do those later, all at once, and put them into a bitfield. AllocateFromShell/FreeToShell will also come later.
Assignee | ||
Updated•21 years ago
|
Attachment #140119 -
Flags: superreview?(dbaron)
Attachment #140119 -
Flags: review?(dbaron)
Comment on attachment 140119 [details] [diff] [review] patch for ReParentStyleContext, GetDefaultFont, GetFontScaler, and default color getters >Index: layout/base/public/nsIPresContext.h >+ /** >+ * Get the default font correponding to the given ID. This data is >+ * read-only, you must copy the font to modify it. >+ */ >+ virtual const nsFont* GetDefaultFont(PRUint8 aFontID) const = 0; s/This data/This object/, unless you want to rewrite the entire sentence in the plural. >Index: layout/base/src/nsPresContext.cpp > case kGenericFont_serif: It would be nice if these IDs were consecutive so you could use an array. Does anything rely on a bitfield? That's better left to a later patch, anyway. Up to here: >Index: layout/html/base/src/nsFirstLetterFrame.cpp
Comment on attachment 140119 [details] [diff] [review] patch for ReParentStyleContext, GetDefaultFont, GetFontScaler, and default color getters r+sr=dbaron with comment change from comment 20
Attachment #140119 -
Flags: superreview?(dbaron)
Attachment #140119 -
Flags: superreview+
Attachment #140119 -
Flags: review?(dbaron)
Attachment #140119 -
Flags: review+
Assignee | ||
Comment 22•21 years ago
|
||
Comment on attachment 140119 [details] [diff] [review] patch for ReParentStyleContext, GetDefaultFont, GetFontScaler, and default color getters Checked in.
Assignee | ||
Comment 23•21 years ago
|
||
Assignee | ||
Comment 24•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #140351 -
Flags: superreview?(dbaron)
Attachment #140351 -
Flags: review?(dbaron)
Comment on attachment 140351 [details] [diff] [review] patch for LoadImage, StopImagesFor, Set/GetContainer, Set/GetLinkHandler, Set/GetVisibleArea, Set/GetPageDim, GetLanguage >Index: layout/base/src/nsPresContext.cpp >@@ -706,8 +707,7 @@ void > nsPresContext::UpdateCharSet(const char* aCharSet) > { > if (mLangService) { >- mLangService->LookupCharSet(aCharSet, >- getter_AddRefs(mLanguage)); >+ mLangService->LookupCharSet(aCharSet, &mLanguage); // addrefs Do an NS_IF_RELEASE(mLanguage) first, since this can be called multiple times. with that, r+sr=dbaron.
Attachment #140351 -
Flags: superreview?(dbaron)
Attachment #140351 -
Flags: superreview+
Attachment #140351 -
Flags: review?(dbaron)
Attachment #140351 -
Flags: review+
Assignee | ||
Comment 26•21 years ago
|
||
Return failure from nsPresContext::Init if a null device context is given, and thereafter assume that the device context is non-null. So, the non-refcounting GetDeviceContext() now becomes DeviceContext(). I changed all callers to use that version and removed the refcounting version of the getter.
Assignee | ||
Comment 27•21 years ago
|
||
This one should be easier to review, since removing null-checks of the device context that often resulted in re-indenting large |if| blocks.
Assignee | ||
Updated•21 years ago
|
Attachment #140408 -
Flags: superreview?(dbaron)
Attachment #140408 -
Flags: review?(dbaron)
Comment on attachment 140408 [details] [diff] [review] clean up GetDeviceContext r+sr=dbaron (although it was attachment 140409 [details] [diff] [review] that I read)
Attachment #140408 -
Flags: superreview?(dbaron)
Attachment #140408 -
Flags: superreview+
Attachment #140408 -
Flags: review?(dbaron)
Attachment #140408 -
Flags: review+
Assignee | ||
Comment 29•21 years ago
|
||
Now that the getters for the t2p/p2t ratio are inlined and no longer use an out-param, we can easily inline the getters on the pres context like this. I think we should go ahead and keep these getters rather than asking callers to go to the device context, because the terminology appears to be slightly different (and I don't pretend to understand it). This was generated using the following script: grep -rl GetTwipsToPixels .|xargs perl -pi -e "s/(\s*)(\S*)GetTwipsToPixels\( *&([^ \)]*) *\)/\1\3 = \2TwipsToPixels\(\)/g" (and the same for GetPixelsToTwips) and then fixing the following files by hand: substitution didn't come out right: accessible/src/atk/nsAccessibleText.cpp accessible/src/msaa/nsTextAccessibleWrap.cpp layout/html/table/src/nsTableCellFrame.cpp didn't match pattern: content/html/content/src/nsGenericHTMLElement.cpp content/events/src/nsDOMEvent.cpp dom/src/base/nsGlobalWindow.cpp
Assignee | ||
Updated•21 years ago
|
Attachment #140643 -
Flags: superreview?(dbaron)
Attachment #140643 -
Flags: review?(dbaron)
Comment 30•21 years ago
|
||
Bug 233240 has a crash and I think the reason is that 'handler' contains invalid data in http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&mark=3112#3086 Is it possible that the patch from feb 01 is the reason for that bug? (attachment 140351 [details] [diff] [review])
Attachment #140643 -
Flags: superreview?(dbaron)
Attachment #140643 -
Flags: superreview+
Attachment #140643 -
Flags: review?(dbaron)
Attachment #140643 -
Flags: review+
Assignee | ||
Comment 31•21 years ago
|
||
This changes to using a bitfield for all of the boolean members on the prescontext, and inlines the getters on nsIPresContext.
Assignee | ||
Comment 32•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #141527 -
Flags: superreview?(roc)
Attachment #141527 -
Flags: review?(roc)
Is it safe to assign a PRBool to an "unsigned mField:1"? What if the PRBool value is 12 or something like that?
Comment 34•21 years ago
|
||
PRBool values should be PR_FALSE or PR_TRUE, no others. If we have bugs where 12 is used for true, we should smoke 'em out fast. Maybe an assertion using & 1? But I'm not worried. /be
So passing "flags & SCOOBY_DOO" as a PRBool is a bug? I wonder how many of those bugs we have
Comment 36•21 years ago
|
||
roc: yes, that's a bug. (flags & SCOOBY_DOO) != 0 is the one true way. dbaron has pointed out such bugs in the past during reviews, and I recall doing the same, so there's a good chance some got checked in. Assert away! /be
Assignee | ||
Comment 37•21 years ago
|
||
I could change the setter here to do a (foo != PR_FALSE)... should I?
Comment 38•21 years ago
|
||
!!foo is an easy way to force 0/1, though in some strange circles that's considered ugly rather than beautiful. :)
I'm happy if you just assert that the value is 0 or 1 (e.g. "!(v & ~1)")
Comment on attachment 141527 [details] [diff] [review] inline boolean getters and change storage to bitfields (diff -w) r+sr=roc if you add the assertion. Or even if you don't, really.
Attachment #141527 -
Flags: superreview?(roc)
Attachment #141527 -
Flags: superreview+
Attachment #141527 -
Flags: review?(roc)
Attachment #141527 -
Flags: review+
Assignee | ||
Comment 41•20 years ago
|
||
Assignee | ||
Comment 42•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #142284 -
Flags: superreview?(roc)
Attachment #142284 -
Flags: review?(roc)
Comment on attachment 142284 [details] [diff] [review] diff -w of GetEventStateManager patch yum. Sure would be nice if the event state manager could be a direct member. The strong-ref holding across potential teardown events would have to be moved to the prescontext. Something to think about later.
Attachment #142284 -
Flags: superreview?(roc)
Attachment #142284 -
Flags: superreview+
Attachment #142284 -
Flags: review?(roc)
Attachment #142284 -
Flags: review+
Assignee | ||
Comment 44•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #142819 -
Flags: superreview?(roc)
Attachment #142819 -
Flags: review?(roc)
Attachment #142819 -
Flags: superreview?(roc)
Attachment #142819 -
Flags: superreview+
Attachment #142819 -
Flags: review?(roc)
Attachment #142819 -
Flags: review+
Assignee | ||
Comment 45•20 years ago
|
||
This one takes care of: AllocateFromShell FreeToShell GetCachedIntPref GetLanguageSpecificTransformType (my hands hurt after typing that) BidiEnabled SetBidiEnabled I also fixed up comments for GetCachedBoolPref, and reordered the members a bit to (hopefully) improve packing.
Assignee | ||
Updated•20 years ago
|
Attachment #144677 -
Flags: superreview?(roc)
Attachment #144677 -
Flags: review?(roc)
+ nsresult AllocateFromShell(size_t aSize, void** aResult) Can't you just return the result? It looks like FrameArena::AllocateFrame always returns NS_OK. + nsresult FreeToShell(size_t aSize, void* aFreeChunk) This seems to always return NS_OK too, so should probably just be void.
Assignee | ||
Updated•20 years ago
|
Attachment #144677 -
Flags: superreview?(roc)
Attachment #144677 -
Flags: review?(roc)
Assignee | ||
Comment 47•20 years ago
|
||
Fixes roc's suggestions.
Attachment #144677 -
Attachment is obsolete: true
Attachment #145849 -
Flags: superreview+
Attachment #145849 -
Flags: review+
Assignee | ||
Comment 48•20 years ago
|
||
This is now done; see bug 253470.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•