1365129 - Cloud Storage System Add-on

Status: RESOLVED WONTFIX

(Firefox :: General, enhancement)

Description

[Punam Dahiya [:pdahiya]]

System Add-on to consume cloud storage back-end module. Add-on should
a) Host prompt UI and localization support
b) Trigger scan and prompts user to opt-in to save downloaded files to provider download folder
b) Use provider icon to highlight items downloaded in provider folders in download panel and downloads history

Comment 1

[Punam Dahiya [:pdahiya]]

Attachment: WIP Add-on patch

Comment 2

[Punam Dahiya [:pdahiya]]

To keep the initial implementation simple I am exploring the possibility of implementing the core cloud storage module in firefox code base and keeping the below code inside a system add-on
a) Trigger scan on download invoked
b) Host and display door-hangar prompt 
c) Highlight cloud storage downloaded item with provider icon

Attached patch shows door-hangar prompt and handles confirm and cancel inside nsHelperAppDialog.js (as suggested using flow when we prompt user for destination folder). Its WIP and I would like your feedback on if its feasible to bring out this code as a module inside add-on and hook it in nsHelperAppDialog.js. It will greatly help if you can provide inputs on how to best implement this using add-on.Thanks

Comment 3

[:Gijs (he/him)]

(In reply to Punam Dahiya [:pdahiya] from comment #2)
> To keep the initial implementation simple I am exploring the possibility of
> implementing the core cloud storage module in firefox code base and keeping
> the below code inside a system add-on
> a) Trigger scan on download invoked
> b) Host and display door-hangar prompt 
> c) Highlight cloud storage downloaded item with provider icon
> 
> Attached patch shows door-hangar prompt and handles confirm and cancel
> inside nsHelperAppDialog.js (as suggested using flow when we prompt user for
> destination folder). Its WIP and I would like your feedback on if its
> feasible to bring out this code as a module inside add-on and hook it in
> nsHelperAppDialog.js. It will greatly help if you can provide inputs on how
> to best implement this using add-on.Thanks

This is an export of multiple diffs, and this means I can't see what's going on in splinter (and lots of the splinter review UI is broken because there are multiple instances of each file and it doesn't know how to deal with that). Can you attach the patches separately, and/or only attach the patch you want me to look at?

I don't really know what would be a good way of doing this via a system add-on. The integration with nsHelperAppDlg I think should probably still land in central... We don't have a good way for add-ons to modify parts of other components without causing serious issues once the original component changes, which would defeat the point of trying to put that in a system add-on (because you would want it to work against multiple versions of Firefox and thus multiple versions of the component in question).

I think the simplest would be to add some kind of hooking API to nsHelperAppDlg once it tries to determine the folder to save into. But doesn't the download API already provide something like this? Paolo?

Comment 4

[:Paolo Amadini]

(In reply to :Gijs from comment #3)
> I think the simplest would be to add some kind of hooking API to
> nsHelperAppDlg once it tries to determine the folder to save into. But
> doesn't the download API already provide something like this? Paolo?

No, but the WebExtensions team is looking into adding something that could at least support some use cases. It may end up being reusable here, but the timeline is still uncertain as the team has to evaluate this against other priorities.

Comment 5

[Punam Dahiya [:pdahiya]]

Attachment: Initial implementation of download hook

Patch that uses destination select flow to hook in cloud storage check and display prompt

Comment 6

[Punam Dahiya [:pdahiya]]

Attachment: WIP Add-on that scans for storage provider

Initial implementation of add-on that scans for storage providers and identify if user is a cloud services candidate

Comment 7

[Punam Dahiya [:pdahiya]]

(In reply to :Paolo Amadini from comment #4)
> (In reply to :Gijs from comment #3)
> > I think the simplest would be to add some kind of hooking API to
> > nsHelperAppDlg once it tries to determine the folder to save into. But
> > doesn't the download API already provide something like this? Paolo?
> 
> No, but the WebExtensions team is looking into adding something that could
> at least support some use cases. It may end up being reusable here, but the
> timeline is still uncertain as the team has to evaluate this against other
> priorities.

I agree it will be useful to have web extension API allow save at user defined download location, and expose something like setDownloadLocation

Comment 8

[Punam Dahiya [:pdahiya]]

> I don't really know what would be a good way of doing this via a system
> add-on. The integration with nsHelperAppDlg I think should probably still
> land in central... We don't have a good way for add-ons to modify parts of
> other components without causing serious issues once the original component
> changes, which would defeat the point of trying to put that in a system
> add-on (because you would want it to work against multiple versions of
> Firefox and thus multiple versions of the component in question).
> 

I will dig into overriding getPreferredDownloadsDirectory or may be create a new method say getPreferredDownloadsDirectoryIncludeCloud in DownloadIntegration thats called from nsHelperAppDialog.js
This new method can encapsulate cloud storage checks, show prompt and return download dir. Is that looks like a viable approach? 

Doorhangar prompt is async and not a modal prompt and that limits options to take it out of nsHelperAppDialog.js and easily hook. Is there a better way to implement it using notifications and observing for the notification in a separate module? Thanks

Comment 9

[:Gijs (he/him)]

(In reply to Punam Dahiya [:pdahiya] from comment #8)
> > I don't really know what would be a good way of doing this via a system
> > add-on. The integration with nsHelperAppDlg I think should probably still
> > land in central... We don't have a good way for add-ons to modify parts of
> > other components without causing serious issues once the original component
> > changes, which would defeat the point of trying to put that in a system
> > add-on (because you would want it to work against multiple versions of
> > Firefox and thus multiple versions of the component in question).
> > 
> 
> I will dig into overriding getPreferredDownloadsDirectory or may be create a
> new method say getPreferredDownloadsDirectoryIncludeCloud in
> DownloadIntegration thats called from nsHelperAppDialog.js
> This new method can encapsulate cloud storage checks, show prompt and return
> download dir. Is that looks like a viable approach? 
> 
> Doorhangar prompt is async and not a modal prompt and that limits options to
> take it out of nsHelperAppDialog.js and easily hook. Is there a better way
> to implement it using notifications and observing for the notification in a
> separate module? Thanks

I really quite honestly have no idea. My understanding is that the download already has to cope with async-ness because, if the user has selected to always be asked about a download, the file might be downloaded by the time the user selects a destination, and then the downloads code has to move it into place once the user has made a decision. Couldn't this use the same mechanism?

Otherwise, you could decide to prompt and return synchronously, and just save the file to the default destination, but to keep track of that and any future downloads until the user makes a decision, and move the files after the fact. That wouldn't require nsHelperAppDialog.js to cope with an asynchronous response.

But I really don't know this code well, and Paolo has already said in the past that there are several paths for downloads here, so I think he would be the best person to help here. I would have to go read all the code just like you.

Comment 10

[Punam Dahiya [:pdahiya]]

Attachment: WIP Add-on that scans for storage provider

Initial implementation of add-on that scans for storage providers and identify if user is a cloud services candidate

Comment 11

[Punam Dahiya [:pdahiya]]

Attachment: Initial implementation of download hook

Patch that uses destination select flow to hook in cloud storage check and display prompt

Comment 12

[Punam Dahiya [:pdahiya]]

> I really quite honestly have no idea. My understanding is that the download
> already has to cope with async-ness because, if the user has selected to
> always be asked about a download, the file might be downloaded by the time
> the user selects a destination, and then the downloads code has to move it
> into place once the user has made a decision. Couldn't this use the same
> mechanism?

I agree that seems to be best solution, I have refactored and tested it to use promptForSaveToCloudStorage called inside the async save to file flow (attached download hook patch). 

We can refactor it further to move PopupNotifications specific code (showCloudStoragePrompt) in a separate browser module (or add-on if possible?). Paolo, if there is another way using notification observer or another path to better integrate, please pitch in. Thanks


> Otherwise, you could decide to prompt and return synchronously, and just
> save the file to the default destination, but to keep track of that and any
> future downloads until the user makes a decision, and move the files after
> the fact. That wouldn't require nsHelperAppDialog.js to cope with an
> asynchronous response.
> 
> But I really don't know this code well, and Paolo has already said in the
> past that there are several paths for downloads here, so I think he would be
> the best person to help here. I would have to go read all the code just like
> you.

Comment 13

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage Add-on

Comment 14

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage Add-on - async destination select as back-end

Comment 15

[:Paolo Amadini]

(In reply to :Gijs from comment #9)
> My understanding is that the download
> already has to cope with async-ness because, if the user has selected to
> always be asked about a download, the file might be downloaded by the time
> the user selects a destination, and then the downloads code has to move it
> into place once the user has made a decision. Couldn't this use the same
> mechanism?

That's correct. Also bug 1306334 is about adding a permission for multiple downloads, that would be implemented with a doorhanger in this code path.

Comment 16

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Comment 17

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Patch updated and rebased

Comment 18

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Comment 19

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Updated and rebased patch

Comment 20

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Comment 21

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Comment 22

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Comment 23

[Punam Dahiya [:pdahiya]]

Attachment: Concept UI - Download List

Comment 24

[Punam Dahiya [:pdahiya]]

Attachment: Concept UI - Offers as door hangar prompt

Comment 25

[Punam Dahiya [:pdahiya]]

To test add-on
Pr-requisite
- Desktop with cloud storage provider client such as Dropbox , Google Drive

Steps
1) Open link e.g. https://www.mozilla.org/en-US/firefox/all/
2) Click on any download link. If you see the prompt 'You have chosen to open' asking to choose between open and save file - select save file.
3) First download save initializes cloud storage API and you will see the success message in logs.
4) Any subsequent downloads should show door hangar prompt asking to save to provider download folder
5) Downloaded item will be marked with provider icon in Download history

Comment 26

[Punam Dahiya [:pdahiya]]

6) Result expected with different options selected in door hangar prompt
  a) Save to provider download folder -  Save downloaded file to provider local download folder
  b) Cancel - Save file to user default download folder
  c) Save with always remember checked - Sets provider download folder as default download by updating pref browser.download.folderlist as 3 and any subsequent download will be saved to provider download folder
  d) Cancel with always remember checked - Set provider as rejected in cloud.services.rejected.key  pref and user will never be prompted again to use the provider. If a user has multiple provider on desktop , other providers will be used in door hangar prompt.
  e) cloud.services.prompt.interval pref is set to 0 days by default, changing this pref sets the interval at which user should be prompted again to opt-in.

Comment 27

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Comment 28

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage add-on using observer notifications

Updated and rebased

Comment 29

[Punam Dahiya [:pdahiya]]

Link to bootstrap add-on that's tested on firefox developer edition

https://github.com/punamdahiya/CloudStorage

Comment 30

[Punam Dahiya [:pdahiya]]

WIP Add-on using shield utils library

https://github.com/punamdahiya/CloudStorage/tree/cloud-storage-shield-study

Comment 31

[Punam Dahiya [:pdahiya]]

Marking won't fix as feature was implemented as shield add-on in bug 1399231