1228419 - Add feature flag for logins manager

Status: RESOLVED FIXED

(Firefox for iOS :: Build & Test, defect)

Description

[Stephan Leroux [:sleroux]]

Need to add a feature flag to the logins manager stuff so user's aren't taken to nothing when tapping the logins settings item.

Comment 1

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1373

Basic implementation for feature flags + flag for login manager. See documentation for details: https://github.com/mozilla/firefox-ios/wiki/Adding-a-Feature-Flag

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8700035 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1373

Looks good. Would you mind documenting the process of adding feature flags for the rest of the team?

Comment 3

[Stephan Leroux [:sleroux]]

https://github.com/mozilla/firefox-ios/wiki/Adding-a-Feature-Flag

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8700035 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1373

I'm broadly in favor of using an AppConstants model, where flags are real runtime booleans rather than preprocessor directives. But this'll do for now.

Comment 5

[Stephan Leroux [:sleroux]]

:rnewman I'm down with using AppConstants instead of runtime booleans. Doesn't seem like a heavy lift or anything and there isn't an immediate need for this to land.

Do you have any thoughts around the details of how it would work for different build variants?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #5)

> Do you have any thoughts around the details of how it would work for
> different build variants?

We already preprocess AppConstants, so we'd just do exactly what you already did in this PR, but in AppConstants instead.

Comment 7

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1387

Alternative patch using AppConstants

Comment 8

[Stephan Leroux [:sleroux]]

Merged

master f8f4788ba64e310ac225ed36f22ec660dc833d71