Closed Bug 1220892 Opened 4 years ago Closed 4 years ago

Remove native Firefox Account UI

Categories

(Firefox for Android :: Firefox Accounts, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed
fennec 45+ ---

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files)

Post 1220891, no native Firefox Account UI will be shipping.  We should kill this code, saving both compile time, classes.dex bytes, and resource space.
(In reply to Nick Alexander :nalexander from comment #0)
> Post 1220891

Not armageddon: Bug 1120891.  This should be tracking Fennec 45 or 46.
tracking-fennec: --- → ?
(In reply to Nick Alexander :nalexander from comment #1)
> (In reply to Nick Alexander :nalexander from comment #0)
> > Post 1220891
> 
> Not armageddon: Bug 1120891.  This should be tracking Fennec 45 or 46.

Heh, my typing is terrible: Bug 1220891.
Assignee: nobody → nalexander
tracking-fennec: ? → 45+
Blocks: fatfennec
Status: NEW → ASSIGNED
Component: General → Firefox Accounts
OS: Unspecified → Android
Hardware: Unspecified → All
Bug 1220892 - Part 1: Remove MOZ_ANDROID_NATIVE_ACCOUNTS_UI. r?mcomella
Attachment #8694497 - Flags: review?(michael.l.comella)
Bug 1220892 - Part 2: Remove Activity sub-classes. r?mcomella
Attachment #8694498 - Flags: review?(michael.l.comella)
Bug 1220892 - Part 3: Remove Task sub-classes. r?mcomella
Attachment #8694499 - Flags: review?(michael.l.comella)
Bug 1220892 - Part 4: Clean up FxAccountAbstractSetupActivity. r?mcomella

The web based Activity sub-classes never used the other intent extras,
and in fact, filter them out immediately; so it doesn't hurt to clean
this all up.
Attachment #8694500 - Flags: review?(michael.l.comella)
Bug 1220892 - Part 5: Remove FxAccountAgeLockoutHelper. r?mcomella
Attachment #8694501 - Flags: review?(michael.l.comella)
Bug 1220892 - Bug 1220892 - Part 6: Remove non-string resources. r?mcomella

I'm reluctant to mass-delete strings, since that could cause our l10n
team some grief in the event of a rollback.  And it's not obviously
easy to get them all back smoothly.  I'll delete strings separately.
Attachment #8694502 - Flags: review?(michael.l.comella)
Comment on attachment 8694497 [details]
MozReview Request: Bug 1220892 - Part 1: Remove MOZ_ANDROID_NATIVE_ACCOUNTS_UI. r?mcomella

https://reviewboard.mozilla.org/r/26791/#review24341

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java:236
(Diff revision 1)
> +      return true;

It looks like before we didn't return here and probably return false at the end of the function – was that a mistake? Is that why we're returning true here?
Attachment #8694497 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8694498 [details]
MozReview Request: Bug 1220892 - Part 2: Remove Activity sub-classes. r?mcomella

https://reviewboard.mozilla.org/r/26793/#review24345

Didn't connect all the dots to make sure this code will compile but if it works for you, it works for me.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountAbstractSetupActivity.java:81
(Diff revision 1)
> -    super(CANNOT_RESUME_WHEN_ACCOUNTS_EXIST | CANNOT_RESUME_WHEN_LOCKED_OUT);
> +    super(CANNOT_RESUME_WHEN_ACCOUNTS_EXIST);

I assume the online interface handles lock out now?
Attachment #8694498 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8694499 [details]
MozReview Request: Bug 1220892 - Part 3: Remove Task sub-classes. r?mcomella

https://reviewboard.mozilla.org/r/26795/#review24349
Attachment #8694499 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8694500 [details]
MozReview Request: Bug 1220892 - Part 4: Clean up FxAccountAbstractSetupActivity. r?mcomella

https://reviewboard.mozilla.org/r/26797/#review24351
Attachment #8694500 - Flags: review?(michael.l.comella) → review+
Attachment #8694501 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8694501 [details]
MozReview Request: Bug 1220892 - Part 5: Remove FxAccountAgeLockoutHelper. r?mcomella

https://reviewboard.mozilla.org/r/26799/#review24353
Comment on attachment 8694502 [details]
MozReview Request: Bug 1220892 - Bug 1220892 - Part 6: Remove non-string resources. r?mcomella

https://reviewboard.mozilla.org/r/26801/#review24355

I'd be curious to see the APK-size improvements after this patch. :)
Attachment #8694502 - Flags: review?(michael.l.comella) → review+
https://reviewboard.mozilla.org/r/26793/#review24345

> I assume the online interface handles lock out now?

This file dies entirely in Part 4, but you are correct: `fxa-content-server` handles any COPPA lock out.
https://reviewboard.mozilla.org/r/26801/#review24355

Locally, I saw about 100kb.  Not a ton, but considering that it's mostly code and before Proguard, not bad.  I'm pleased to remove a lot of untested UI code.
(In reply to Michael Comella (:mcomella) from comment #16)

> I'd be curious to see the APK-size improvements after this patch. :)

(In reply to Nick Alexander :nalexander from comment #18)

> Locally, I saw about 100kb.  Not a ton, but considering that it's mostly
> code and before Proguard, not bad.  I'm pleased to remove a lot of untested
> UI code.

116KB decrease in APK size. 76KB from classes.dex

Nice!
You need to log in before you can comment on or make changes to this bug.