Closed Bug 1304829 Opened 4 years ago Closed 3 years ago



(Firefox Build System :: General, defect)

Not set


(firefox52 fixed)

Tracking Status
firefox52 --- fixed


(Reporter: aryx, Assigned: aryx)



(4 files, 1 obsolete file)

It's not obvious that the value of RELEASE_BUILD is true in release and beta builds. There is confusion and uncertainty among a share of the developers, so lets make their lives easier and rename it to RELEASE_OR_BETA.
Attached patch main patch, v1 (obsolete) — Splinter Review
There is also a small patch with changes the check for "release" (mean Release or Beta) in a Spidermonkey test helper function which a spidermonkey peer should review.
Attachment #8793917 - Flags: feedback?(ryanvm)
Comment on attachment 8793917 [details] [diff] [review]
main patch, v1

Sounds OK to me.
Attachment #8793917 - Flags: feedback?(ryanvm) → feedback+
Attached patch main patch, v2Splinter Review
Feel free to redirect.

Try push is:
Android is busted because it didn't get built with secrets.

Reasoning for change: It's not obvious that RELEASE_BUILD is also true on beta.
Attachment #8793917 - Attachment is obsolete: true
Attachment #8795269 - Flags: review?(ted)
Comment on attachment 8795270 [details] [diff] [review]
spidermonkey test 'release' change, v1

This is a change to code written by you.
Attachment #8795270 - Attachment description: spidermonkey test → spidermonkey test 'release' change, v1
Attachment #8795270 - Flags: review?(arai.unmht)
Comment on attachment 8795270 [details] [diff] [review]
spidermonkey test 'release' change, v1

Review of attachment 8795270 [details] [diff] [review]:

Thank you :D
Attachment #8795270 - Flags: review?(arai.unmht) → review+
Comment on attachment 8795269 [details] [diff] [review]
main patch, v2

Review of attachment 8795269 [details] [diff] [review]:

::: toolkit/modules/AppConstants.jsm
@@ +21,5 @@
>  #else
>    false,
>  #endif

I feel like you might want a Firefox peer to sign off on this, since this is available to content JS. It's possible we could break extensions if there are any using this.

I note you didn't change the `releaseBuild` property on nsIXULRuntime, so an argument could be made for keeping this as-is (although having the names in AppConstants match the names in the build system is pretty sensible).
Attachment #8795269 - Flags: review?(ted) → review+
Attached patch xpconnect, v1Splinter Review
One-line-change of an at the moment unused variable.
Attachment #8797607 - Flags: review?(gkrizsanits)
Thank you for the review. This two-line patch changes isReleaseBuild to isReleaseOrBeta as suggested in comment 7.
Attachment #8797609 - Flags: review?(ted)
Attachment #8797607 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8795269 [details] [diff] [review]
main patch, v2

Requesting review from Firefox peer as suggested in comment 7.
Attachment #8795269 - Flags: review+ → review?(dtownsend)
Comment on attachment 8795269 [details] [diff] [review]
main patch, v2

Review of attachment 8795269 [details] [diff] [review]:

I see no extensions using that property. Ted's review is fine for the rest.
Attachment #8795269 - Flags: review?(dtownsend) → review+
Pushed by
rename RELEASE_BUILD to RELEASE_OR_BETA: main part. r=ted,Mossop
rename Spidermonkey's 'release' to 'release_or_beta'. r=arai
Rename test_xrayToJS.xul's isReleaseBuild to isReleaseOrBeta. r=gabor
Comment on attachment 8797609 [details] [diff] [review]
xulruntime's isReleaseBuild

Review of attachment 8797609 [details] [diff] [review]:

Huh, this isn't used anywhere in the tree? Interesting!
Attachment #8797609 - Flags: review?(ted) → review+
Pushed by
Rename nsXULAppInfo's isReleaseBuild to isReleaseOrBeta. r=ted
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.