Remove alert_app_animation drawables

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcomella, Assigned: bdahl, Mentored)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 48
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [lang=java])

Attachments

(1 attachment, 1 obsolete attachment)

In bug 1186020, we convert the download animation icon assets to white because it appears grey on some devices – I'd expect alert_app_animation to have the same problem.

The animation appears to occur when we update outdated apps [2], which doesn't seem like it'd take long enough to warrant an animation (or if we even still use this often) so maybe we can remove it and save some APK space. 

Finkle, thoughts? How can I test this?

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/alert_app_animation_1.png
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/WebappManager.jsm?rev=3c6b38653a58#470
Flags: needinfo?(mark.finkle)
The animation was added in bug 970203 by Martyn. He might remember how to get updates to work, or at least trigger the action for testing.
Flags: needinfo?(mark.finkle) → needinfo?(mhaigh)
I can't honestly remember how to go about triggering a webapp update.  Myk may have some idea or links to test pages to help
Flags: needinfo?(mhaigh) → needinfo?(myk)
(In reply to Martyn Haigh (:mhaigh) from comment #2)
> I can't honestly remember how to go about triggering a webapp update.  Myk
> may have some idea or links to test pages to help

It used to be possible to initiate a manual check for updates via some UI on about:apps, but there isn't any UI for doing so now that about:apps has been removed.


(In reply to Michael Comella (:mcomella) from comment #0)
> The animation appears to occur when we update outdated apps [2], which
> doesn't seem like it'd take long enough to warrant an animation (or if we
> even still use this often) so maybe we can remove it and save some APK
> space. 

It doesn't necessarily take that long, a few seconds at most on a fast network connection.  But that's still long enough to be confusing for the user who initiates an update check and then doesn't see anything happening.

However, as noted above, there's no way for the user to initiate an update check anymore.  And we only show that animation when the user initiates a check (i.e. when *userInitiated* is `true`).  We don't show anything when Fennec initiates its periodic (daily) update check in the background.  So we never show that animation anymore, and we can remove it.
Flags: needinfo?(myk)
Blocks: 942609
Mentor: michael.l.comella@gmail.com, mhaigh@mozilla.com
Summary: Check color of alert_app_animation → Remove alert_app_animation drawables
Whiteboard: [lang=java]

Comment 4

2 years ago
Hello all, I was hoping to be assigned to this bug if possible! And let me reiterate what I read above to be sure I am making the correct change. Do we just want to remove the line containing alert_app_animation drawables or would you like the entire logic removed? By entire logic I mean the  if (userInitiated) structure.
(In reply to Matherin from comment #4)
> Hello all, I was hoping to be assigned to this bug if possible! And let me
> reiterate what I read above to be sure I am making the correct change. Do we
> just want to remove the line containing alert_app_animation drawables or
> would you like the entire logic removed? By entire logic I mean the  if
> (userInitiated) structure.

Hey Matherin. We're actually removing our Web App runtime, which will remove these assets and their use altogether. Perhaps you'd like to tackle bug 1245952 instead?
Depends on: 1235869
(Assignee)

Updated

2 years ago
Assignee: nobody → bdahl
(Assignee)

Comment 6

2 years ago
Created attachment 8726549 [details] [diff] [review]
0001-Bug-1199015-Remove-alert_app_animation-drawables.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c7560f36373
Attachment #8726549 - Flags: review?(michael.l.comella)
Comment on attachment 8726549 [details] [diff] [review]
0001-Bug-1199015-Remove-alert_app_animation-drawables.patch

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

Splinter may be hiding some deletions from me, but it appears you can also delete the `.../drawable-hdpi*/alert_app*.png` assets. See:
  https://mxr.mozilla.org/mozilla-central/find?string=alert_app&tree=mozilla-central&hint=

Nice find on the UnusedResourcesUtil. :)
Attachment #8726549 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8727679 [details] [diff] [review]
0001-Bug-1199015-Remove-alert_app_animation-drawables.patch

Splinter was hiding some removals, but I missed the ones outside of xhdpi. Carrying r+ forward
Attachment #8726549 - Attachment is obsolete: true
Attachment #8727679 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7d800609f31
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.