Last Comment Bug 663036 - gfx should use mozilla::Preferences
: gfx should use mozilla::Preferences
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 656826 663784
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 23:13 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-06-15 08:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part.1 When creating Preferences singleton instance, it should be created by service manager (2.61 KB, patch)
2011-06-08 23:13 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Part.2 Implement GetComplex() and SetComplex() (2.08 KB, patch)
2011-06-08 23:14 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Part.3 gfx should use mozilla::Preferences (72.38 KB, patch)
2011-06-08 23:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
joe: review+
Details | Diff | Splinter Review
Part.1 When creating Preferences singleton instance, it should be created by service manager (mq) (2.61 KB, patch)
2011-06-09 17:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.2 Implement GetComplex() and SetComplex() (mq) (2.09 KB, patch)
2011-06-09 17:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-08 23:13:46 PDT
Created attachment 538181 [details] [diff] [review]
Part.1 When creating Preferences singleton instance, it should be created by service manager

gfx accesses Preferences instance on plugin process. At that time, current implementation fails to initialize the instance (Preferences::Init() fails). The reason is that Omnijar hasn't been initialized yet at that time. For fix this issue, InitStaticMembsers() should create the instance via service manager.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-08 23:14:41 PDT
Created attachment 538182 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex()
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-08 23:16:06 PDT
Created attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-09 03:30:47 PDT
Comment on attachment 538181 [details] [diff] [review]
Part.1 When creating Preferences singleton instance, it should be created by service manager

Review of attachment 538181 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-09 03:33:32 PDT
Comment on attachment 538182 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex()

Review of attachment 538182 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-09 04:32:43 PDT
Comment on attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences

Review of attachment 538183 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-09 17:58:03 PDT
Created attachment 538399 [details] [diff] [review]
Part.1 When creating Preferences singleton instance, it should be created by service manager (mq)
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-09 17:58:41 PDT
Created attachment 538400 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex() (mq)
Comment 8 Joe Drew (not getting mail) 2011-06-09 18:23:27 PDT
Comment on attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences

Review of attachment 538183 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxDWriteFontList.cpp
@@ +945,5 @@
>      }
>  
> +    nsAdoptingCString classicFamilies =
> +        Preferences::GetCString("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families");
> +    if (classicFamilies) {

Is this legal? I looked through nsString stuff, and didn't find any obvious conversions to a pointer or bool there.

::: gfx/thebes/gfxFT2Fonts.cpp
@@ +511,5 @@
>      key.Append("-");
>      key.AppendInt(mStyle.weight);
>  
>      if (!platform->GetPrefFontEntries(key, &aFontEntryList)) {
> +        NS_ENSURE_TRUE(Preferences::GetRootBranch(), );

Do we even need this anymore?

::: gfx/thebes/gfxFont.cpp
@@ +2288,5 @@
>              prefName.Append(lcFamily);
>              prefName.AppendLiteral(".");
>              prefName.Append(groupString);
> +            nsAdoptingString value = Preferences::GetString(prefName.get());
> +            if (value) {

Same question as above.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-09 21:21:56 PDT
(In reply to comment #8)
> > +    nsAdoptingCString classicFamilies =
> > +        Preferences::GetCString("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families");
> > +    if (classicFamilies) {
> 
> Is this legal? I looked through nsString stuff, and didn't find any obvious
> conversions to a pointer or bool there.

Yes. |if (classicFamilies) {}| means if (classicFamilies.get()) {}. And also nsAdopting*String may have NULL for get() because it steals string buffer ownership at constructing.  If nsIPrefBranch::GetCharPref() failed, .get() returns NULL. If nsIPrefBranch::GetCharPref() gets empty string, .get() returns non-null pointer.

> ::: gfx/thebes/gfxFT2Fonts.cpp
> @@ +511,5 @@
> >      key.Append("-");
> >      key.AppendInt(mStyle.weight);
> >  
> >      if (!platform->GetPrefFontEntries(key, &aFontEntryList)) {
> > +        NS_ENSURE_TRUE(Preferences::GetRootBranch(), );
> 
> Do we even need this anymore?

I think that it's not necessary. I write the patch without logical change. If you think I should remove it, I'll do it before landing.

> ::: gfx/thebes/gfxFont.cpp
> @@ +2288,5 @@
> >              prefName.Append(lcFamily);
> >              prefName.AppendLiteral(".");
> >              prefName.Append(groupString);
> > +            nsAdoptingString value = Preferences::GetString(prefName.get());
> > +            if (value) {
> 
> Same question as above.

same as above.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-11 20:33:22 PDT
http://hg.mozilla.org/mozilla-central/rev/d5ce62d06d20
http://hg.mozilla.org/mozilla-central/rev/a1cebab4dc0f
http://hg.mozilla.org/mozilla-central/rev/1abe9e627533

(In reply to comment #9)
> > ::: gfx/thebes/gfxFT2Fonts.cpp
> > @@ +511,5 @@
> > >      key.Append("-");
> > >      key.AppendInt(mStyle.weight);
> > >  
> > >      if (!platform->GetPrefFontEntries(key, &aFontEntryList)) {
> > > +        NS_ENSURE_TRUE(Preferences::GetRootBranch(), );
> > 
> > Do we even need this anymore?
> 
> I think that it's not necessary. I write the patch without logical change.
> If you think I should remove it, I'll do it before landing.

I'll post a follow up patch if you think I should do it.
Comment 11 neil@parkwaycc.co.uk 2011-06-12 04:21:34 PDT
Comment on attachment 538400 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex() (mq)

Might be interesting to write some templates i.e.
template<class T>
static nsresult GetComplex(const char* aPref, T** aResult)
{
  return GetComplex(aPref, NS_GET_IID(T), reinterpret_cast<void**>(aResult));
}
template<class T>
static nsresult SetComplex(const char* aPref, T* aValue)
{
  return SetComplex(aPref, NS_GET_IID(T), aValue);
}
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-12 17:58:13 PDT
(In reply to comment #11)
> Comment on attachment 538400 [details] [diff] [review] [review]
> Part.2 Implement GetComplex() and SetComplex() (mq)
> 
> Might be interesting to write some templates i.e.
> template<class T>
> static nsresult GetComplex(const char* aPref, T** aResult)
> {
>   return GetComplex(aPref, NS_GET_IID(T), reinterpret_cast<void**>(aResult));
> }
> template<class T>
> static nsresult SetComplex(const char* aPref, T* aValue)
> {
>   return SetComplex(aPref, NS_GET_IID(T), aValue);
> }

Can we use template everywhere now?
Comment 13 neil@parkwaycc.co.uk 2011-06-15 01:18:56 PDT
Comment on attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences

>diff --git a/gfx/thebes/gfxWindowsPlatform.cpp b/gfx/thebes/gfxWindowsPlatform.cpp
>--- a/gfx/thebes/gfxWindowsPlatform.cpp
>+++ b/gfx/thebes/gfxWindowsPlatform.cpp
>@@ -81,16 +80,18 @@
> #ifdef CAIRO_HAS_D2D_SURFACE
> #include "gfxD2DSurface.h"
> 
> #include <d3d10_1.h>
> 
> #include "nsIMemoryReporter.h"
> #include "nsMemory.h"
> 
>+using namespace mozilla;
This is inside the #ifdef block, so the compile fails if you're not using d2d.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-15 02:03:58 PDT
Neil: See bug 663784.
Comment 15 neil@parkwaycc.co.uk 2011-06-15 03:51:12 PDT
(In reply to comment #12)
> Can we use template everywhere now?
Not sure what you mean here but we have lots of templates all over these days.

(In reply to comment #14)
> Neil: See bug 663784.
Aha, just the fix that I had applied locally to get my build working :-)
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-15 08:26:56 PDT
(In reply to comment #15)
> (In reply to comment #12)
> > Can we use template everywhere now?
> Not sure what you mean here but we have lots of templates all over these
> days.

According to our old coding rules, we must not use template except xpcom. I'm not sure the current rule though.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-15 08:37:33 PDT
Um, but adding GetLocalFile() and GetRelativeFilePref() is better for readable.

# I don't understand what's the nsIRelativeFilePref doing.

nsISupportsString doesn't need for GetComplex() because we already have GetString() which does same thing.

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