Eliminate remaining NoSystemCurrentTimeMillis lint baseline entries in mobile/android
Categories
(Firefox for Android :: General, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox155 | --- | fixed |
People
(Reporter: RyanVM, Assigned: RyanVM)
References
(Blocks 1 open bug)
Details
Attachments
(13 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 2055298 - Move the impression timestamp default into the entity constructor r?#android-reviewers
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 2055298 - Suppress NoSystemCurrentTimeMillis for the capture-image filename r?#android-reviewers
48 bytes,
text/x-phabricator-request
|
Details | Review |
Follow-up to bug 2039540, which introduced the NoSystemCurrentTimeMillis Android Lint detector and recorded the pre-existing violations in per-module lint-baseline.xml files. Currently there are 97 such baselined violations across the tree.
This bug removes those baseline entries by making each System.currentTimeMillis() call injectable, so the check is enforced with an empty baseline across mobile/android.
Approach:
- Route
System.currentTimeMillis()through the form the detector permits: a default-valued() -> Long(orLong) parameter on a class constructor or a top-level function, so tests can supply a fake clock. - For call sites with no injectable seam (framework-instantiated singletons/objects, the sanctioned
DefaultDateTimeProviderwrapper, and the sample apps), annotate with a justified@Suppress("NoSystemCurrentTimeMillis")carrying the rationale inline. - Drop the now-empty
NoSystemCurrentTimeMillisentries from each module'slint-baseline.xml.
Result: no remaining NoSystemCurrentTimeMillis findings or baseline entries in mobile/android.
Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=0e682d89a945869d4206febee84e8d19b96af8ec (the Fenix unit test failure is already fixed and verified locally)
|
||
Comment 1•2 months ago
|
||
The Bugbug bot thinks this bug should belong to the 'Firefox for Android::Tooling' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
|
Assignee | |
Updated•2 months ago
|
|
|
Assignee | |
Comment 2•2 months ago
|
||
Route direct System.currentTimeMillis() calls in android-components and fenix classes through an
injectable currentTimeMillis provider defaulted in the primary constructor, the form accepted by the
NoSystemCurrentTimeMillis lint check, and drop the corresponding baseline entries. Also removes a
stale baseline entry for a since-deleted file.
|
|
Assignee | |
Comment 3•2 months ago
|
||
Route direct System.currentTimeMillis() calls in top-level functions through an injectable default
parameter, the form accepted by the NoSystemCurrentTimeMillis lint check, and drop the corresponding
baseline entries. One-shot functions take a Long value default matching existing convention, while
functions that read the clock repeatedly take a () -> Long provider.
|
|
Assignee | |
Comment 4•2 months ago
|
||
Move the System.currentTimeMillis() default out of the injectable provider property initializer and
into a primary-constructor parameter default, the form accepted by the NoSystemCurrentTimeMillis lint
check, while keeping the existing mutable test seam intact.
|
|
Assignee | |
Comment 5•2 months ago
|
||
DefaultDateTimeProvider is the sanctioned wrapper around System.currentTimeMillis() that exists so
the clock can be injected everywhere else, so annotate its override with a justified suppression
rather than routing it through yet another provider.
|
|
Assignee | |
Comment 6•2 months ago
|
||
Relocate SponsoredContentImpressionEntity.impressionDateInSeconds from a body-property initializer to
a primary-constructor parameter default, the form accepted by the NoSystemCurrentTimeMillis lint
check. This pulls the field into the data class's generated equals/copy, so SponsoredContentsRepositoryTest
now compares recorded impressions by url rather than by whole-entity equality, which would otherwise
depend on which wall-clock second the entities were constructed. Room still populates the column via
the constructor.
|
|
Assignee | |
Comment 7•2 months ago
|
||
Object, companion, and singleton members give the lint check no constructor or top-level function to
attach an injectable default to. Move each System.currentTimeMillis() into an accepted form: a
top-level bundle-reading helper for the Crash companion factories, a constructor-defaulted
DefaultStartTimeProvider class in place of the object, and an AnnouncementDebouncer class backing
AccessibilityUtils.
|
|
Assignee | |
Comment 8•2 months ago
|
||
The lint check only accepts a System.currentTimeMillis() default on a top-level function or a class
constructor, not on interface methods. Remove the current-time defaults from the PrivacyNoticeBanner,
TermsOfUsePrompt, and onboarding terms-of-service interfaces and have their implementations read the
time from an injected constructor provider instead. Production callers already invoke these with no
argument; the affected tests inject a fixed clock through the constructor.
|
|
Assignee | |
Comment 9•2 months ago
|
||
The lint check does not accept a System.currentTimeMillis() default on interface, Dao, or data-class
methods. The sponsored-content impressions Dao now takes the timestamp explicitly, since its one
production caller already supplies it. SitePermissions gains a savedAt constructor default, so both
SitePermissionsRules.toSitePermissions and SitePermissionsFeature can drop their own clocks, and
BookmarksStorage.getRecentBookmarks drops its currentTime parameter while PlacesBookmarksStorage
reads its injected constructor clock instead.
|
|
Assignee | |
Comment 10•2 months ago
|
||
Fragments cannot take an injectable constructor parameter because the framework instantiates them
reflectively. Move the settings.<timestamp> = System.currentTimeMillis() stamps out of
BrowserFragment, HomeFragment, and OnboardingFragment into Settings recorder methods that use the
clock Settings already holds in its constructor, so the call sites carry no direct clock read.
|
|
Assignee | |
Comment 11•2 months ago
|
||
The sync worker and the account-settings observer are framework-instantiated and can't inject a
clock. Give the top-level setLastSynced() a current-time default so both drop their direct
System.currentTimeMillis() reads, and route the worker's stagger-buffer check and timestamp store
through top-level helpers with the same default.
|
|
Assignee | |
Comment 12•2 months ago
|
||
FenixApplication and GrowthDataWorker are framework-instantiated, so their time-based cutoffs move
into top-level functions with a current-time default parameter: historyMetadataCleanupCutoff() for
the history-metadata cleanup age, and isAfterFirstWeekFromInstall() for the growth-data first-week
check.
|
|
Assignee | |
Comment 13•2 months ago
|
||
The browser and sync sample apps reach System.currentTimeMillis() through framework entry points (an
Application.onCreate and a lazily-built FxaAccountManager) that offer no injectable clock seam and no
time-dependent behavior worth testing. Annotate both with a justified suppression rather than
restructuring sample-only code.
|
|
Assignee | |
Comment 14•2 months ago
|
||
LensCameraFragment uses System.currentTimeMillis() only to make a captured-image filename unique;
there is no time-dependent behavior to test, so the call is annotated with a justified suppression
rather than injected.
|
||
Comment 15•1 month ago
|
||
|
||
Updated•1 month ago
|
|
||
Comment 16•1 month ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/cd5624cb7783
https://hg.mozilla.org/mozilla-central/rev/4d95c3d9d360
https://hg.mozilla.org/mozilla-central/rev/8a669d3b3d6b
https://hg.mozilla.org/mozilla-central/rev/00d265828f91
https://hg.mozilla.org/mozilla-central/rev/e34d0be9816b
https://hg.mozilla.org/mozilla-central/rev/6bf269fff594
https://hg.mozilla.org/mozilla-central/rev/7f57ddd8d195
https://hg.mozilla.org/mozilla-central/rev/fd775ec088a3
https://hg.mozilla.org/mozilla-central/rev/10013dadf42d
https://hg.mozilla.org/mozilla-central/rev/3710c98efd99
https://hg.mozilla.org/mozilla-central/rev/121cd0f9c897
https://hg.mozilla.org/mozilla-central/rev/548516b47e8d
https://hg.mozilla.org/mozilla-central/rev/d45e17686707
Description
•