mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.14k stars 2.91k forks source link

Update DownloadQueue and related scene handling code for multi-window #19720

Open data-sync-user opened 5 months ago

data-sync-user commented 5 months ago

We have code in our SceneDelegate which appears to be scene-specific, ex:

// Resume previously stopped downloads for, and on, THIS scene only.
        downloadQueue.resumeAll()

However the downloadQueue itself is a shared instance stored in the AppContainer.

Action Items:

┆Issue is synchronized with this Jira Task

data-sync-user commented 4 months ago

➤ Matt Reagan commented:

Right now there is a bit of a UX/UI predicament with file downloading and multi-window. The UI for cancelling / resuming downloads is displayed as a banner/toast pinned to a particular window. So if users are in the midst of downloading a file in one window and they close it, the download will continue in the background but they have no way (AFAIK) of monitoring progress or cancelling it directly. (The downloads panel only shows completed downloads, not in-progress ones afaics).

Because this is kind of an edge case and can be avoided by just not closing the window, for the MVP I’m inclined to move forward with the simplest solution which is to just keep the behavior we have and not add any new UI for this. But I wanted to be sure to keep folks in the loop so we can begin considering solutions moving forward for 1.x of MW. If anyone thinks the current behavior (described above) would be a deal-breaker for the MVP please let me know, happy to discuss further.

cc Norberto Andres Furlan Nicole Weber Orla Mitchell

data-sync-user commented 4 months ago

➤ Matt Reagan commented:

Another potential issue here that will need consideration is how we are currently calculating combined bytes. At some point we will need to revisit this, though I suspect that will happen after MVP ships. Currently, combined bytes is calculated for all downloads, if we want to treat downloads as being handled on a per-window basis then the combined bytes total will be incorrect as it’s currently handled.

data-sync-user commented 4 months ago

➤ Nicole Weber commented:

I'm all for getting stuff out and agree that this seems like a corner case. That said, I want to make sure we have a feasible plan to address that, worst case: you accidentally download some very large stuff on a limited data plan and then realize it, panic and close the window because you think that would stop the download.

Desktop is showing a modal if you close Firefox while something is downloading, this might be something we could do too, not sure how much work that would be

!Screenshot 2024-04-25 at 14.02.36.png|width=600,height=231,alt="Screenshot 2024-04-25 at 14.02.36.png"!

data-sync-user commented 4 months ago

➤ Matt Reagan commented:

Nicole Weber I would love to add this but it doesn’t appear to be possible on iPadOS; I’ve not been able to find any APIs that allow us to cancel the closing of a window (in order to show an alert and let the user potentially cancel the closure). This isn't surprising really since apps themselves on iOS also can't cancel being closed from the app switcher etc. I also haven't been able to find any Apple iPad apps yet that do anything like this either, so I don't believe there is even any SPI which supports it. I will update if I find a solution for this but in the meantime I think most likely we will need to explore other solutions (some of which will probably have to wait until after the MVP is released, if I had to guess, based on expected LOE).

data-sync-user commented 3 months ago

➤ Andrei Bodea commented:

Hello, Matt Reagancan you please help with some steps in order for this issue to be verified? Is it everything from the action items?

data-sync-user commented 3 months ago

➤ Matt Reagan commented:

Hi Andrei Bodea this was principally a refactor to allow downloads to work in multi-window, though there are some aspects of the UX which are still TBD. In short we can probably verify this simply by regression-testing basic downloading in a multi-window environment. Ex:

  1. Ensure that users can download a file successfully with a single window open (regression test)
  2. Ensure that users can download a file successfully with multiple windows open
  3. Ensure that downloaded files show up correctly in the download panel (file list)

I believe that based on the tickets I’ve seen from QA that much of this has already been exercised by the multi-window test cases QA has been running, if so you can probably simply close this since it is primarily a refactor ticket.

data-sync-user commented 3 months ago

➤ Andrei Bodea commented:

Verified as fixed on v128 (42711) with iPad Pro (16.4.1).

  1. I was able to download it without issue with a single window open.
  2. I was able to download a file with multiple windows open - one at a time and two at the same time.
  3. Downloaded files did update correctly on the downloads section.