Closed Bug 240543 Opened 20 years ago Closed 20 years ago

Hide some nsIPref bloatiness in nsContentUtils

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

Details

Attachments

(2 files)

Add pref methods to nsContentUtils.
199.29 KB, patch
caillon
: review+
peterv
: superreview+
Details | Diff | Splinter Review
diff -w for review
178.07 KB, patch
Details | Diff | Splinter Review 
Using the pref API (same goes for other frequently used XPCOM APIs) ends up
generating more code than we'd ideally want, and I decided to attempt to hide
some of this for the pref API in nsContentUtils, and it turned out to be a win.
Not sure if we want to do this here, or in a more generic place, or at all. But
I'll attach a patch, and we'll go from there.
This saves us ~12k of code on Linux (RedHat 9).
I have actually been thinking abuot this, and the solution I came up with is to
utlize function overloading and (for getComplexValue) templatization to do
things like:

  const char prefName[] = "foo.bar";
  PRBool prefVal;
  nsresult rv = do_GetPrefValue(prefName, &prefVal);

  const char prefLocalName[] = "foo.bar.blah";
  nsCOMPtr<nsIPrefLocalizedString> prefLocalVal;
  rv = do_GetPrefValue(prefLocalName, getter_AddRefs(prefLocalVal));

etc.

That header could be incorporated into libpref, so this can be used by more than
just content (there are quite a few places which define their own helpers, which
kind of sucks).
(In reply to comment #2)
>   rv = do_GetPrefValue(prefLocalName, getter_AddRefs(prefLocalVal));

CallGetPrefValue, maybe? do_Get* usually returns the value and assigns an error
code into its parameter...
Whatever.  I was illustrating the idea, and didn't really focus on the naming. 
We can argue naming when a patch is in place.  I haven't actually started this;
I was just pondering it this afternoon.  Funny that jst filed a bug on more or
less what I was thinking about.

I can whip up a patch if this idea is feasible.
I presume that your do_GetPrefValue() (or whatever you call them) methods would
be inline? If so, you're not gaining much, if any, footprint by introducing such
methods.

I'd love to see something like this in a common places (e.g. XPCOM) where we
could have a non-bloaty API for something as widely used as prefs, but I didn't
want to either fight the battle for pushing this into XPCOM, or writing another
library for pref utilities that the world could link with. But maybe it's worth
doing?

Personally I like my API, not just because I wrote it myself :-), but because
the methods return the value of the pref as the return value, and it hides the
nsresult error codes that nobody really cares about when using this API in the
first place. If someone truly cares about those error codes, those could be
returned through an out param if needed, but not a single caller in content,
layout, or dom, really cared about the return value.
They don't have to be inline (which is probably good to avoid the virtual call
overhead), but there would be no need to get the prefbranch, etc. yourself. 
That would be handled by libpref which would have a singleton pref service ready
to use, so it knows there is always a pref service to use (and we can avoid the
runtime check you are doing).  And it would also be much nicer to use with
GetComplexValue, for instance.

Think:

  template<class PrefInterface>
  nsresult CallGetPrefValue(const char* aPrefName, PrefInterface **aValue)
  {
    return sPrefBranch->GetComplexValue(aPrefName, NS_GET_IID(PrefInterface),
                                        aValue);
  }

The downside I guess though is that due to our PRTypes, we can't really overload
ints and bools, or nscolors if we wanted to do that too.  So maybe on second
thought, having one function across the board isn't that great, and people
really do need to call GetBoolPref, GetIntPref, etc.  though that is a bit sucky.

Anyway, I hate to see yet another set of helpers here.  It seems that content is
the only module without a helper nowadays, and that is The Real Suck(tm).
So you're proposing that everyone links with libpref then?
Comment on attachment 146134 [details] [diff] [review]
Add pref methods to nsContentUtils.

Ok, seems like people like this idea in general, so I'm requesting reviews for
this patch. If we choose to take this even further later on, that's fine, it'll
be easy enough to remove the helpers from nsContentUtils and replace callers
with the new helpers at that point.
Attachment #146134 - Flags: superreview?(peterv)
Attachment #146134 - Flags: review?(caillon)
Jst, how about a diff -w since you seem to have quite a few branches which have
their indentation changed?
(Also you may want to run page load tests with this patch.  I had some perf
issues when I tried to touch some of these files with pref work at one point.)

    
    

    
  
Comment on attachment 146134 [details] [diff] [review]
Add pref methods to nsContentUtils.

Do you also want |static nsIPrefBranch* nsContentUtils::GetPrefBranch() {
return sPrefBranch; }| ? since there are some places which still want the
prefbranch to install observers or something.  They could then just use the
reference from nsContentUtils instead of using a strong ref themselves.


>Index: content/events/src/nsEventStateManager.cpp

>   rv = getPrefBranch();
> 
>   if (NS_SUCCEEDED(rv)) {
>     if (sESMInstanceCount == 1) {
>       mPrefBranch->GetBoolPref("nglayout.events.dispatchLeftClickOnly",
>                                &sLeftClickOnly);
> 
>       mPrefBranch->GetIntPref("ui.key.generalAccessKey",

Are you intentionally leaving these pref callers alone?


>@@ -2033,25 +2029,23 @@ nsEventStateManager::PostHandleEvent(nsI
> 
>       mPrefBranch->GetIntPref(actionKey.get(), &action);
>       mPrefBranch->GetBoolPref(sysNumLinesKey.get(),
>                                &useSysNumLines);

And these?

>       if (useSysNumLines) {
>         numLines = msEvent->delta;
>         if (msEvent->scrollFlags & nsMouseScrollEvent::kIsFullPage)
>           action = MOUSE_SCROLL_PAGE;
>       }
>-      else
>-        {
>+      else {
>           nsCAutoString numLinesKey(baseKey);
>           numLinesKey.Append(numlinesslot);
> 
>-          mPrefBranch->GetIntPref(numLinesKey.get(),
>-                                  &numLines);
>+        mPrefBranch->GetIntPref(numLinesKey.get(), &numLines);

Here too?


- In HTMLContentSink::Init()

>+  // default to -1

This comment is redundant.  The code is self explanatory, and you don't make
this comment anywhere else.  Just remove the comment.

>+  mBackoffCount =
>+    nsContentUtils::GetIntPref("content.notify.backoffcount", -1);
> 



>Index: layout/base/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/layout/base/src/Makefile.in,v
>retrieving revision 1.87
>diff -u -9 -p -w -r1.87 Makefile.in
>--- layout/base/src/Makefile.in	18 Apr 2004 14:30:22 -0000	1.87
>+++ layout/base/src/Makefile.in	21 Apr 2004 05:21:00 -0000
>@@ -53,18 +53,20 @@ REQUIRES	= xpcom \
> 		  view \
> 		  locale \
> 		  webshell \
> 		  necko \
> 		  uconv \
> 		  pref \
> 		  uriloader \
> 		  docshell \
> 		  imglib2 \
>+		  js \
>+		  xpconnect \
>Index: layout/html/style/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/style/src/Makefile.in,v
>retrieving revision 1.60
>diff -u -9 -p -w -r1.60 Makefile.in
>--- layout/html/style/src/Makefile.in	18 Apr 2004 14:30:27 -0000	1.60
>+++ layout/html/style/src/Makefile.in	21 Apr 2004 05:21:12 -0000
>@@ -53,18 +53,20 @@ REQUIRES	= xpcom \
> 		  locale \
> 		  view \
> 		  necko \
> 		  webshell \
> 		  pref \
> 		  docshell \
> 		  plugin \
> 		  xuldoc \
> 		  imglib2 \
>+		  js \
>+		  xpconnect \


Clear the above REQUIRES changes with dbaron?


>Index: layout/base/src/nsPresContext.cpp

>+  // NOTE! nsPresContext::operator new() zeroes out all members, so don't
>+  // bother initializing members to 0.
> 
> nsPresContext::nsPresContext()
>   : mDefaultVariableFont("serif", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL,
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(12)),
>     mDefaultFixedFont("monospace", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL,
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(10)),
>     mDefaultSerifFont("serif", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL,
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(12)),
>     mDefaultSansSerifFont("sans-serif", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL,
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(12)),
>     mDefaultMonospaceFont("monospace", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL,
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(10)),
>     mDefaultCursiveFont("cursive", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL, 
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(12)),
>     mDefaultFantasyFont("fantasy", NS_FONT_STYLE_NORMAL, NS_FONT_VARIANT_NORMAL, 
>       NS_FONT_WEIGHT_NORMAL, 0, NSIntPointsToTwips(12))
> {
>+
>+  // NOTE! nsPresContext::operator new() zeroes out all members, so don't
>+  // bother initializing members to 0.

You must really like that comment, eh?	How about picking one place for it? 
:-)


Again, test your changes.  The prefs stuff in nsPresContext get accessed a LOT
:-( so you may possibly hurt performance.


And please find a better place to put these wrappers.  I hate that we have to
put them in every library.

        Attachment #146134 -
        Flags: review?(caillon) → review+

    
    

    
  
Oh, one more thing I have to add is that your patch removes a bunch of
nsIObserver callers which are the new, preferred, frozen way of implementing
pref observers and replaces them with old PR_CALLBACK deprecated way of doing
them, which adds more users nsIPref.... I've sort of given up on us ever using
the new, frozen APIs.  But others might not have.  It might be good for you to
not do that.... but then again, it seems to be the case that we won't be getting
rid of nsIPref for quite some time, so maybe it doesn't matter.... Just some
food for your thought.

    
    

    
  
Just a question.... is the new API frozen?  If not, could we extend it with the
ability to do static function callbacks (noscript) and just move over consumers
to those as needed?  Because using nsIObserver is a major PITA in many cases...

    
    

    
  
Yes, its frozen.  La la la.

    
    

    
  
> Yes, its frozen.  La la la.

Actually, no.  It is not.  nsIPrefBranch is frozen.  Observer stuff is on
nsIPrefBranchInternal, which is most distinctly not frozen (though perhaps
should get so?).

    
    

    
  
Comment on attachment 146134 [details] [diff] [review]
Add pref methods to nsContentUtils.

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

> +NS_IMETHODIMP
> +DocumentViewerImpl::GetDefaultCharacterSet(nsACString& aDefaultCharacterSet)
>  {
>    NS_ENSURE_STATE(mContainer);
>  
>    if (mDefaultCharacterSet.IsEmpty())
>    {

> +    const nsAdoptingString& defCharset =
> +      nsContentUtils::GetLocalizedStringPref("intl.charset.default");
>  
>      if (!defCharset.IsEmpty())
>        CopyUCS2toASCII(defCharset, mDefaultCharacterSet);

Make this CopyUTF16toASCII while you're in the neighbourhood :-).

> Index: content/html/document/src/nsHTMLContentSink.cpp
> ===================================================================

> @@ -2149,83 +2146,64 @@ HTMLContentSink::Init(nsIDocument* aDoc,

> -  mBackoffCount = -1; // never
> -  if (prefBranch) {
> -    prefBranch->GetIntPref("content.notify.backoffcount", &mBackoffCount);
> -  }
> +  // default to -1

Don't lose the "never" comment.

> Index: content/html/document/src/nsHTMLDocument.cpp
> ===================================================================

>  PRBool
>  nsHTMLDocument::UseWeakDocTypeDefault(PRInt32& aCharsetSource,
>                                        nsACString& aCharset)

> +
> +  const nsAdoptingString& defCharset =
> +    nsContentUtils::GetLocalizedStringPref("intl.charset.default");
> +
> +  if (!defCharset.IsEmpty()) {
> +    CopyUCS2toASCII(defCharset, aCharset);

CopyUTF16toASCII

>  void
>  nsHTMLDocument::StartAutodetection(nsIDocShell *aDocShell, nsACString& aCharset,
>                                     const char* aCommand)

>    if(! gInitDetector) {

> +    const nsAdoptingString& detector_name =
> +      nsContentUtils::GetLocalizedStringPref("intl.charset.detector");
> +
> +    if(!detector_name.IsEmpty()) {
> +      PL_strncpy(g_detector_contractid, NS_CHARSET_DETECTOR_CONTRACTID_BASE,
> +                 DETECTOR_CONTRACTID_MAX);
> +      PL_strncat(g_detector_contractid,
> +                 NS_ConvertUTF16toUTF8(detector_name).get(),
> +                 DETECTOR_CONTRACTID_MAX);
> +      gPlugDetector = PR_TRUE;
>      }
> -    gInitDetector = PR_TRUE;
> +
> +    nsContentUtils::RegisterPrefCallback("intl.charset.detector",
> +                                         MyPrefChangedCallback,
> +                                         nsnull);
>    }
> +
> +  gInitDetector = PR_TRUE;

Why move this outside of the if(! gInitDetector)-block?

> Index: layout/base/src/nsPresContext.cpp
> ===================================================================

> @@ -274,46 +282,44 @@ nsPresContext::GetFontPreferences()

>    pref.Assign("font.minimum-size.");
>    AppendUTF16toUTF8(langGroup, pref);
>  
> -  rv = mPrefs->GetIntPref(pref.get(), &size);
> -  if (NS_SUCCEEDED(rv)) {
> +  PRInt32 size = nsContentUtils::GetIntPref(pref.get(), -1);
> +  if (size != -1) {

  if (size > 0) { ?

> Index: layout/html/base/src/nsImageFrame.cpp
> ===================================================================

> @@ -1942,70 +1942,58 @@ static const char kIconLoadPrefs[][40] =
>    "browser.display.force_inline_alttext",
>    "network.image.imageBehavior",
>    "browser.display.show_image_placeholders"
>  };
>  
>  nsImageFrame::IconLoad::IconLoad(imgIDecoderObserver *aObserver)
>    : mLoadObserver(aObserver),
>      mIconsLoaded(0)
>  {
> -  nsCOMPtr<nsIPrefBranch> prefService =
> +  nsCOMPtr<nsIPrefBranchInternal> prefService =
>      do_GetService(NS_PREFSERVICE_CONTRACTID);

I second caillon's suggestion of nsContentUtils::GetPrefBranch.

> Index: layout/html/base/src/nsTextFrame.cpp
> ===================================================================

> @@ -2989,58 +2985,58 @@ nsTextFrame::PaintTextSlowly(nsIPresCont

> +          }
>  
>          if (isPaginated && !iter.IsBeforeOrAfter()) {
>            aRenderingContext.SetColor(nsCSSRendering::TransformColor(aTextStyle.mColor->mColor,canDarkenColor));
>            RenderString(aRenderingContext, aStyleContext, aPresContext,
>                         aTextStyle, currenttext, currentlength,
>                         currentX, dy, width, details);
>          } else if (!isPaginated) {
>            aRenderingContext.SetColor(nsCSSRendering::TransformColor(currentFGColor,canDarkenColor));
>            RenderString(aRenderingContext,aStyleContext, aPresContext,
>                         aTextStyle, currenttext, currentlength, currentX,
>                         dy, width, details);
>          }
>  
>            //increment twips X start but remember to get ready for next draw by reducing current x by letter spacing amount
> -	      currentX+=newDimensions.width;// + aTextStyle.mLetterSpacing;
> +        currentX+=newDimensions.width;// + aTextStyle.mLetterSpacing;
>  
> -	      iter.Next();
> -	      }
> +        iter.Next();
> +        }

Pffff. Something's still screwed with the whitespace here

> Index: layout/html/document/src/nsFrameSetFrame.cpp
> ===================================================================

> +// static
> +int
> +nsHTMLFramesetFrame::FrameResizePrefCallback(const char* aPref, void* aClosure)

>  static NS_DEFINE_IID(kViewCID, NS_VIEW_CID);

Make this NS_DEFINE_CID while you're in the neighbourhood.

> Index: dom/src/base/nsGlobalWindow.cpp
> ===================================================================

> @@ -3175,21 +3142,21 @@ GlobalWindowImpl::CheckForAbusePoint()

> +    intPref =
> +      nsContentUtils::GetIntPref("dom.popup_maximum", -1);

Why on two lines?

> @@ -6329,23 +6277,21 @@ NavigatorImpl::Preference()

> +  nsCOMPtr<nsIPrefBranch> prefBranch =
> +    do_GetService(NS_PREFSERVICE_CONTRACTID);
> +  NS_ENSURE_STATE(prefBranch);

nsContentUtils::GetPrefBranch?

Do we really check some of these prefs on every image load or every
MOUSE_SCROLL event etc? Seems like those could use callbacks too, no? :-/

        Attachment #146134 -
        Flags: superreview?(peterv) → superreview+

    
    

    
  
(In reply to comment #12)
> (From update of attachment 146134 [details] [diff] [review])
> Do you also want |static nsIPrefBranch* nsContentUtils::GetPrefBranch() {
> return sPrefBranch; }| ? since there are some places which still want the
> prefbranch to install observers or something.  They could then just use the
> reference from nsContentUtils instead of using a strong ref themselves.

Added.

> >       mPrefBranch->GetIntPref("ui.key.generalAccessKey",
> 
> Are you intentionally leaving these pref callers alone?

I was, but now that I added GetPrefBranch() I eliminated mPrefBranch from
nsEventStateManager and made those use the helpers.

> >+		  js \
> >+		  xpconnect \
> 
> Clear the above REQUIRES changes with dbaron?

Done. He was ok with them unless it's easy to get rid of the dependencies, and
it's not. nsIDOMClassInfo (through nsIDOMScriptObjectFactory) is what makes it
hard, it needs nsIXPCScriptable for obvious reasons. So these dependency changes
remain.

> >+  // NOTE! nsPresContext::operator new() zeroes out all members, so don't
> >+  // bother initializing members to 0.
...
> >+  // NOTE! nsPresContext::operator new() zeroes out all members, so don't
> >+  // bother initializing members to 0.
> 
> You must really like that comment, eh?	How about picking one place for it? 
> :-)

I do! :-) The reason I duplicated it is the same as in nsHTMLDocument etc,
people don't see them and don't realize what's going on and waste code on
initializing members for no reason. I'll leave it duplicated here too. I can't hurt.

> Again, test your changes.  The prefs stuff in nsPresContext get accessed a LOT
> :-( so you may possibly hurt performance.

I see no way my code will be slower than the old code, but I'll keep my eyes on it.

> And please find a better place to put these wrappers.  I hate that we have to
> put them in every library.

I'd love to, but that means a new dll for this code. So for now, they'll live
where they are.

    
    

    
  
(In reply to comment #17)
> Make this CopyUTF16toASCII while you're in the neighbourhood :-).

Done, but it's LossyCopyUTF16toASCII().

> Don't lose the "never" comment.

Added back.

> > +    CopyUCS2toASCII(defCharset, aCharset);
> 
> CopyUTF16toASCII

LossyCopyUTF16toASCII() it is.

> >    }
> > +
> > +  gInitDetector = PR_TRUE;
> 
> Why move this outside of the if(! gInitDetector)-block?

Good qeustion. Moved back.

>   if (size > 0) { ?

Done.

> Pffff. Something's still screwed with the whitespace here

Fixed.

> > +    intPref =
> > +      nsContentUtils::GetIntPref("dom.popup_maximum", -1);
> 
> Why on two lines?

Fixed.

> > +  nsCOMPtr<nsIPrefBranch> prefBranch =
> > +    do_GetService(NS_PREFSERVICE_CONTRACTID);
> > +  NS_ENSURE_STATE(prefBranch);
> 
> nsContentUtils::GetPrefBranch?

Done.

> 
> Do we really check some of these prefs on every image load or every
> MOUSE_SCROLL event etc? Seems like those could use callbacks too, no? :-/
> 

Yeah, but I'll stop here. That's to be fixed in another bug, if at all.

Thanks for the reviews!

    
    

    
  
Fix checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
I think this caused an increase in leaks on the brad tinderbox.
Lk:298KB -> Lk:391KB

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsPrefService                                    40          -
nsPrefBranch                                    120          -
nsSharedPrefHandler                              84          -
nsVoidArray                                      24          -
nsPref                                           40          -
nsLocalFile                                     116          -
TOTAL                                           424


    
    

    
  
Oh, bah, do you need to NS_IF_RELEASE(sPrefBranch) in nsContentUtils::Shutdown?

    
    

    
  
Oh, duh. How stupid. Yeah, I need to NS_IF_RELEASE() sPrefBranch and sPref. Fixed.
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: