229371 - deCOMtaminate nsIPresContext

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

I'm using this as a tracking bug for all nsIPresContext deCOMtamination, though
this will happen in several patches.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch for Get/SetImageAnimationMode

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).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137941 [details] [diff] [review]
patch for Get/SetImageAnimationMode

r+sr=bzbarsky.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch for GetImageLoadFlags, GetLookAndFeel, GetIOService

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?

Comment 4

[Brian Ryner (not reading)]

Attachment: diff -w of the above

Comment 5

[Brian Ryner (not reading)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

None of the callers of nsPresContext::Init check the return value:

DocumentViewerImpl::InitInternal
DocumentViewerImpl::Show
nsPrintEngine::ReflowPrintObject
PresShell::VerifyIncrementalReflow

Comment 8

[Brian Ryner (not reading)]

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

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138021 [details] [diff] [review]
patch for GetImageLoadFlags, GetLookAndFeel, GetIOService

r+sr=bzbarsky with the changes mentioned in comment 8

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 138021 [details] [diff] [review]
patch for GetImageLoadFlags, GetLookAndFeel, GetIOService

Checked in.

Comment 11

[Brian Ryner (not reading)]

Attachment: patch for GetBaseURL, GetMedium, ClearStyleDataAndReflow, Resolve*, and GetXBLBindingURL

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.

Comment 12

[Brian Ryner (not reading)]

Oops, I missed the BaseURL changes in this patch...

Comment 13

[Brian Ryner (not reading)]

Attachment: review this one instead

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 15

[Brian Ryner (not reading)]

* 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.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> 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.

Comment 17

[Brian Ryner (not reading)]

Attachment: revised patch

Let's try this one

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 19

[Brian Ryner (not reading)]

Attachment: patch for ReParentStyleContext, GetDefaultFont, GetFontScaler, and default color getters

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.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 140119 [details] [diff] [review]
patch for ReParentStyleContext, GetDefaultFont, GetFontScaler, and default color getters

Checked in.

Comment 23

[Brian Ryner (not reading)]

Attachment: patch for LoadImage, StopImagesFor, Set/GetContainer, Set/GetLinkHandler, Set/GetVisibleArea, Set/GetPageDim, GetLanguage

Comment 24

[Brian Ryner (not reading)]

Attachment: diff -w of the above

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[Brian Ryner (not reading)]

Attachment: clean up GetDeviceContext

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.

Comment 27

[Brian Ryner (not reading)]

Attachment: diff -w of the above

This one should be easier to review, since removing null-checks of the device
context that often resulted in re-indenting large |if| blocks.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 140408 [details] [diff] [review]
clean up GetDeviceContext

r+sr=dbaron (although it was attachment 140409 [details] [diff] [review] that I read)

Comment 29

[Brian Ryner (not reading)]

Attachment: inline getters for TwipsToPixels, PixelsToTwips

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

Comment 30

[Daniel de Wildt]

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])

Comment 31

[Brian Ryner (not reading)]

Attachment: inline boolean getters and change storage to bitfields

This changes to using a bitfield for all of the boolean members on the
prescontext, and inlines the getters on nsIPresContext.

Comment 32

[Brian Ryner (not reading)]

Attachment: inline boolean getters and change storage to bitfields (diff -w)

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Brendan Eich [:brendan]]

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

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So passing "flags & SCOOBY_DOO" as a PRBool is a bug?

I wonder how many of those bugs we have

Comment 36

[Brendan Eich [:brendan]]

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

Comment 37

[Brian Ryner (not reading)]

I could change the setter here to do a (foo != PR_FALSE)... should I?

Comment 38

[Adam D. Moss]

!!foo is an easy way to force 0/1, though in some strange circles that's
considered ugly rather than beautiful. :)

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm happy if you just assert that the value is 0 or 1 (e.g. "!(v & ~1)")

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 41

[Brian Ryner (not reading)]

Attachment: take care of GetEventStateManager

Comment 42

[Brian Ryner (not reading)]

Attachment: diff -w of GetEventStateManager patch

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 44

[Brian Ryner (not reading)]

Attachment: inline pagination-related getters/setters

Comment 45

[Brian Ryner (not reading)]

Attachment: another patch

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.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 47

[Brian Ryner (not reading)]

Attachment: another patch

Fixes roc's suggestions.

Comment 48

[Brian Ryner (not reading)]

This is now done; see bug 253470.