Closed Bug 912794 Opened 6 years ago Closed 6 years ago

Extract color management prefs and values out of gfxPlatform into a separate class

Categories

(Core :: Graphics, defect)

25 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: milan, Assigned: milan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 10 obsolete files)

11.78 KB, patch
milan
: review+
Details | Diff | Splinter Review
gfxPlatform has a number of static methods that handle CMS preferences as well as the cached values and state.  Taking them into a separate class cleans things up and stops some of the other classes from including all of gfxPlatform.  It also makes the "all prefs on the main thread" work more obvious.
Blocks: 910860
QA Contact: milan
The patch looks large because the code has been moved to a separate file, but  the logic remains the same.
Attachment #799886 - Flags: review?(ncameron)
Comment on attachment 799886 [details] [diff] [review]
Extract the CMS prefs/values out of gfxPlatform into a separate singleton

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

Can you use hg cp to create the new files rather than hg adding them? It makes it easier to review and we don't lose the hg history. (You don't need to start again either, you can hg cp, then copy and paste the entire file and we still get what we want).
Attachment #799886 - Flags: review?(ncameron)
Separate out the CMS globals and prefs into a singleton gfxColorManagement. Preferences are now initialized at startup, then updated with callbacks. The methods that access the cached values are not checking the preferences. This lets us better control which thread reads the prefs.
Attachment #799886 - Attachment is obsolete: true
Attachment #799892 - Flags: review?(ncameron)
Comment on attachment 799892 [details] [diff] [review]
Extract the CMS prefs/values out of gfxPlatform into a separate singleton

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

::: gfx/thebes/gfxColorManagement.cpp
@@ +111,2 @@
>  {
> +  // Blank lines below just to be able to read the code.

doesn't need this comment

::: gfx/thebes/gfxColorManagement.h
@@ +26,5 @@
>  
> +/**
> + * Preferences, utilities and CMS profile management. These are "global".
> + */
> +class gfxColorManagement

MOZ_FINAL

@@ +32,5 @@
> +public:
> +  /**
> +   * Manage the singleton.  instance() will create it if it isn't there.
> +   */
> +  static gfxColorManagement& instance();

Instance(), please. Can this return a const ref? All the uses only use getters and you've marked a bunch of fields mutable, so it seems like this ought to be const. Maybe you will need a non-const private version.

@@ +34,5 @@
> +   * Manage the singleton.  instance() will create it if it isn't there.
> +   */
> +  static gfxColorManagement& instance();
> +  static void destroy();
> +  static bool exists();

initial capitals for these two too

@@ +101,5 @@
> +
> +  qcms_profile* mDefaultPlatformProfile;
> +
> +private:
> +  static gfxColorManagement* mInstance;

sInstance

@@ +104,5 @@
> +private:
> +  static gfxColorManagement* mInstance;
> +
> +  gfxColorManagement();
> +  ~gfxColorManagement();

should probably have private or deleted copy constructor and assignment operator

::: image/decoders/nsJPEGDecoder.cpp
@@ +312,5 @@
>          if (mInfo.out_color_space == JCS_CMYK)
>            type |= FLAVOR_SH(mInfo.saw_Adobe_marker ? 1 : 0);
>  #endif
>  
> +        gfxColorManagement& colorManagement = gfxColorManagement::instance();

can this be const?

::: image/decoders/nsPNGDecoder.cpp
@@ +552,5 @@
>  
>    qcms_data_type inType;
>    uint32_t intent = -1;
>    uint32_t pIntent;
> +  gfxColorManagement& colorManagement = gfxColorManagement::instance();

can this be const?
Attachment #799892 - Flags: review?(ncameron) → review+
(In reply to Nick Cameron [:nrc] from comment #4)
> Comment on attachment 799892 [details] [diff] [review]
> Extract the CMS prefs/values out of gfxPlatform into a separate singleton
> 
> Review of attachment 799892 [details] [diff] [review]:

Thanks, that was fast!


> @@ +32,5 @@
> > +public:
> > +  /**
> > +   * Manage the singleton.  instance() will create it if it isn't there.
> > +   */
> > +  static gfxColorManagement& instance();
> 
> Instance(), please. Can this return a const ref? All the uses only use
> getters and you've marked a bunch of fields mutable, so it seems like this
> ought to be const. Maybe you will need a non-const private version.

We do need a non-const version in gfxPlatform, to initialize it. I could make it private and gfxPlatform a friend, or something like that.  Let me think about it.
Addressed the review comments.  Carry r=ncameron.  Waiting for the try run (using =delete, not sure if all compilers support it) before requesting checkin.
Attachment #799892 - Attachment is obsolete: true
Attachment #800488 - Flags: review+
Rebased and addressed review comments. Right now we have a non-const access to the singleton instance, but only by friends.  Not a big design decision, so if there is a need for a non-const access for all, we can reconsider.
This try run showed =delete isn't supported on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=362efb740030 
so it's the private copy constructor/operator= approach that was taken.
Attachment #800488 - Attachment is obsolete: true
Attachment #801017 - Flags: review+
Landed and backed out for asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca38bd9ec80a

https://tbpl.mozilla.org/php/getParsedLog.php?id=27522453&tree=Mozilla-Inbound

19:31:23     INFO -  ###!!! ASSERTION: Prefs have not been initialized: 'mPrefsInitialized', file ../../../gfx/thebes/gfxColorManagement.cpp, line 220
19:31:38     INFO -  nsXPLookAndFeel::GetColorImpl(mozilla::LookAndFeel::ColorID, unsigned int&) [widget/xpwidgets/nsXPLookAndFeel.cpp:619]
19:31:38     INFO -  nsPresContext::GetDocumentColorPreferences() [obj-firefox/dist/include/mozilla/LookAndFeel.h:506]
19:31:38     INFO -  nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool) [obj-firefox/dist/include/nsAutoPtr.h:1035]
19:31:38     INFO -  nsDocumentViewer::Init(nsIWidget*, nsIntRect const&) [layout/base/nsDocumentViewer.cpp:643]
19:31:38     INFO -  nsDocShell::SetupNewViewer(nsIContentViewer*) [docshell/base/nsDocShell.cpp:8346]
19:31:38     INFO -  nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) [obj-firefox/dist/include/nsCOMPtr.h:800]
19:31:38     INFO -  nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIURI*, bool) [obj-firefox/dist/include/nsCOMPtr.h:819]
19:31:38     INFO -  nsWebShellWindow::Initialize(nsIXULWindow*, nsIXULWindow*, nsIURI*, int, int, bool, nsWidgetInitData&) [xpfe/appshell/src/nsWebShellWindow.cpp:219]
19:31:38     INFO -  nsAppShellService::JustCreateTopWindow(nsIXULWindow*, nsIURI*, unsigned int, int, int, bool, nsWebShellWindow**) [xpfe/appshell/src/nsAppShellService.cpp:586]
19:31:38     INFO -  nsAppShellService::CreateHiddenWindowHelper(bool) [xpfe/appshell/src/nsAppShellService.cpp:128]
19:31:38     INFO -  nsAppStartup::CreateHiddenWindow() [toolkit/components/startup/nsAppStartup.cpp:238]
19:31:38     INFO -  XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:3803]
19:31:38     INFO -  XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:3937]
19:31:38     INFO -  XRE_main [toolkit/xre/nsAppRunner.cpp:4139]
19:31:38     INFO -  main [browser/app/nsBrowserApp.cpp:275]
Assignee: nobody → milan
Keywords: checkin-needed
(In reply to Milan Sreckovic [:milan] from comment #7)
> Created attachment 801017 [details] [diff] [review]
> Extract the CMS prefs/values out of gfxPlatform into a separate singleton. 

> This try run showed =delete isn't supported on all platforms:
> https://tbpl.mozilla.org/?tree=Try&rev=362efb740030 
> so it's the private copy constructor/operator= approach that was taken.

Use MOZ_DELETE. (See http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#183)
Sorry about the bustage.  I'm glad I put that assert in, because it did catch the call that comes before the platform is initialized.  It still works, but it is a bit messy as to what preferences can be changed more than once per session and it's unclear if they actually behave correctly.
Just to record the old logic confusion.  qcms_enable_v4() would be set if gfx.color_management.enablev4 was set to true at start up or reset to true and followed by changing the value of gfx.color_management.force_srgb.  So, it was neither "start up only", nor "live preference" but some weird mixture.
I don't really like this, but let's see if it works: https://tbpl.mozilla.org/?tree=Try&rev=826ca5ce6075
Follow up patches start converting gfxPlatform calls to gfxPrefs calls.  This should make it "preferences are always on main thread" safe, as well as delete some code, make preferences easier to use, and make the switch from live to session preference simple.

Asking for some feedback just to see if I'm breaking some (unwritten or otherwise) rules with this type of code.
Attachment #801017 - Attachment is obsolete: true
Attachment #8371614 - Flags: feedback?(bjacob)
Attachment #8371614 - Flags: feedback?(bas)
Attachment #8371614 - Attachment is patch: true
Comment on attachment 8371614 [details] [diff] [review]
Setup a place where graphics preferences can live, safe from multi-threading issues.

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

::: gfx/thebes/gfxPrefs.h
@@ +51,5 @@
> +// from any thread, you will get the most up to date preference value of
> +// "gl.msaa-level".  If the value is not set, the default would be 2.
> +
> +
> +#define GFX_PREFS_DECL(update,name,vartype,prefname,defaultval)   \

My only big objection to this patch is making such heavy usage of macros to generate code.

Let's look at it from the perspective of making this code optimally easy for other people to read.

From that perspective, macros can be a good thing when they eliminate redundancy and goop, but they can be a bad thing then e.g. they make it hard to grep through code to find where a method is defined (that fails for method names assembled from ## preprocessor tokens).

With that in mind, is there room for improvement over the current patch?

For example, here is a quick strawman idea to illustrate what I mean. This macro is used to generate a part of a class's definition. Could that code be moved to a standalone class, possibly templated? Could macros then be used in a more gentle way to simply refer to that outside class, rather than using macros to generate its code from preprocessor tokens?
Attachment #8371614 - Flags: feedback?(bjacob)
I like this version better, because we just need to list the items in one place.
Attachment #8371614 - Attachment is obsolete: true
Attachment #8371614 - Flags: feedback?(bas)
Attachment #8371858 - Flags: review?(bjacob)
Attachment #8371858 - Flags: review?(bas)
Comment on attachment 8371858 [details] [diff] [review]
Setup a place where graphics preferences can live, safe from multi-threading issues.

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

::: gfx/thebes/gfxPrefs.h
@@ +64,5 @@
> +#error We need to change this macro to something else
> +#endif
> +
> +#define ONCE false
> +#define LIVE true

The key objection was that I wanted to make sure that we were keeping macro usage low, only using macros for where they definitely add value, using as much c++ typing as we can, and avoiding global scope pollution as much as we can. This patch has more room for improvement in this respect.

For example, you could start by making ONCE/LIVE values of a ENUM_CLASS like this:

#include "mozilla/TypedEnum.h"

MOZ_BEGIN_ENUM_CLASS(gfxPrefUpdatePolicy)
  Once,
  Live
MOZ_END_ENUM_CLASS(gfxPrefUpdatePolicy)

The values are then namespaced by the enum type, so they're not polluting the global scope:

gfxPrefUpdatePolicy::Once
gfxPrefUpdatePolicy::Live

You can then have your macro like
#define GFX_PREFS_DECLX(aUpdate, ...) \
  gfxPrefUpdatePolicy::aUpdate ...

@@ +71,5 @@
> +// This is just necessary in special cases where we want to
> +// override the default behaviour. You should be able to just use
> +// the second one _DECL.
> +#define GFX_PREFS_DECLX(aUpdate,aPref,aName,aVarTpe,aValDflt)\
> +public:                                                      \

As commented on the previous iteration this patch, I still need to be convinced that there isn't a lot of room for improvement here to replace this big class-generating macro by regular c++ code, only using macros for what only macros can do.

Let me elaborate on the strawman idea in my previous comment. Instead of using a macro to generate a class body, and then declare a class member of that type, you could simply make a _templated_ helper class, taking as template parameters all what your macro currently takes, minus aName. Keep the existing good idea of having a member in gfxPrefs for each pref, letting the default constructor do the registration. Let all the API to access that pref go through that member, so that you don't need to add any per-pref method to gfxPrefs.

Note that if you follow the above suggestion of making ONCE and LIVE values of a ENUM_CLASS gfxPrefUpdatePolicy, you will need to use MOZ_ENUM_CLASS_ENUM_TYPE(gfxPrefUpdatePolicy) to declare a template parameter of that type. This is because of the trick that mfbt/TypedEnum.h has to use to implement enum classes on c++98; this will go away soon as all compilers support c++11.

Toy example:

template<MOZ_ENUM_CLASS_ENUM_TYPE(gfxPrefUpdatePolicy) Update,
         const char* Pref,
         typename VarType,
         VarType ValDefault>
class gfxPrefHelper
{
  VarType Value() {
    return mValue;  
  }

  void Register() {
    if (Update == gfxPrefUpdatePolicy::Live) {
      AddVarCache<VarType>(&mValue, Pref);
    } else {
      mValue = Get<VarType>(Pref, ValDefault);
    }
  }
};

(yes, you will have to make templated wrappers for the Preferences API if these don't already exist).

then, in gfxPrefs:

class gfxPrefs
{

public:

  // this is how you add a pref. No macro!
  gfxPrefHelper<gfxPrefUpdatePolicy::Live, "gfx.color_management.enablev4", bool, false> CMSEnableV4;
  ...
}
Attachment #8371858 - Flags: review?(bjacob) → review-
(In reply to Benoit Jacob [:bjacob] from comment #16)
> Comment on attachment 8371614 [details] [diff] [review]
> Setup a place where graphics preferences can live, safe from multi-threading
> issues.
> 
> Review of attachment 8371614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ...
> 
> For example, here is a quick strawman idea to illustrate what I mean. This
> macro is used to generate a part of a class's definition. Could that code be
> moved to a standalone class, possibly templated? Could macros then be used
> in a more gentle way to simply refer to that outside class, rather than
> using macros to generate its code from preprocessor tokens?

I like the idea of templates instead of macros, let me take a look at that.
Comment on attachment 8371858 [details] [diff] [review]
Setup a place where graphics preferences can live, safe from multi-threading issues.

I'll save Bas time on the review until I do the templated version.
Attachment #8371858 - Flags: review?(bas)
There'll be hoops to jump through because const char* is not allowed as a template argument...
Ah.. I didn't realize that when I wrote that, sorry.
Thanks for the template suggestion, much better in the end.

A few options that were considered:

1. Everything in a single template class - var type, preference name, default value, update policy.
2. Templated base class with var type, derived template class with update policy, preference name, default value.
3. Templated base class just with var type.  Derived class in a macro.

Debug build sizes for the gfxPrefs.o:

1. 151188
2. 118236
3. 96208

With #2 the macro is as simple as with #1, but small enough to take over #3 which has a derived class in macro, so going with that.
Attachment #8371858 - Attachment is obsolete: true
Attachment #8372656 - Flags: review?(bjacob)
(In reply to Milan Sreckovic [:milan] from comment #23)
> Debug build sizes for the gfxPrefs.o:
> 
> 1. 151188
> 2. 118236
> 3. 96208

Measuring object code size is a fine art that I don't quite master. Just measuring .o file size is not great --- especially from a debug build. In particular, in the present case, templates are going to generate a lot more debug info than macros, so measuring .o file size is biased in favor of the macro version.

To get some solid measurement of code size I would:
 1. made an optimized non-debug build
 2. Only measure code size from libxul.so, not individual .o files, since nontrivial redundancy elimination occurs when libxul is linked;
 3. Use 'nm' or 'objdump' to measure symbol size and filter the part of libxul that interests you.

Like this on linux:

nm -S --radix=d obj-firefox-release/dist/bin/libxul.so | grep gfxPref | awk '{ SUM += $2} END { print SUM/1024 }'

(to print the result in kilobytes)

Though I seem remember that :jrmuizel mentioned that there was a caveat even with 'nm' -- but I don't remember what.
In the present case, I couldn't easily believe that either approach makes a significant real code size difference. I would attribute any code size difference beyond a few kb to abstract issues such as debug information size.
The suggestion is then to go with the single template and avoid the two classes?  Makes sense.  I may try and measure the difference in size, but, as you said, there are caveats.
My only suggestion here is to not worry about code size at all. I can't imagine anything here making a significant difference. So choose whichever approach you prefer based on other criteria. (I haven't looked into any specifics of this patch yet).
Not that compiler optimization implies that these template helpers will typically evaporate, as they get massively inlined.
Comment on attachment 8372895 [details] [diff] [review]
Setup a place where graphics preferences can live, safe from multi-threading issues.  The single template version.

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

R+ on the template helper. One important comment to address on the way that it's used: the 'Skip' behavior used for mPrefAsyncVideoEnabled and mPrefComponentAlphaEnabled does not seem to match current m-c behavior?

::: gfx/thebes/gfxPrefs.cpp
@@ +32,5 @@
> +{
> +  // Needs to be created on the main thread.
> +  MOZ_ASSERT(NS_IsMainThread(), "first call must be from the main thread");
> +
> +  // These two used UpdatePolicy::Skip

I don't understand what is going on with mPrefAsyncVideoEnabled and mPrefComponentAlphaEnabled, can you elaborate? The Skip behavior used for these preferences does not seem to match their existing behavior: as far as I can see, they _are_ being read currently in m-c code, and using Skip for them means that they will no longer be read.

@@ +46,5 @@
> +#endif
> +}
> +
> +gfxPrefs::~gfxPrefs()
> +{

How about asserting, here too, that we are on the main thread?

::: gfx/thebes/gfxPrefs.h
@@ +13,5 @@
> +// First time gfxPrefs::One() needs to be called on the main thread,
> +// before any of the methods accessing the values are used, but after
> +// the Preferences system has been initialized.
> +
> +// The static methods to access the preference value is safe to call

s/is/are/

@@ +24,5 @@
> +// or connect with a callback.  See UpdatePolicy enum below.
> +// The second is the string with the preference name.
> +// The third argument is the name of the static function to create.
> +// The fourth is the type of the preference - bool, int32_t, uint32_t.
> +// The fifth is the default value for the preference.

Rather than referring to arguments as 'the first, the second, ...', better refer to them by name, after having copied the 'prototype' of that macro,
   GFX_PREFS_DECL(Update,Pref,Name,Type,Default)

@@ +50,5 @@
> +// in the first place, so be careful.  You need to be ready for the
> +// values changing mid execution, and if you're using those preferences
> +// in any setup and initialization, you may need to do extra work.
> +
> +#define GFX_PREFS_DECL(Update,Pref,Name,Type,Default)                         \

Here and everywhere else in this patch, please always put a space after every comma: a, b, c

@@ +54,5 @@
> +#define GFX_PREFS_DECL(Update,Pref,Name,Type,Default)                         \
> +public:                                                                       \
> +static Type Name() { MOZ_ASSERT(Exists()); return One().mPref##Name.mValue; } \
> +private:                                                                      \
> +static const char* Func##Name() { return Pref; }                              \

I would say something like Get##Name##PrefName, that would be more descriptive than Func##Name.

For example, for orkAroundDriverBugs, that would give GetWorkAroundDriverBugsPrefName() instead of FuncWorkAroundDriverBugs().

@@ +81,5 @@
> +    }
> +    void Register(UpdatePolicy aHow, const char* aPreference)
> +    {
> +      switch(aHow) {
> +        case gfxPrefs::UpdatePolicy::Skip:

Since here we are inside of gfxPrefs, you can drop the gfxPrefs:: .

@@ +144,5 @@
> +
> +public:
> +  // Manage the singleton:
> +  static gfxPrefs& One()
> +  { 

one trailing space here.
Attachment #8372895 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #31)
> Comment on attachment 8372895 [details] [diff] [review]
> ...
> 
> I don't understand what is going on with mPrefAsyncVideoEnabled and
> mPrefComponentAlphaEnabled, can you elaborate? The Skip behavior used for
> these preferences does not seem to match their existing behavior: as far as
> I can see, they _are_ being read currently in m-c code, and using Skip for
> them means that they will no longer be read.

First - thanks for the review!  Really good suggestions and details, I'll address them all.  Just wanted to answer this question here.

These two are initialized in the .cpp file, with the non-Skip argument - Live for async video and Once for component alpha.  This is because async is only setup as Live preference (or any preference, for that matter) on non-Windows platforms, and on Windows just hardcoded as false; similarly, the component alpha is hardcoded as false when MOZ_GFX_OPTIMIZE_MOBILE is created.

We have two options here - leave it as is, but put some comments in the .h file, pointing the user to the .cpp where things are "clearer", or perhaps better, put the #ifdef into the .h itself, so that it's all in one place.
(In reply to Milan Sreckovic [:milan] from comment #32)
> (In reply to Benoit Jacob [:bjacob] from comment #31)
> > Comment on attachment 8372895 [details] [diff] [review]
> > ...
> > 
> 
> These two are initialized in the .cpp file, with the non-Skip argument -
> Live for async video and Once for component alpha.  This is because async is
> only setup as Live preference (or any preference, for that matter) on
> non-Windows platforms, and on Windows just hardcoded as false; similarly,
> the component alpha is hardcoded as false when MOZ_GFX_OPTIMIZE_MOBILE is
> created.

Never mind this comment.  This is what the code is meant to do; this is not what it was doing, you caught the actual bugs.
Comment on attachment 8372895 [details] [diff] [review]
Setup a place where graphics preferences can live, safe from multi-threading issues.  The single template version.

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

::: gfx/thebes/gfxPrefs.h
@@ +105,5 @@
> +  GFX_PREFS_DECL(UpdatePolicy::Live,"gfx.color_management.enablev4",CMSEnableV4,bool,false);
> +  GFX_PREFS_DECL(UpdatePolicy::Live,"gfx.color_management.force_srgb",CMSForceSRGB,bool,false);
> +  GFX_PREFS_DECL(UpdatePolicy::Live,"gfx.color_management.mode",CMSMode,int32_t,-1);
> +  GFX_PREFS_DECL(UpdatePolicy::Live,"gfx.color_management.rendering_intent",CMSRenderingIntent,int32_t,-2);
> +  GFX_PREFS_DECL(UpdatePolicy::Live,"gfx.gralloc.fence-with-readpixels",ForceReadPixelsToFence,bool,false);

drive-by style nit..

We'll be keeping this list updated, and it may well change.  So please use generous white space, after the commas, maybe even tab-align the columns (at least for the first two params).  Also consider moving the UpdatePolicy after the variable name, and omitting the "UpdatePolicy::" (make it part of the macro):

GFX_PERFS_DECL("gfx.color_management.enablev4",         CMSEnableV4, Live, bool, false);
GFX_PERFS_DECL("gfx.color_management.force_srgb",       CMSForceSRGB, Live, bool, false);

etc.  I wouldn't care much if they get misaligned for really long prefs, so no worries about feeling like everything needs to get shifted to add a long pref.  But would be much more readable than the current, imo.
+1 to the suggestion about alignment. I considered making it, then thought about the "everything needs to be realigned for one long pref" problem, but like Vlad says this isn't a big problem if we make it OK to make exceptions for long prefs.

I also considered the suggestion to make the UpdatePolicy:: part of the macro, then thought that having on the caller side made it more explicit (easier for a newcomer to read this code and understand where this comes from), but indeed it's easy enough since these enums are defined in the same file, so +1 to that too.


While we're in the cosmetics department, DECL_GFX_PREF might be more to the point of what it is doing than GFX_PREFS_DECL.
Alignment makes sense and I'll rename the macro. I'm not sure about dropping UpdatePolicy:: part.  It is a very important part of the behaviour, can lead to bugs if we pick a wrong one, and I was trying to make it as visible as possible - I think either having it be the first argument, without UpdatePolicy:: or having it be the third argument, with UpdatePolicy:: would accomplish this.
Re: UpdatePolicy, I'm fine either way. The only thing that I really really care about is that the compiler will catch any mistake where arguments are passed in the wrong order; that is ensured by UpdatePolicy being a typed enum.
I think this is what we want in the end.  This takes a number of prefs into gfxPrefs, and the subsequent and blocked bugs will start using it.
Attachment #8372895 - Attachment is obsolete: true
Attachment #8373764 - Flags: review+
Don't want to tempt fate and just carry the r+.
Attachment #8373764 - Attachment is obsolete: true
Attachment #8373767 - Flags: review?(bjacob)
(Not all part of this bug, but a preview of what's coming in other bugs if it's green: https://tbpl.mozilla.org/?tree=Try&rev=684d5a8222a1)
Blocks: 971943
No longer depends on: 971943
Just the class, different preferences will be moved into it in different bugs.
Attachment #8373767 - Attachment is obsolete: true
Attachment #8373767 - Flags: review?(bjacob)
Attachment #8375629 - Flags: review+
Sorry, got behind my reviews. The interdiff looks fine.
Glad you did fall behind, gave me a chance to think.  I'm going to just return the base work, and add different preferences in other dependent bugs so that we add and use the pref at the same time.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65a9fb29d2f9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.