Preferences UI to support cloud storage download

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Preferences
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox56 verified, firefox57 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

11 months ago
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.
(Assignee)

Updated

11 months ago
Blocks: 1357160
(Assignee)

Comment 1

11 months ago
Follow up bug to 1381066 focusing on preferences UI fix for testing in en-US
(Assignee)

Updated

11 months ago
Assignee: nobody → pdahiya
(Assignee)

Comment 2

11 months ago
Created attachment 8894723 [details]
Preferences UI with cloud storage
(Assignee)

Comment 3

11 months ago
Created attachment 8894724 [details]
Preferences UI  with cloud storage option selected
Attachment #8894723 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 5

11 months ago
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!

Updated

10 months ago
Attachment #8894729 - Flags: review?(gijskruitbosch+bugs)

Comment 6

10 months ago
Jared's review is enough here. Jared, let me know if you don't have cycles and I can take it.
(Assignee)

Comment 7

10 months ago
Created attachment 8895181 [details] [diff] [review]
WIP patch to handle cloud storage preference strings using chrome url

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 8

10 months ago
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+

Updated

10 months ago
Attachment #8894729 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)

Comment 9

10 months ago
mozreview-review
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)
(Assignee)

Comment 10

10 months ago
mozreview-review
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'
(Assignee)

Comment 11

10 months ago
(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)

Comment 12

10 months ago
(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)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

10 months ago
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!
(Assignee)

Comment 15

10 months ago
> 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
(Assignee)

Comment 16

10 months ago
> 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)

Comment 17

10 months ago
(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 18

10 months ago
mozreview-review
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

Updated

10 months ago
Flags: needinfo?(florian)
(Assignee)

Comment 20

10 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 23

10 months ago
mozreview-review
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)
(Assignee)

Comment 25

10 months ago
mozreview-review
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.
(Assignee)

Comment 26

10 months ago
mozreview-review-reply
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
Comment hidden (mozreview-request)
(Assignee)

Comment 28

10 months ago
Updated patch with feedback, please review. Thanks!

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

Comment 29

10 months ago
mozreview-review
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)
(Assignee)

Comment 30

10 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 33

10 months ago
mozreview-review
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+
(Assignee)

Comment 34

10 months ago
(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!
(Assignee)

Comment 35

10 months ago
Created attachment 8897634 [details]
Preferences UI with cloud storage as option
Attachment #8895181 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8894724 - Attachment description: Preferences UI with cloud storage screenshot → Preferences UI with cloud storage option selected
(Assignee)

Comment 36

10 months ago
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)
Comment hidden (mozreview-request)

Comment 38

10 months ago
Looks great!
Flags: needinfo?(rfeeley)

Comment 40

10 months ago
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c665e5e43e4
Preferences UI to support cloud storage download r=Gijs

Comment 41

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c665e5e43e4
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

10 months ago
Depends on: 1391486
(Assignee)

Comment 42

10 months ago
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?
status-firefox56: --- → affected
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+

Comment 44

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/95e016111eb8
status-firefox56: affected → fixed
Flags: in-testsuite+
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)
(Assignee)

Comment 49

9 months ago
(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
status-firefox56: fixed → verified
status-firefox57: fixed → verified

Updated

8 months ago
Attachment #8894729 - Flags: review?(rfeeley) → review+
You need to log in before you can comment on or make changes to this bug.