For #5799: document architecture choices #5800

severinrudie · 2019-10-04T22:22:31Z

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Documentation changes only
Tests: This PR includes thorough tests or an explanation of why it does not
Documentation changes only
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Documentation changes only
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features
Documentation changes only

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:

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

colintheshots · 2019-10-08T17:00:51Z

docs/architecture-overview.md

+  - added complexity
+  - distinction was removed
+- Rx 
+  - slowed startup


Coroutines were actually worse until we implemented a fix in R8. Coroutines Dispatchers.Main blocked the main thread >200ms. Rx AndroidSchedulers.MainThread blocked the main thread 42ms on the same hardware. Coroutines are now faster because we strip out the wasted, blocking code using an early version of R8. Long story short, we were using both and had both blocking.

Good context, thank you

st3fan · 2019-10-08T17:04:41Z

Isn't the HomeActivity the last component that still needs to be moved to the new architecture? It is probably not a good example at this point.

docs/architecture-overview.md

severinrudie · 2019-10-08T19:58:08Z

I also saw that the View's Store is kept in that StoreProvider (an androidx.lifecycle.ViewModel) and also the Fragment state is kept in another ViewModel.
I consider this to be an incorrect usage of the ViewModel given that HomeActivity (and others) do register in manifest for orientation config changes and so the ViewModel's usecase - survice state across screen rotations, is not something that we actually need.

For this doc I'm focused on describing what we have at present for new contributors, rather than suggesting any changes. That is definitely something you could bring up in future architecture work though!

- Remove references to old architecture (Soon it will all have been replaced. No need for the additional cognitive load) - Add some subheadings - 'Simplified Example' seems like a good idea. Update the language to clarify that it will be done

codecov-io · 2019-10-08T20:46:25Z

Codecov Report

Merging #5800 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master    #5800     +/-   ##
===========================================
+ Coverage      14.3%   14.41%   +0.1%     
  Complexity      324      324             
===========================================
  Files           259      256      -3     
  Lines         10700    10522    -178     
  Branches       1560     1532     -28     
===========================================
- Hits           1531     1517     -14     
+ Misses         9044     8879    -165     
- Partials        125      126      +1

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/org/mozilla/fenix/ext/BookmarkNode.kt	`0% <0%> (-50%)`	`0% <0%> (ø)`
.../fenix/browser/browsingmode/BrowsingModeManager.kt	`22.22% <0%> (-44.45%)`	`0% <0%> (ø)`
...pp/src/main/java/org/mozilla/fenix/FeatureFlags.kt	`88.88% <0%> (-11.12%)`	`2% <0%> (ø)`
...g/mozilla/fenix/crashes/CrashReporterController.kt	`94.11% <0%> (-1.34%)`	`6% <0%> (ø)`
...in/java/org/mozilla/fenix/settings/SupportUtils.kt	`60% <0%> (-1.12%)`	`8% <0%> (ø)`
...illa/fenix/components/toolbar/BrowserInteractor.kt	`95.65% <0%> (-0.19%)`	`11% <0%> (ø)`
...org/mozilla/fenix/crashes/CrashReporterFragment.kt	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...kmarks/selectfolder/SelectBookmarkFolderAdapter.kt	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...a/org/mozilla/fenix/components/IntentProcessors.kt	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...x/home/sessioncontrol/viewholders/TabViewHolder.kt	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
... and 59 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 d0a39f0...fdde820. Read the comment docs.

…t objects'

severinrudie · 2019-10-16T22:41:10Z

docs/architecture-overview.md

+- see `differences from previous architecture` for more in depth description of changes made to reduce complexity
+
+
+## Architecture Overview


NOTE TO REVIEWERS: the formatted version of this may be easier to review

EDIT: forgot I renamed that file. Link fixed

severinrudie · 2019-10-16T22:41:20Z

docs/architecture-overview.md

@@ -0,0 +1,159 @@
+## Architecture Overview


NOTE TO REVIEWERS: the formatted version of this may be easier to review

EDIT: forgot I renamed that file. Link fixed

csadilek

Great work @baron-severin! I've added a few comments but this is already super comprehensive.

docs/architecture-overview.md

csadilek · 2019-10-18T14:49:17Z

docs/architecture-overview.md

+Is pushed to: [Store](#store)
+
+#### Description
+Simple data object that carries information about a [State](#state) change to a [Store](#store).  An Action describes _something that happened_, and carries any data relevant to that change. For example, `AccountSettingsFragmentAction.SyncFailed(val time: Long)`, which describes a sync that failed at a specific time.


Interesting we're not super consistent with Action naming. They can be reactive, describing something that has happened or active, describing something that should happen e.g. UpdateQuery.

Yeah, I think this is a symptom of some blending of responsibilities between Controllers and Reducers. When the Controller handles the logic, the Action winds up essentially being a setter.

docs/architecture-overview.md

boek · 2019-10-21T20:36:43Z

docs/architecture-overview.md

+
+See [mozilla.components.lib.state.Action](https://github.com/mozilla-mobile/android-components/blob/master/components/lib/state/src/main/java/mozilla/components/lib/state/Action.kt)
+
+Created by: [Controller](#controller)


Do we want to list every possible thing that can create these? There are cases where Actions are created outside of the controller

Oof, I didn't realize. I know they used to be created in Interactors, but my understanding was that we were moving away from that and towards Controllers. ~~Where else are they created?~~

EDIT: better question, is it fine that they are sent from a variety of places, or do we want them to only be sent by Controllers in the future? I don't have the context to answer this one.

boek

This is awesome! I'm going to land this, do you want to update this wiki page with a reference to this? https://github.com/mozilla-mobile/fenix/wiki/Architecture-Decisions

For mozilla-mobile#5799: add architecture document outline for review

6c9903f

severinrudie changed the title ~~For #5799: add architecture document outline for review~~ For #5799: document architecture choices Oct 4, 2019

colintheshots reviewed Oct 8, 2019

View reviewed changes

csadilek reviewed Oct 8, 2019

View reviewed changes

docs/architecture-overview.md Outdated Show resolved Hide resolved

docs/architecture-overview.md Outdated Show resolved Hide resolved

docs/architecture-overview.md Outdated Show resolved Hide resolved

For 5799: update architecture doc outline

b2a35df

- Remove references to old architecture (Soon it will all have been replaced. No need for the additional cognitive load) - Add some subheadings - 'Simplified Example' seems like a good idea. Update the language to clarify that it will be done

severinrudie added 5 commits October 10, 2019 15:52

For 5799: add additional known limitations

0a7209c

For 5799: wrote first draft for architecture 'overview' and 'importan…

10f896b

…t objects'

For 5799: wrote first draft for arch doc 'important notes'

2f0ef62

For 5799: wrote arch doc 'known limitations' section

ea91025

For 5799: wrote example code for architecture doc

742b6d6

severinrudie force-pushed the 5799-document-architecture branch 4 times, most recently from c9b7fed to 12cd76a Compare October 16, 2019 22:00

For 5799: added example app wireframe for arch docs

b307fab

severinrudie force-pushed the 5799-document-architecture branch from 12cd76a to b307fab Compare October 16, 2019 22:04

severinrudie added 3 commits October 16, 2019 15:13

For 5799: update arch docs 'Simplified Example section'

52b5e31

For 5799: improve formatting for architecture docs

16fc395

For 5799: minor tweaks to architecture docs

b29e4aa

severinrudie force-pushed the 5799-document-architecture branch from 14818a8 to b29e4aa Compare October 16, 2019 22:40

severinrudie commented Oct 16, 2019

View reviewed changes

severinrudie marked this pull request as ready for review October 16, 2019 22:41

csadilek approved these changes Oct 18, 2019

View reviewed changes

severinrudie force-pushed the 5799-document-architecture branch from a23bb86 to 939603d Compare October 18, 2019 16:47

For 5799: link 'simplified example' section to example code

25c668e

severinrudie force-pushed the 5799-document-architecture branch from 939603d to 25c668e Compare October 18, 2019 16:47

For 5799: update arch doc per review

fdde820

boek reviewed Oct 21, 2019

View reviewed changes

boek approved these changes Oct 21, 2019

View reviewed changes

boek merged commit a0ca8b8 into mozilla-mobile:master Oct 21, 2019

severinrudie mentioned this pull request Oct 21, 2019

Document current approach to architecture #5799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #5799: document architecture choices #5800

For #5799: document architecture choices #5800

severinrudie commented Oct 4, 2019 •

edited

Loading

colintheshots Oct 8, 2019 •

edited

Loading

severinrudie Oct 8, 2019

st3fan commented Oct 8, 2019

severinrudie commented Oct 8, 2019

codecov-io commented Oct 8, 2019 •

edited

Loading

severinrudie Oct 16, 2019 •

edited

Loading

severinrudie Oct 16, 2019 •

edited

Loading

csadilek left a comment

csadilek Oct 18, 2019

severinrudie Oct 18, 2019

boek Oct 21, 2019

severinrudie Oct 21, 2019 •

edited

Loading

boek left a comment

		- see `differences from previous architecture` for more in depth description of changes made to reduce complexity


		## Architecture Overview


		See [mozilla.components.lib.state.Action](https://github.com/mozilla-mobile/android-components/blob/master/components/lib/state/src/main/java/mozilla/components/lib/state/Action.kt)

		Created by: [Controller](#controller)

For #5799: document architecture choices #5800

For #5799: document architecture choices #5800

Conversation

severinrudie commented Oct 4, 2019 • edited Loading

Pull Request checklist

After merge

To download an APK when reviewing a PR:

colintheshots Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

severinrudie Oct 8, 2019

Choose a reason for hiding this comment

st3fan commented Oct 8, 2019

severinrudie commented Oct 8, 2019

codecov-io commented Oct 8, 2019 • edited Loading

Codecov Report

severinrudie Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

severinrudie Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

csadilek left a comment

Choose a reason for hiding this comment

csadilek Oct 18, 2019

Choose a reason for hiding this comment

severinrudie Oct 18, 2019

Choose a reason for hiding this comment

boek Oct 21, 2019

Choose a reason for hiding this comment

severinrudie Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

boek left a comment

Choose a reason for hiding this comment

severinrudie commented Oct 4, 2019 •

edited

Loading

colintheshots Oct 8, 2019 •

edited

Loading

codecov-io commented Oct 8, 2019 •

edited

Loading

severinrudie Oct 16, 2019 •

edited

Loading

severinrudie Oct 16, 2019 •

edited

Loading

severinrudie Oct 21, 2019 •

edited

Loading