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

Enforce IO thread inside of components #14704

Merged
merged 3 commits into from Sep 8, 2020

Conversation

NotWoods
Copy link
Contributor

@NotWoods NotWoods commented Sep 4, 2020

Having to guess which thread a controller should run on is a potential mistake waiting to happen. This change enforces that controllers run on a background thread with a suspend function.

Additionally TabCollectionStorage now has a dedicated scope that doesn't get cancelled. This way, the operation will complete even if a fragment is closed.

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.

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

@firefoxci-taskcluster
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

Copy link
Member

@gabrielluong gabrielluong left a comment

Choose a reason for hiding this comment

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

This is 🔥

@gabrielluong gabrielluong added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Sep 8, 2020
@liuche liuche mentioned this pull request Sep 8, 2020
3 tasks
@codecov-commenter
Copy link

Codecov Report

Merging #14704 into master will decrease coverage by 0.02%.
The diff coverage is 17.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #14704      +/-   ##
============================================
- Coverage     29.85%   29.83%   -0.03%     
- Complexity     1167     1168       +1     
============================================
  Files           445      445              
  Lines         18080    18089       +9     
  Branches       2346     2354       +8     
============================================
- Hits           5398     5397       -1     
- Misses        12306    12316      +10     
  Partials        376      376              
Impacted Files Coverage Δ Complexity Δ
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../org/mozilla/fenix/components/PermissionStorage.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ssions/SitePermissionsDetailsExceptionsFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...tepermissions/SitePermissionsExceptionsFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...PermissionsManageExceptionsPhoneFeatureFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...org/mozilla/fenix/tabtray/TabTrayDialogFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...g/mozilla/fenix/components/TabCollectionStorage.kt 26.19% <9.09%> (-6.07%) 2.00 <0.00> (ø)
.../fenix/collections/CollectionCreationController.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...la/fenix/collections/CollectionCreationFragment.kt 91.37% <100.00%> (ø) 6.00 <0.00> (ø)
...ix/home/sessioncontrol/SessionControlController.kt 86.23% <100.00%> (ø) 0.00 <0.00> (ø)
... and 1 more

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 09d1dba...c1c23bf. Read the comment docs.

@liuche liuche merged commit 113241e into mozilla-mobile:master Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants