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

RESOLVED FIXED in Firefox 47

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla48
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 fixed, firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 10 obsolete attachments)

58 bytes, text/x-review-board-request
ted
: review+
Details | Review
(Assignee)

Description

2 years ago
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.
status-firefox46: --- → unaffected
status-firefox47: --- → affected
tracking-firefox47: --- → ?
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Keywords: dev-doc-needed
Comment hidden (spam)
(Assignee)

Updated

2 years ago
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.
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8719887 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8719888 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8719889 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8719890 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8720389 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8720726 - Flags: review?(dbaron)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Comment 20

2 years ago
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)
(Assignee)

Comment 23

2 years ago
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.
(Assignee)

Comment 25

2 years ago
Created attachment 8722849 [details] [diff] [review]
0001-Bug-1243734-Part-1.-Add-MOZ_ENABLE_MASK_AS_SHORTHAND.patch
(Assignee)

Comment 26

2 years ago
Created attachment 8722850 [details] [diff] [review]
0002-Bug-1243734-Part-2.-Use-MOZ_ENABLE_MASK_AS_SHORTHAND.patch
(Assignee)

Comment 27

2 years ago
Created attachment 8722851 [details] [diff] [review]
0003-Bug-1243734-Part-3.-Fix-incorrect-rendering-result-w.patch
(Assignee)

Comment 28

2 years ago
Created attachment 8722852 [details] [diff] [review]
0004-Bug-1243734-Part-4.-Set-up-gCSSProperties-depends-on.patch
(Assignee)

Updated

2 years ago
Attachment #8722849 - Flags: feedback?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8722850 - Flags: feedback?(dbaron)
(Assignee)

Comment 29

2 years ago
Created attachment 8722853 [details] [diff] [review]
0003-Bug-1243734-Part-3.-Fix-incorrect-rendering-result-w.patch
Attachment #8722851 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8722853 - Flags: feedback?(dbaron)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 31

2 years ago
(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)
(Assignee)

Comment 32

2 years ago
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)
(Assignee)

Updated

2 years ago
Depends on: 1251115
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 42

2 years ago
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/
(Assignee)

Comment 43

2 years ago
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)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 44

2 years ago
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/
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 45

2 years ago
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/
(Assignee)

Comment 46

2 years ago
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)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Attachment #8722849 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8722850 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8722853 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8722852 - Attachment is obsolete: true

Updated

2 years ago
Status: NEW → ASSIGNED
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Updated

2 years ago
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
Comment hidden (spam)
(Assignee)

Updated

2 years ago
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
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8719887 - Flags: review?(ted)
(Assignee)

Comment 58

2 years ago
Hi Ted,
Since Part 1 changed old-configure.in, please help to review.
(Assignee)

Comment 59

2 years ago
(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
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 65

2 years ago
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
Attachment #8719887 - Flags: review?(ted)
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).
(Assignee)

Comment 72

2 years ago
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...
(Assignee)

Updated

2 years ago
Attachment #8719887 - Flags: review?(ted)
(Assignee)

Comment 73

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8719888 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8719889 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8719890 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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.
tracking-firefox47: ? → +
(Assignee)

Comment 77

2 years ago
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
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 78

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7fd5da36595
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 79

2 years ago
(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.
(Assignee)

Updated

2 years ago
tracking-firefox47: + → ---

Updated

2 years ago
status-firefox47: affected → fixed

Updated

2 years ago
Blocks: 1301638
You need to log in before you can comment on or make changes to this bug.