Closed Bug 1478438 Opened 2 years ago Closed 1 year ago

Remove unused mobile/android config variables

Categories

(Firefox for Android :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: sysrqb, Assigned: sysrqb)

Details

Attachments

(1 file, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

There are unused/dead variable in `mobile/android/confvars.sh`. Specifically, I don't see MOZ_XULRUNNER and MOZ_CAPTURE used anywhere in the codebase.

https://dxr.mozilla.org/mozilla-central/search?q=MOZ_XULRUNNER
https://dxr.mozilla.org/mozilla-central/search?q=MOZ_CAPTURE



Expected results:

Can these be deleted?
Status: RESOLVED → UNCONFIRMED
Resolution: INACTIVE → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Hi! I've never contributed to the open-source community. Can I be assigned this bug?
Attached patch unusedVars.diff (obsolete) — Splinter Review
Attachment #9009725 - Flags: review+
Attachment #9009725 - Flags: feedback+
Can someone please review the patch I uploaded?
Flags: needinfo?(matthew.finkel)

Id like to contribute to this bug?

Sam

This bug already has a patch. It is not a good one to take. The needinfo that Korina left went to the wrong person so this got missed.

Nick is it possible to review this patch in its current state or does everything go through Phabricator these days?

Flags: needinfo?(matthew.finkel) → needinfo?(nalexander)

(In reply to Kevin Brosnan [:kbrosnan] from comment #8)

This bug already has a patch. It is not a good one to take. The needinfo that Korina left went to the wrong person so this got missed.

Nick is it possible to review this patch in its current state or does everything go through Phabricator these days?

Yes. We should also remove the following lines: https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/old-configure.in#1730-1733 and https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/old-configure.in#1742.

Everything does go through Phab these days, which raises the barrier to entry.

samueljselvarajah, if you would like to make these additional changes, please do!

Flags: needinfo?(nalexander)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:korina.houghtaling, could you have a look please?

Flags: needinfo?(korina.houghtaling)

The patch ready to land, but it is based on mozilla-beta. I probably did this backwards. Can this be uplifted?

Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7cfd61c3840
Delete unused confvars entries on Android r=nalexander

Keywords: checkin-needed

(In reply to Matthew Finkel [:sysrqb] from comment #14)

The patch ready to land, but it is based on mozilla-beta. I probably did this backwards. Can this be uplifted?

Heh -- I didn't even notice that it was against beta :) I landed it using Lando (in Phabricator), so we're good. Thanks, Matthew!

Flags: needinfo?(korina.houghtaling)

(In reply to Matthew Finkel [:sysrqb] from comment #14)

The patch ready to land, but it is based on mozilla-beta. I probably did this backwards. Can this be uplifted?

relman: this patch is essentially no risk; it's a build time change (a tiny one) that should only result in broken builds. I'll request approval.

Summary: Unused Config Variables → Remove unused mobile/android config variables
Attachment #9009725 - Attachment is obsolete: true

Comment on attachment 9065368 [details]
Bug 1478438 - Delete unused confvars entries on Android

Beta/Release Uplift Approval Request

  • User impact if declined: None. It's only deleting variables that aren't used.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Not risky, but uplifting a tor browser patch.
  • String changes made/needed:
Attachment #9065368 - Flags: approval-mozilla-beta?
Status: NEW → RESOLVED
Closed: 2 years ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Assignee: nobody → matthew.finkel

Comment on attachment 9065368 [details]
Bug 1478438 - Delete unused confvars entries on Android

beta is now 68

Attachment #9065368 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.