Closed Bug 888982 Opened 6 years ago Closed 6 years ago

fennec should use channel-specific build defines rather than MOZ_UPDATE_CHANNEL

Categories

(Firefox for Android :: General, defect)

20 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: Gavin, Assigned: mtp.boon)

Details

(Whiteboard: [lang=java][mentor=bnicholson])

Attachments

(1 file, 1 obsolete file)

In cases where you want to distinguish "release"-ness of a build, rather than something that actually cares about the update channel, the defines listed at https://wiki.mozilla.org/Platform/Channel-specific_build_defines are a better tool than MOZ_UPDATE_CHANNEL.

I think that means most of these hits should be replaced: http://mxr.mozilla.org/mozilla-central/search?string=AppConstants.MOZ_UPDATE_CHANNEL

I guess that might require porting those defines to AppConstants.java.in?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #0)

> I think that means most of these hits should be replaced:
> http://mxr.mozilla.org/mozilla-central/search?string=AppConstants.
> MOZ_UPDATE_CHANNEL
> 
> I guess that might require porting those defines to AppConstants.java.in?

Yeah. I actually made a patch that started to do this, but then we ended up not needing that patch:
https://bug867354.bugzilla.mozilla.org/attachment.cgi?id=756707

This bug should be pretty straightforward, so I'm marking it as a mentor bug. Hopefully bnicholson doesn't mind that I'm marking him as the mentor, since he's the person who created AppConstants :)
Whiteboard: [lang=java][mentor=bnicholson]
Hey,

I'd like to have a go at this please. I'll take a look at it over the next few days if that's OK.
(In reply to Michael from comment #2)

> I'd like to have a go at this please. I'll take a look at it over the next
> few days if that's OK.

Awesome! Feel free to ask questions in #mobile if you're confused about what needs to be done.
Assignee: nobody → mtp.boon
Attached patch 888982.patch (obsolete) — Splinter Review
Only 2/6 of the mxr results changed. 3 of them I am quite sure shouldn't be changed and http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1150 doesn't have a nice way to put it says Gavin in IRC.
Attachment #786501 - Flags: review?(margaret.leibovic)
Comment on attachment 786501 [details] [diff] [review]
888982.patch

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

Looking good, but let's not include constants that we're not using.

::: mobile/android/base/AppConstants.java.in
@@ +141,5 @@
> +#ifdef NIGHTLY_BUILD
> +    true;
> +#else
> +    false;
> +#endif

If we're not using NIGHTLY_BUILD or EARLY_BETA_OR_EARLIER anywhere in our code, we don't need to include them in here. If we do ever need to use them in our Java code, we can add this as part of whatever patch does that.

To be helpful to whatever future person comes along and might do that, you could add a comment above the RELEASE_BUILD constant with a link to the list of channel-specific build defines here:
https://wiki.mozilla.org/Platform/Channel-specific_build_defines
Attachment #786501 - Flags: review?(margaret.leibovic) → feedback+
Attached patch 888982v2.patchSplinter Review
Sorry about that, hopefully this one is better!
Attachment #786501 - Attachment is obsolete: true
Attachment #787007 - Flags: review?(margaret.leibovic)
Comment on attachment 787007 [details] [diff] [review]
888982v2.patch

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

Looks good, thanks!
Attachment #787007 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b70d74ada86b
Keywords: checkin-needed
Whiteboard: [lang=java][mentor=bnicholson] → [lang=java][mentor=bnicholson][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b70d74ada86b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][mentor=bnicholson][fixed-in-fx-team] → [lang=java][mentor=bnicholson]
Target Milestone: --- → Firefox 26
I'd like to nom this for uplift to aurora, because it's a small, simple patch, and having it in Aurora makes things much easier and cleaner for uplifting bug 909938, bug 900564, and bug 906339.

Aurora try build here: https://tbpl.mozilla.org/?tree=Try&rev=731d3696542d

I'll nom when it's green.

Also, I'm assuming that subsequent release/beta spins will define the RELEASE_BUILD flag, but need-info-ing gavin just to make sure there isn't any other code necessary or releng people I need to talk to.
Flags: needinfo?(gavin.sharp)
Comment on attachment 787007 [details] [diff] [review]
888982v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Having this flag would greatly simplify uplift for bug 909938, bug 900564, and bug 906339, which update channel-specific UI elements

User impact if declined: no user impact, much greater code complexity for 3 uplifts

Testing completed (on m-c, etc.): all green try build on aurora: https://tbpl.mozilla.org/?tree=Try&rev=731d3696542d
 
Risk to taking this patch (and alternatives if risky): low - adds conditional and #ifdefs

String or IDL/UUID changes made by this patch: none
Attachment #787007 - Flags: approval-mozilla-aurora?
Comment on attachment 787007 [details] [diff] [review]
888982v2.patch

Sounds safer than trying to munge something new together, and this is the right way to do things anyway :)
Attachment #787007 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Chenxia Liu [:liuche] from comment #10)
> Also, I'm assuming that subsequent release/beta spins will define the
> RELEASE_BUILD flag, but need-info-ing gavin just to make sure there isn't
> any other code necessary or releng people I need to talk to.

Bug 853071 (and its depedencies) all made Firefox 23, so it should all work.
Flags: needinfo?(gavin.sharp)
You need to log in before you can comment on or make changes to this bug.