gfx should use mozilla::Preferences

RESOLVED FIXED in mozilla7

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Attachment #538181 - Flags: review?(roc)
Created attachment 538182 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex()
Attachment #538182 - Flags: review?(roc)
Created attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences
Attachment #538183 - Flags: review?(roc)
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]:
-----------------------------------------------------------------
Attachment #538181 - Flags: review?(roc) → review+
Comment on attachment 538182 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex()

Review of attachment 538182 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #538182 - Flags: review?(roc) → review+
Comment on attachment 538183 [details] [diff] [review]
Part.3 gfx should use mozilla::Preferences

Review of attachment 538183 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #538183 - Flags: review?(roc) → review+
Attachment #538183 - Flags: review?(joe)
Created attachment 538399 [details] [diff] [review]
Part.1 When creating Preferences singleton instance, it should be created by service manager (mq)
Attachment #538181 - Attachment is obsolete: true
Created attachment 538400 [details] [diff] [review]
Part.2 Implement GetComplex() and SetComplex() (mq)
Attachment #538182 - Attachment is obsolete: true
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.
Attachment #538183 - Flags: review?(joe) → review+
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
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?

Updated

6 years ago
Depends on: 663784
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.
You need to log in before you can comment on or make changes to this bug.