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 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 on attachment 538182 [details] [diff] [review] Part.2 Implement GetComplex() and SetComplex() Review of attachment 538182 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 538183 [details] [diff] [review] Part.3 gfx should use mozilla::Preferences Review of attachment 538183 [details] [diff] [review]: -----------------------------------------------------------------

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.

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

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 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); }

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

Neil: See bug 663784.

(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 :-)

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

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.