As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 656826 663784
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 23:13 PDT by Masayuki Nakano [:masayuki]
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]
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]
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]
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]
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]
no flags Details | Diff | Splinter Review

Description User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 2011-06-08 23:14:41 PDT
Created attachment 538182 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex()
Comment 2 User image Masayuki Nakano [:masayuki] 2011-06-08 23:16:06 PDT
Created attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences
Comment 3 User image Robert O'Callahan (:roc) (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 User image Robert O'Callahan (:roc) (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 User image Robert O'Callahan (:roc) (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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 2011-06-09 17:58:41 PDT
Created attachment 538400 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex() (mq)
Comment 8 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image Masayuki Nakano [:masayuki] 2011-06-15 02:03:58 PDT
Neil: See bug 663784.
Comment 15 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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.