Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Login storage refactor. #8875

Merged
merged 1 commit into from
Mar 3, 2020
Merged

Conversation

grigoryk
Copy link
Contributor

Companion PR for mozilla-mobile/android-components#6128

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@grigoryk
Copy link
Contributor Author

grigoryk commented Mar 3, 2020

Just waiting for mozilla-mobile/android-components#6128 to be available in a snapshot (triggered, progress in https://firefox-ci-tc.services.mozilla.com/tasks/groups/fXhTXbmHT7WjMLCYInkwyA)

@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

Snapshots are ready. Let's rerun this. :)

@pocmo pocmo changed the title Login storage refactor Login storage refactor. Mar 3, 2020
@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

This looks like it needs a rebase on top of master to address some other changes that have happened since opening the PR. I'll do that now.

@pocmo pocmo force-pushed the loginsStorageRefacto branch from 4758a09 to 5cb4f46 Compare March 3, 2020 14:21
@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

I still see some other failures locally:

e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt: (332, 21): No value passed for parameter 'keyStore'
e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt: (32, 47): Unresolved reference: SyncableLoginsStorage
e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt: (53, 23): Unresolved reference: SyncableLoginsStorage
e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/components/Core.kt: (40, 47): Unresolved reference: SyncableLoginsStorage
e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/components/Core.kt: (76, 71): No value passed for parameter 'securePreferences'
e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/components/Core.kt: (88, 71): No value passed for parameter 'securePreferences'
e: /Users/sebastiankaspari/Projects/Mozilla/fenix/app/src/main/java/org/mozilla/fenix/components/Core.kt: (195, 36): Unresolved reference: SyncableLoginsStorage

@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

Ah, I think this PR only contains the changes for the beta flavor. I'll adapt it and add the changes for the nightly flavor to it.

@pocmo pocmo force-pushed the loginsStorageRefacto branch from 5cb4f46 to 04f33cf Compare March 3, 2020 14:42
@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

Build passes, but tests not yet... looking. :)

@pocmo pocmo force-pushed the loginsStorageRefacto branch from 04f33cf to 91d790b Compare March 3, 2020 15:08
@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

... and ktlint.

The a-c side of this work is in mozilla-mobile/android-components#6128

This switches Fenix to use `SyncableLoginsStorage`, which caches a connection internally
on first access, and doesn't expose any lock/unlock APIs at the public boundary.
@pocmo pocmo force-pushed the loginsStorageRefacto branch from 91d790b to 537390d Compare March 3, 2020 15:24
@codecov-io
Copy link

Codecov Report

Merging #8875 into master will increase coverage by 0.04%.
The diff coverage is 18.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #8875      +/-   ##
===========================================
+ Coverage     18.95%     19%   +0.04%     
  Complexity      500     500              
===========================================
  Files           327     327              
  Lines         13007   12991      -16     
  Branches       1720    1720              
===========================================
+ Hits           2466    2469       +3     
+ Misses        10332   10313      -19     
  Partials        209     209
Impacted Files Coverage Δ Complexity Δ
...in/java/org/mozilla/fenix/components/Components.kt 36.36% <0%> (+0.8%) 1 <0> (ø) ⬇️
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...enix/settings/logins/SavedLoginSiteInfoFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...zilla/fenix/settings/logins/SavedLoginsFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...org/mozilla/fenix/components/BackgroundServices.kt 21.25% <0%> (+0.26%) 0 <0> (ø) ⬇️
...src/main/java/org/mozilla/fenix/components/Core.kt 20.87% <50%> (+1.83%) 1 <0> (ø) ⬇️
.../fenix/collections/CollectionCreationInteractor.kt 8.33% <0%> (-2.78%) 0% <0%> (ø)
...nix/components/toolbar/BrowserToolbarController.kt 64.53% <0%> (+2.32%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a75d82...537390d. Read the comment docs.

@pocmo
Copy link
Contributor

pocmo commented Mar 3, 2020

Alright, green, phew. Lets get this in to unblock master.

@pocmo pocmo merged commit e6e2dd9 into mozilla-mobile:master Mar 3, 2020
@grigoryk grigoryk deleted the loginsStorageRefacto branch March 3, 2020 20:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants