For #7364 - Adds sessionId to DownloadState and populates it in DowloadFeature #7365
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7365 +/- ##
============================================
+ Coverage 77.37% 77.56% +0.19%
+ Complexity 5064 4919 -145
============================================
Files 675 652 -23
Lines 24774 23926 -848
Branches 3663 3563 -100
============================================
- Hits 19168 18558 -610
+ Misses 4094 3909 -185
+ Partials 1512 1459 -53
Continue to review full report at Codecov.
|
|
Some unit tests are failing. :( Looking into them now. |
codrut-topliceanu
force-pushed
the
tabid_download_State
branch
from
9c8827f to
e3dd6bf
Compare
| val download = state.content.download | ||
| if (download != null) { | ||
| processDownload(state, download) | ||
| state.content.download?.let { downloadState -> |
There was a problem hiding this comment.
I think will be better if we populate this property when the download request is issued
This way we are going to have a consistent state in BrowserStore. what do you think?
There was a problem hiding this comment.
@Amejia481 Thanks for taking a look at this. That's a really good solution that hadn't occurred to me! I just tried it now and it works great. Thank you.
There was a problem hiding this comment.
@Amejia481 @codrut-topliceanu Thanks and sorry for the late response.
When creating a DownloadState we always know the sessionId. So, we can simply set the sessionId here when creating the download: https://github.com/mozilla-mobile/android-components/blob/master/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt#L185
It doesn't need to be a side-effect / unexpected change of reducing.
There was a problem hiding this comment.
I already tried that and it didn't work, I'm afraid. It only worked for downloads that had skip confirmation == true.
There was a problem hiding this comment.
@codrut-topliceanu Hm can you share some more details? It effectively shouldn't matter when the sessionId is added, so why not add it directly when the download is created? It seems like a potential source of error to do this "later" (when updating in the store).
There was a problem hiding this comment.
Do you mean that when you try to download a file in some cases this line doesn't get executed? If that it's the case then this is a GeckoView issue and it's a separate issue from this one.
There was a problem hiding this comment.
Yep that's the case.
There was a problem hiding this comment.
Would you mind opening a bug in Bugzilla describing the the issue and the step to reproduce it?
There was a problem hiding this comment.
Ok, I'll give it a try. I've never opened one in Bugzilla before.
There was a problem hiding this comment.
If you need any help just let me know, happy to help!
codrut-topliceanu
force-pushed
the
tabid_download_State
branch
from
e3dd6bf to
0f9d037
Compare
…es it in ContentStateReducer
codrut-topliceanu
force-pushed
the
tabid_download_State
branch
from
0f9d037 to
fc06a09
Compare
|
bors r=csadilek |
|
Build succeeded: |
For #7364
Pull Request checklist
After merge