Closed Bug 1243734 Opened 4 years ago Closed 4 years ago

Have a pref to enable/ disable all new mask CSS properties added in bug 686281

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 10 obsolete files)

58 bytes, text/x-review-board-request
ted
: review+
Details
According to bug 686281 comment 447, add a pref to enable all of the new work in bug 686281 when it's set to true, but make us support only what we supported before this patch when set to false
[Tracking Requested - why for this release]:  We either need to land this in this cycle in order to properly pref-off the feature that landed which is not ready to ship, or we need to back that feature out.
Attachment #8719887 - Attachment description: MozReview Request: Bug 1243734 - Create a new pref to swtich mask between a longhand and a shorthand property. → MozReview Request: Bug 1243734: Part 1. Create a preference to swtich mask between longhand and shorthand.
Attachment #8719887 - Flags: review?(dbaron)
Attachment #8719888 - Flags: review?(dbaron)
Attachment #8719889 - Flags: review?(dbaron)
Attachment #8719890 - Flags: review?(dbaron)
Attachment #8720389 - Flags: review?(dbaron)
Attachment #8720726 - Flags: review?(dbaron)
bug 1239441 comment 34 need to be addressed before this bug landed
Comment on attachment 8719887 [details]
MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag;

https://reviewboard.mozilla.org/r/35159/#review32227

The separation between patches doesn't really seem to make sense here, but I'm not going to worry about it this time.

Please fix the spelling of "switch" in the commit message.
Attachment #8719887 - Flags: review?(dbaron) → review+
Comment on attachment 8719888 [details]
MozReview Request: Bug 1243734 - Part 2. Use MOZ_ENABLE_MASK_AS_SHORTHAND to define the type of mask property; r=dbaron

https://reviewboard.mozilla.org/r/35161/#review32359

::: layout/style/nsCSSDataBlock.cpp:722
(Diff revision 3)
> -  CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aPropID, aEnabledState) {
> +  CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aPropID,
> +                                       (aEnabledState |
> +                                        nsCSSProps::eForceEnableMaskImage)) {

It seems to me like you want this change for all callers of CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES, not just the 3 that you modified in this patch.

It seems like instead of adding a new enabled flag and making these changes to the users of CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES, you could just change the CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES macro itself to explicitly check for mask_image after calling IsEnabled.

Even with that, though, this approach seems a little bit sketchy; I'm trying to think if there's a better way.

::: layout/style/nsCSSParser.cpp:11728
(Diff revision 3)
> +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  nsCSSValueList* imageList = image.SetListValue();
> +  // At the moment mask been treated as longhand, it only accept VARIANT_HUO
> +  // typed value.
> +  if (keyword == eCSSKeyword_none) {
> +    if (ParseSingleValueProperty(imageList->mValue, eCSSProperty_mask_image) !=
> +          CSSParseResult::Ok) {
> +      NS_NOTREACHED("should be able to parse");
> +      return false;
> +    }
> +  } else if (mToken.mType == eCSSToken_URL) {
> +    if (ParseSingleValueProperty(imageList->mValue, eCSSProperty_mask_image) !=
> +          CSSParseResult::Ok) {
> +      return false;
> +    }
> +  } else {
> +    return false;
> +  }

You don't need this complexity.  Skip testing anything on keyword, and just call ParseVariant(imageList->mValue, VARIANT_URL | VARIANT_NONE, nullptr), return false if it wasn't CSSParseResult::OK, and then (otherwise) call AppendValue.
Attachment #8719888 - Flags: review?(dbaron)
https://reviewboard.mozilla.org/r/35161/#review32359

> It seems to me like you want this change for all callers of CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES, not just the 3 that you modified in this patch.
> 
> It seems like instead of adding a new enabled flag and making these changes to the users of CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES, you could just change the CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES macro itself to explicitly check for mask_image after calling IsEnabled.
> 
> Even with that, though, this approach seems a little bit sketchy; I'm trying to think if there's a better way.

Since I didn't find another case that need shorthand to/from longhand runtime switching, so I choosed this simple solution, which work for mask only and change the least code.
The alternative approach would be to use #ifdefs for the "backend" half of the code, i.e., nsCSSPropList.h and things that fall out from it, while leaving the same structures in nsStyleStruct.  This would mean you'd have to write a little bit of new code in nsRuleNode to turn the single mask property into a one-item list with just a mask-image.

This seems like it's likely to be cleaner, and less risky in terms of missing things or introducing weird behaviors, although slightly more risky in terms of risk of compile-time bustage when merging.

It's hard to tell for sure without trying it, though.
Attachment #8722849 - Flags: feedback?(dbaron)
Attachment #8722850 - Flags: feedback?(dbaron)
Attachment #8722851 - Attachment is obsolete: true
Attachment #8722853 - Flags: feedback?(dbaron)
Attachment #8722852 - Flags: feedback?(dbaron)
Could you explain why Part 3 is needed?  Shouldn't part 2 have been done so that the structure is the same, and it's not needed?
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #30)
> Could you explain why Part 3 is needed?  Shouldn't part 2 have been done so
> that the structure is the same, and it's not needed?

Short answer: we have a bug in nsSVGIntegrationUtils::PaintFramesWithEffects, change in part 2 bring this bug to the surface. 
And I am writing a longer explanation.
Flags: needinfo?(cku)
I catch this issue while testing https://dxr.mozilla.org/mozilla-central/source/layout/reftests/svg/mask-basic-01.svg

Let have a look on this if statement in nsSVGIntegrationUtils::PaintFramesWithEffects
if (svgMaskFrame) {
  // (a)
} else {
  // (b)
}

1. Before bug 686281, (b) dose not exist yet. If mask url is not resolvable, (a) will be skipped, so we do nothing at that moment.
2. After bug 686281, if mask url is not resolvable, (b) is executed.
3. After Part 2 of this bug, if mask url is not resolvable, (b) is executed.

#2: the type of svgReset->mMask.mLayers[i].mImage(which is nsStyleImage) is eStyleImageType_Image
If current document is a nsSVGDocument, this document will assign an empty vector image to svgReset->mMask.mLayers[i].mImage(which is nsStyleImage) even if the url path is wrong. We still have one mask image render onto target maskSurface. So the render result is _magically_ correct while testing the patches of bug 686281.

#3: the type of svgReset->mMask.mLayers[i].mImage is eStyleImageType_Null.
In Part2, nsRuleNode.cpp only set up mSourceURI and left mImage blank. Now we don’t have any mask image render onto maskSurface, left it totally transparent, which mean every pixels of background will be filter out.
Comment on attachment 8722853 [details] [diff] [review]
0003-Bug-1243734-Part-3.-Fix-incorrect-rendering-result-w.patch

It sounds like you're saying that part 3 fixes an independent problem that just happened to not show up when you landed bug 686281.  Could you put that in its own bug report (blocking this one), explain the problem clearly there, and request review from an appropriate reviewer (which I don't think I am).
Attachment #8722853 - Flags: feedback?(dbaron)
Comment on attachment 8722849 [details] [diff] [review]
0001-Bug-1243734-Part-1.-Add-MOZ_ENABLE_MASK_AS_SHORTHAND.patch

I don't think you need or want a configure option; this is something that we should decide about.  You also don't need an AC_SUBST(), since all you use are #ifdefs in C++ files.  So you should just add a commented-out top-level AC_DEFINE() line somewhere to indicate that uncommenting it would enable the feature.

(This has now moved to old-configure.in.)

You probably want build peer review as well given how much configure is changing right now.
Attachment #8722849 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 8722850 [details] [diff] [review]
0002-Bug-1243734-Part-2.-Use-MOZ_ENABLE_MASK_AS_SHORTHAND.patch

>+#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
>         MOZ_ASSERT(aTable == nsStyleImageLayers::kMaskLayerTable);
>+#endif

>+#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
>       MOZ_ASSERT(aTable == nsStyleImageLayers::kMaskLayerTable);
>+#endif

These should have an #else with a MOZ_ASSERT_UNREACHABLE() in it.

>diff --git a/layout/style/nsCSSPropAliasList.h b/layout/style/nsCSSPropAliasList.h
>-
>+#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
> CSS_PROP_ALIAS(-webkit-mask,

Can't you leave -webkit-mask and ifdef just the others?

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>+#else
>+  // mask: none | <url>
>+  const nsCSSValue* maskValue = aRuleData->ValueForMask();
>+  if (eCSSUnit_URL == maskValue->GetUnit()) {
>+    svgReset->mMask.mLayers[0].mSourceURI = maskValue->GetURLValue();
>+    rebuild = true;
>+  } else if (eCSSUnit_None == maskValue->GetUnit() ||
>+             eCSSUnit_Initial == maskValue->GetUnit() ||
>+             eCSSUnit_Unset == maskValue->GetUnit()) {
>+    svgReset->mMask.mLayers[0].mSourceURI = nullptr;
>+    rebuild = true;
>+  } else if (eCSSUnit_Inherit == maskValue->GetUnit()) {
>+    conditions.SetUncacheable();
>+    svgReset->mMask.mLayers[0].mSourceURI =
>+      parentSVGReset->mMask.mLayers[0].mSourceURI;
>+    rebuild = true;
>+  }
>+#endif
> 
>   if (rebuild) {

Instead of setting rebuild in this block, you should just be able to #ifdef out the maxItemCount and rebuild variables, and the entire if (rebuild) block.



With that, I think this looks good, and I think, assuming that tests pass, that it's a lot safer than the other approach.
Attachment #8722850 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 8722852 [details] [diff] [review]
0004-Bug-1243734-Part-4.-Set-up-gCSSProperties-depends-on.patch

>+function IsSupportMasAsShorthand() {

This should be called SupportsMaskShorthand.

>+  var div = document.createElement("div");
>+  var url = "url(#myurl)";
>+  div.style.setProperty("mask-image", url);
>+  var maskImageValue = div.style.getPropertyValue("mask-image", "");
>+  return (maskImageValue == url);

Instead of this, you should be able to just do:

  return "maskImage" in document.documentElement.style;



I didn't review the rest in detail yet since you didn't request review, but I'm assuming you're just moving chunks of code appropriately.
Attachment #8722852 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 8719889 [details]
MozReview Request: Bug 1243734 - Part 3. Set up gCSSProperties depends on mask-as-shorthand; r=dbaron

https://reviewboard.mozilla.org/r/35163/#review33047

This should no longer be needed with the new approach.
Attachment #8719889 - Flags: review?(dbaron)
Comment on attachment 8719890 [details]
MozReview Request: Bug 1243734 - Part 4. Set mask-mode reftest as failure before enable mask-as-shorthand; r=dbaron

https://reviewboard.mozilla.org/r/35165/#review33049

I *think* this should no longer be needed with the new approach.
Attachment #8719890 - Flags: review?(dbaron)
Comment on attachment 8720389 [details]
MozReview Request: Bug 1243734 - Part 5. Convert mask-image to mask; r=dbaron

https://reviewboard.mozilla.org/r/35283/#review33053

This will need some sort of replacement with the new approach, but it won't look like this patch.
Attachment #8720389 - Flags: review?(dbaron)
Comment on attachment 8720726 [details]
MozReview Request: Bug 1243734: Part 6. Bring mask-as-shorthand-pref into mochitest.

https://reviewboard.mozilla.org/r/35429/#review33055

Seems like I've already looked at the new version of this patch, so this one isn't needed anymore.
Attachment #8720726 - Flags: review?(dbaron)
Depends on: 1251115
Attachment #8719887 - Attachment description: MozReview Request: Bug 1243734: Part 1. Create a preference to swtich mask between longhand and shorthand. → MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND in configure.in
Comment on attachment 8719887 [details]
MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35159/diff/3-4/
Comment on attachment 8719888 [details]
MozReview Request: Bug 1243734 - Part 2. Use MOZ_ENABLE_MASK_AS_SHORTHAND to define the type of mask property; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35161/diff/3-4/
Attachment #8719888 - Attachment description: MozReview Request: Bug 1243734: Part 2. Depend on preference, parse mask as longhand or shorthand. → MozReview Request: Bug 1243734 - Part 2. Use MOZ_ENABLE_MASK_AS_SHORTHAND to define the type of mask property
Attachment #8719888 - Flags: review?(dbaron)
Attachment #8719889 - Attachment description: MozReview Request: Bug 1243734: Part 3. Access mask declaration from DOM API as longhand property. → MozReview Request: Bug 1243734 - Part 3. Set up gCSSProperties depends on whether mask-as-shorthand is enabled.
Attachment #8719889 - Flags: review?(dbaron)
Comment on attachment 8719889 [details]
MozReview Request: Bug 1243734 - Part 3. Set up gCSSProperties depends on mask-as-shorthand; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35163/diff/3-4/
Attachment #8719890 - Attachment description: MozReview Request: Bug 1243734: Part 4. Handle mask webkit alias. → MozReview Request: Bug 1243734 - Part 4. Set mask-mode reftest as failure before enable mask-as-shorthand.
Attachment #8719890 - Flags: review?(dbaron)
Comment on attachment 8719890 [details]
MozReview Request: Bug 1243734 - Part 4. Set mask-mode reftest as failure before enable mask-as-shorthand; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35165/diff/3-4/
Comment on attachment 8720389 [details]
MozReview Request: Bug 1243734 - Part 5. Convert mask-image to mask; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35283/diff/2-3/
Attachment #8720389 - Attachment description: MozReview Request: Bug 1243734: Part 5. Bring mask-as-shorthand-pref into reftest. → MozReview Request: Bug 1243734 - Part 5. Remove mask-image used in index.css of devtools
Attachment #8720389 - Flags: review?(dbaron)
Attachment #8720726 - Attachment is obsolete: true
Comment on attachment 8719887 [details]
MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag;

The contents of this attachment are no longer related to the thing I reviewed.  You should ask a build peer for review on this patch.
Attachment #8719887 - Flags: review+
Comment on attachment 8719888 [details]
MozReview Request: Bug 1243734 - Part 2. Use MOZ_ENABLE_MASK_AS_SHORTHAND to define the type of mask property; r=dbaron

https://reviewboard.mozilla.org/r/35161/#review33301

::: layout/style/Declaration.cpp:357
(Diff revision 4)
> +        MOZ_ASSERT_UNREACHABLE("Should never get here when disable mask-as-shorthand");

..."when mask-as-shorthand is disabled"

::: layout/style/Declaration.cpp:380
(Diff revision 4)
> +      MOZ_ASSERT_UNREACHABLE("Should never get here when disable mask-as-shorthand");

same here
Attachment #8719888 - Flags: review?(dbaron) → review+
Comment on attachment 8719889 [details]
MozReview Request: Bug 1243734 - Part 3. Set up gCSSProperties depends on mask-as-shorthand; r=dbaron

https://reviewboard.mozilla.org/r/35163/#review33309

I notice that you don't test -webkit-mask here.  However, on second thoughts, I think I was probably wrong in asking you to leave -webkit-mask enabled, since I think the main Web-compatibility use case for -webkit-mask is for the new syntax and not the old syntax.  So I think you should probably leave this patch as-is, but change the previous patch to hide -webkit-mask inside the #ifdef again.  However, you should probably double-check that logic with dholbert first.
Attachment #8719889 - Flags: review?(dbaron) → review+
Attachment #8719890 - Flags: review?(dbaron) → review+
Comment on attachment 8719890 [details]
MozReview Request: Bug 1243734 - Part 4. Set mask-mode reftest as failure before enable mask-as-shorthand; r=dbaron

https://reviewboard.mozilla.org/r/35165/#review33311
Comment on attachment 8720389 [details]
MozReview Request: Bug 1243734 - Part 5. Convert mask-image to mask; r=dbaron

https://reviewboard.mozilla.org/r/35283/#review33313

The commit message should probably say "Convert mask-image to mask" instead of "Remove mask-image used"
Attachment #8720389 - Flags: review?(dbaron) → review+
Attachment #8722849 - Attachment is obsolete: true
Attachment #8722850 - Attachment is obsolete: true
Attachment #8722853 - Attachment is obsolete: true
Attachment #8722852 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8719890 - Attachment description: MozReview Request: Bug 1243734 - Part 4. Set mask-mode reftest as failure before enable mask-as-shorthand. → MozReview Request: Bug 1243734 - Part 4. Set mask-mode reftest as failure before enable mask-as-shorthand; r=dbaron
Attachment #8720389 - Attachment description: MozReview Request: Bug 1243734 - Part 5. Remove mask-image used in index.css of devtools → MozReview Request: Bug 1243734 - Part 5. Convert mask-image to mask; r=dbaron
Attachment #8719887 - Flags: review?(ted)
Hi Ted,
Since Part 1 changed old-configure.in, please help to review.
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #35)
> Comment on attachment 8722849 [details] [diff] [review]
> 0001-Bug-1243734-Part-1.-Add-MOZ_ENABLE_MASK_AS_SHORTHAND.patch
> 
> I don't think you need or want a configure option; this is something that we
> should decide about.  You also don't need an AC_SUBST(), since all you use
> are #ifdefs in C++ files.  So you should just add a commented-out top-level
> AC_DEFINE() line somewhere to indicate that uncommenting it would enable the
> feature.
Actually, AC_SUBST() is needed since we need to to test MOZ_ENABLE_MASK_AS_SHORTHAND in layout/style/test/moz.build:
if CONFIG['MOZ_ENABLE_MASK_AS_SHORTHAND']:
   HOST_DEFINES['MOZ_ENABLE_MASK_AS_SHORTHAND'] = True
   DEFINES['MOZ_ENABLE_MASK_AS_SHORTHAND'] = True
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/99c71eb074cfacb8fbbbc51c6f7c52d2efe5f464
Bug 1243734 - Part 1. Use MOZ_ENABLE_MASK_AS_SHORTHAND to define the type of mask property; r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1c774bd78b71e69af4e8804edc1a80ac3cd5c5
Bug 1243734 - Part 2. Set up gCSSProperties depends on mask-as-shorthand; r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/a66ae52f9dc9ddd13baf60f8b94c47ca4247fa5e
Bug 1243734 - Part 3. Set mask-mode reftest as failure before enable mask-as-shorthand; r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd725321463610780d209963e9a6999d52faff0e
Bug 1243734 - Part 4. Convert mask-image to mask; r=dbaron
Comment on attachment 8719887 [details]
MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag;

https://reviewboard.mozilla.org/r/35159/#review34383

::: old-configure.in:9008
(Diff revision 6)
> +  AC_DEFINE(MOZ_ENABLE_MASK_AS_SHORTHAND)

You don't need both the AC_DEFINE and the DEFINES in the moz.build below. If you only need this defined in that one directory then just use the moz.build version below. If you need this defined globally then you can use AC_DEFINE.
I am wondering why this was implemented as a compiler flag instead of a preference. Or will this preference still be implemented?

Sebastian
Flags: needinfo?(cku)
It's implemented as a compiler flag because it's not practical to implement as a preference, since we need to continue supporting the old 'mask' syntax when the new syntax is disabled.
Flags: needinfo?(cku)
Well, I assume it could have been implemented as a boolean preference, where false = old 'mask' syntax and true = new 'mask' syntax plus longhands. With the compiler flag in place authors are not able to try out the new features (if they don't compile the code by themselves).

Sebastian
Not easily, and not without serious risk of leaving one or the other options quite broken.  See comments above (esp. comment 22 to 24).
https://reviewboard.mozilla.org/r/35159/#review34383

> You don't need both the AC_DEFINE and the DEFINES in the moz.build below. If you only need this defined in that one directory then just use the moz.build version below. If you need this defined globally then you can use AC_DEFINE.

I don't need DEFINES in layout/style/test/moz.build, but I do need HOST_DEFINE in that file. 

Without "HOST_DEFINES['MOZ_ENABLE_MASK_AS_SHORTHAND'] = True" in layout/style/test/moz.build, 
"./mach build -v" got:
/usr/bin/clang++ -o host_ListCSSProperties.o -c -std=gnu++0x -MD -MP -MF .deps/host_ListCSSProperties.o.pp -DDEBUG=1 -DTRACING=1  -I....
Miss -DMOZ_ENABLE_MASK_AS_SHORTHAND and make test case failed.

With HOST_DEFINES
"./mach build -v" got:
/usr/bin/clang++ -o host_ListCSSProperties.o -c -std=gnu++0x -MD -MP -MF .deps/host_ListCSSProperties.o.pp -DDEBUG=1 -DTRACING=1 -DMOZ_ENABLE_MASK_AS_SHORTHAND  -I...
Which is what I need.

I actually have no idea of what is HOST_DEFINES. Please enlighten me if you don't mind...
Attachment #8719887 - Flags: review?(ted)
Comment on attachment 8719887 [details]
MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35159/diff/6-7/
Attachment #8719888 - Attachment is obsolete: true
Attachment #8719889 - Attachment is obsolete: true
Attachment #8719890 - Attachment is obsolete: true
Attachment #8720389 - Attachment is obsolete: true
Attachment #8719887 - Flags: review?(ted) → review+
Comment on attachment 8719887 [details]
MozReview Request: Bug 1243734 - Part 1. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag;

https://reviewboard.mozilla.org/r/35159/#review34661
https://reviewboard.mozilla.org/r/35159/#review34383

> I don't need DEFINES in layout/style/test/moz.build, but I do need HOST_DEFINE in that file. 
> 
> Without "HOST_DEFINES['MOZ_ENABLE_MASK_AS_SHORTHAND'] = True" in layout/style/test/moz.build, 
> "./mach build -v" got:
> /usr/bin/clang++ -o host_ListCSSProperties.o -c -std=gnu++0x -MD -MP -MF .deps/host_ListCSSProperties.o.pp -DDEBUG=1 -DTRACING=1  -I....
> Miss -DMOZ_ENABLE_MASK_AS_SHORTHAND and make test case failed.
> 
> With HOST_DEFINES
> "./mach build -v" got:
> /usr/bin/clang++ -o host_ListCSSProperties.o -c -std=gnu++0x -MD -MP -MF .deps/host_ListCSSProperties.o.pp -DDEBUG=1 -DTRACING=1 -DMOZ_ENABLE_MASK_AS_SHORTHAND  -I...
> Which is what I need.
> 
> I actually have no idea of what is HOST_DEFINES. Please enlighten me if you don't mind...

HOST_DEFINES is just the defines we use for compiling host binaries (binaries that run on the system doing the build, our terminology doesn't match modern autoconf here). We build ListCSSProperties as a host binary, so it needs this define.
Tracked, will be happy to uplift to Aurora47 once the fix is ready and baked enough on Nightly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fd5da36595139efc6d103249b36a5338d0efb9
Bug 1243734 - Part 5. Add MOZ_ENABLE_MASK_AS_SHORTHAND compile flag; r=ted r=dbaron
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/b7fd5da36595
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Ritu Kothari (:ritu) from comment #76)
> Tracked, will be happy to uplift to Aurora47 once the fix is ready and baked
> enough on Nightly.

Thanks, but no need. I had pushed part 2 ~ part 4 into gecko 47 before 3/7, please see comment 66, which is all we need to disable image-mask. Part 1 can be stayed in gecko 48.
Blocks: 1301638
You need to log in before you can comment on or make changes to this bug.