Closed Bug 1387153 Opened 2 years ago Closed 2 years ago

Preferences UI to support cloud storage download

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: pdahiya, Assigned: pdahiya)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Under Preferences -> Files & Applications -> Downloads

Downloads section should show third radio button with option
‘Save files to <Preferred Cloud storage Provider> Download folder' if

a) system add-on (bug 1365129) is present

b) pref cloud.services.storage.key exists with provider key as value and browser.download.useDownloadDir is true.

c) pref browser.download.folderlist is 3 and CloudStorage.getDownloadfolder returns a valid provider download folder path.
Blocks: 1357160
Follow up bug to 1381066 focusing on preferences UI fix for testing in en-US
Assignee: nobody → pdahiya
Attached image Preferences UI with cloud storage (obsolete) —
Attachment #8894723 - Attachment is obsolete: true
Pr-requisite
- Desktop with cloud storage provider client such as Dropbox , Google Drive

Steps to test preference UI by manually creating pref and Downloads folder

1. Set pref cloud.services.storage.key pref to provider key such as ’Dropbox’
2. Set bool pref cloud.services.addon.enabled to true
3. Ensure you have a valid download path for cloud storage downloads by manually creating folder ‘Downloads’ in provider folder e.g. ~/Dropbox/Downloads
4. Open about:preferences it should show radio button ‘Save to <provider name>'

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

Please review. Thanks!
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs)
Jared's review is enough here. Jared, let me know if you don't have cycles and I can take it.
Hi Mike
As discussed, putting up patch to handle cloud storage pref strings as chrome URL that can be overridden from system add-on as needed, would like your feedback before touching main patch.
Thanks!
Attachment #8895181 - Flags: feedback?(mconnor)
Comment on attachment 8895181 [details] [diff] [review]
WIP patch to handle cloud storage preference strings using chrome url

I'd add an explicit comment to the properties file noting that the file will be moved into localization in a future revision.
Attachment #8895181 - Flags: feedback?(mconnor) → feedback+
Attachment #8894729 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review171870

To be safe, please ensure you make the same modifications to the "old" in-content prefs, in case we end up shipping those with 56.

::: browser/components/preferences/in-content-new/main.js:2291
(Diff revision 1)
> +    const CLOUD_STORAGE_PREF_PROVIDER_KEY = "cloud.services.storage.key";
> +    const CLOUD_STORAGE_ADD_ON_ENABLED = "cloud.services.addon.enabled";
> +
> +    let cloudProviderPref = Services.prefs.getCharPref(CLOUD_STORAGE_PREF_PROVIDER_KEY, "");
> +    let cloudAddOnPref = Services.prefs.getBoolPref(CLOUD_STORAGE_ADD_ON_ENABLED, false);
> +
> +    // Check if CloudStorage system add-on is present and user has set preferred cloud storage provider key
> +    // and a valid download path exist on user desktop before continuing
> +    if (cloudAddOnPref && cloudProviderPref && await CloudStorage.getDownloadFolder()) {
> +      // Get preferred provider details
> +      let provider = CloudStorage.getPreferredProviderMetaData();
> +      if (provider && provider.displayName) {

The preferences shouldn't need to poke at so much of the details of the cloud storage code. Everything including the provider and dir name check should be a single method on CloudStorage.jsm that just returns a provider name or null, depending on whether we need to display this. That's all the prefs care about. In here, you should be able to just do:

```js
let providerDisplayName = CloudStorage.getProviderIfInUse();
if (providerDisplayName) {
  // create radio and select based on the preference value.
}

::: browser/components/preferences/in-content-new/main.js:2333
(Diff revision 1)
> +   * Handle clicks to 'Save To <custom path> or <system default downloads>' and
> +   * 'Save to <cloud storage provider>' if cloud storage radio button is displayed in UI.
> +   * Sets browser.download.folderList value and Enables/disables the folder field and Browse
> +   * button based on option selected.
> +   */
> +  handleSaveToClick(event) {

This looks like it should be a change handler on the set of radio buttons instead. Right now you don't handle keyboard access.

::: browser/components/preferences/in-content-new/main.js:2343
(Diff revision 1)
> +    // before continuing.
> +    let saveToCloudRadio = document.getElementById("saveToCloud");
> +    if (!saveToCloudRadio.hidden) {
> +      let downloadFolder = document.getElementById("downloadFolder");
> +      let chooseFolder = document.getElementById("chooseFolder");
> +      let currentDirPref = document.getElementById("browser.download.dir");

Put this next to where you use it.

::: browser/components/preferences/in-content-new/main.js:2346
(Diff revision 1)
> +      let downloadFolder = document.getElementById("downloadFolder");
> +      let chooseFolder = document.getElementById("chooseFolder");
> +      let currentDirPref = document.getElementById("browser.download.dir");
> +      let folderListPref = document.getElementById("browser.download.folderList");
> +
> +      let saveWhere = document.getElementById("saveWhere");

This too, after the comment.

::: browser/components/preferences/in-content-new/main.js:2347
(Diff revision 1)
> +      let chooseFolder = document.getElementById("chooseFolder");
> +      let currentDirPref = document.getElementById("browser.download.dir");
> +      let folderListPref = document.getElementById("browser.download.folderList");
> +
> +      let saveWhere = document.getElementById("saveWhere");
> +      let saveTo = document.getElementById("saveTo");

And this before the if statement.

::: browser/components/preferences/in-content-new/main.xul:687
(Diff revision 1)
> +    <!-- Additional radio button added to support CloudStorage - Bug 1357171 --> 
> +    <radio id="saveToCloud"
> +          value="true"
> +          hidden="true"/>

I'm really surprised you're even able to have 2 radio buttons with the same value, but if this works, I guess it's OK...
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review171952

::: browser/components/preferences/in-content-new/main.js:2291
(Diff revision 1)
> +    const CLOUD_STORAGE_PREF_PROVIDER_KEY = "cloud.services.storage.key";
> +    const CLOUD_STORAGE_ADD_ON_ENABLED = "cloud.services.addon.enabled";
> +
> +    let cloudProviderPref = Services.prefs.getCharPref(CLOUD_STORAGE_PREF_PROVIDER_KEY, "");
> +    let cloudAddOnPref = Services.prefs.getBoolPref(CLOUD_STORAGE_ADD_ON_ENABLED, false);
> +
> +    // Check if CloudStorage system add-on is present and user has set preferred cloud storage provider key
> +    // and a valid download path exist on user desktop before continuing
> +    if (cloudAddOnPref && cloudProviderPref && await CloudStorage.getDownloadFolder()) {
> +      // Get preferred provider details
> +      let provider = CloudStorage.getPreferredProviderMetaData();
> +      if (provider && provider.displayName) {

+1 updated Cloud Storage API to expose getPreferredProviderName and method getPreferredProviderMetaData in case needed from system add-on.

::: browser/components/preferences/in-content-new/main.js:2333
(Diff revision 1)
> +   * Handle clicks to 'Save To <custom path> or <system default downloads>' and
> +   * 'Save to <cloud storage provider>' if cloud storage radio button is displayed in UI.
> +   * Sets browser.download.folderList value and Enables/disables the folder field and Browse
> +   * button based on option selected.
> +   */
> +  handleSaveToClick(event) {

Updated it to command event listener to allow keyboard access, change doesn't work in this case as two radio button has same value 'true'

::: browser/components/preferences/in-content-new/main.xul:687
(Diff revision 1)
> +    <!-- Additional radio button added to support CloudStorage - Bug 1357171 --> 
> +    <radio id="saveToCloud"
> +          value="true"
> +          hidden="true"/>

It works and helps to keep pref browser.download.useDownloadDir in sync which is true for both 'SaveTo' and 'SaveToCloud'
(In reply to :Gijs from comment #9)
> Comment on attachment 8894729 [details]
> Bug 1387153 - Preferences UI to support cloud storage download
> 
> https://reviewboard.mozilla.org/r/165896/#review171870
> 
> To be safe, please ensure you make the same modifications to the "old"
> in-content prefs, in case we end up shipping those with 56.
> 
I would like to test with cloud storage updates before submitting patch for review, how can we switch to old in-content pref UI? Thanks
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Punam Dahiya [:pdahiya] from comment #11)
> (In reply to :Gijs from comment #9)
> > Comment on attachment 8894729 [details]
> > Bug 1387153 - Preferences UI to support cloud storage download
> > 
> > https://reviewboard.mozilla.org/r/165896/#review171870
> > 
> > To be safe, please ensure you make the same modifications to the "old"
> > in-content prefs, in case we end up shipping those with 56.
> > 
> I would like to test with cloud storage updates before submitting patch for
> review, how can we switch to old in-content pref UI? Thanks

Flip the browser.preferences.useOldOrganization pref in about:config . :-)

I don't know off-hand if that works at runtime, but I would expect it to.
Flags: needinfo?(gijskruitbosch+bugs)
browser.preferences.useOldOrganization pref works, tested with old in-content pref UI.

Submitted patch with below updates
a) Update value of folderList pref by checking file path value from browser.download.dir using _folderToIndex to handle revert back to folderList pref value 1 (system default downloads) - (Line 2366, 2451 in-content-new/main.js) 
b) Access cloud storage string using chrome URL for preferences.properties

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

Try server is complaining about referenced files for chrome://cloudstorage/content/providers.json

Need your input on fixing jar.mn so that it generates only resource://cloudstorage/providers.json and chrome://cloudstorage/content/preferences.properties . As of now its generating respective chrome and resource URL for both files inside content folder, even though jar.mn has specific file names instead of content/*.

Thanks!
> Need your input on fixing jar.mn so that it generates only
> resource://cloudstorage/providers.json and
> chrome://cloudstorage/content/preferences.properties . As of now its
> generating respective chrome and resource URL for both files inside content
> folder, even though jar.mn has specific file names instead of content/*.
> 

figured the issue, looks like %content doesn’t go well when using both chrome
content and resource in jar manifest

Changing resource declaration to below fixes the referenced file issue
% resource cloudstorage %res/
    res/providers.json       (content/providers.json)

Link to new try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e63657de46dd802a83c5a43c5d75aee18aa1ab7e
> Link to new try
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e63657de46dd802a83c5a43c5d75aee18aa1ab7e

issue still exist with try server complaining about unreferenced file: resource://gre-resources/providers.json, not sure if registering specific chrome and resource URI in single jar.mn  is supported, if not should we have both providers.json and preferences.properties shipped as resource URI. Please recommend. Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Punam Dahiya [:pdahiya] from comment #14)
> browser.preferences.useOldOrganization pref works, tested with old
> in-content pref UI.
> 
> Submitted patch with below updates
> a) Update value of folderList pref by checking file path value from
> browser.download.dir using _folderToIndex to handle revert back to
> folderList pref value 1 (system default downloads) - (Line 2366, 2451
> in-content-new/main.js) 
> b) Access cloud storage string using chrome URL for preferences.properties
> 
> Link to try server
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2d65f752880bc8ee5c2275e8fe765a57ef8e6669
> 
> Try server is complaining about referenced files for
> chrome://cloudstorage/content/providers.json
> 
> Need your input on fixing jar.mn so that it generates only
> resource://cloudstorage/providers.json and
> chrome://cloudstorage/content/preferences.properties . As of now its
> generating respective chrome and resource URL for both files inside content
> folder, even though jar.mn has specific file names instead of content/*.

I don't understand why the test would complain as long as each file is accessed correctly over 1 of the 2 methods of accessing it. Needinfo->Florian for this.

Equally, I don't understand why we need both resource: and chrome: URLs.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(florian)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review172780

::: browser/components/preferences/in-content/main.js:554
(Diff revision 2)
> +    let cloudProviderPref = Services.prefs.getCharPref(CLOUD_STORAGE_PREF_PROVIDER_KEY, "");
> +    let cloudAddOnPref = Services.prefs.getBoolPref(CLOUD_STORAGE_ADD_ON_ENABLED, false);
> +
> +    // Check if CloudStorage system add-on is present and user has set preferred cloud storage provider key
> +    // and a valid download path exist on user desktop before continuing
> +    if (cloudAddOnPref && cloudProviderPref && await CloudStorage.getDownloadFolder()) {

My previous feedback must have been unclear. You're still checking all kinds of prefs and making other calls (getDownloadFolder, getPreferredProviderName) into CloudStorage jsm.

What I tried to say last time is that the prefs code should make only one (1) call into cloudstorage. Inside that method, CloudStorage should be responsible for all the relevant checks, and it should return either a preferred provider name to use for displaying this preference, or null (if, for whatever reason, we don't have a cloud storage provider to display).

Then the prefs code can have just 1 if statement that checks the return value. If it's non-empty (truthy), you create the element and everything else with that value. If not, you don't. We shouldn't have to add this much complexity to the preferences code just to check whether a valid cloud storage provider is present - that's the responsibility of the cloudstorage module.

::: toolkit/components/cloudstorage/jar.mn:9
(Diff revision 2)
> +% content cloudstorage %content/
> +  content/preferences.properties     (content/preferences.properties)

Why can't we load this file through a resource: URI?
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #17)

> I don't understand why the test would complain as long as each file is
> accessed correctly over 1 of the 2 methods of accessing it.
> Needinfo->Florian for this.

The test works by finding all files inside omni.ja, and then converting their paths to chrome URLs if possible; if they can't be converted to a chrome URL, then it converts the path to a resource url.

Files accessible through both chrome:// and resource:// at the same path will cause test failures. Usually this isn't a problem though (except for devtools), because I can't think of a good reason to make a file accessible through both url schemes.

> Equally, I don't understand why we need both resource: and chrome: URLs.

+1, I don't understand why this file is accessed from resource://cloudstorage/providers.json rather than from chrome://cloudstorage/providers.json
Flags: needinfo?(florian)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review173188

::: browser/components/preferences/in-content/main.js:554
(Diff revision 2)
> +    let cloudProviderPref = Services.prefs.getCharPref(CLOUD_STORAGE_PREF_PROVIDER_KEY, "");
> +    let cloudAddOnPref = Services.prefs.getBoolPref(CLOUD_STORAGE_ADD_ON_ENABLED, false);
> +
> +    // Check if CloudStorage system add-on is present and user has set preferred cloud storage provider key
> +    // and a valid download path exist on user desktop before continuing
> +    if (cloudAddOnPref && cloudProviderPref && await CloudStorage.getDownloadFolder()) {

Thanks Gijs for clarifying, updated pref UI to call single method getProviderIfInUse that checks if we have a provider to display.

As discussed, Cloud Storage API can now be turned on/off using generic pref 'cloud.services.api.enabled'. Add-on consuming CloudStorage module should set this pref to true.

::: toolkit/components/cloudstorage/jar.mn:9
(Diff revision 2)
> +% content cloudstorage %content/
> +  content/preferences.properties     (content/preferences.properties)

Updated to load providers.json and preferences.properties using resource URI
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review173378

::: browser/components/preferences/in-content/main.js:94
(Diff revision 3)
>      setEventListener("browser.privatebrowsing.autostart", "change",
>                       gMainPane.updateBrowserStartupLastSession);
>      setEventListener("browser.download.dir", "change",
>                       gMainPane.displayDownloadDirPref);
> +    setEventListener("saveWhere", "command",
> +                     gMainPane.handleSaveToClick);

Nit: rename to handleSaveToCommand

::: browser/components/preferences/in-content/main.js:553
(Diff revision 3)
> +      let cloudStrings = Services.strings.createBundle("resource://cloudstorage/preferences.properties");
> +      saveToCloudRadio.label = cloudStrings.formatStringFromName("saveFilesToCloudStorage",
> +                                                                 [providerDisplayName], 1);
> +      saveToCloudRadio.hidden = false;
> +
> +      let preference = document.getElementById("browser.download.useDownloadDir");

Nit: use a better name than 'preference' for this variable.

::: browser/components/preferences/in-content/main.js:560
(Diff revision 3)
> +        let saveWhere = document.getElementById("saveWhere");
> +        let downloadFolder = document.getElementById("downloadFolder");
> +        let chooseFolder = document.getElementById("chooseFolder");
> +
> +        saveWhere.selectedItem = saveToCloudRadio;
> +        downloadFolder.disabled = true;
> +        chooseFolder.disabled = true;

Nit: I would not use temp variables here anymore and just assign `.disabled = true` directly on the result of `getElementById`.

::: browser/components/preferences/in-content/main.js:691
(Diff revision 3)
> +      // Handle Cloud Storage pref and use value in currentDirPref to
> +      // display download folder label and icon
> +      let index = await this._folderToIndex(currentDirPref.value);
> +      if (index == 2) {
> +        downloadFolder.label = this._getDisplayNameOfFile(currentDirPref.value);
> +        iconUrlSpec = fph.getURLSpecFromFile(currentDirPref.value);
> +      } else if (index == 1) {
> +        downloadFolder.label = bundlePreferences.getString("downloadsFolderName");
> +        iconUrlSpec = fph.getURLSpecFromFile(await this._indexToFolder(1));
> +      } else {
> +        // 'Desktop'
> +        downloadFolder.label = bundlePreferences.getString("desktopFolderName");
> +        iconUrlSpec = fph.getURLSpecFromFile(await this._getDownloadsFolder("Desktop"));
> +      }

Can you explain why we need to do this? I'm confused about the fact that apparently, even if the user has selected to save downloads to cloud storage, we then add a bunch of code to display the 'right' folder in the non-selected (and therefore disabled) option that shows the otherwise selected download folder.

More generally, it feels like there's a lot of duplication with the existing code in this block - just nested inside the `if (folderlistPref.value == 3)`. If we really needed to do this, maybe rewrite this to something like:

```js
let folderIndex = folderListPref.value;
if (folderIndex == 3) {
  folderIndex = await this._folderToIndex(currentDirPref.value);
}
```

followed by the existing `if...else if...else` block but checking `folderIndex` instead of checking `folderListPref.value` directly. That avoids all the duplication. Then add a comment before the `if (folderIndex)` block that explains why we need to still show the right downloads thing even if the user has selected a cloud folder instead.

::: toolkit/components/cloudstorage/CloudStorage.jsm:205
(Diff revision 3)
> +   *
> +   * @return {Object}
> +   * Object with preferred provider metadata. Return null
> +   * if user has not selected a preferred provider.
> +   */
> +  getPreferredProviderMetaData() {

Nothing calls this now, so please remove it.

::: toolkit/components/cloudstorage/CloudStorage.jsm:622
(Diff revision 3)
> +   */
> +  async getProviderIfInUse() {
> +    // Check if consumer add-on is present and user has set preferred provider key
> +    // and a valid download path exist on user desktop
> +    if (this.isAPIEnabled && this.preferredProviderKey && await this.getDownloadFolder()) {
> +      let provider = this.getPreferredProviderMetaData();

Just put the ternary inside `getPreferredProviderMetaData()` in here instead, and remove the method, given nobody else calls it.

::: toolkit/components/cloudstorage/CloudStorage.jsm:623
(Diff revision 3)
> +  async getProviderIfInUse() {
> +    // Check if consumer add-on is present and user has set preferred provider key
> +    // and a valid download path exist on user desktop
> +    if (this.isAPIEnabled && this.preferredProviderKey && await this.getDownloadFolder()) {
> +      let provider = this.getPreferredProviderMetaData();
> +      if (provider && provider.displayName) {

Is it actually possible to get a downloads folder but not have metadata for that provider?

::: toolkit/components/cloudstorage/CloudStorage.jsm:659
(Diff revision 3)
> +XPCOMUtils.defineLazyPreferenceGetter(CloudStorageInternal, "isAPIEnabled",
> +  CLOUD_SERVICES_PREF + "api.enabled", false, () => CloudStorage.init());

If we get turned off from the pref, shouldn't we be re-setting the download folder pref to a non-cloudstorage value?
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

Gijs' review here is sufficient
Attachment #8894729 - Flags: review?(jaws)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review173654

::: browser/components/preferences/in-content/main.js:691
(Diff revision 3)
> +      // Handle Cloud Storage pref and use value in currentDirPref to
> +      // display download folder label and icon
> +      let index = await this._folderToIndex(currentDirPref.value);
> +      if (index == 2) {
> +        downloadFolder.label = this._getDisplayNameOfFile(currentDirPref.value);
> +        iconUrlSpec = fph.getURLSpecFromFile(currentDirPref.value);
> +      } else if (index == 1) {
> +        downloadFolder.label = bundlePreferences.getString("downloadsFolderName");
> +        iconUrlSpec = fph.getURLSpecFromFile(await this._indexToFolder(1));
> +      } else {
> +        // 'Desktop'
> +        downloadFolder.label = bundlePreferences.getString("desktopFolderName");
> +        iconUrlSpec = fph.getURLSpecFromFile(await this._getDownloadsFolder("Desktop"));
> +      }

+1, will update patch with your refactor feedback. This is needed to display ‘right’ folder in filefield  on load of about:preference when folderListPref value is 3. Not explicitly handling it here will display filefield blank.

::: toolkit/components/cloudstorage/CloudStorage.jsm:205
(Diff revision 3)
> +   *
> +   * @return {Object}
> +   * Object with preferred provider metadata. Return null
> +   * if user has not selected a preferred provider.
> +   */
> +  getPreferredProviderMetaData() {

In future there could be a use case where we need a public method in API to retrieve selected cloud provider metadata without scanning (promisePromptInfo, getStorageProviders method in API initiate scan / check if providers exist on user desktop before returning metadata). 
I will clarify this in documentation.

::: toolkit/components/cloudstorage/CloudStorage.jsm:622
(Diff revision 3)
> +   */
> +  async getProviderIfInUse() {
> +    // Check if consumer add-on is present and user has set preferred provider key
> +    // and a valid download path exist on user desktop
> +    if (this.isAPIEnabled && this.preferredProviderKey && await this.getDownloadFolder()) {
> +      let provider = this.getPreferredProviderMetaData();

needed by getPreferredProviderMetaData inpublic module as explained above

::: toolkit/components/cloudstorage/CloudStorage.jsm:623
(Diff revision 3)
> +  async getProviderIfInUse() {
> +    // Check if consumer add-on is present and user has set preferred provider key
> +    // and a valid download path exist on user desktop
> +    if (this.isAPIEnabled && this.preferredProviderKey && await this.getDownloadFolder()) {
> +      let provider = this.getPreferredProviderMetaData();
> +      if (provider && provider.displayName) {

That's an edge case if cloud.services.storage.key pref is manually touched and entered wrong key, in that case getPreferredMetadata will return null.
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review173378

> Is it actually possible to get a downloads folder but not have metadata for that provider?

Previous comment about returning null provider was in context of getPreferredProviderMetaData, (line 623) you are right we will always have a valid provider , updated to check only for valid display name

> If we get turned off from the pref, shouldn't we be re-setting the download folder pref to a non-cloudstorage value?

Updated patch to reset folderList pref when API is disabled
Updated patch with feedback, please review. Thanks!

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acd4ae8f06fdf3ff6ffa24a114e6ff31b20d3ff
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review173968

::: browser/components/preferences/in-content/main.js:590
(Diff revision 4)
> +      downloadFolder.disabled = saveWhere.selectedIndex;
> +      chooseFolder.disabled = saveWhere.selectedIndex;

This is duplicating the code in readUseDownloadDir, which also takes into account whether the preference is locked (which this code doesn't).

Can we instead make the saveToClickTask thing only set the preference correctly, and otherwise just call `readUseDownloadDir` to update this stuff? If that doesn't work for some reason, this code should at least also check `preference.locked` when determining whether to set these elements to disabled.

::: toolkit/components/cloudstorage/CloudStorage.jsm:266
(Diff revision 4)
> +    let downloadDirPath = Services.prefs.getComplexValue("browser.download.dir",
> +                                                   Ci.nsIFile).path;

Nit: indentation here is wrong.

::: toolkit/components/cloudstorage/CloudStorage.jsm:294
(Diff revision 4)
> +    // Cloud Storage API should continue initialization and load providers metadata
> +    // only if a consumer add-on using API sets pref 'cloud.services.api.enabled' to true
> +    // If API is not enabled, check and reset cloud storage value in folderList pref.
> +    if (!this.isAPIEnabled) {
> +      this.resetFolderListPref().catch(() => {
> +        Cu.reportError("CloudStorage: Failed to reset browser.download.folderList pref");

This should append the original exception message.

::: toolkit/components/cloudstorage/CloudStorage.jsm:657
(Diff revision 4)
> +  async getProviderIfInUse() {
> +    // Check if consumer add-on is present and user has set preferred provider key
> +    // and a valid download path exist on user desktop
> +    if (this.isAPIEnabled && this.preferredProviderKey && await this.getDownloadFolder()) {
> +      let provider = this.getPreferredProviderMetaData();
> +      return provider.displayName ? provider.displayName : null;

Nit: provider.displayName || null;
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review174156

::: browser/components/preferences/in-content/main.js:590
(Diff revision 4)
> +      downloadFolder.disabled = saveWhere.selectedIndex;
> +      chooseFolder.disabled = saveWhere.selectedIndex;

readUseDownloadDir is invoked when value in useDownloadDir pref changes. Since SaveTo and SaveToCloud has both useDownloadDir as true , we need to handle it in saveToClickTask when switching between these two options. 
Updated code to check for useDownloadDirPref.locked and invoke this check to disable filefield and button only when useDownloadDirPref is true
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

https://reviewboard.mozilla.org/r/165896/#review174230

With this nit, r=me

::: toolkit/components/cloudstorage/CloudStorage.jsm:657
(Diff revisions 4 - 5)
>    async getProviderIfInUse() {
>      // Check if consumer add-on is present and user has set preferred provider key
>      // and a valid download path exist on user desktop
>      if (this.isAPIEnabled && this.preferredProviderKey && await this.getDownloadFolder()) {
>        let provider = this.getPreferredProviderMetaData();
> -      return provider.displayName ? provider.displayName : null;
> +      return (provider.displayName || null) ? provider.displayName : null;

No, my point was, this line can just be:

```js
return provider.displayName || null;
```

which behaves the same way as:

```js
return provider.displayName ? provider.displayName : null;
```

but is shorter.
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #33)
> Comment on attachment 8894729 [details]
> Bug 1387153 - Preferences UI to support cloud storage download
> 
> https://reviewboard.mozilla.org/r/165896/#review174230
> 
> With this nit, r=me
> 
> ::: toolkit/components/cloudstorage/CloudStorage.jsm:657
> (Diff revisions 4 - 5)
> No, my point was, this line can just be:
> 
> ```js
> return provider.displayName || null;
> ```
> 
> which behaves the same way as:
> 
> ```js
> return provider.displayName ? provider.displayName : null;
> ```
> 
> but is shorter.

my bad, it was done without thinking, thanks for catching it!
Attachment #8895181 - Attachment is obsolete: true
Attachment #8894724 - Attachment description: Preferences UI with cloud storage screenshot → Preferences UI with cloud storage option selected
Hi Ryan
Preferences UI display order is fixed, see attached screenshot. NI you to check if it's ok to carry r+ from Bug 1381066. Please suggest. Thanks!
Flags: needinfo?(rfeeley)
Looks great!
Flags: needinfo?(rfeeley)
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c665e5e43e4
Preferences UI to support cloud storage download r=Gijs
https://hg.mozilla.org/mozilla-central/rev/9c665e5e43e4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1391486
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1357171
[User impact if declined]:
User set targeted by planned cloud storage shield study for 56 will not be able to see cloud storage option in  preference UI

[Is this code covered by automated tests?]:Yes
[Has the fix been verified in Nightly?]:Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
Pr-requisite
- Desktop with cloud storage provider client such as Dropbox

Steps to test preference UI by manually creating pref and Downloads folder

1. Open about:config and enable Cloud Storage API by setting pref ‘cloud.services.api.enabled’ to true
2. Set pref 'cloud.services.storage.key' to provider key such as ’Dropbox’
3. Ensure you have a valid download path for cloud storage downloads by manually creating folder ‘Downloads’ in provider folder e.g. ~/Dropbox/Downloads
4. Open about:preferences it should show radio button ‘Save to <provider name>' under Downloads

[List of other uplifts needed for the feature/fix]:
Bug 1391486 . In the order first Bug 1387153 followed by Bug 1391486 fix

[Is the change risky?]:No
[Why is the change risky/not risky?]:
This change is hidden behind prefs.  ‘cloud.services.api.enabled’ should be true to initialize API and ‘cloud.services.storage.key’ should be set to provider key to view cloud storage option under Downloads in about:preferences 

[String changes made/needed]: None
Attachment #8894729 - Flags: approval-mozilla-beta?
Comment on attachment 8894729 [details]
Bug 1387153 - Preferences UI to support cloud storage download

UI work supporting Cloud Storage API testing for 56 beta.
Attachment #8894729 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Are we OK with touching .properties files in beta? https://hg.mozilla.org/releases/mozilla-beta/rev/95e016111eb8#l6.3
(In reply to Florian Quèze [:florian] [:flo] from comment #45)
> Are we OK with touching .properties files in beta?
> https://hg.mozilla.org/releases/mozilla-beta/rev/95e016111eb8#l6.3

Definitely not without good reasons :-\

> [String changes made/needed]: None
(In reply to Francesco Lodolo [:flod] from comment #46)
> (In reply to Florian Quèze [:florian] [:flo] from comment #45)
> > Are we OK with touching .properties files in beta?
> > https://hg.mozilla.org/releases/mozilla-beta/rev/95e016111eb8#l6.3
> 
> Definitely not without good reasons :-\
> 
> > [String changes made/needed]: None
> 
> …

Never mind, this file is not exposed to localization.
Flags: qe-verify+
I've tested on Windows 7 x64, Ubuntu 14.04 x64 and macOS 10.12 using latest Nightly 57.0a1 (2017-09-13) and Firefox 56 Beta 12: 
- Dropbox: I've created ‘cloud.services.api.enabled’ (true) and 'cloud.services.storage.key' (Dropbox) prefs and  the "Downloads" folder in provider folder -> I opened the "about:preferences" page and the "Save to Dropbox" radio button is displayed under Downloads

- Google Drive: I've created ‘cloud.services.api.enabled’ (true) and 'cloud.services.storage.key' (Google Drive) prefs and  the "Downloads" folder in provider folder -> I opened the "about:preferences" page and the "Save to Google Drive" radio button is NOT displayed under Downloads -> any thoughts here?
Flags: needinfo?(pdahiya)
(In reply to Camelia Badau, QA [:cbadau] from comment #48)
> I've tested on Windows 7 x64, Ubuntu 14.04 x64 and macOS 10.12 using latest
> Nightly 57.0a1 (2017-09-13) and Firefox 56 Beta 12: 
> - Dropbox: I've created ‘cloud.services.api.enabled’ (true) and
> 'cloud.services.storage.key' (Dropbox) prefs and  the "Downloads" folder in
> provider folder -> I opened the "about:preferences" page and the "Save to
> Dropbox" radio button is displayed under Downloads
> 
> - Google Drive: I've created ‘cloud.services.api.enabled’ (true) and
> 'cloud.services.storage.key' (Google Drive) prefs and  the "Downloads"
> folder in provider folder -> I opened the "about:preferences" page and the
> "Save to Google Drive" radio button is NOT displayed under Downloads -> any
> thoughts here?

'cloud.services.storage.key' pref value should be provider key as defined in providers.json 

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/cloudstorage/content/providers.json

For Google Drive, please set 'cloud.services.storage.key' pref value to 'GDrive'. Thanks
Flags: needinfo?(pdahiya)
Verified fixed with Google Drive on Windows 7 x64 using latest Nightly 57.0a1 (2017-09-25) and Firefox 56 RC (build4).
Status: RESOLVED → VERIFIED
Attachment #8894729 - Flags: review?(rfeeley) → review+
You need to log in before you can comment on or make changes to this bug.