1297333 - Enable "layout.css.clip-path-shapes.enabled" in test suite

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

We might want to enable "layout.css.clip-path-shapes.enabled" in "testing/profiles/prefs_general.js" so that any CSS changes to clip-path will go through testing.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Enable "layout.css.clip-path-shapes.enabled" in "testing/profiles/prefs_general.js" to see if anything fails.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5b9cd3647d

Comment 2

[Daniel Holbert [:dholbert]]

Sounds good to me, as long as the tests pass.

(I thought I tested this somewhat recently & tests still failed; but I'd be pleasantly surprised to be wrong. :))

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1297333 - Enable basic shape for clip-path in test suite.

Review commit: https://reviewboard.mozilla.org/r/73754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73754/

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Try result in comment 1 looks good! It seems CJ fixed the remaining test failures in bug 1269990.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tend to think it's important to test the code that we ship, so I prefer not to enable different things for tests than for the builds on the branch in question.

It seems more valuable to make progress on whatever is needed to enable the feature on nightly and aurora.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #5)
> I tend to think it's important to test the code that we ship, so I prefer
> not to enable different things for tests than for the builds on the branch
> in question.
> 
> It seems more valuable to make progress on whatever is needed to enable the
> feature on nightly and aurora.

I agree it would be more valuable to resolve bugs (those depend on bug 1247229) to enable the feature on nightly and aurora, but in the mean time, it's good to have the style system part of clip-path covered by automated tests, isn't it?

Comment 7

[Daniel Holbert [:dholbert]]

I agree with comment 5 in spirit, but in practice, it means none of our general all-properties property_database.js-based tests get exercised, which means we don't catch regressions.  And in some cases, we may even inadvertently check in preffed-off code with property_database.js/test_transitions.html test-code that leaks, fails, or fatally asserts, as was the case in this feature (per bug 1266868 & bug 1269990). :-/  I don't want that to be something that we can easily do by accident again.

To avoid taking too much of a diversion on this bug, I've filed bug 1297747 on coming up with a better way to deal with this.  Not sure what action (if any) we should take on this bug, in the meantime. If nothing else, we could just strongly-encourage anyone working on clip-path-basic-shapes to apply this bug's one-liner-patch locally & in their Try runs.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8784225 [details]
Bug 1297333 - Enable basic shape for clip-path in test suite.

https://reviewboard.mozilla.org/r/73754/#review71740

(I can't in good conscience r+ a patch that the module-owner has reservations about -- so, this is r- for the time being, and I'll defer to dbaron on whether we do eventually want to take this after all.)

Comment 9

[Daniel Holbert [:dholbert]]

OK: per bug 1297747 comment 2, 4, and 5, sounds like we can proceed here with a slightly-restricted version of the patch.

TYLin, could you update the patch so that the new line is wrapped in #ifndef RELEASE_BUILD (like we do in all.js when enabling new features), and then do some local test runs to make sure that the #ifdef works as expected?

(e.g. you could probably run ./mach mochitest layout/style/test/test_clip-path_polygon.html -- and you'll want to do that with your normal Nightly build, and then again with RELEASE_BUILD defined. I'm not sure offhand what the "correct" way is to get RELEASE_BUILD defined -- hopefully some DXR searching and asking in #developers may be able to help)

Comment 10

[Daniel Holbert [:dholbert]]

(I'm not 100% sure the RELEASE_BUILD guard will work as-expected -- hence the need for testing. :)  We might need to opt in that file for preprocessing or something, for example.)

Comment 11

[Daniel Holbert [:dholbert]]

(ah, prefs_general.js does indeed have one single preprocessor macro right now, for XP_WIN:
https://dxr.mozilla.org/mozilla-central/rev/bd7645928990649c84609d3f531e803c2d41f269/testing/profiles/prefs_general.js#314-316
...so it's likely that a similar #ifndef RELEASE_BUILD check will work.)

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Sadly, prefs_general.js isn't preprocessed like all.js. I filed bug 1297975 for XP_WIN macro won't work as expected.

Comment 13

[Daniel Holbert [:dholbert]]

So, per dbaron's comment 5, the best way forward here (towards getting this feature reliably tested) is to fix whatever's blocking it from being enabled-by-default on Nightly & Aurora.

It might be best to just WONTFIX this bug here, and file a new bug (blocking bug 1247229), which would add the standard #ifndef RELEASE_BUILD dance for this pref in all.js.  If our tests pass with this preffed, and none of the bugs around this feature are especially bad, I'd r+ a patch like that.

TYLin, since you were driving this bug here, perhaps you'd be up for doing that?

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 13:
> TYLin, since you were driving this bug here, perhaps you'd be up for doing that?

Sure. Filed bug 1303654 to enable basic shapes clip-path clipping on Nightly & Aurora.