[Azure] Create a set abstraction for enumerations used in gfxPlatform.

RESOLVED WONTFIX

Status

()

Core
Graphics
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: kentuckyfriedtakahe)

Tracking

(Blocks: 1 bug)

Trunk
mozilla19
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

9.82 KB, patch
Details | Diff | Splinter Review
12.38 KB, patch
Details | Diff | Splinter Review
12.60 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Using bitfields directly and the << operator creates unclear semantics and it is easy to make mistakes. This needs a higher level construct.
Created attachment 663188 [details] [diff] [review]
Part 1: Added EnumSet helper class.
Attachment #663188 - Flags: review?(jwalden+bmo)
Created attachment 663209 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs
Assignee: ajones → nobody
Component: Canvas: 2D → Graphics
Created attachment 663291 [details] [diff] [review]
Part1: Added EnumSet to replace 1 << n bit fields v2
Attachment #663188 - Attachment is obsolete: true
Attachment #663188 - Flags: review?(jwalden+bmo)
Attachment #663291 - Flags: review?(jwalden+bmo)
Created attachment 663292 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v2
Attachment #663209 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=ba715d328c68
Attachment #663292 - Flags: review?(ncameron)

Comment 6

5 years ago
Comment on attachment 663292 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v2

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

Cool, looks much nicer! r+ with the nits fixed

::: gfx/thebes/gfxPlatform.cpp
@@ +1184,5 @@
>  }
>  
>  void
> +gfxPlatform::InitBackendPrefs(EnumSet<BackendType> aCanvasSupportedBackends,
> +                                  EnumSet<BackendType> aContentSupportedBackends)

nit: indentation

@@ +1205,5 @@
>  }
>  
>  /* static */ BackendType
> +gfxPlatform::GetBackendPref(const char* aEnabledPrefName,
> +                                const char* aBackendPrefName,

indentation again

::: gfx/thebes/gfxPlatform.h
@@ +482,5 @@
>       * The backend used is determined by aBackendBitmask and the order specified
>       * by the gfx.canvas.azure.backends pref.
>       */
> +    void InitBackendPrefs(mozilla::EnumSet<mozilla::gfx::BackendType> aCanvasSet,
> +                             mozilla::EnumSet<mozilla::gfx::BackendType> aContentSet);

nit: indentation

@@ +488,5 @@
>      /**
>       * returns the first backend named in the pref gfx.canvas.azure.backends
>       * which is a component of aBackendBitmask, a bitmask of backend types
>       */
> +    static mozilla::gfx::BackendType GetCanvasBackendPref(mozilla::EnumSet<mozilla::gfx::BackendType> aSupportedBackends);

please update the comments for these methods too

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +517,5 @@
>  #endif
>  
> +    EnumSet<BackendType> canvasMask(BACKEND_CAIRO);
> +    EnumSet<BackendType> contentMask;
> +     if (mRenderMode == RENDER_DIRECT2D) {

indentation
Attachment #663292 - Flags: review?(ncameron) → review+
Comment on attachment 663292 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v2

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

There are some pretty drastic line length issues here; could we please shorten and/or wrap as needed?
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> There are some pretty drastic line length issues here; could we please
> shorten and/or wrap as needed?

Have you looked at gfxPlatform.h?
Yes. (And I expect I carry blame for some of the lines!) But we should be fixing the issues when the opportunity arises (it may not be worth polluting hg blame just for line-length, but if we're touching the code anyway...) rather than introducing more cases.

ISTM that one thing exacerbating this is that we've become rather namespace-happy, so that we now have "mozilla::" where we might formerly have had "ns", or "mozilla::gfx::" instead of a simple "gfx" prefix. Apply those changes throughout a function signature and it can expand alarmingly... to compensate, we may need to put the return type and each parameter on its own line, for example.
Created attachment 667297 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v3
Attachment #663292 - Attachment is obsolete: true
Comment on attachment 663291 [details] [diff] [review]
Part1: Added EnumSet to replace 1 << n bit fields v2

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

Sorry for the delay getting to this.  :-(

Whatever eventually happens here, this needs tests in mfbt/tests/, patterned off the existing tests there.

I took a look at the usual place I steal knowledge from, wtf, and found that WebKit has three different bit-array types.  None of them are quite like what this bug wants, so it looks like we can't directly steal from their experience.  (BitVector is a resizable thing for arbitrary numbers of bits, not for a fixed set of flags.  BitArray<unsigned arraySize> asserts in-rangeness at runtime and has no set-theoretic methods.  Bitmap<size_t size, BitmapAtomicMode atomicMode> adds atomicity to the BitArray concept but really nothing more.)

Anyway, see comments, opinions appreciated.

::: mfbt/EnumSet.h
@@ +12,5 @@
> +
> +/**
> + * EnumSet<T> is a set of values defined by an enumeration. It is implemented
> + * using a 32 bit mask for each value so it will only work for enums with an int
> + * representation less than 32. It works both for enum and enum class types.

Going purely from the name, I would think this represents a set of arbitrary enums.  But this only represents a set of enums from [0, n) where n < 32, really, and it uses those only as flag bits.  What do you think of FlagSet as a clearer name?

@@ +24,5 @@
> +    { }
> +
> +    EnumSet(T aEnum)
> +     : mBitField(1 << aEnum)
> +    { 

Trailing whitespace (splinter view points it out, check there for all instances).

@@ +25,5 @@
> +
> +    EnumSet(T aEnum)
> +     : mBitField(1 << aEnum)
> +    { 
> +      NS_ASSERTION(uint32_t(aEnum) < 32, "Enum value out of range");

MOZ_ASSERT, found in "mozilla/Assertions.h", throughout this patch.  mfbt is self-contained and doesn't depend on NS_ASSERTION, which is an XPCOM thing.

So, this means that every modification to one of these has to check for in-rangeness.  But this seems like something we should be able to check at compile time, perhaps.  Might it possibly make sense to add a limit value to the template parameters, then make the flag in question for each of the class methods a template parameter to that method?  Then you could statically assert that the flag in question was in range.  This would make it more verbose to type out a FlagSet type signature, but that could perhaps be solved with typedefs, at a slight loss of clarity.  This would also make it impossible to add a dynamically-selected flag -- but not really, you could just require users doing so to have to specify another FlagSet (where the ops to add into it would do that type-checking, so you'd be inductively safe).  Which would be less clear and more verbose.  I'm not sure how the tradeoffs here really should work, and what would make the most sense.  Comments?

@@ +30,5 @@
> +    }
> +
> +    EnumSet(T aEnum1, T aEnum2)
> +     : mBitField((1 << aEnum1) |
> +                 (1 << aEnum2))

Alas, if only we had C++11 variadic templates, I think we could not have to repeat ourselves on this.  :-\

@@ +65,5 @@
> +
> +    /**
> +     * Add an element
> +     */
> +    void operator+=(T aEnum)

Operator overloading for this?  This seems just a bit cute.  Could you just have add/remove/addAll/removeAll instead?

The differentiation between mutating and non-mutating methods here also seems a bit funky.  Why not make all methods mutate (and return void to clearly indicate this), and then provide the copy constructor as a way to create a fresh copy when needed?  Users can use const FlagSet& easily enough whenever they don't want mutation happening.

@@ +157,5 @@
> +
> +    /**
> +     * Test is an element is contained in the set
> +     */
> +    bool Contains(T aEnum)

mfbt style for class methods is camelCaps, so this should be contains().
Attachment #663291 - Flags: review?(jwalden+bmo)
> Going purely from the name, I would think this represents a set of arbitrary
> enums.  But this only represents a set of enums from [0, n) where n < 32,
> really, and it uses those only as flag bits.  What do you think of FlagSet
> as a clearer name?

Flag is only conceptually relevant to the implementation not the meaning public API. You can consider it to be a specialisation of Set<T> where T is an int:5 type.

> Operator overloading for this?  This seems just a bit cute.  Could you just
> have add/remove/addAll/removeAll instead?

The reason I chose operator overloading is that I wanted to create a clear distinction between mutating and non-mutating operations. A mutable add() would be mutable and an immutable add() would be confusing. However operator+ conveys the immutable meaning clearly. I can accept that += is an unnecessary concession to state mutation.
 
> The differentiation between mutating and non-mutating methods here also
> seems a bit funky.  Why not make all methods mutate (and return void to
> clearly indicate this), and then provide the copy constructor as a way to
> create a fresh copy when needed?  Users can use const FlagSet& easily enough
> whenever they don't want mutation happening.

A mutation based API is not for me.
Created attachment 670215 [details] [diff] [review]
Part 1: Added EnumSet to replace 1 << n bit fields v3
Attachment #663291 - Attachment is obsolete: true
Created attachment 670220 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v4
Assignee: nobody → ajones
Attachment #667297 - Attachment is obsolete: true

Comment 15

5 years ago
Try run for e203b31b5e20 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e203b31b5e20
Results (out of 66 total builds):
    success: 61
    warnings: 4
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-e203b31b5e20
Created attachment 670552 [details] [diff] [review]
Part 1: Added EnumSet to replace 1 << n bit fields v4
Attachment #670215 - Attachment is obsolete: true
Blocks: 800690

Comment 17

5 years ago
Try run for 6822a89edce4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6822a89edce4
Results (out of 91 total builds):
    success: 83
    warnings: 7
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-6822a89edce4
Attachment #670552 - Flags: review?(jwalden+bmo)
Created attachment 671262 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v5
Attachment #670220 - Attachment is obsolete: true
Attachment #671262 - Flags: review?(ncameron)
Attachment #671262 - Flags: review?(ncameron)

Comment 19

5 years ago
Try run for 2db63274af7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2db63274af7c
Results (out of 13 total builds):
    exception: 2
    success: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-2db63274af7c
Created attachment 671279 [details] [diff] [review]
Part 1.5: Remove white space
Created attachment 671280 [details] [diff] [review]
Part 2: Use EnumSet in gfxPlatform for prefs v6
Attachment #671262 - Attachment is obsolete: true

Comment 22

5 years ago
Try run for eb3422b2760b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eb3422b2760b
Results (out of 13 total builds):
    success: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-eb3422b2760b

Comment 23

5 years ago
Try run for 15a8fe343ac4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=15a8fe343ac4
Results (out of 90 total builds):
    success: 82
    warnings: 7
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-15a8fe343ac4
Attachment #671280 - Attachment description: art 2: Use EnumSet in gfxPlatform for prefs v6 → Part 2: Use EnumSet in gfxPlatform for prefs v6

Comment 24

5 years ago
Try run for 15a8fe343ac4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=15a8fe343ac4
Results (out of 91 total builds):
    success: 83
    warnings: 7
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-15a8fe343ac4
Comment on attachment 670552 [details] [diff] [review]
Part 1: Added EnumSet to replace 1 << n bit fields v4

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

::: mfbt/EnumSet.h
@@ +5,5 @@
> +
> +/* A set abstraction for enumeration values. */
> +
> +#ifndef mozilla_EnumSet_h_
> +#define mozilla_EnumSet_h_

No trailing _, please

@@ +8,5 @@
> +#ifndef mozilla_EnumSet_h_
> +#define mozilla_EnumSet_h_
> +
> +#include "mozilla/Types.h"
> +#include "mozilla/Assertions.h"

Sort alphabetically
Created attachment 674099 [details] [diff] [review]
Part 1: Added EnumSet to replace 1 << n bit fields v5
Attachment #670552 - Attachment is obsolete: true
Attachment #670552 - Flags: review?(jwalden+bmo)
Attachment #674099 - Flags: review?(jwalden+bmo)
Comment on attachment 674099 [details] [diff] [review]
Part 1: Added EnumSet to replace 1 << n bit fields v5

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

::: mfbt/EnumSet.h
@@ +8,5 @@
> +#ifndef mozilla_EnumSet_h
> +#define mozilla_EnumSet_h
> +
> +#include "mozilla/Assertions.h"
> +#include "mozilla/Types.h"

Use "mozilla/StandardInteger.h" instead, please -- I'd like to split up Types.h a bit at some point, and I think you're only using the standard-integer portion of that header.

@@ +22,5 @@
> +class EnumSet
> +{
> +  public:
> +    EnumSet()
> +     : mBitField(0)

EnumSet()
  : mBitField(0)
{ }

@@ +26,5 @@
> +     : mBitField(0)
> +    { }
> +
> +    EnumSet(T aEnum)
> +     : mBitField(1 << uint8_t(aEnum))

Two-space indent again.

@@ +69,5 @@
> +    /**
> +     * Add an element
> +     */
> +    void operator+=(T aEnum)
> +    {

void operator+=(T aEnum) {

Opening brace on same line for the rest of the methods, too (excepting the constructors).

@@ +71,5 @@
> +     */
> +    void operator+=(T aEnum)
> +    {
> +      MOZ_ASSERT(uint32_t(aEnum) < 32);
> +      mBitField |= 1 << uint32_t(aEnum);

Let's make a helper for (1 << uint32_t(aEnum)), not least because 1, being signed, left-shifted like that, could hit the sign bit, which triggers undefined behavior.  How about this:

  uint32_t bitFor(T aEnum) {
    uint32_t bitNumber(aEnum);
    MOZ_ASSERT(bitNumber < 32);
    return 1U << bitNumber;
  }

And use that in the various places that have these left-shifts, removing the asserts next to them at the same time.

@@ +161,5 @@
> +    /**
> +     * Equality
> +     */
> +
> +    bool operator==(EnumSet<T> aEnumSet)

bool operator==(const EnumSet<T>& aEnumSet) const, I think?

@@ +185,5 @@
> +      uint8_t count = 0;
> +      for (uint32_t bitField = mBitField; bitField; bitField >>= 1) {
> +        if (bitField & 1) {
> +          count++;
> +        }

Don't brace a single-line if.

There are faster ways to get a bit-count -- definitely O(number of bits set), I think even maybe O(1), too.  If someone ever needs this to be faster, we can make the change then.

::: mfbt/tests/TestEnumSet.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/EnumSet.h"

Explicitness suggests adding a #include "mozilla/Assertions.h" before this.

@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/EnumSet.h"
> +
> +using namespace mozilla;

Opening namespaces is kind of dodgy, as you get everything -- everything -- when you do it.  I'm experimenting with just using individual parts of namespaces, instead, so see how workable that is.  Please make this |using mozilla::EnumSet;|.

@@ +30,5 @@
> +  AUK
> +};
> +
> +class EnumSetSuite {
> +public:

class EnumSetSuite
{
  public:
    ...

@@ +33,5 @@
> +class EnumSetSuite {
> +public:
> +    EnumSetSuite()
> +      : mAlcidae()
> +      , mDiomedeidae(ALBATROSS)

EnumSetSuite()
  : mAlcidae()
    mDiomedeidae(ALBATROSS),
    ...
{ }

@@ +34,5 @@
> +public:
> +    EnumSetSuite()
> +      : mAlcidae()
> +      , mDiomedeidae(ALBATROSS)
> +      , mPetrelProcellariidae(GADFLY_PETREL, SeaBird::TRUE_PETREL)

Er, I thought enum initializers were added to the enclosing scope, and you couldn't access them via EnumName::Initializer.  At least, not unless you made the enum into a C++11 enum class, which we can't do generally yet.  I'm not sure why this works, but I don't think we should rely on it working -- remove the SeaBird:: prefixes, please.

@@ +41,5 @@
> +      , mPetrels(GADFLY_PETREL, SeaBird::TRUE_PETREL,
> +                 DIVING_PETREL, SeaBird::STORM_PETREL)
> +    { }
> +
> +    void RunTests()

Class methods are camelCaps: runTests(), testSize(), testContains(), and so on.

@@ +42,5 @@
> +                 DIVING_PETREL, SeaBird::STORM_PETREL)
> +    { }
> +
> +    void RunTests()
> +    {

void runTests() {

Same nit applies to all the other methods below here.  (Not to the constructor, tho -- empty method bodies are a special case.)

@@ +58,5 @@
> +      TestInsersection();
> +      TestEquality();
> +    }
> +
> +private:

Indent two spaces.

@@ +61,5 @@
> +
> +private:
> +    void TestSize()
> +    {
> +      MOZ_ASSERT(mAlcidae.size() == 0);

MOZ_ASSERT doesn't test anything in release builds, so this will only test in debug builds.  The right alternative is up in the air now (bug 807607), just noting it so that the issue is apparent even if we're not going to do anything about it right now.

@@ +65,5 @@
> +      MOZ_ASSERT(mAlcidae.size() == 0);
> +      MOZ_ASSERT(mDiomedeidae.size() == 1);
> +      MOZ_ASSERT(mPetrelProcellariidae.size() == 2);
> +      MOZ_ASSERT(mNonPetrelProcellariidae.size() == 3);
> +      MOZ_ASSERT(mPetrels.size() == 4);

Seems like it'd be good to test something that includes a bird more than once, to make sure that size() isn't affected by adding the same thing twice.  Although, hmm.  In the constructor case it's arguably a bug if the ctor is called with the same argument twice.  Maybe we should assert that constructor arguments are all distinct?  Followup territory.

@@ +140,5 @@
> +      procellariidae += mPetrelProcellariidae;
> +      procellariidae += mNonPetrelProcellariidae;
> +      MOZ_ASSERT(procellariidae.size() == 5);
> +
> +      // Both procellariidae and mPetrels include GADFLY_PERTEL and TRUE_PETREL

Complete sentence, should have a period at the end.  (Applies elsewhere too.)

@@ +154,5 @@
> +      EnumSet<SeaBird> procellariidae = mPetrelProcellariidae +
> +                                        mNonPetrelProcellariidae;
> +      MOZ_ASSERT(procellariidae.size() == 5);
> +
> +      // Both procellariidae and mPetrels include GADFLY_PERTEL and TRUE_PETREL

GADFLY_PETREL

@@ +231,5 @@
> +int main()
> +{
> +  EnumSetSuite suite;
> +  suite.RunTests();
> +}

Add a return 0 here for explicitness.
Attachment #674099 - Flags: review?(jwalden+bmo) → review+
Created attachment 678169 [details] [diff] [review]
Part 1: Added EnumSet to replace 1 << n bit fields v6
Attachment #674099 - Attachment is obsolete: true

Comment 29

5 years ago
Try run for 659069abfafe is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=659069abfafe
Results (out of 14 total builds):
    success: 13
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-659069abfafe

Comment 30

5 years ago
Try run for 659069abfafe is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=659069abfafe
Results (out of 15 total builds):
    success: 13
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-659069abfafe
Attachment #671280 - Flags: review?(ncameron)

Comment 31

5 years ago
Has anything changed significantly? Looks like you can just carry r=me. Let me know if there is anything I should look at.
Attachment #671280 - Flags: review?(ncameron)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a702087f7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c394c477d2a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3933841e06e
Flags: in-testsuite+
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe2b7e65899 - Win7 (with hardware accel, didn't affect Ru), the reftests bugs/632781-verybig.html and canvas/size-1.html were not verybig, they were both invisible, or at least blank.
https://hg.mozilla.org/mozilla-central/rev/40a702087f7d
https://hg.mozilla.org/mozilla-central/rev/c394c477d2a8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Comment 35

5 years ago
Try run for 4cfec568fcf4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cfec568fcf4
Results (out of 102 total builds):
    success: 84
    warnings: 14
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-4cfec568fcf4

Comment 36

5 years ago
Try run for 4cfec568fcf4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cfec568fcf4
Results (out of 103 total builds):
    success: 84
    warnings: 15
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-4cfec568fcf4

Comment 37

5 years ago
Try run for 4cfec568fcf4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cfec568fcf4
Results (out of 104 total builds):
    success: 84
    warnings: 16
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-4cfec568fcf4
I don't think this is actually fixed, one of the patches got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 39

5 years ago
Try run for 4cfec568fcf4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cfec568fcf4
Results (out of 105 total builds):
    success: 84
    warnings: 17
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-4cfec568fcf4
Created attachment 789332 [details] [diff] [review]
Used EnumSet in gfxPlatform instead of 1 << n;
Attachment #789332 - Flags: review?(matt.woodrow)
Attachment #671280 - Attachment is obsolete: true
Comment on attachment 789332 [details] [diff] [review]
Used EnumSet in gfxPlatform instead of 1 << n;

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

::: gfx/thebes/gfxPlatform.h
@@ +615,3 @@
>       */
> +    void InitBackendPrefs(
> +        mozilla::EnumSet<mozilla::gfx::BackendType> aCanvasSupportedBackends,

Lets add a typedef for this to something shorter, like BackendTypeSet.
Attachment #789332 - Flags: review?(matt.woodrow) → review+
https://tbpl.mozilla.org/?tree=Try&rev=fc6265a536a9
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.