Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

2 years ago
Implement Cloud Storage JSM module that facilitates
- Cloud storage service discovery on user desktop
- let user opt-in and set a preferred local storage service.
(Assignee)

Updated

2 years ago
Depends on: 1357160
(Assignee)

Updated

2 years ago
No longer depends on: 1357160
(Assignee)

Updated

2 years ago
Blocks: 1357160
(Assignee)

Comment 1

2 years ago
Created attachment 8859252 [details] [diff] [review]
WIP Cloud Storage API patch
(Assignee)

Updated

2 years ago
Assignee: nobody → pdahiya
(Assignee)

Updated

2 years ago
Summary: Cloud Storage Discovery → Cloud Storage API
(Assignee)

Comment 2

2 years ago
browser.download preferences helper from a comment in code base

  /*
   * Preferences:
   *
   * browser.download.useDownloadDir - bool
   *   True - Save files directly to the folder configured via the
   *   browser.download.folderList preference.
   *   False - Always ask the user where to save a file and default to
   *   browser.download.lastDir when displaying a folder picker dialog.
   * browser.download.dir - local file handle
   *   A local folder the user may have selected for downloaded files to be
   *   saved. Migration of other browser settings may also set this path.
   *   This folder is enabled when folderList equals 2.
   * browser.download.lastDir - local file handle
   *   May contain the last folder path accessed when the user browsed
   *   via the file save-as dialog. (see contentAreaUtils.js)
   * browser.download.folderList - int
   *   Indicates the location users wish to save downloaded files too.
   *   It is also used to display special file labels when the default
   *   download location is either the Desktop or the Downloads folder.
   *   Values:
   *     0 - The desktop is the default download location.
   *     1 - The system's downloads folder is the default download location.
   *     2 - The default download location is elsewhere as specified in
   *         browser.download.dir.
   * browser.download.downloadDir
   *   deprecated.
   * browser.download.defaultFolder
   *   deprecated.
   */
(Assignee)

Comment 3

2 years ago
Preferences used by Cloud Storage API

cloud.services.candidate — set to true by cloud storage API scan if user suffice below criteria
a) user frequently downloading things b) has dropbox (or other cloud storage service) on desktop

cloud.services.storage - set to string by cloud storage API with cloud storage service name if a user accepts saving to cloud storage in door hanger prompt

cloud.services.lastPromptShown - set by cloud storage API to time when last door hangar prompt was shown to avoid prompting user too soon

cloud.services.lastScan - set by cloud storage API to time when last scan was run on user desktop, cloud storage scan should be lazy and should run once in couple of days

browser.download.folderList - set to to custom value as ‘2’ if user decide to use cloud storage 

browser.download.dir - set by CloudStorage API to cloud storage download directory path if user decide to use cloud storage
(Assignee)

Comment 4

2 years ago
Possible Cloud Storage API hooks in downloads flow

a) https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/DownloadsCommon.jsm#756
 
When a download is invoked and sufficient time has passed since last scan (pref cloud.services.lastScan) ,  scan user desktop to set pref cloud.services.candidate

b) https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#259

If user has browser.download.useDownloadDir preference set, show cloud storage door hanger prompt (and continue the download in the background, similar to how we do this when we prompt the user for a destination folder) and handle confirm and cancel (save locally). We need to also handle no action that is user ignore door hanger.

Remove Cloudstorage.js from attached WIP patch to implement a and b. Inputs on above downloads flow touch points and direction are welcome!
>    * browser.download.folderList - int
>    *   Indicates the location users wish to save downloaded files too.
>    *   It is also used to display special file labels when the default
>    *   download location is either the Desktop or the Downloads folder.
>    *   Values:
>    *     0 - The desktop is the default download location.
>    *     1 - The system's downloads folder is the default download location.
>    *     2 - The default download location is elsewhere as specified in
>    *         browser.download.dir.

I was thinking we would add "3 - The default download location is elsewhere as specified by CloudStorage.jsm" or similar.  Rather than relying on changing the directory pref manually, the download manager would always check the service for the correct path.  That keeps the logic for checking/updating paths entirely in CloudStorage.jsm, and avoids any potential for a stale value in the download pref.
(Assignee)

Comment 6

2 years ago
Plan to split attached patch in two parts

Part 1 implements CloudStorage.jsm and associated tests
- Scan for cloud storage
- Check scan and last prompt shown time intervals
- check and update cloud.services and related prefs
- CloudStorage metadata and related accessor functions
- Any edge cases such as multiple cloud storage services on desktop

Part 2 will consume CloudStorage API in downloads flow
(Assignee)

Comment 7

2 years ago
Created attachment 8863923 [details] [diff] [review]
Cloud Storage back-end module
(Assignee)

Updated

2 years ago
Attachment #8859252 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8864332 [details] [diff] [review]
Cloud Storage back-end module
Attachment #8863923 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
Passing try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08a55a1f74da75eaded30fa817ebae2a80fc90d

Mike, can you please take a first review pass at it. Thanks!
Flags: needinfo?(mconnor)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review139398

I've given some high-level feedback but Mike should probably take a closer look.

One generic concern I have is what the division of labour is supposed to be. What does this module do on its own, and who calls into it? The most common pattern is to try to lazy-load the module based on some kind of topical event (like a download starting) but from the point of it being loaded it should basically do what it does on its own. Right now it seems to provide a bunch of utility functions that are being unit-tested (sort of) but it's hard to work out what its responsibilities are, and which responsibilities are to be delegated to other code (that isn't in this patch).

::: toolkit/modules/CloudStorage.jsm:66
(Diff revision 1)
> + *
> + */
> +
> +// URI to access icon files
> +function getIconURI(name) {
> +  let dirPath = "chrome://browser/content/cloudstorage/";

If this code lives in toolkit the images should also be in toolkit. But there are no images in this commit... I expect this will break mochitests we have that check that files have references and that no files are missing.

::: toolkit/modules/CloudStorage.jsm:98
(Diff revision 1)
> +    WINNT_DROPBOX: {
> +      displayName: "DropBox",

If we've got Windows code, why aren't there tests for Windows?

::: toolkit/modules/CloudStorage.jsm:101
(Diff revision 1)
> +        get default() {
> +          return OS.Constants.Path.homeDir ?
> +          OS.Path.join(OS.Constants.Path.homeDir, "Dropbox") : null
> +        },
> +        get custom() {
> +          return OS.Constants.Path.homeDir ?
> +          OS.Path.join(OS.Constants.Path.homeDir, "Dropbox") : null
> +        },

The indentation here is wrong, but also these two methods do the exact same thing. Why do we need both?

::: toolkit/modules/CloudStorage.jsm:115
(Diff revision 1)
> +        get default() { return getIconURI("dropbox_18x18.png") },
> +        get tiny() { return getIconURI("dropbox.png") },

These don't exist, but also they're the same across platform, so we should probably not be duplicating this information.

More generally it feels like the information should be per-provider, with simple if() code that returns the right paths depending on AppConstants.platform.

::: toolkit/modules/CloudStorage.jsm:153
(Diff revision 1)
> +        screenshot: "Screenshots"
> +      },
> +    },
> +
> +    WINNT_GDRIVE: {
> +      displayName: "Google Drive",

Does this need localizing?

::: toolkit/modules/CloudStorage.jsm:157
(Diff revision 1)
> +    WINNT_GDRIVE: {
> +      displayName: "Google Drive",
> +      downloadPath: {
> +        get default() {
> +          return OS.Constants.Path.homeDir ?
> +          OS.Path.join(OS.Constants.Path.homeDir, "Google Drive") : null

Is this the same on different localized versions of Google Drive?

::: toolkit/modules/CloudStorage.jsm:292
(Diff revision 1)
> +      let lastScan = this.getLastScanTime();
> +      let now = Math.floor(Date.now() / 1000);
> +      let interval = now - lastScan;
> +
> +      // Convert SCAN_INTERVAL to seconds
> +      let maxAllow = SCAN_INTERVAL * 24 * 60 * 60;

Why doesn't this use .getScanInterval() ?

More generally, the scan interval should probably be a pref with a default value. Use XPCOMUtils.defineLazyPreferenceGetter to define it as a property on this object that automatically updates if people change the preference.

::: toolkit/modules/CloudStorage.jsm:295
(Diff revision 1)
> +    } catch (ex) {
> +      // Exception: Pref not set or found
> +      return true;

This seems broken. Just provide a default value  when getting the pref in getLastScanTime(), rather than try...catch()ing this entire block, which might hide other failures.

::: toolkit/modules/CloudStorage.jsm:436
(Diff revision 1)
> +
> +
> +  /**
> +   * gets access to metadata of cloud storage provider accepted by user for default download
> +   */
> +

Please tighten up the whitespace. No empty lines at the start and end of functions, no empty lines between methods/properties and their docstrings.

::: toolkit/modules/CloudStorage.jsm:447
(Diff revision 1)
> +
> +      // Use key to retrieve metadata from ProvidersMetaData
> +      return this._providersMetaData.hasOwnProperty(this._prefStorageKey) ?
> +        this._providersMetaData[this._prefStorageKey] : null;
> +    } catch (ex) {
> +      // Exception: Pref not set or found

Again, please provide a default value when calling getCharPref(), here and elsewhere.

::: toolkit/modules/CloudStorage.jsm:485
(Diff revision 1)
> +    if (!folderList) {
> +      folderList = 2;
> +    }

Just provide a default value for the argument, and document that this is the default.

::: toolkit/modules/CloudStorage.jsm:586
(Diff revision 1)
> +var debug;
> +if (DEBUG) {
> +  debug = function(msg) {
> +    dump("cloud storage: " + msg + "\n");
> +  };
> +} else {
> +  debug = function(msg) {};
> +}

Consider using Log.jsm or just removing this and using Cu.reportError() for cases where we need actual debug output.

::: toolkit/modules/tests/xpcshell/test_cloudstorage_maclinux.js:49
(Diff revision 1)
> +  return file;
> +}
> +
> +function mock_gdrive() {
> +  mockGDrive = false;
> +  let file = FileUtils.getFile("Home", ["Library", "Application Support", "Google", "Drive"]);

This test generally seems problematic in that it looks for the 'mac' dropbox provider even on non-mac platforms. It shouldn't do that, it should test the right platform-specific stuff.

::: toolkit/modules/tests/xpcshell/test_cloudstorage_maclinux.js:174
(Diff revision 1)
> +      // No storage services installed on user desktop
> +      Assert.equal(result.size, 0, "Number of storage providers");
> +      preExistProviders = false;
> +    } else if ( result.size > 0 ) {
> +      Assert.ok(result.size, "Number of storage providers");
> +      preExistProviders = true;

So if you have any providers installed all the tests are skipped? That doesn't seem right - we should still be testing this stuff. You can use the mock directory providers to avoid hitting the 'real' providers in the tests. See e.g. the chrome profile migrator tests in browser/components/migration.
Attachment #8864579 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review139398

Thanks Gijs for the review feedback.  I will address/clarify the issues raised.  Regarding responsibilities of this module, this is how I see the external facing interface: 
a) Expose “scan” method to be invoked by “OnDownloadAdded” event callback. 
b) Expose “setCloudStorage” method to be invoked by “Door Hanger prompt UI confirm” callback to enable saving of files to provider download folder.

In terms of what responsibilities are delegated to other code, there are still some open questions I need to sort through like:
- which module will host “Door Hanger prompt UI”
- who will build “Door Hanger prompt UI”

I will be working through these in the next patch. Feel free to give additional feedback on the above.

Comment 14

2 years ago
(In reply to Punam Dahiya [:pdahiya] from comment #13)
> Comment on attachment 8864579 [details]
> Bug 1357171 - cloud storage module
> 
> https://reviewboard.mozilla.org/r/136256/#review139398
> 
> Thanks Gijs for the review feedback.  I will address/clarify the issues
> raised.  Regarding responsibilities of this module, this is how I see the
> external facing interface: 
> a) Expose “scan” method to be invoked by “OnDownloadAdded” event callback. 
> b) Expose “setCloudStorage” method to be invoked by “Door Hanger prompt UI
> confirm” callback to enable saving of files to provider download folder.
> 
> In terms of what responsibilities are delegated to other code, there are
> still some open questions I need to sort through like:
> - which module will host “Door Hanger prompt UI”
> - who will build “Door Hanger prompt UI”

I think a module in browser/modules would be appropriate for this.
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review139398

> If we've got Windows code, why aren't there tests for Windows?

yes, will be adding tests for windows in updated patch

> The indentation here is wrong, but also these two methods do the exact same thing. Why do we need both?

custom is placeholder for handling usecase if user changes provider download folder location. This usecase needs to be vetted for different providers, but I agree we can remove and keep it simple by using one method.

> These don't exist, but also they're the same across platform, so we should probably not be duplicating this information.
> 
> More generally it feels like the information should be per-provider, with simple if() code that returns the right paths depending on AppConstants.platform.

+1

> Why doesn't this use .getScanInterval() ?
> 
> More generally, the scan interval should probably be a pref with a default value. Use XPCOMUtils.defineLazyPreferenceGetter to define it as a property on this object that automatically updates if people change the preference.

+1

> Just provide a default value for the argument, and document that this is the default.

will address missing images, default value for prefs and spacing issue. As for localization, need to clarify with product before addressing.

> Consider using Log.jsm or just removing this and using Cu.reportError() for cases where we need actual debug output.

+1

> This test generally seems problematic in that it looks for the 'mac' dropbox provider even on non-mac platforms. It shouldn't do that, it should test the right platform-specific stuff.

+1

> So if you have any providers installed all the tests are skipped? That doesn't seem right - we should still be testing this stuff. You can use the mock directory providers to avoid hitting the 'real' providers in the tests. See e.g. the chrome profile migrator tests in browser/components/migration.

If we have providers installed, we are running task test_multipleStorageProviders. Thanks for the pointer, its much safer to avoid hitting real providers and will update in next patch.
(Assignee)

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review139398

> This seems broken. Just provide a default value  when getting the pref in getLastScanTime(), rather than try...catch()ing this entire block, which might hide other failures.

Is this feedback still valid, if pref is non-existent? My understanding is attempting to read a nonexistent preference will throw exception even if it has default value in get*Pref method
(Assignee)

Comment 17

2 years ago
Setting NI to clarify review feedback. Thanks!
Flags: needinfo?(gijskruitbosch+bugs)

Comment 18

2 years ago
(In reply to Punam Dahiya [:pdahiya] from comment #16)
> Comment on attachment 8864579 [details]
> Bug 1357171 - cloud storage module
> 
> https://reviewboard.mozilla.org/r/136256/#review139398
> 
> > This seems broken. Just provide a default value  when getting the pref in getLastScanTime(), rather than try...catch()ing this entire block, which might hide other failures.
> 
> Is this feedback still valid, if pref is non-existent? My understanding is
> attempting to read a nonexistent preference will throw exception even if it
> has default value in get*Pref method

No, the point of providing the default value is that it doesn't throw and just returns the default. Try in the browser console:


Services.prefs.getBoolPref("adfgdsafgsdfgsd", false)

(or any other kind of gobbledygook as the pref name)

vs.

Services.prefs.getBoolPref("adfgdsafgsdfgsd")

and you can see the difference :-)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 19

2 years ago
Created attachment 8871508 [details] [diff] [review]
CloudStorage back-end using async destination select flow
Attachment #8864332 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8871508 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Comment 20

2 years ago
Comment on attachment 8871508 [details] [diff] [review]
CloudStorage back-end using async destination select flow

Attaching patch to get feedback on updated cloudstorage.jsm and cloudstorageprompt browser module used to hook in to download manager.

Patch is tested using system add-on in bug 1365129.

Features missing that are being worked on:

- Tests for updated cloud storage module on all platforms
- Door hangar prompt - Provider Icon and always remember my decision
- Provider custom download folders e.g. for dropbox read download path from info.json
Attachment #8871508 - Flags: feedback?(mconnor)

Comment 21

2 years ago
Comment on attachment 8871508 [details] [diff] [review]
CloudStorage back-end using async destination select flow

I'm swamped with other stuff right now. mconnor can probably provide initial feedback here.
Attachment #8871508 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Comment 22

2 years ago
Trying to arrive at code where DownloadTarget is populated with path inside newly created Download object and have narrowed down to below link. 

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1477

Gijs, Paolo, is there any other place in code base where we are updating target.path? Thanks!

The sequence of call is:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1191

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/Downloads.jsm#105

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadLegacy.js#228
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 23

2 years ago
(In reply to Punam Dahiya [:pdahiya] from comment #22)
> Trying to arrive at code where DownloadTarget is populated with path inside
> newly created Download object and have narrowed down to below link. 
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> jsdownloads/src/DownloadCore.jsm#1477
> 
> Gijs, Paolo, is there any other place in code base where we are updating
> target.path? Thanks!

Paolo will know for sure, but I think the three places in DownloadCore.jsm that link points to are all the important ones for your purposes, given these DXR queries:

https://dxr.mozilla.org/mozilla-central/search?q=%27.path+%3D+%27+target&redirect=false

https://dxr.mozilla.org/mozilla-central/search?q=%27.path+%3D+%27+path%3Adownload&redirect=false
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

2 years ago
Attachment #8871508 - Attachment description: CloudStorage back-end module → CloudStorage back-end using async destination select flow
(Assignee)

Comment 24

2 years ago
Created attachment 8873234 [details] [diff] [review]
WIP - Cloud Storage hook using observers

Hi Mike, 
Attaching WIP patch to hook cloud storage downloads using observer notification. I am updating bug 1365129 with the system add-on handling notifications. Thanks

Comment 25

2 years ago
The target path is immutable once the download is created. The WebExtensions team looked into how much work it would be to make this mutable in order to implement onDeterminingFilename, and eventually decided to take this off the list for Firefox 57 in bug 1245652.

This means the path will stay the same as what is set by callers of createDownload:

http://searchfox.org/mozilla-central/search?q=createDownload%5Cb&case=false&regexp=true&path=

The DownloadLegacy one that you already identified handles the main download flow.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 26

2 years ago
Thanks Gijs and Paolo for helping clarify. It will be good to know issues faced by WebExtensions team to make target.path mutable  and will ask that in bug 1245652.
(Assignee)

Comment 27

2 years ago
Created attachment 8874165 [details] [diff] [review]
WIP - Cloud Storage hook using observers

Patch updated and rebased
Attachment #8873234 - Attachment is obsolete: true
Comment on attachment 8874165 [details] [diff] [review]
WIP - Cloud Storage hook using observers

Review of attachment 8874165 [details] [diff] [review]:
-----------------------------------------------------------------

First pass, mostly design notes:

* minimize your public methods, that's what allows you to hide implementation details from callers.
* I think there's probably a getPreferredProvider method that includes key, display name, icon

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +354,5 @@
>            // Either the preference isn't set or the directory cannot be created.
>            directoryPath = await this.getSystemDownloadsDirectory();
>          }
>          break;
> +      case 3: // Cloud Storage

As discussed, this should call CloudStorage.getDownloadFolder directly instead of using the pref-cached value.  (See comments on getDownloadFolderPath)

@@ +364,5 @@
> +        } catch(ex) {
> +          // Either the preference isn't set or the directory cannot be created.
> +          directoryPath = await this.getSystemDownloadsDirectory();
> +        }
> +        break;

Nit: I know you're following existing flow here, but I hate the flow. :)

With the API change, it can be as simple as:

let directory = CloudStorage.getDownloadFolder();
if (directory) {
  return directory.path;
}
// fall through

::: toolkit/modules/CloudStorage.jsm
@@ +64,5 @@
> + */
> +
> +var ProvidersMetaData = {
> +    MAC_DROPBOX: {
> +      displayName: "DropBox", // Storage Service Name

Nit: Dropbox, not DropBox

@@ +139,5 @@
> +        default: "Downloads",
> +        photo: "Photos",
> +        screenshot: "Screenshots"
> +      },
> +    },

future TODO: make these values configurable from a JSON file.  That'll mean some standardized values instead of getters.

@@ +154,5 @@
> +    * programatically from ~/.dropbox/info.json
> +    */
> +  init() {
> +    if (AppConstants.platform == "macosx" || AppConstants.platform == "linux") {
> +      CloudStorageInternal.initPath("MAC_DROPBOX");

general: if you're going to use a const/name for both linux and mac, use UNIX.

* This code looks like it's hardcoding dropbox.  Probably not ideal.
* I'd do if == "win" else assume Unix, and catch more long tail platforms.  Matters less for Dropbox, will matter more when we enable more pluggability.

@@ +169,5 @@
> +   * object prefService with found provider key and value as provider metadata
> +   */
> +  scan() {
> +    return CloudStorageInternal.scan();
> +  },

Does this need to be public?  Or can we roll this into shouldPrompt() and return the key/display name on success?  We should only scan if we're going to prompt, and I don't think the consumer needs to know about it.

@@ +178,5 @@
> +   * if we should be scanning user desktop for providers.
> +   */
> +  hasAccepted() {
> +    return CloudStorageInternal.acceptedProviderKey ? true : false;
> +  },

I'd move this into shouldPrompt.

@@ +185,5 @@
> +   * return true if user is a candidate to prompt for default download
> +   * to provider download folder. Used to hook door hanagr prompt
> +   * inside download manager destination select flow.
> +   */
> +  isCandidate() {

I'd move this logic inside of shouldPrompt.

@@ +202,5 @@
> +   * returns true if time elapsed since last prompt shown has exceeded
> +   * maximum allowed interval in pref cloud.services.interval.prompt
> +   * and it's the first prompt of user session
> +   */
> +  shouldDisplayPrompt() {

nit: shorten to shouldPrompt

@@ +203,5 @@
> +   * maximum allowed interval in pref cloud.services.interval.prompt
> +   * and it's the first prompt of user session
> +   */
> +  shouldDisplayPrompt() {
> +    if (!CloudStorageInternal._promptInitialized) {

Why not keep this logic entirely in the internal method?

Also, promptInitialized seems to mean "promptShown" more than initialized.  I'm not sure per-session is the right interval here, why not rely always on the timestamp?

@@ +215,5 @@
> +   * After display prompt set pref cloud.services.lastprompt
> +   * with the last prompt shown time inside download manager
> +   * destination select flow.
> +   */
> +  setLastPromptTime(value) {

I'd collapse this and saveDownloadSettings into a single method that:

* records the user's choice, if appropriate.  If they dismiss the dialog, don't record a setting.
* sets the prompt time for use in shouldPrompt
* saves the candidate provider as rejected, so if a new provider becomes the candidate, we can re-prompt (think Android intent defaults, if new thing is available, maybe you want to use that)

savePromptResponse(key, selected = false);

@@ +253,5 @@
> +   * @return string with path to provider download folder
> +   */
> +  getDownloadFolderPath(key, dataType) {
> +    return CloudStorageInternal.getDownloadFolderPath(key, dataType);
> +  },

I think getDownloadFolderPath should be getDownloadFolder and return an nsIFile.  nsIFile.path works for the testing or pref case.

@@ +363,5 @@
> +
> +    let storage = this._providersMetaData[key];
> +    return storage ?
> +      OS.Path.join(storage.downloadPath, storage.typeSpecificData[dataType]) :
> +      null;

While we're here making changes, we can ensure the folder exists [file.exists()] and can be written to [file.isWritable()].  If either fail, we'll return null.  Also, on first check in a session, we should validate that the folder is still the correct one.

@@ +368,5 @@
> +  },
> +
> +  /**
> +   * Check if a user has multiple cloud storage providers on desktop, if yes
> +   * use dropbox as default

This is fine for now, but we should probably add some more nuance in the future around what's actively getting used.
(Assignee)

Comment 29

2 years ago
Comment on attachment 8874165 [details] [diff] [review]
WIP - Cloud Storage hook using observers

Review of attachment 8874165 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Mike for feedback , will update the patch. Will add getPreferredProvider which for now I believe can return key and download path metadata, retrieving UI specific locale strings and icon files from add-on.

::: toolkit/modules/CloudStorage.jsm
@@ +169,5 @@
> +   * object prefService with found provider key and value as provider metadata
> +   */
> +  scan() {
> +    return CloudStorageInternal.scan();
> +  },

Assumption was to keep scan and display prompt separate, but you are right its cleaner to only scan if we're going to prompt. If we do so, we might not need cloud.services.candidate.key pref.

@@ +203,5 @@
> +   * maximum allowed interval in pref cloud.services.interval.prompt
> +   * and it's the first prompt of user session
> +   */
> +  shouldDisplayPrompt() {
> +    if (!CloudStorageInternal._promptInitialized) {

yes, if we don't have display prompt once per session as requirement we can get rid of this logic.

@@ +215,5 @@
> +   * After display prompt set pref cloud.services.lastprompt
> +   * with the last prompt shown time inside download manager
> +   * destination select flow.
> +   */
> +  setLastPromptTime(value) {

+1, should  we save a provider as rejected only if user has checked 'always remember' checkbox?

@@ +363,5 @@
> +
> +    let storage = this._providersMetaData[key];
> +    return storage ?
> +      OS.Path.join(storage.downloadPath, storage.typeSpecificData[dataType]) :
> +      null;

+1
(Assignee)

Updated

2 years ago
Flags: needinfo?(mconnor)
(Assignee)

Updated

2 years ago
Attachment #8864579 - Flags: review?(mconnor)
(Assignee)

Updated

2 years ago
Attachment #8871508 - Flags: feedback?(mconnor)
(Assignee)

Comment 30

2 years ago
Created attachment 8877284 [details] [diff] [review]
WIP - Cloud Storage hook using observers

Hi Mike
Attaching patch updated with review feedback. Key changes incorporated in this patch are
- Loading providers metadata from metadata.json exposed using chrome URL by add-on
- Handle rejected providers by saving response in cloud.services.rejected.key

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c492467d607e7f1e5748a4b6233e4870dd9d53a

Thanks!
Attachment #8874165 - Attachment is obsolete: true
Attachment #8877284 - Flags: feedback?(mconnor)
(Assignee)

Comment 31

2 years ago
Created attachment 8879825 [details] [diff] [review]
Cloud Storage hook using observers

Patch updated with unit tests. Thanks
Attachment #8877284 - Attachment is obsolete: true
Attachment #8877284 - Flags: feedback?(mconnor)
Attachment #8879825 - Flags: feedback?(mconnor)
Comment on attachment 8879825 [details] [diff] [review]
Cloud Storage hook using observers

Review of attachment 8879825 [details] [diff] [review]:
-----------------------------------------------------------------

Second pass.  Haven't looked in depth at everything, but there's some more tweaks to structure needed to really split config and code.  Definitely on the right path.

::: toolkit/modules/CloudStorage.jsm
@@ +33,5 @@
> +  "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +  "resource://gre/modules/Console.jsm");
> +
> +

nit: multiple newlines?

@@ +76,5 @@
> + */
> +
> +this.CloudStorage = Object.freeze({
> +  /**
> +    * Init method invoked from add-on startup method.

because of how we're using this now, we can't rely on the add-on to call init.  This should happen when the module is invoked (i.e. through the lazy getter in DownloadIntegration.jsm).  And unless there's a reason for enabling external init, you could make this an internal call.

@@ +138,5 @@
> +    } else if (remember && !selected) {
> +      // Store provider as rejected and not use in re-prompt
> +      // set cloud.services.rejected.key and use that in handleMultipleProviders
> +      CloudStorageInternal.handleRejected(key);
> +    }

nit: both cases have if (remember), so I'd nest the two cases inside an if (remember) block.

@@ +263,5 @@
> +    request.send();
> +    let metadata = await deferred.promise;
> +
> +    return this._parseJSON(metadata);
> +  },

You've got two different implementations of loading JSON from a local path.  I'd suggest using something like this as a helper, and using it both here and in the Dropbox case

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm#166

@@ +368,5 @@
> +        }
> +
> +        var data = NetUtil.readInputStreamToString(inputStream, inputStream.available(),
> +                                                   { charset: "UTF-8" });
> +        let info = JSON.parse(data).personal;

see other comment about a JSON loader function.

I think this can be generalized to use metadata that describes the structure (i.e. path => personal.path )

@@ +470,5 @@
> +        if (key === "UNIX_DROPBOX" || key === "WINNT_DROPBOX") {
> +          prefService.key = key;
> +          prefService.value = value;
> +        }
> +      });

You shouldn't need this code anymore, see my comments on the metadata.json form.  The metadata.json format should specify in preferred order, and code should just follow that ordering.

@@ +700,5 @@
> +  /**
> +   * sets lastPrompt time to allow sufficient time before initiating re-prompt
> +   */
> +  setLastPromptTime(value) {
> +    Services.prefs.setIntPref(CLOUD_SERVICES_PREF + "lastprompt", value);

We may want to sanity check the value here.

::: toolkit/modules/tests/xpcshell/cloud/metadata.json
@@ +17,5 @@
> +      "default": "Downloads",
> +      "photo": "Photos",
> +      "screenshot": "Screenshots"
> +    }
> +  },

Dropbox, not DropBox ;)

Is this test-only code?  If not, it shouldn't live with tests!

This is a good start, but doesn't quite go far enough.  All per-provider details should be expressed within this format, with no code-level special handling.  That basically would end up looking more like this.  Let me know if you want more detail/help on what I'm thinking.


{
  available_providers: {
    "mac": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
    "linux": ["UNIX_DROPBOX"], // no gdrive on Linux as far as I can tell
    "windows": ["WINNT_DROPBOX", "WINNT_GDRIVE"]
  },
  providers: { // flat list of provider definitions

  }
}

We probably want a geo component as well.  So available_providers would have a default set as above, but we might want to recommend/include different services based on region (i.e. Yandex.Drive in Russia).
Attachment #8879825 - Flags: feedback?(mconnor) → feedback+
(Assignee)

Comment 34

2 years ago
> @@ +76,5 @@
> > + */
> > +
> > +this.CloudStorage = Object.freeze({
> > +  /**
> > +    * Init method invoked from add-on startup method.
> 
> because of how we're using this now, we can't rely on the add-on to call
> init.  This should happen when the module is invoked (i.e. through the lazy
> getter in DownloadIntegration.jsm).  And unless there's a reason for
> enabling external init, you could make this an internal call.

As discussed, Initializing using lazy getter means bringing metadata.json content with the API. Will move the API to toolkit/components as we need resource URL for metadata.json

> @@ +138,5 @@
> > +    } else if (remember && !selected) {
> > +      // Store provider as rejected and not use in re-prompt
> > +      // set cloud.services.rejected.key and use that in handleMultipleProviders
> > +      CloudStorageInternal.handleRejected(key);
> > +    }
> 
> nit: both cases have if (remember), so I'd nest the two cases inside an if
> (remember) block.
> 
Done


> 
> @@ +700,5 @@
> > +  /**
> > +   * sets lastPrompt time to allow sufficient time before initiating re-prompt
> > +   */
> > +  setLastPromptTime(value) {
> > +    Services.prefs.setIntPref(CLOUD_SERVICES_PREF + "lastprompt", value);
> 
> We may want to sanity check the value here.
> 
Done

> Is this test-only code?  If not, it shouldn't live with tests!
> 
> This is a good start, but doesn't quite go far enough.  All per-provider
> details should be expressed within this format, with no code-level special
> handling.  That basically would end up looking more like this.  Let me know
> if you want more detail/help on what I'm thinking.
> 
> 
> {
>   available_providers: {
>     "mac": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
>     "linux": ["UNIX_DROPBOX"], // no gdrive on Linux as far as I can tell
>     "windows": ["WINNT_DROPBOX", "WINNT_GDRIVE"]
>   },
>   providers: { // flat list of provider definitions
> 
>   }
> }
> 
> We probably want a geo component as well.  So available_providers would have
> a default set as above, but we might want to recommend/include different
> services based on region (i.e. Yandex.Drive in Russia).

As discussed json format that handles geo-component will look something like
{
  available_providers: {
    "default": {
    "mac": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
    "linux": ["UNIX_DROPBOX"], // no gdrive on Linux as far as I can tell
    "windows": ["WINNT_DROPBOX", "WINNT_GDRIVE"]
    },
    "RU":  {
      "mac": ["UNIX_YANDEX", "UNIX_DROPBOX", "UNIX_GDRIVE"],
      "linux": ["UNIX_DROPBOX"], // no gdrive on Linux as far as I can tell
      "windows": ["WINNT_YANDEX", "WINNT_DROPBOX", "WINNT_GDRIVE"]
    }
  },
  providers: { // flat list of provider definitions

  }
}

will update API with feedback to handle available_providers for a platform using json
I definitely use GDrive on Linux via nautilus, see for example: https://www.linuxtechi.com/access-google-drive-in-ubuntu-16-04/
(Assignee)

Comment 36

2 years ago
(In reply to Panos Astithas [:past] (please needinfo?) from comment #35)
> I definitely use GDrive on Linux via nautilus, see for example:
> https://www.linuxtechi.com/access-google-drive-in-ubuntu-16-04/

Thanks Panos, I tried nautilus and it works.  The first iteration of Cloud Storage API relies on main application/ desktop client from provider. 

We should definitely evaluate how to easily extend API and support helper applications on platform where we don’t have provider main app support.
(Assignee)

Comment 37

2 years ago
Created attachment 8884596 [details] [diff] [review]
Cloud Storage hook using observers

Attaching patch updated with feedback

- Metadata in providers.json
- init method invoked via internal call
- refactored loading json
- providers json updated with available providers by platform and locale

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a384ef2fb74aeae56d35ca6cd4d56856273bc2
Attachment #8879825 - Attachment is obsolete: true
Attachment #8884596 - Flags: review?(mconnor)
Comment hidden (mozreview-request)
(Assignee)

Comment 39

2 years ago
Hi Gijs

Submitting updated CloudStorage API patch for your review. Mike has taken a first pass at it and has verbal go
ahead to submit review request to you.

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28b6b5dcff8093e4a78d9f75bc2d97cf28143fe3

Thanks!

Comment 40

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review163656

OK, I'm sorry for the delay, but I've tried to do a review. Some general points:

- anywhere you do: `method() { return new Promise()... }` you really want to use an async method instead.
- likewise, almost anywhere you're currently using `.then`, you probably want to use `await`.
- There are a lot of single-line helper methods that have only 1 or 2 callsites. There's no need. They make the patch a lot longer and harder to understand.
- If you need a getter for a pref (which I'm not sure the patch really does), defining a lazy getter using XPCOMUtils is a better way of doing things than calling getCharPref or whatever every time the getter is called.

Otherwise, I am confused about how to test this patch and where the notification goes. It also looks, without being able to test this, like the notification would only fire if the user isn't using site-specific download prefs (that is, I would have expected a notification from the site-specific download code, too, but there doesn't seem to be one).

Finally, I think this patch looks like it's trying to do the right thing in not initializing this module until something is downloaded. However, I'm not sure if this was deliberate and/or how this works with the notification (for which, if the module was to be an observer, it might not have been initialized yet).

::: commit-message-fd8a7:6
(Diff revision 2)
> +Bug 1357171 - cloud storage module r=gijs r=mconnor
> +* Has storage providers metadata
> +* Scan for storage providers returning preferred provider
> +* Helper methods to access cloud services prefs
> +* Helper method to set cloud storage as default download directory
> +* Notify observers for displaying cloud storage prompt

But nothing listens to this. Is this in a separate patch/bug? Where?

::: toolkit/components/cloudstorage/CloudStorage.jsm:19
(Diff revision 2)
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

Destructuring assignment please.

::: toolkit/components/cloudstorage/CloudStorage.jsm:31
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +                                  "resource://gre/modules/Console.jsm");

Don't do this, use either Log.jsm if you want proper logging, or Cu.reportError for actual unexpected error conditions.

::: toolkit/components/cloudstorage/CloudStorage.jsm:33
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "ServiceRequest",
> +                                  "resource://gre/modules/ServiceRequest.jsm");

Shouldn't need this.

::: toolkit/components/cloudstorage/CloudStorage.jsm:101
(Diff revision 2)
> +  /**
> +    * Init method invoked from add-on startup method.
> +    *
> +    */
> +  init() {
> +    return new Promise(resolve => {

Use `async` for functions that return a promise.

::: toolkit/components/cloudstorage/CloudStorage.jsm:104
(Diff revision 2)
> +    */
> +  init() {
> +    return new Promise(resolve => {
> +      // Invoke internal method asynchrnously to
> +      // read and parse providers metadata from JSON
> +      CloudStorageInternal.initProviders().then(data => {

Then use `await` in them rather than chained callback-hell `.then()` calls.

::: toolkit/components/cloudstorage/CloudStorage.jsm:152
(Diff revision 2)
> +   * @param selected
> +   *        bool value by default set to false indicating if user has selected
> +   *        to save downloaded file with cloud provider
> +   */
> +  savePromptResponse(key, remember, selected = false) {
> +    CloudStorageInternal.setLastPromptTime(Math.floor(Date.now() / 1000));

This always uses `Date.now()` - why does it need to be a parameter (in fact, why does it need to be a separate function at all?).

::: toolkit/components/cloudstorage/CloudStorage.jsm:224
(Diff revision 2)
> +  /**
> +   * Gets the currently selected locale for display.
> +   * @return the selected locale
> +   */
> +  get locale() {
> +    return Services.locale.getRequestedLocale() || "default";

Just use this as a local variable in the one function where you use this.

::: toolkit/components/cloudstorage/CloudStorage.jsm:228
(Diff revision 2)
> +  get locale() {
> +    return Services.locale.getRequestedLocale() || "default";
> +  },
> +
> +  get platform() {
> +    return AppConstants.platform || null;

Just use AppConstants.platform directly where you currently use this. You don't need a fallback either.

::: toolkit/components/cloudstorage/CloudStorage.jsm:232
(Diff revision 2)
> +  get platform() {
> +    return AppConstants.platform || null;
> +  },
> +
> +  _downloadJSON(uri) {
> +    return new Promise((resolve, reject) => {

Instead, define `_downloadJSON` as async and use the fetch() API. You can make it available in JSMs by using `Cu.importGlobalProperties`.

::: toolkit/components/cloudstorage/CloudStorage.jsm:540
(Diff revision 2)
> +  _checkIfAssetExist(path) {
> +    return new Promise((resolve, reject) => {
> +      OS.File.exists(path).then((exists) => {
> +        resolve(exists);
> +      }).catch(e => { Cu.reportError("_checkIfAssetExist: Error while looking for asset"); Cu.reportError(e); });
> +    });
> +  },

Why doesn't this just:

```js
return OS.File.exists(path);
```

It's not clear why you need the catch here, and besides, in what cases does this throw / reject?

If we do really need to log something here, it should include the path that we were trying to reach.

::: toolkit/components/cloudstorage/CloudStorage.jsm:556
(Diff revision 2)
> +  getStorageProviders() {
> +    return new Promise((resolve, reject) => {
> +

Again, please use async methods.

::: toolkit/components/cloudstorage/CloudStorage.jsm:568
(Diff revision 2)
> +      let arrPromises =
> +      Object.getOwnPropertyNames(this.providersMetaData).map(prop => {

The indenting here is wrong, plus it looks like this would be easier if written as:

```
let promises = Object.values(this.providersMetaData)
                     .map(metadata => this._checkIfAssetExists(metadata.discoveryPath));
```

Again, not sure why you need extra logging here.

::: toolkit/components/cloudstorage/CloudStorage.jsm:582
(Diff revision 2)
> +          }
> +        );
> +      });
> +
> +
> +      Promise.all(arrPromises).then(results => {

await

::: toolkit/components/cloudstorage/CloudStorage.jsm:595
(Diff revision 2)
> +            let key = storageKeys[idx];
> +            result.set(key, this.providersMetaData[key]);
> +          }
> +        });
> +        resolve(result);
> +      });

So... all of this, as far as I can tell, is to convert a JS object into a Map where the keys are the property names of the JS object, and the values are the same as they were before, and to filter out the ones that don't exist, right?

Here's my suggested implementation:

```js
let providers = Object.entries(this.providersMetaData || {});
let promises = providers.map(([, provider]) => this._checkIfAssetExists(provider));
let results = await Promise.all(providers);
providers = providers.filter((_, idx) => results[idx]);
return new Map(providers);
```

::: toolkit/components/cloudstorage/CloudStorage.jsm:614
(Diff revision 2)
> +    if (!rejected) {
> +      this.setCloudStorageRejectedPref(key);
> +    } else {
> +      // Pref exists with previous rejected keys, append
> +      // key at the end and update pref
> +      let arr = rejected.split(",");

Nit: better name than 'arr'. Name lists/objects after what they contain, not just "obj" or "list" or "ary".

::: toolkit/components/cloudstorage/CloudStorage.jsm:615
(Diff revision 2)
> +      this.setCloudStorageRejectedPref(key);
> +    } else {
> +      // Pref exists with previous rejected keys, append
> +      // key at the end and update pref
> +      let arr = rejected.split(",");
> +      let length = arr ? arr.push(key) : 0;

There's no way `arr` can be falsy here.

::: toolkit/components/cloudstorage/CloudStorage.jsm:639
(Diff revision 2)
> +   *     3 - The default download location is elsewhere as specified by
> +   *         cloud storage API getDownloadFolder

This will also require updating UI in about:preferences and potentially other places so it works correctly if the value is 3.

::: toolkit/components/cloudstorage/CloudStorage.jsm:703
(Diff revision 2)
> +   *
> +   * @param key
> +   *        cloud storage provider key from provider metadata
> +   */
> +  setCloudStorageRejectedPref(key) {
> +    // Pass null to reset cloud storage

Nobody does this, so I don't think this helper is useful.

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:16
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

No point fetching this lazily.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:363
(Diff revision 2)
>          }
>          break;
> +      case 3: // Cloud Storage
> +        let directory = await CloudStorage.getDownloadFolder();
> +        if (directory) {
> +          directoryPath = directory.path;

Just return directory.path here and then fall through to get the default. Mike already pointed this out in an earlier review...

::: toolkit/mozapps/downloads/nsHelperAppDlg.js:272
(Diff revision 2)
>              // prompt the user for a different target file.
>            }
>  
>            // Check to make sure we have a valid directory, otherwise, prompt
>            if (result) {
> +            Services.obs.notifyObservers(null, "cloudstorage-prompt-notification", result.path);

Nothing seems to be observing this. Why do we need it? Also, what is a 'prompt-notification', given that this is the path taken when we *don't* prompt, but have a path already.
Attachment #8864579 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review164280

::: commit-message-fd8a7:6
(Diff revision 2)
> +Bug 1357171 - cloud storage module r=gijs r=mconnor
> +* Has storage providers metadata
> +* Scan for storage providers returning preferred provider
> +* Helper methods to access cloud services prefs
> +* Helper method to set cloud storage as default download directory
> +* Notify observers for displaying cloud storage prompt

I should mention it in comments, that’s correct it’s is a separate bug - Bug 1365129 (See patch - WIP CloudStorage add-on using observer) implementing DownloadManager as consumer for CloudStorage API. It’s still a WIP and plan is to launch this add-on as a shield study.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js:272
(Diff revision 2)
>              // prompt the user for a different target file.
>            }
>  
>            // Check to make sure we have a valid directory, otherwise, prompt
>            if (result) {
> +            Services.obs.notifyObservers(null, "cloudstorage-prompt-notification", result.path);

It’s Bug 1365129 (See comments above) patch is listening to these notifications. While downloading these acts as hook to show offers prompt such as door hanger prompt to save downloaded file to dropbox download folder. I will update comment here to clarify better.
(Assignee)

Comment 42

2 years ago
mozreview-review-reply
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review163656

> This always uses `Date.now()` - why does it need to be a parameter (in fact, why does it need to be a separate function at all?).

+1 , updated to set lastprompt pref directly

> Why doesn't this just:
> 
> ```js
> return OS.File.exists(path);
> ```
> 
> It's not clear why you need the catch here, and besides, in what cases does this throw / reject?
> 
> If we do really need to log something here, it should include the path that we were trying to reach.

Its capturing edge cases where file existence check fails, for example access is denied, updated log to include path

> So... all of this, as far as I can tell, is to convert a JS object into a Map where the keys are the property names of the JS object, and the values are the same as they were before, and to filter out the ones that don't exist, right?
> 
> Here's my suggested implementation:
> 
> ```js
> let providers = Object.entries(this.providersMetaData || {});
> let promises = providers.map(([, provider]) => this._checkIfAssetExists(provider));
> let results = await Promise.all(providers);
> providers = providers.filter((_, idx) => results[idx]);
> return new Map(providers);
> ```

+1 this simplify it a lot, thanks!

> This will also require updating UI in about:preferences and potentially other places so it works correctly if the value is 3.

Opened bug 1381066 to address about:preferences

> Nobody does this, so I don't think this helper is useful.

It's used in test_cloudstorage.js - inside checkSavedPromptResponse to reset pref and cached _prefStorageKey property
Comment hidden (mozreview-request)
(Assignee)

Comment 44

2 years ago
Hi Gijs
I have updated patch with review feedback. You should be able to test using the steps in Bug 1365129.
Only place patch triggers notification for CloudStorage API consumers is nsHelperAppDialog.js.
I will look in to site-specific download code and ensure that it works as expected  and update bug with findings.

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9796d1168aa86050a162b2d34f8f9c7ccd308fd5

Thanks!

Comment 45

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review165148

::: toolkit/components/cloudstorage/CloudStorage.jsm:22
(Diff revision 3)
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://gre/modules/Log.jsm");

Please sort these alphabetically.

::: toolkit/components/cloudstorage/CloudStorage.jsm:33
(Diff revision 3)
> +  log.level = Log.Level.Debug;
> +  let appender = new Log.DumpAppender();

We can't do this in production code and spam stderr/stdout output for everyone. See e.g. this code for how to use a pref for this, to configure both the log level and whether it outputs to stderr/stdout in addition to the browser console.

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryController.jsm#106-110

::: toolkit/components/cloudstorage/CloudStorage.jsm:108
(Diff revision 3)
> +      let data = await CloudStorageInternal.initProviders();
> +      CloudStorageInternal.providersMetaData = data;

Nobody else calls this so initProviders can just assign the property on the internal object itself.

::: toolkit/components/cloudstorage/CloudStorage.jsm:111
(Diff revision 3)
> +      // init download path while checking for custom download paths.
> +      // For Dropbox, sets provider metadata with download path read
> +      // programatically from ~/.dropbox/info.json
> +      result =  CloudStorageInternal.initDownloadPath();

It seems like it would be simpler to call this from inside initProviders(), and to use `await` directly rather than returning the promise.

::: toolkit/components/cloudstorage/CloudStorage.jsm:116
(Diff revision 3)
> +      // init download path while checking for custom download paths.
> +      // For Dropbox, sets provider metadata with download path read
> +      // programatically from ~/.dropbox/info.json
> +      result =  CloudStorageInternal.initDownloadPath();
> +    } catch (err) {
> +      Cu.reportError("init: Error while initializing resources"); Cu.reportError(err);

One statement per line. Also, this probably wants to use log.error().

::: toolkit/components/cloudstorage/CloudStorage.jsm:118
(Diff revision 3)
> +      // programatically from ~/.dropbox/info.json
> +      result =  CloudStorageInternal.initDownloadPath();
> +    } catch (err) {
> +      Cu.reportError("init: Error while initializing resources"); Cu.reportError(err);
> +    }
> +    return result;

AFAICT initDownloadPath always returns null, so this always returns null as the result of the promise. I don't really understand why we're returning anything, but it should at least not be null.

::: toolkit/components/cloudstorage/CloudStorage.jsm:131
(Diff revision 3)
> +  shouldPrompt() {
> +    if (!CloudStorageInternal.acceptedProviderKey &&
> +        CloudStorageInternal.shouldPrompt()) {
> +      return CloudStorageInternal.scan();

Either all the logic should live in the internal object, or all of it in this one, not half/half.

Also, 'shouldPrompt' sounds like it returns a boolean, but this returns an object with data that the caller will presumably use for the prompt. I would rename to 'promisePromptInfo()' or something.

::: toolkit/components/cloudstorage/CloudStorage.jsm:372
(Diff revision 3)
> +          break;
> +        default:
> +          Promise.resolve();
> +          break;
> +      }
> +      return result;

result is always null here, and the Promise.resolve() in the default case doesn't go anywhere... so the documentation here is wrong and the code doesn't make a lot of sense as-is.

::: toolkit/components/cloudstorage/CloudStorage.jsm:381
(Diff revision 3)
> +
> +  /**
> +   * Read Dropbox info.json and override providers metadata
> +   * downloadPath property
> +   */
> +  async _initDropbox(key) {

There are several issues with this:

1) there's some tight coupling here between the dropbox info in the json file, the hardcoded keys, and this method. I don't have a great solution to this, so I guess we can keep it for now...
2) We're currently always opening download paths and checking their existence and so on, when the component initializes, for dropbox, even if it might have been rejected already. We also don't seem to save any status, so we will just keep doing this on subsequent runs, even if we detect dropbox is not available. It doesn't seem right to always do this when we get a download. If we reduce the number of init() calls, at least we'll do this once per Firefox instance, but that still seems relatively frequent. If the last scan was less than a day (maybe a week?) ago, I'm not sure it's useful to keep re-scanning and/or keep checking for dropbox stuff...

::: toolkit/components/cloudstorage/CloudStorage.jsm:412
(Diff revision 3)
> +      } catch (ex) {
> +        log.debug("_initDropbox: Error accessing dropbox download path for " + key);
> +        return;
> +      }
> +      // Update metadata with validated download path
> +      this.providersMetaData[key].downloadPath = info.path;

Why do we need to do this for dropbox, and if this is necessary for dropbox, is it not also necessary for Google Drive?

Also, we should ensure somehow that this code has actually run and finished before using the data, which right now you don't do.

::: toolkit/components/cloudstorage/CloudStorage.jsm:424
(Diff revision 3)
> +    return directory.exists() && directory.isDirectory() &&
> +           directory.isWritable();

All of these are main-thread IO. The callsite is already an async function, so please use OS.File for this, too.

::: toolkit/components/cloudstorage/CloudStorage.jsm:438
(Diff revision 3)
> +    if (!this.providersMetaData) {
> +      let isInitialized = await CloudStorage.init();

CloudStorage.init() should probably store a local promise somewhere, and then you can await() that promise in any public async methods to ensure you wait for initialization, without initializing more than once.

::: toolkit/components/cloudstorage/CloudStorage.jsm:441
(Diff revision 3)
> +  async getDownloadFolder(dataType) {
> +    // Wait for cloudstorage to initialize if providers metadata is not available
> +    if (!this.providersMetaData) {
> +      let isInitialized = await CloudStorage.init();
> +      if (!isInitialized && !this.providersMetaData) {
> +        Cu.reportError("CloudStorage: Failed to initialize and retrieve download folder ");

I guess this should also use log.warn() or something?

::: toolkit/components/cloudstorage/CloudStorage.jsm:475
(Diff revision 3)
> +   *
> +   * @param providers
> +   *        Map with local providers found on user desktop
> +   * @return prefService object with key and metadata value of selected storage
> +   */
> +  handleMultipleProviders(providers) {

There is only 1 call-site for this, too - can we just make it part of scan()? Then we can also remove the rejected keys before checking if there are any providers left, which simplifies the rest of the code.

::: toolkit/components/cloudstorage/CloudStorage.jsm:481
(Diff revision 3)
> +    if (arrRejected && arrRejected.length > 0) {
> +      providers.forEach((value, key) => {
> +        if (arrRejected.includes(key)) {
> +          providers.delete(key);
> +        }
> +      });
> +    }

Items that have been rejected are almost certain to be in the new map we have here (otherwise, how were they rejected before?), and besides map.delete() is perfectly fine with calling delete() with keys that don't exist.

So a simpler version here would be:

```
for (let rejectedKey of rejectedKeys) {
  providers.delete(rejectedKey);
}
```

::: toolkit/components/cloudstorage/CloudStorage.jsm:519
(Diff revision 3)
> +  /**
> +   * Scans for local storage providers available on user desktop
> +   *
> +   * @return {Promise}
> +   * @resolves
> +   * object prefService with found provider key and value as provider metadata

The term "prefService" is confusing. It normally refers to Services.prefs. Maybe "providerInfo" would make more sense?

::: toolkit/components/cloudstorage/CloudStorage.jsm:523
(Diff revision 3)
> +   * @resolves
> +   * object prefService with found provider key and value as provider metadata
> +   * or null if no valid provider found
> +   */
> +  async scan() {
> +    let result = await this.getStorageProviders();

Nit: use a topical variable name instead of 'result'

::: toolkit/components/cloudstorage/CloudStorage.jsm:524
(Diff revision 3)
> +   * object prefService with found provider key and value as provider metadata
> +   * or null if no valid provider found
> +   */
> +  async scan() {
> +    let result = await this.getStorageProviders();
> +    if ( result.size < 1 ) {

Nit: `if (!result.size)`


No spaces around if condition conditionals, please. eslint should notice this. Please make sure you run it - easiest is to configure the commit hook, see https://www.oxymoronical.com/blog/2015/12/Running-ESLint-on-commit )

::: toolkit/components/cloudstorage/CloudStorage.jsm:542
(Diff revision 3)
> +   * boolean value of file existence check
> +   */
> +  _checkIfAssetExists(path) {
> +    let exist;
> +    try {
> +      exist = OS.File.exists(path);

Just replace the body of this method with:


```
return OS.File.exists(path).catch(err => {
  log.warn(`Couldn't check existance of ${path}`, err);
  return false;
});
```

::: toolkit/components/cloudstorage/CloudStorage.jsm:655
(Diff revision 3)
> +   */
> +  setCloudStoragePref(key) {
> +    // Pass null to reset cloud storage
> +    if (!key) {
> +      Services.prefs.clearUserPref(CLOUD_SERVICES_PREF + "storage.key");
> +      this._prefStorageKey = null;

Looks like this can just be another lazy pref getter, and then you can get rid of acceptedProviderKey.

::: toolkit/components/cloudstorage/CloudStorage.jsm:675
(Diff revision 3)
> +CloudStorage.init().then(success => {
> +  if (success) {
> +    log.debug("CloudStorage: successfully initialized");
> +  }
> +});

This is not the right way to solve my comment in the previous changeset. You need a consistent plan for how this component gets initialized. Right now you call init() both here and in getDownloadFolder, but you don't actually check against re-entrancy.

::: toolkit/components/cloudstorage/content/providers.json:9
(Diff revision 3)
> +      "macosx": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
> +      "linux": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
> +      "win": ["WINNT_DROPBOX", "WINNT_GDRIVE"]
> +    },
> +    "RU": {
> +      "macosx": ["UNIX_YANDEX", "UNIX_DROPBOX", "UNIX_GDRIVE"],

Yandex isn't in this file though, so this doesn't look like it works / was tested.

::: toolkit/components/cloudstorage/content/providers.json:21
(Diff revision 3)
> +        "default": "Downloads",
> +        "photo": "Photos",
> +        "screenshot": "Screenshots"

This seems to be duplicated for all the providers. Do they all actually support this?

More generally, the names of these providers are also all the same. Can we just put the path stuff in platform-specfic properties, but not the other things? That would simplify the code a lot. In fact, the `_downloadPath` also seems to be identical across OSes.

::: toolkit/components/cloudstorage/moz.build:18
(Diff revision 3)
> +EXTRA_JS_MODULES += [
> +    'CloudStorage.jsm',
> +]
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Toolkit', 'cloudstorage')

This component doesn't exist...

::: toolkit/mozapps/downloads/nsHelperAppDlg.js:272
(Diff revision 3)
> +            // Notifications for CloudStorage API consumers to show offer
> +            // prompts while downloading. See Bug 1365129
> +            Services.obs.notifyObservers(null, "cloudstorage-prompt-notification", result.path);

If we're not using it here, it shouldn't be in this patch.
Attachment #8864579 - Flags: review?(gijskruitbosch+bugs)

Comment 46

2 years ago
mozreview-review-reply
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review163656

> Opened bug 1381066 to address about:preferences

Note that I don't really think this is landable/shippable without updating the preferences. Even just a minimal patch that adds a "Cloud Storage" entry to the dropdown that we show in this section would be helpful to ensure that the prefs don't get into an inconsistent state, where I *think* just opening the preferences will risk resetting the preference (because the list of options does not include the new value, so it will use something else, which will then get saved depending on how the preference XBL bindings determine changes - maybe you'd need to try to touch the dropdown, or another pref, maybe just opening the prefs would be enough, I'm not sure).
(Assignee)

Comment 47

2 years ago
(In reply to :Gijs from comment #46)
> Comment on attachment 8864579 [details]
> Bug 1357171 - cloud storage module
> 
> https://reviewboard.mozilla.org/r/136256/#review163656
> 
> > Opened bug 1381066 to address about:preferences
> 
> Note that I don't really think this is landable/shippable without updating
> the preferences. Even just a minimal patch that adds a "Cloud Storage" entry
> to the dropdown that we show in this section would be helpful to ensure that
> the prefs don't get into an inconsistent state, where I *think* just opening
> the preferences will risk resetting the preference (because the list of
> options does not include the new value, so it will use something else, which
> will then get saved depending on how the preference XBL bindings determine
> changes - maybe you'd need to try to touch the dropdown, or another pref,
> maybe just opening the prefs would be enough, I'm not sure).

My understanding is back-end CloudStorage API in itself is a self-contained patch and stay docile until it’s initialized by consumer add-on for example 1365129.  As for the concern about with patch landed changing the folderList pref value to 3, I tested and the behavior stays as it’s now that is downloaded files saves to default system download folder and preference UI sets download folder to desktop. please let me know if there is any other concern about preference UI that I am missing here. Thanks

Comment 48

2 years ago
(In reply to Punam Dahiya [:pdahiya] from comment #47)
> (In reply to :Gijs from comment #46)
> > Comment on attachment 8864579 [details]
> > Bug 1357171 - cloud storage module
> > 
> > https://reviewboard.mozilla.org/r/136256/#review163656
> > 
> > > Opened bug 1381066 to address about:preferences
> > 
> > Note that I don't really think this is landable/shippable without updating
> > the preferences. Even just a minimal patch that adds a "Cloud Storage" entry
> > to the dropdown that we show in this section would be helpful to ensure that
> > the prefs don't get into an inconsistent state, where I *think* just opening
> > the preferences will risk resetting the preference (because the list of
> > options does not include the new value, so it will use something else, which
> > will then get saved depending on how the preference XBL bindings determine
> > changes - maybe you'd need to try to touch the dropdown, or another pref,
> > maybe just opening the prefs would be enough, I'm not sure).
> 
> My understanding is back-end CloudStorage API in itself is a self-contained
> patch and stay docile until it’s initialized by consumer add-on for example
> 1365129.

Yes, but we can't really fix the prefs UI from the add-on very well.

>  As for the concern about with patch landed changing the folderList
> pref value to 3, I tested and the behavior stays as it’s now that is
> downloaded files saves to default system download folder and preference UI
> sets download folder to desktop.

So the behaviour and what the preferences show the user are out of sync? That doesn't seem very good.

Also, I'm a bit confused, because shouldn't this patch itself be enough to make things save to the dropbox/gdrive folder, when the pref is set to 3?

> please let me know if there is any other
> concern about preference UI that I am missing here. Thanks

Well, I expect in some cases, the prefs UI will just save the int value matching desktop (maybe 0? Not sure off-hand) back to prefs and lose state. That seems bad. This is also really simple to address by just adding one more item to the relevant menulist, with 1 or 2 strings in the relevant dtd file. It would take slightly more work if we wanted to hide that option unless it was the actual value of the pref (ie not expose "Cloud Storage" as a value in there unless that was the value of the pref), but even that would be a few lines of JS at most.
(Assignee)

Comment 49

2 years ago
(In reply to :Gijs from comment #48)
> (In reply to Punam Dahiya [:pdahiya] from comment #47)
> > (In reply to :Gijs from comment #46)
> > > Comment on attachment 8864579 [details]
> > > Bug 1357171 - cloud storage module
> > > 
> > > https://reviewboard.mozilla.org/r/136256/#review163656
> > > 
> > > > Opened bug 1381066 to address about:preferences
> > > 
> > > Note that I don't really think this is landable/shippable without updating
> > > the preferences. Even just a minimal patch that adds a "Cloud Storage" entry
> > > to the dropdown that we show in this section would be helpful to ensure that
> > > the prefs don't get into an inconsistent state, where I *think* just opening
> > > the preferences will risk resetting the preference (because the list of
> > > options does not include the new value, so it will use something else, which
> > > will then get saved depending on how the preference XBL bindings determine
> > > changes - maybe you'd need to try to touch the dropdown, or another pref,
> > > maybe just opening the prefs would be enough, I'm not sure).
> > 
> > My understanding is back-end CloudStorage API in itself is a self-contained
> > patch and stay docile until it’s initialized by consumer add-on for example
> > 1365129.
> 
> Yes, but we can't really fix the prefs UI from the add-on very well.
> 
 
I agree bug 1381066 is blocker for add-on Bug 1365129

> >  As for the concern about with patch landed changing the folderList
> > pref value to 3, I tested and the behavior stays as it’s now that is
> > downloaded files saves to default system download folder and preference UI
> > sets download folder to desktop.
> 
> So the behaviour and what the preferences show the user are out of sync?
> That doesn't seem very good.
> 

Yes, you can replicate in nightly by setting browser.download.folderlist to any value other than 1 and 2. I think that's why we have the big blue button on about:config ‘I accept the risk!’ before manually touching the prefs :)

> Also, I'm a bit confused, because shouldn't this patch itself be enough to
> make things save to the dropbox/gdrive folder, when the pref is set to 3?
>

You need browser.download.folderList to 3 and pref cloud.services.storage.key that’s set to preferred provider key for example “UNIX_DROPBOX”. 

Without Add-on,  by itself API will not be initialized till browser.download.folderList is set to 3( see DownloadIntegration.jsm - getPreferredDownloadsDirectory in patch). getDownloadFolder method in CloudStorage API will return null until it finds a valid provider key in pref cloud.services.storage.key 


> > please let me know if there is any other
> > concern about preference UI that I am missing here. Thanks
> 
> Well, I expect in some cases, the prefs UI will just save the int value
> matching desktop (maybe 0? Not sure off-hand) back to prefs and lose state.
> That seems bad. This is also really simple to address by just adding one
> more item to the relevant menulist, with 1 or 2 strings in the relevant dtd
> file. It would take slightly more work if we wanted to hide that option
> unless it was the actual value of the pref (ie not expose "Cloud Storage" as
> a value in there unless that was the value of the pref), but even that would
> be a few lines of JS at most.


what I have found so far, from pref UI the only values (for browser.download.folderList)  that can be set are 1 and 2. 1 for system default download, 2 for any other folder location selected by user by clicking button 'choose'. With back-end API in this patch user cannot set browser.download.folderList to 3 from UI and this new value is muted. We should definitely address it with bug 1381066 before any UI facing changes.


On site specific download code (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadLastDir.jsm), my findings are when user selects ‘Always ask you where to select files’,  it sets browser.download.useDownloadDir to false. User is prompted before downloading and the directory path selected get saved in pref - browser.download.lastDir.  

With this patch, users opted into site specific download code will not be impacted. If they manually choose option Save files to <folder chosen>, that sets browser.download.useDownloadDir to true and uses browser.download.folderlist for download paths. If user goes back to ‘Always ask you where to select files’ after opting into cloud storage download folder, directory path from browser.download.lastDir is used.
(Assignee)

Comment 50

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review165826

::: toolkit/components/cloudstorage/CloudStorage.jsm:33
(Diff revision 3)
> +  log.level = Log.Level.Debug;
> +  let appender = new Log.DumpAppender();

Since goal here is to capture messages for debugging purpose, updating code to use Cu.reportError and not use Log.jsm in back-end API.

::: toolkit/components/cloudstorage/CloudStorage.jsm:108
(Diff revision 3)
> +      let data = await CloudStorageInternal.initProviders();
> +      CloudStorageInternal.providersMetaData = data;

Moving providersMetaData assignment inside internal method initProviders

::: toolkit/components/cloudstorage/CloudStorage.jsm:111
(Diff revision 3)
> +      // init download path while checking for custom download paths.
> +      // For Dropbox, sets provider metadata with download path read
> +      // programatically from ~/.dropbox/info.json
> +      result =  CloudStorageInternal.initDownloadPath();

Moving custom DownloadPath handling inside initProviders. Renamed method name to initDownloadPathIfProvidersExist and updated documentation to clarify its purpose

::: toolkit/components/cloudstorage/CloudStorage.jsm:372
(Diff revision 3)
> +          break;
> +        default:
> +          Promise.resolve();
> +          break;
> +      }
> +      return result;

Thanks for catching it, I think this is miss from updating code to use async and await in previous changeset. Updated code and documenation to explain returned values

::: toolkit/components/cloudstorage/CloudStorage.jsm:381
(Diff revision 3)
> +
> +  /**
> +   * Read Dropbox info.json and override providers metadata
> +   * downloadPath property
> +   */
> +  async _initDropbox(key) {

init method which internally calls _initDropbox is invoked per Firefox instance with the first non site specific download to initialize.

Scan call that differs from initialization gets triggered with download and can be timed by setting cloud.services.interval.prompt pref.

::: toolkit/components/cloudstorage/CloudStorage.jsm:412
(Diff revision 3)
> +      } catch (ex) {
> +        log.debug("_initDropbox: Error accessing dropbox download path for " + key);
> +        return;
> +      }
> +      // Update metadata with validated download path
> +      this.providersMetaData[key].downloadPath = info.path;

This handles an edge case where a user changes the default location of a provider download folder (from provider desktop client). That's what we are calling  as custom download path.

Default Download folder location from a provider are set in providers.json.

Since this is an edge case and Dropbox provides an easy way to retrieve download path set by user (inside info.json), we decided to support it for Dropbox in this version of API. From other providers there isn't an official way to retrieve custom download paths yet.

Added check inside initProviders to ensure initDownloadPathIfProvidersExist has handled all providers in property providersMetaData.

::: toolkit/components/cloudstorage/CloudStorage.jsm:424
(Diff revision 3)
> +    return directory.exists() && directory.isDirectory() &&
> +           directory.isWritable();

Updated code to use OS.File to check for directory and its existence. To check for writable should we use OS.File.open in write mode or is there a better way?

::: toolkit/components/cloudstorage/CloudStorage.jsm:438
(Diff revision 3)
> +    if (!this.providersMetaData) {
> +      let isInitialized = await CloudStorage.init();

That's so much better, thank you for suggesting this pattern. Updated code to use promiseInit property in CloudStorage API. Please feel free to suggest if it can be improved on.

::: toolkit/components/cloudstorage/content/providers.json:9
(Diff revision 3)
> +      "macosx": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
> +      "linux": ["UNIX_DROPBOX", "UNIX_GDRIVE"],
> +      "win": ["WINNT_DROPBOX", "WINNT_GDRIVE"]
> +    },
> +    "RU": {
> +      "macosx": ["UNIX_YANDEX", "UNIX_DROPBOX", "UNIX_GDRIVE"],

Removed Yandex from RU locale as the provider is not supported yet.

::: toolkit/components/cloudstorage/content/providers.json:21
(Diff revision 3)
> +        "default": "Downloads",
> +        "photo": "Photos",
> +        "screenshot": "Screenshots"

For now we are handling default in typeSpecificData. These values will be used in future versions of API that will use type specific folder such as Screenshots and Photos for downloading data. I will remove as we aren't supporting in current patch.

AFAIK, in future when we add more providers, there is possibility a provider can have different downloadPath, discoveryPath, displayName, and typeSpeicifcData between platforms, thats why its simpler to keep provider separate for every platform.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js:272
(Diff revision 3)
> +            // Notifications for CloudStorage API consumers to show offer
> +            // prompts while downloading. See Bug 1365129
> +            Services.obs.notifyObservers(null, "cloudstorage-prompt-notification", result.path);

These notifications are needed for CloudStorage API consumers. Without this planned add-on for shield study will not work.

As it cannot be part of add-on, I need to submit this as a separate one-line patch for nightly, not sure if that's better alternative.
Comment hidden (mozreview-request)
(Assignee)

Comment 52

2 years ago
Hi Gijs

Updated patch with review feedback. I need your input on best way to use OS.File to check if a directory is writable before addressing _isUsableDirectory main thread I/O issue (See comments above)

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b2e6d70fa5c74457836a0cfb78c1f536bffacd

Thanks!
(Assignee)

Comment 53

2 years ago
mozreview-review-reply
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review139398

> Does this need localizing?

yes, this will be localized as needed in add-on consuming back-end API

> Is this the same on different localized versions of Google Drive?

providers.json now support localized version of provider and will be able to handle locale-specific provider download and discovery path

> +1

yes, we should remove "UNIX_GDRIVE" from linux platform in providers.json. I will update provider.json with this change.

> If we have providers installed, we are running task test_multipleStorageProviders. Thanks for the pointer, its much safer to avoid hitting real providers and will update in next patch.

Issue is fixed in change set 3 and forward

Comment 54

2 years ago
(In reply to Punam Dahiya [:pdahiya] from comment #52)
> Hi Gijs
> 
> Updated patch with review feedback. I need your input on best way to use
> OS.File to check if a directory is writable before addressing
> _isUsableDirectory main thread I/O issue (See comments above)
> 
> Link to try server
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e0b2e6d70fa5c74457836a0cfb78c1f536bffacd
> 
> Thanks!

You can use OS.File.stat() - https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread#OS.File.stat() . It will reject if the file doesn't exist, and it will resolve with an object that will have an `isDir` property set to true if it is a directory, see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File.Info .

Comment 55

2 years ago
> To check for writable should we use OS.File.open in write mode or is there a better way?

I'm not sure. It would be sad to try to write to it on every startup, and I'm not sure how common it would be for the user to have dropbox/gdrive as a default download directory but then not be able to write to it.

Do the other getters for download paths check if the result is writable? If they don't, I don't think we need to do so, either.

Comment 56

2 years ago
Comment on attachment 8871508 [details] [diff] [review]
CloudStorage back-end using async destination select flow

I'm guessing this patch is obsoleted by the other work?
Attachment #8871508 - Attachment is obsolete: true

Comment 57

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review166452

::: toolkit/components/cloudstorage/CloudStorage.jsm:227
(Diff revision 4)
> +        let contentType = response.headers.get("content-type");
> +        if (contentType && contentType.includes("application/json")) {

Isn't this a file path from which we're reading? Do we really need to check headers? What is even setting these headers?

::: toolkit/components/cloudstorage/CloudStorage.jsm:404
(Diff revision 4)
> +    }
> +
> +    let data = await this._downloadJSON(Services.io.newFileURI(file).spec);
> +
> +    if (!data) {
> +      Cu.reportError("_initDropbox: Error fetching dropbox info.json for " + key);

My understanding is that this will fail and report to the error console on every machine that doesn't have dropbox. I don't think that's reasonable. We shouldn't spam the error console for what should be considered normal operation. Please audit your other uses of Cu.reportError for this as well.

::: toolkit/components/cloudstorage/CloudStorage.jsm:414
(Diff revision 4)
> +
> +    // Check if its valid path
> +    if (info && info.path) {
> +      try {
> +        let downloadDir = new FileUtils.File(info.path);
> +        if (!(downloadDir && await this._isUsableDirectory(downloadDir))) {

Does `new FileUtils.File` ever not throw and return null? When does that happen? If not, why the null-check?

::: toolkit/components/cloudstorage/CloudStorage.jsm:440
(Diff revision 4)
> +      if (info.isDir) {
> +        isUsable = true;
> +      }

```js
isUsable = info.isDir;
```

::: toolkit/components/cloudstorage/CloudStorage.jsm:470
(Diff revision 4)
> +    if (!dataType) {
> +      dataType = "default";
> +    }

This should be moved to after checks for the `preferredProviderKey` and that provider actually being in the metadata.

::: toolkit/components/cloudstorage/CloudStorage.jsm:474
(Diff revision 4)
> +
> +    if (!dataType) {
> +      dataType = "default";
> +    }
> +
> +    let key = this.preferredProviderKey;

I think we should just return early here if `preferredProviderKey` is falsy, or if `this.providersMetaData.hasOwnProperty(key)` is falsy, and return null, as we'll never have a path. That will reduce the null-checks later and make the code easier to follow.

::: toolkit/components/cloudstorage/CloudStorage.jsm:495
(Diff revision 4)
> +   *
> +   * @return {Promise}
> +   * @resolves
> +   * object key as found provider and value as provider metadata
> +   */
> +

Nit: No newline between doc comment and the method it relates to.

::: toolkit/components/cloudstorage/CloudStorage.jsm:563
(Diff revision 4)
> +   * file system
> +   * @return {Promise}
> +   * @resolves
> +   * boolean value of file existence check
> +   */
> +  _checkIfAssetExists(path) {

Do we really need both this and `_isUsableDirectory`?

::: toolkit/components/cloudstorage/content/providers.json:18
(Diff revision 4)
> +      "_downloadPath": ["homeDir", "Dropbox"],
> +      "_discoveryPath": ["homeDir", ".dropbox", "info.json"],
> +      "typeSpecificData": {
> +        "default": "Downloads"
> +      }
> +    },
> +
> +    "WINNT_DROPBOX": {
> +      "displayName": "Dropbox",
> +      "_downloadPath": ["homeDir", "Dropbox"],
> +      "_discoveryPath": ["LocalAppData", "Dropbox", "info.json"],
> +      "typeSpecificData": {
> +        "default": "Downloads"

In my previous review, I wrote:

> More generally, the names of these providers are also all the same. Can we just put the path stuff in platform-specfic properties, but not the other things? That would simplify the code a lot. In fact, the `_downloadPath` also seems to be identical across OSes.


But it doesn't seem like you've done this - the displayName and `_downloadPath` and `typeSpecificData` is still duplicated everywhere. You've also removed the some of the typeSpecificData, but not removed it from the example comment and code, so now I'm not sure what the state of that is, and if we really need that property at all.

Smaller nit: I don't understand why some of these properties have underscores and others don't. Can you clarify why that is? Normally we use `_` as a prefix for private properties or methods (that are only meant to be accessed / called from within the object on which they're defined), but because this is a JSON file that distinction doesn't make a lot of sense.
Attachment #8864579 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 58

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review166554

::: toolkit/components/cloudstorage/CloudStorage.jsm:227
(Diff revision 4)
> +        let contentType = response.headers.get("content-type");
> +        if (contentType && contentType.includes("application/json")) {

_downloadJSON is used at two places a) reading providers.json b) inside _initDropbox to read  info.json.

My rational behind checking content-type is from recommendation inside Fetch API https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

Under Headers - "A good use case for headers is checking whether the content type is correct before you process it further"

You are right both of these are determined JSON format and rechecking it seems like an overkill.

::: toolkit/components/cloudstorage/CloudStorage.jsm:404
(Diff revision 4)
> +    }
> +
> +    let data = await this._downloadJSON(Services.io.newFileURI(file).spec);
> +
> +    if (!data) {
> +      Cu.reportError("_initDropbox: Error fetching dropbox info.json for " + key);

For machines that doesn't have dropbox, _initDropbox will exist after first check line 390. Will check and remove Cu.reportError from expected flows.

::: toolkit/components/cloudstorage/CloudStorage.jsm:414
(Diff revision 4)
> +
> +    // Check if its valid path
> +    if (info && info.path) {
> +      try {
> +        let downloadDir = new FileUtils.File(info.path);
> +        if (!(downloadDir && await this._isUsableDirectory(downloadDir))) {

+1, new FileUtils.File will throw for invalid path, will remove explicit check for info.path

::: toolkit/components/cloudstorage/CloudStorage.jsm:563
(Diff revision 4)
> +   * file system
> +   * @return {Promise}
> +   * @resolves
> +   * boolean value of file existence check
> +   */
> +  _checkIfAssetExists(path) {

_isUsableDirectory is specific to directory check. where as _checkifAssetExist is more generic and checks existence of file assets such as info.json as well.

I don't see cleaner way of combining two unless we pass an extra argument specifying explicit for directory check.

::: toolkit/components/cloudstorage/content/providers.json:18
(Diff revision 4)
> +      "_downloadPath": ["homeDir", "Dropbox"],
> +      "_discoveryPath": ["homeDir", ".dropbox", "info.json"],
> +      "typeSpecificData": {
> +        "default": "Downloads"
> +      }
> +    },
> +
> +    "WINNT_DROPBOX": {
> +      "displayName": "Dropbox",
> +      "_downloadPath": ["homeDir", "Dropbox"],
> +      "_discoveryPath": ["LocalAppData", "Dropbox", "info.json"],
> +      "typeSpecificData": {
> +        "default": "Downloads"

I checked and Dropbox provides support for Screenshots, thats why I have added back screenshot property. For Dropbox, this property should  be needed when API is used with screenshots as consumer e.g.
CloudStorage.getDownloadFolder("screenshots");

I have updated the comments to reflect that.

My rationale behing keeping _ prefix for downloadPath and discoveryPath in providers.json is to indicate the values stored inside these properties are concatenated further to create platform specific downloadPath and discoveryPath property in metadata (See comments Line 72- 78) and Line 309 and 311 in CloudStorage.jsm assigning concatenated path to downloadPath and discoveryPath.

I thought its helpful to keep this distinction but if its confusing we can remove prefix.

The reason behind keeping the displayName and `_downloadPath` and `typeSpecificData` together for provider even though the values are duplicated for Dropbox and GDrive is keeping this structure generic for future providers e.g. Nautilus support of GDrive on linux. I don't have exact answer on how these value will differ but keeping this structure generic gives the flexibility for adding support for more providers across platforms.

Please free to suggest if there is a better way to keep this generic and simplify code further.
(Assignee)

Comment 59

2 years ago
(In reply to :Gijs from comment #55)
> > To check for writable should we use OS.File.open in write mode or is there a better way?
> 
> I'm not sure. It would be sad to try to write to it on every startup, and
> I'm not sure how common it would be for the user to have dropbox/gdrive as a
> default download directory but then not be able to write to it.
> 
> Do the other getters for download paths check if the result is writable? If
> they don't, I don't think we need to do so, either.

I agree, download folder for cloud provider should be writable. We do check isWritable inside validateLeafName method in nsHelperAppDialog.js after getting it from Downloads.getPreferredDownloadsDirectory() but no there is no check inside the getter.
Comment hidden (mozreview-request)
(Assignee)

Comment 61

2 years ago
Thanks Gijs for quick turnaround with reviews. Trying to aim for 56. Updated patch with today’s feedback. Thanks!

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a1c96f8f91ceaefe32f455ec6d7e9a2d44f4b99

Comment 62

2 years ago
(In reply to Punam Dahiya [:pdahiya] from comment #61)
> Thanks Gijs for quick turnaround with reviews. Trying to aim for 56.

Can you elaborate on how serious this requirement is? I haven't seen it before, and it's not mentioned in the plexus deck either. Merge day is in one week, so this seems very tight. :-\

(In reply to Punam Dahiya [:pdahiya] from comment #58)
> ::: toolkit/components/cloudstorage/CloudStorage.jsm:404
> (Diff revision 4)
> > +    }
> > +
> > +    let data = await this._downloadJSON(Services.io.newFileURI(file).spec);
> > +
> > +    if (!data) {
> > +      Cu.reportError("_initDropbox: Error fetching dropbox info.json for " + key);
> 
> For machines that doesn't have dropbox, _initDropbox will exist after first
> check line 390. Will check and remove Cu.reportError from expected flows.

Ah, right, I missed that. Thanks!

> ::: toolkit/components/cloudstorage/content/providers.json:18
> (Diff revision 4)
> > +      "_downloadPath": ["homeDir", "Dropbox"],
> > +      "_discoveryPath": ["homeDir", ".dropbox", "info.json"],
> > +      "typeSpecificData": {
> > +        "default": "Downloads"
> > +      }
> > +    },
> > +
> > +    "WINNT_DROPBOX": {
> > +      "displayName": "Dropbox",
> > +      "_downloadPath": ["homeDir", "Dropbox"],
> > +      "_discoveryPath": ["LocalAppData", "Dropbox", "info.json"],
> > +      "typeSpecificData": {
> > +        "default": "Downloads"
> 
> I checked and Dropbox provides support for Screenshots, thats why I have
> added back screenshot property. For Dropbox, this property should  be needed
> when API is used with screenshots as consumer e.g.
> CloudStorage.getDownloadFolder("screenshots");
> 
> I have updated the comments to reflect that.
> 
> My rationale behing keeping _ prefix for downloadPath and discoveryPath in
> providers.json is to indicate the values stored inside these properties are
> concatenated further to create platform specific downloadPath and
> discoveryPath property in metadata (See comments Line 72- 78) and Line 309
> and 311 in CloudStorage.jsm assigning concatenated path to downloadPath and
> discoveryPath.
> 
> I thought its helpful to keep this distinction but if its confusing we can
> remove prefix.

In this case I think calling these properties something like "relativeDownloadPath" or "relativeDiscoveryPath" might make more sense.

> The reason behind keeping the displayName and `_downloadPath` and
> `typeSpecificData` together for provider even though the values are
> duplicated for Dropbox and GDrive is keeping this structure generic for
> future providers e.g. Nautilus support of GDrive on linux. I don't have
> exact answer on how these value will differ but keeping this structure
> generic gives the flexibility for adding support for more providers across
> platforms.
> 
> Please free to suggest if there is a better way to keep this generic and
> simplify code further.

I would do something like this:

{
  displayName: "Dropbox",
  relativeDownloadPath: ["homeDir", "Dropbox"],
  relativeDiscoveryPath: {
    linux: ["homeDir", ".dropbox", "info.json"],
    macosx: ["homeDir", ".dropbox", "info.json"],
    win: ["LocalAppData", "Dropbox", "info.json"],
  }
}

{
  displayName: "Google Drive",
  relativeDownloadPath: ["homeDir", "Google Drive"],
  relativeDiscoveryPath: {
    macosx: ["homeDir", "Library", "Application Support", "Google", "Drive"],
    win: ["LocalAppData", "Google", "Drive"],
  }
}

and then filter the list based on whether the relativeDiscoveryPath exists for the platform (the keys are based on AppConstants.platform) and exists on disk.

This avoids duplicating the rest of the data (typeSpecificData) for the providers, and it avoids having a separate available_providers structure indicating on which platforms/locales a given provider may be available. If we need extra filters for locales, we can add them later, but for now they seem like unused additional complexity.

Comment 63

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review166712

::: toolkit/components/cloudstorage/CloudStorage.jsm:90
(Diff revision 5)
> +  /**
> +   * promiseInit saves init method promise in internal property that
> +   * can be used to wait for initialization to complete.
> +   */
> +  promiseInit: null,

Seems this should live on the internal object, which creates it, right?

::: toolkit/components/cloudstorage/CloudStorage.jsm:98
(Diff revision 5)
> +   */
> +  promiseInit: null,
> +
> +  /**
> +    * Init method to initialize providers metadata
> +    *

Nit: empty line

::: toolkit/components/cloudstorage/CloudStorage.jsm:103
(Diff revision 5)
> +    *
> +    */
> +  async init() {
> +    let isInitialized = null;
> +    try {
> +      // Invoke internal method asynchrnously to read and

Nit: asynchronously

::: toolkit/components/cloudstorage/CloudStorage.jsm:113
(Diff revision 5)
> +   * Return info used to hook door hangar prompt inside download manager destination select
> +   * flow. Return scanned provider info, if time elapsed since last prompt shown has
> +   * exceeded maximum allowed interval in pref cloud.services.interval.prompt
> +   * and if user has not previously accepted to download items in provider folder
> +   *

How about just saying: "Returns information to allow the consumer to decide whether showing a doorhanger prompt is appropriate", and then updating the @resolves annotation to show the exact property names, and what each property name represents?

Note that actually, from this comment it wasn't obvious to me that this rejects if we prompted before and/or there is a preferred provider set. How about making it resolve to null if it's not appropriate to prompt?

::: toolkit/components/cloudstorage/CloudStorage.jsm:127
(Diff revision 5)
> +  promisePromptInfo() {
> +    return CloudStorageInternal.promisePromptInfo();
> +  },
> +
> +  /**
> +   * Save user response to cloud door hangar prompt.

Nit: "doorhanger"

::: toolkit/components/cloudstorage/CloudStorage.jsm:160
(Diff revision 5)
> +
> +  /**
> +   * Retrieve download folder of an opted-in storage provider
> +   * by type specific data
> +   * @param typeSpecificData
> +   *        type of data downloaded, options are 'default', 'screenshot' and 'photo'

Please remove 'photo' here and elsewhere in the comments if we don't support it.

::: toolkit/components/cloudstorage/CloudStorage.jsm:161
(Diff revision 5)
> +  /**
> +   * Retrieve download folder of an opted-in storage provider
> +   * by type specific data
> +   * @param typeSpecificData
> +   *        type of data downloaded, options are 'default', 'screenshot' and 'photo'
> +   * @return nsIFile with path to provider download folder

This should say it returns a promise which resolves to an nsIFile, because the internal method is async and therefore returns a promise.

::: toolkit/components/cloudstorage/CloudStorage.jsm:171
(Diff revision 5)
> +
> +  /**
> +   * Get provider opted-in by user to store downloaded files
> +   *
> +   * @return {String}
> +   * Storage provider key from provider metadata

This should document what it returns if the user hasn't selected a provider at all.

::: toolkit/components/cloudstorage/CloudStorage.jsm:200
(Diff revision 5)
> +  /**
> +   * Provider key retrieved from service pref
> +   * cloud.services.storage.key
> +   */
> +  preferredProviderKey: null,
> +
> +  /**
> +   * show prompt interval in days, by default set to 0
> +   */
> +  promptInterval: null,
> +
> +  /**
> +   * Lastprompt time in seconds, by default set to 0
> +   */
> +  lastPromptTime: null,
> +
> +  /**
> +   * Provider keys rejected by user for default download
> +   */
> +  cloudStorageRejectedKeys: null,

These four properties shouldn't be defined here, as you redefine them as lazy getters later. Move the comments to the lazy getter definitions.

::: toolkit/components/cloudstorage/CloudStorage.jsm:358
(Diff revision 5)
> +    let arrPromises = Object.getOwnPropertyNames(this.providersMetaData).map(key => {
> +      switch (key) {
> +        case "WINNT_DROPBOX":
> +        case "UNIX_DROPBOX":
> +          return this._initDropbox(key);
> +        default:
> +          return Promise.resolve(false);
> +      }
> +    });

After the refactoring of the providers metadata this can just be:

```js
let providerKeys = Object.keys(this.providersMetaData);
let promises = providerKeys.map(key => {
  return key == "Dropbox" ?
         this._initDropbox(key) :
         Promise.resolve(false));
return Promise.all(promises);
```

::: toolkit/components/cloudstorage/CloudStorage.jsm:401
(Diff revision 5)
> +    if (!data) {
> +      return false;
> +    }
> +
> +    let info = data.personal;
> +
> +    try {
> +      let downloadDir = info ? new FileUtils.File(info.path) : null;
> +      if (!(downloadDir && await this._isUsableDirectory(downloadDir))) {

This can still be simplified some more. `_isUsableDirectory` should just take a path string rather than an nsIFile, because the only thing it does with the file is read the path. Then you can largely avoid creating new FileUtils.File() instances, and just pass the path directly. This code then becomes:

```js
let path = data && data.personal && data.personal.path;
if (!path) {
  return false;
}
let isUsable = await this._isUsableDirectory(path);
if (isUsable) {
  this.providersMetaData.Dropbox.downloadPath = path;
}
return isUsable;
```

Though, looking at this more closely, maybe you need to make sure you resolve `path` relative to some kind of base path? You don't seem to be doing that right now, at least not explicitly, and it's not clear to me if the info.json file normally contains an absolute or relative path, but in the test you use a relative path...

::: toolkit/components/cloudstorage/CloudStorage.jsm:429
(Diff revision 5)
> +   *
> +   * @param directory of type nsIFile
> +   *        The directory to check.
> +   * @return true if we can use the directory, false otherwise.
> +   */
> +  async _isUsableDirectory(directory) {

As noted elsewhere, I think this should just take the path instead of the nsIFile.

::: toolkit/components/cloudstorage/CloudStorage.jsm:435
(Diff revision 5)
> +      if (!(e instanceof OS.File.Error) || !e.becauseNoSuchFile) {
> +        // Directory doesn't exist
> +        isUsable = false;
> +      }

You can just omit this - `isUsable` is initialized as false, and there is no way for the code to have thrown *and* modified `isUsable`, so it's guaranteed to still be false anyway. You can add a comment in the empty catch statement saying something like "The directory doesn't exist, so `isUsable` will still be false" or something.

::: toolkit/components/cloudstorage/CloudStorage.jsm:448
(Diff revision 5)
> +  /**
> +   * Retrieve download folder of preferred provider
> +   * by type specific data
> +
> +   * @param dataType
> +   *        type of data downloaded, options are 'default', 'screenshot' and 'photo'

Add a comment that the default is `default`. :-)

::: toolkit/components/cloudstorage/CloudStorage.jsm:462
(Diff revision 5)
> +        return null;
> +      }
> +    }
> +
> +    let key = this.preferredProviderKey;
> +    if (!key && !this.providersMetaData.hasOwnProperty(key)) {

This should be an "or", ie `!key || !this.providersMetaData.hasOwnProperty(key)`, right?

::: toolkit/components/cloudstorage/CloudStorage.jsm:467
(Diff revision 5)
> +    if (!key && !this.providersMetaData.hasOwnProperty(key)) {
> +      return null;
> +    }
> +
> +    if (!dataType) {
> +      dataType = "default";

Just make `default` the default in the argument, ie define this method as:

```js
async getDownloadFolder(dataType = "default") {
```

::: toolkit/components/cloudstorage/CloudStorage.jsm:469
(Diff revision 5)
> +    }
> +
> +    if (!dataType) {
> +      dataType = "default";
> +    }
> +    let provider = this.providersMetaData[key];

Should we add a check that `provider.typeSpecificData[dataType]` exists and return null if it doesn't?

::: toolkit/components/cloudstorage/CloudStorage.jsm:470
(Diff revision 5)
> +    let downloadDir =
> +      new FileUtils.File(OS.Path.join(provider.downloadPath,
> +                                      provider.typeSpecificData[dataType]));
> +
> +    if (!(downloadDir && await this._isUsableDirectory(downloadDir))) {

If we make `_isUsableDirectory` take a path here we can omit the FileUtils.File() code and the nullcheck for `downloadDir`.

::: toolkit/components/cloudstorage/CloudStorage.jsm:493
(Diff revision 5)
> +   */
> +  promisePromptInfo() {
> +    if (!this.preferredProviderKey && this.shouldPrompt()) {
> +      return this.scan();
> +    }
> +    return Promise.reject("NO_VALID_PROMPT");

I wonder if this should return null instead of rejecting?

::: toolkit/components/cloudstorage/CloudStorage.jsm:530
(Diff revision 5)
> +    if (!providers.size) {
> +      // No storage services installed on user desktop
> +      return null;
> +    }
> +
> +    let providerInfo = {};

Please remove this...

::: toolkit/components/cloudstorage/CloudStorage.jsm:542
(Diff revision 5)
> +      providerInfo.key = provider[0];
> +      providerInfo.value = provider[1];

And here just:

```js
return {key: provider[0], value: provider[1]};
```

and `return null` outside the if block.

::: toolkit/components/cloudstorage/CloudStorage.jsm:599
(Diff revision 5)
> +      Services.prefs.setCharPref(CLOUD_SERVICES_PREF + "rejected.key", key);
> +    } else {
> +      // Pref exists with previous rejected keys, append
> +      // key at the end and update pref
> +      let keys = rejected.split(",");
> +      if (keys.push(key) > 0) {

What are you trying to do here? The split always returns an array of at least length 1, and array.push() returns the new length, which is always going to be at least 1 more than the previous length, which then means that this is always going to be true, right? Why do we need the if statement?

::: toolkit/components/cloudstorage/CloudStorage.jsm:605
(Diff revision 5)
> +  /**
> +   *
> +   * Internal cloud services prefs helper methods
> +   *
> +   * cloud.services.storage.key - set to string with cloud storage provider key if a user accepts saving to cloud
> +   * storage in door hanger prompt
> +   *

This comment looks a bit lost. Was it meant to go somewhere else?

::: toolkit/components/cloudstorage/CloudStorage.jsm:635
(Diff revision 5)
> +   * get access to metadata of storage provider set in pref cloud.services.storage.key
> +   */
> +  get acceptedProviderMetadata() {

This doesn't seem to be used at all? It's internal-only, so it's not exposed to the add-on right now...

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:155
(Diff revision 5)
> +  if (metadata.size < 1) {
> +    Assert.equal(metadata.size, 0, "Number of storage providers");
> +    Assert.ok(!scanProvider, "No provider in scan results");

This is confusing, in that it doesn't look like it's possible for these checks to fail, because you check the same thing in the if statement first.

If you're using this with a null key and with a valid key to check there are / aren't providers, shouldn't the if statement be based on whether a key is passed? As it is, this just looks like we're hiding failures by checking for them first.

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:201
(Diff revision 5)
> +    }
> +  }
> +}
> +
> +add_task(async function test_checkInit() {
> +  let isInitialized = await CloudStorage.promiseInit;

Moving this promise to the internal object, you can get to that internal object by using the result of Cu.import, that is:

```js
let {CloudStorageInternal} = Cu.import('resource://gre/modules/CloudStorage.jsm');
let isInitialized = await CloudStorageInternal.promiseInit;
```

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:217
(Diff revision 5)
> + */
> +add_task(async function test_dropboxStorageProvider() {
> +  nsIDropboxFile = mock_dropbox();
> +  let result = await checkScan(DROPBOX_KEY);
> +
> +  if (result.size) {

Why do we need an if() check? When isn't this true?
Attachment #8864579 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 64

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review167056

::: toolkit/components/cloudstorage/CloudStorage.jsm:401
(Diff revision 5)
> +    if (!data) {
> +      return false;
> +    }
> +
> +    let info = data.personal;
> +
> +    try {
> +      let downloadDir = info ? new FileUtils.File(info.path) : null;
> +      if (!(downloadDir && await this._isUsableDirectory(downloadDir))) {

info.json contains an absolute path. Tests are running off default download path from providers.json and not reading path from info.json.

To tests with path read from info.json, I need to  write absolute path to downloadFolder (Line 81 mock_dropbox) in info.json. It's on my ToDo list but not sure if it's a blocker.

::: toolkit/components/cloudstorage/CloudStorage.jsm:448
(Diff revision 5)
> +  /**
> +   * Retrieve download folder of preferred provider
> +   * by type specific data
> +
> +   * @param dataType
> +   *        type of data downloaded, options are 'default', 'screenshot' and 'photo'

Done :)

::: toolkit/components/cloudstorage/CloudStorage.jsm:462
(Diff revision 5)
> +        return null;
> +      }
> +    }
> +
> +    let key = this.preferredProviderKey;
> +    if (!key && !this.providersMetaData.hasOwnProperty(key)) {

Good catch, thanks!

::: toolkit/components/cloudstorage/CloudStorage.jsm:470
(Diff revision 5)
> +    let downloadDir =
> +      new FileUtils.File(OS.Path.join(provider.downloadPath,
> +                                      provider.typeSpecificData[dataType]));
> +
> +    if (!(downloadDir && await this._isUsableDirectory(downloadDir))) {

With this change getDownloadFolder returns path to download folder instead of nsIFile object, updated comments to reflect that

::: toolkit/components/cloudstorage/CloudStorage.jsm:635
(Diff revision 5)
> +   * get access to metadata of storage provider set in pref cloud.services.storage.key
> +   */
> +  get acceptedProviderMetadata() {

Removing as getPreferredProvider with metadata exposed using promisePromptInfo and getStorageProviders suffice add-on requirement

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:201
(Diff revision 5)
> +    }
> +  }
> +}
> +
> +add_task(async function test_checkInit() {
> +  let isInitialized = await CloudStorage.promiseInit;

It works, didn't know that. I thought value in EXPORTED_SYMBOLS is pre-req for accessing an internal object outside. What's the significance of EXPORTED_SYMBOLS?

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:217
(Diff revision 5)
> + */
> +add_task(async function test_dropboxStorageProvider() {
> +  nsIDropboxFile = mock_dropbox();
> +  let result = await checkScan(DROPBOX_KEY);
> +
> +  if (result.size) {

Removing it from Dropbox as it's available for all platforms. We do need it in gDriveStorageProvider where we don't support GDrive on linux
(Assignee)

Comment 65

2 years ago
(In reply to :Gijs from comment #62)
> (In reply to Punam Dahiya [:pdahiya] from comment #61)
> > Thanks Gijs for quick turnaround with reviews. Trying to aim for 56.
> 
> Can you elaborate on how serious this requirement is? I haven't seen it
> before, and it's not mentioned in the plexus deck either. Merge day is in
> one week, so this seems very tight. :-\
> 
Few shield studies targeted for Q4 are dependent on this requirement.Will NI Mike for more insight.

> I would do something like this:
> 
> {
>   displayName: "Dropbox",
>   relativeDownloadPath: ["homeDir", "Dropbox"],
>   relativeDiscoveryPath: {
>     linux: ["homeDir", ".dropbox", "info.json"],
>     macosx: ["homeDir", ".dropbox", "info.json"],
>     win: ["LocalAppData", "Dropbox", "info.json"],
>   }
> }
> 
> {
>   displayName: "Google Drive",
>   relativeDownloadPath: ["homeDir", "Google Drive"],
>   relativeDiscoveryPath: {
>     macosx: ["homeDir", "Library", "Application Support", "Google", "Drive"],
>     win: ["LocalAppData", "Google", "Drive"],
>   }
> }
> 
> and then filter the list based on whether the relativeDiscoveryPath exists
> for the platform (the keys are based on AppConstants.platform) and exists on
> disk.
> 
> This avoids duplicating the rest of the data (typeSpecificData) for the
> providers, and it avoids having a separate available_providers structure
> indicating on which platforms/locales a given provider may be available. If
> we need extra filters for locales, we can add them later, but for now they
> seem like unused additional complexity.

That's fair, I have updated providers.json . We might need to extend relativeDownloadPath to be platform specific if providers have different downloadPath by platform. Keeping it as is for now as GDrive, Dropbox doesn't have that requirement.
Flags: needinfo?(mconnor)
Comment hidden (mozreview-request)
(Assignee)

Comment 67

2 years ago
Updated patch with feedback, please review. Thanks!

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=323c6a486a84f5ed9a65ce79d1f864b1138511be

Comment 68

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review167254

r=me with the big comment about preferences moved somewhere more sensible - perhaps it just wants to live at the top of the file, with the other big comment?

::: toolkit/components/cloudstorage/CloudStorage.jsm:59
(Diff revision 6)
> + * @Provider - Unique cloud provider key, possible values: "Dropbox", "GDrive"
> + *
> + * @displayName - cloud storage name displayed in the prompt.
> + *
> + * @relativeDownloadPath - download path on user desktop for a cloud storage provider.
> + * By default downloadPath is a concatentation of home dir and name of dropbox folder.

Nit: concatenation

::: toolkit/components/cloudstorage/CloudStorage.jsm:226
(Diff revision 6)
> +
> +  /**
> +   * Load parsed metadata inside providers object
> +   */
> +  _parseProvidersJSON(providers) {
> +

Nit: no empty line at the beginning of a method

::: toolkit/components/cloudstorage/CloudStorage.jsm:365
(Diff revision 6)
> +    try {
> +      let info = await OS.File.stat(path);
> +      isUsable = info.isDir;
> +    } catch (e) {
> +      if (!(e instanceof OS.File.Error) || !e.becauseNoSuchFile) {
> +        // Directory doesn't exist, so isUsable will still be false

I meant you can remove the if() statement in the catch() here. :-)

::: toolkit/components/cloudstorage/CloudStorage.jsm:421
(Diff revision 6)
> +    // and if time elapsed since last prompt shown has exceeded maximum allowed interval
> +    // in pref cloud.services.interval.prompt before continuing to scan for providers
> +    if (!this.preferredProviderKey && this.shouldPrompt()) {
> +      return this.scan();
> +    }
> +    return Promise.resolve();

Explicitly pass `null` to Promise.resolve() here, because right now it will resolve to `undefined`, which isn't the same thing.

::: toolkit/components/cloudstorage/CloudStorage.jsm:543
(Diff revision 6)
> +  /**
> +   *
> +   * Internal cloud services prefs
> +   *
> +   * cloud.services.storage.key - set to string with preferred provider key
> +   *
> +   * cloud.services.lastPrompt - set to time when last door hangar prompt was shown to avoid prompting user

Still confused about this comment. Why is it here / what is it meant for? :-)

::: toolkit/components/cloudstorage/CloudStorage.jsm:549
(Diff revision 6)
> +   *
> +   * Internal cloud services prefs
> +   *
> +   * cloud.services.storage.key - set to string with preferred provider key
> +   *
> +   * cloud.services.lastPrompt - set to time when last door hangar prompt was shown to avoid prompting user

Nit: doorhanger

::: toolkit/components/cloudstorage/tests/unit/test_cloudstorage.js:244
(Diff revision 6)
> +    result = await checkScan();
> +  } else {
> +    result = await checkScan(GDRIVE_KEY);
> +  }
> +
> +  if (result.size) {

Please check for AppConstants.platform here, too, so the test fails if result.size is 0 on other platforms.
Attachment #8864579 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 69

2 years ago
mozreview-review
Comment on attachment 8864579 [details]
Bug 1357171 - cloud storage module

https://reviewboard.mozilla.org/r/136256/#review167404

::: toolkit/components/cloudstorage/CloudStorage.jsm:543
(Diff revision 6)
> +  /**
> +   *
> +   * Internal cloud services prefs
> +   *
> +   * cloud.services.storage.key - set to string with preferred provider key
> +   *
> +   * cloud.services.lastPrompt - set to time when last door hangar prompt was shown to avoid prompting user

Content writing is hard :). Aim is to explain where these prefs are used in code. Updated it to keep generic "set to time when last prompt was shown"
Comment hidden (mozreview-request)
(Assignee)

Comment 71

2 years ago
Thanks Gijs for review. Moved preferences documentation to the top of the file. Pushed updated and rebased patch to moz-review.

Comment 74

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7ba51fec8d0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

2 years ago
Depends on: 1385402
(Assignee)

Updated

a year ago
Flags: needinfo?(mconnor)
(Assignee)

Updated

a year ago
Attachment #8864579 - Flags: review?(mconnor)
(Assignee)

Updated

a year ago
Attachment #8884596 - Flags: review?(mconnor)
You need to log in before you can comment on or make changes to this bug.