Closed Bug 984761 Opened 6 years ago Closed 6 years ago

Add IPDL serialization and operator== for FilterDescription

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(18 files, 1 obsolete file)

1.26 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.25 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.56 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.39 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
49.59 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.04 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
9.65 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.56 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.58 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
2.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.54 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.45 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8392718 - Attachment description: part 9: IPDL serialization for the PrimitiveType enum → part 9: IPDL serialization for the ColorSpace enum
I didn't make this a a typed enum because it's converted between int and enum values in a few places and because I want to spare the users the "AttributeName::" prefix.
Is the intention to make all enums typed?
Attachment #8392720 - Flags: review?(bjacob)
We need this for AttributeMap IPDL serialization.
Attachment #8392725 - Flags: review?(roc)
fixed whitespace
Attachment #8392713 - Attachment is obsolete: true
Attachment #8392713 - Flags: review?(bjacob)
Attachment #8392733 - Flags: review?(bjacob)
Comment on attachment 8392722 [details] [diff] [review]
part 11: Add FilterAttribute::operator==

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

::: gfx/src/FilterSupport.cpp
@@ +1570,5 @@
>  
> +template<typename T>
> +static bool
> +AreArraysEqual(const nsTArray<T>& aLeft, const nsTArray<T>& aRight)
> +{

Use nsTArray operator==
Attachment #8392722 - Flags: review?(roc) → review+
Comment on attachment 8392724 [details] [diff] [review]
part 13: Add FilterPrimitiveDescription::operator==

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

::: gfx/src/FilterSupport.cpp
@@ +1600,5 @@
> +bool
> +FilterDescription::operator==(const FilterDescription& aOther) const
> +{
> +  return mFilterSpaceBounds.IsEqualInterior(aOther.mFilterSpaceBounds) &&
> +      AreArraysEqual(mPrimitives, aOther.mPrimitives);

nsTArray operator== here and above
Attachment #8392724 - Flags: review?(roc) → review+
Attachment #8392708 - Flags: review?(bjacob) → review+
Comment on attachment 8392709 [details] [diff] [review]
part 2: IPDL serialization for gfx::Point3D

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

::: gfx/ipc/GfxMessageUtils.h
@@ +483,5 @@
> +  static bool Read(const Message* msg, void** iter, paramType* result)
> +  {
> +    return (ReadParam(msg, iter, &result->x) &&
> +            ReadParam(msg, iter, &result->y) &&
> +            ReadParam(msg, iter, &result->z));

Note that here and in other Read() methods, the outer (...) brackets of the return expression are unneeded.
Attachment #8392709 - Flags: review?(bjacob) → review+
Attachment #8392711 - Flags: review?(bjacob) → review+
Comment on attachment 8392733 [details] [diff] [review]
part 4: Use a typed enum for attribute types

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

Excellent, I love typed enums.
Attachment #8392733 - Flags: review?(bjacob) → review+
Comment on attachment 8392714 [details] [diff] [review]
part 5: IPDL serialization for the AttributeType enum

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

::: gfx/ipc/GfxMessageUtils.h
@@ +24,5 @@
>  #include "gfxTypes.h"
>  #include "mozilla/layers/LayersTypes.h"
>  #include "mozilla/layers/CompositorTypes.h"
>  #include "FrameMetrics.h"
> +#include "FilterSupport.h"

Note that GfxMessageUtils.h is the primary path through which gfx headers get included all over Gecko, all the way to netwerk/  (see bug 918651). So we should be careful when adding #includes here. R+, because most of what FilterSupport.h includes seems to already be (recursively) included here anyway. Just something to keep in mind. A way to make this better would be to split the stuff you need here (the enum) into a new light header, "FilterSupportTypes.h", and only include that here; and then, do the same with the other headers included here.
Attachment #8392714 - Flags: review?(bjacob) → review+
Comment on attachment 8392715 [details] [diff] [review]
part 6: Use a typed enum for filter primitive types

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

::: gfx/src/FilterSupport.h
@@ +227,5 @@
>    AlphaModel mAlphaModel;
>  };
>  
> +MOZ_BEGIN_ENUM_CLASS(PrimitiveType)
> +  None = 0,

'None' is a macro defined by X11 headers. This might collide here, especially with unified builds, especially as this gets included in GfxMessageUtils.h which is included all over the place. I know that in other places I have had to back out from using None as an identifier because of this issue. It's fine to keep it if you don't run into collisions. Maybe push to try on linux.
Attachment #8392715 - Flags: review?(bjacob) → review+
Attachment #8392716 - Flags: review?(bjacob) → review+
Comment on attachment 8392717 [details] [diff] [review]
part 8: Use a typed enum for ColorSpace and AlphaModel

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

::: layout/svg/nsSVGFilterInstance.cpp
@@ +376,5 @@
> +
> +      ColorSpace inputColorSpace = ColorSpace::SRGB;
> +      if (inputIndex >= 0) {
> +        inputColorSpace = aPrimitiveDescrs[inputIndex].OutputColorSpace();
> +      }

I would prefer if you kept it as the ternary ?: that it was.

(Generally I prefer ternary expressions over updating in place a local variable).
Attachment #8392717 - Flags: review?(bjacob) → review+
Attachment #8392718 - Flags: review?(bjacob) → review+
Attachment #8392720 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #26)
> Comment on attachment 8392717 [details] [diff] [review]
> part 8: Use a typed enum for ColorSpace and AlphaModel
> 
> Review of attachment 8392717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/svg/nsSVGFilterInstance.cpp
> @@ +376,5 @@
> > +
> > +      ColorSpace inputColorSpace = ColorSpace::SRGB;
> > +      if (inputIndex >= 0) {
> > +        inputColorSpace = aPrimitiveDescrs[inputIndex].OutputColorSpace();
> > +      }
> 
> I would prefer if you kept it as the ternary ?: that it was.
> 
> (Generally I prefer ternary expressions over updating in place a local
> variable).

Oh yes, I should have mentioned the reason for that change. Without it, I got the following build error on B2G:

> operands to ?: have different types 'mozilla::gfx::ColorSpace::Enum' and 'mozilla::gfx::ColorSpace'
https://tbpl.mozilla.org/php/getParsedLog.php?id=36099057&tree=Try#error0

Do you know of any other simple remedies for that? I also prefer the ternary expression.
(In reply to Markus Stange [:mstange] from comment #27)
> (In reply to Benoit Jacob [:bjacob] from comment #26)
> > Comment on attachment 8392717 [details] [diff] [review]
> > part 8: Use a typed enum for ColorSpace and AlphaModel
> > 
> > Review of attachment 8392717 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/svg/nsSVGFilterInstance.cpp
> > @@ +376,5 @@
> > > +
> > > +      ColorSpace inputColorSpace = ColorSpace::SRGB;
> > > +      if (inputIndex >= 0) {
> > > +        inputColorSpace = aPrimitiveDescrs[inputIndex].OutputColorSpace();
> > > +      }
> > 
> > I would prefer if you kept it as the ternary ?: that it was.
> > 
> > (Generally I prefer ternary expressions over updating in place a local
> > variable).
> 
> Oh yes, I should have mentioned the reason for that change. Without it, I
> got the following build error on B2G:
> 
> > operands to ?: have different types 'mozilla::gfx::ColorSpace::Enum' and 'mozilla::gfx::ColorSpace'
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36099057&tree=Try#error0
> 
> Do you know of any other simple remedies for that? I also prefer the ternary
> expression.

Ah yes, this is because B2G still uses GCC 4.4 which does not support C++11 enum classes, and so on B2G the MOZ_BEGIN_ENUM_CLASS implementation has to use a class containing a nested enum type, which is the ColorSpace::Enum here. As a result, the actual type of the enum values is ColorSpace::Enum, not ColorSpace, but the OutputColorSpace() function returns ColorSpace, so we have a mismatch between the two branches of this ternary ?: which is the error here.

The simple fix for this is to cast the enum value, SRGB, to ColorSpace  (from ColorSpace::Enum).
(In reply to Benoit Jacob [:bjacob] (on vacation until March 24) from comment #22)
> Comment on attachment 8392709 [details] [diff] [review]
> part 2: IPDL serialization for gfx::Point3D
> 
> Review of attachment 8392709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/ipc/GfxMessageUtils.h
> @@ +483,5 @@
> > +  static bool Read(const Message* msg, void** iter, paramType* result)
> > +  {
> > +    return (ReadParam(msg, iter, &result->x) &&
> > +            ReadParam(msg, iter, &result->y) &&
> > +            ReadParam(msg, iter, &result->z));
> 
> Note that here and in other Read() methods, the outer (...) brackets of the
> return expression are unneeded.

Yeah, I've kept it this way because all the other Read() methods in that file do the same thing.

(In reply to Benoit Jacob [:bjacob] (on vacation until March 24) from comment #25)
> Comment on attachment 8392715 [details] [diff] [review]
> part 6: Use a typed enum for filter primitive types
> 
> Review of attachment 8392715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/FilterSupport.h
> @@ +227,5 @@
> >    AlphaModel mAlphaModel;
> >  };
> >  
> > +MOZ_BEGIN_ENUM_CLASS(PrimitiveType)
> > +  None = 0,
> 
> 'None' is a macro defined by X11 headers. This might collide here,
> especially with unified builds, especially as this gets included in
> GfxMessageUtils.h which is included all over the place. I know that in other
> places I have had to back out from using None as an identifier because of
> this issue. It's fine to keep it if you don't run into collisions. Maybe
> push to try on linux.

You were right, I ran into exactly this problem. I've renamed None to Empty to get around it.
Depends on: 1215900
You need to log in before you can comment on or make changes to this bug.