1391486 - Preference UI cloud storage option should handle browser.download.dir pref unspecified in new profile.

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Punam Dahiya [:pdahiya]]

Steps to replicate:
Pr-requisite
- Desktop with cloud storage provider client such as Dropbox

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 pref 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 and ‘Save to <system default downloads>’ first option as selected
5. Open about:config browser.download.folderList is set to 1 and browser.download.dir is unspecified
6. Selecting ‘Save to <provider name>' changes Download FileField label to ‘Desktop’

Expected:

Download FileField label should stay ‘Downloads’

Comment 1

[Punam Dahiya [:pdahiya]]

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content-new/main.js#2487
From documentation above, we incorrectly assumed if browser.download.dir doesn’t exist or unspecified, browser.download.folderList is 0.

It works till third option CloudStorage changes folderList value to 3 without touching browser.download.dir.

In new profile, browser.download.dir is not specified and browser.download.folderList value is 1, we should handle that in preference UI with cloud storage option available.

Comment 2

[Punam Dahiya [:pdahiya]]

Attachment: WIP patch to handle unspecified browser.download.dir

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1391486 - Cloud Storage - Handle browser.download.dir unspecified in new profile

Review commit: https://reviewboard.mozilla.org/r/170016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170016/

Comment 4

[Punam Dahiya [:pdahiya]]

Hi Gijs
For new profile, starting default value for browser.download.folderList is 1, any subsequent change in folderList value sets browser.download.dir. 
With cloud storage option available, its possible to switch to folderList 3 without setting browser.download.dir causing this bug. Its a miss in Bug 1387153 and handling this in attached patch, please review. Thanks

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8898636 [details]
Bug 1391486 - Cloud Storage - Handle browser.download.dir unspecified in new profile

https://reviewboard.mozilla.org/r/170016/#review175214

Looks good, thanks for catching this!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/549b756fc868
Cloud Storage - Handle browser.download.dir unspecified in new profile r=Gijs

Comment 7

[Punam Dahiya [:pdahiya]]

Comment on attachment 8898636 [details]
Bug 1391486 - Cloud Storage - Handle browser.download.dir unspecified in new profile

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1387153
[User impact if declined]:
For new profiles, selecting cloud storage option under Download in preference UI will change Download File field folder label to ‘Desktop’

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

Steps:

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 and ‘Save to <system default downloads>’ first option as selected
5. Open about:config browser.download.folderList is set to 1 and browser.download.dir is unspecified
6. Selecting ‘Save to <provider name>' should keep Download FileField label to ‘Downloads’


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

[Is the change risky?]:No
[Why is the change risky/not risky?]:
Change hidden behind prefs and needed for shield study planned for 56 for a limited set of users.
[String changes made/needed]:None

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/549b756fc868

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8898636 [details]
Bug 1391486 - Cloud Storage - Handle browser.download.dir unspecified in new profile

OK for uplift to beta. please land the work in bug 1387153 first.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f0bb349eced

Comment 11

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I can confirm that both 56.0b11 build1 (20170911193316) and 57.0a1 (2017-09-13) are fixed on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6. But there seems to be a side effect, reproducible on all above platforms and both mentioned builds: when switching between the third ("Always ask you where to save files") and the second option ("Save files to <provider name>") from the "Downloads" section, the first option ("Save files to <system default downloads>")gets selected for a very short time instead of the second one (see the screencast https://goo.gl/EbFQA2). This is reproducible once in a few attempts. 
Any thoughts about this?

Comment 12

[Punam Dahiya [:pdahiya]]

This fraction of seconds flicker happens when ‘Save to <provider>’ cloud storage option is selected by user under Downloads.  

Inside readCloudStorage, if browser.download.folderList pref satisfies value 3, we disable downloadFolder element
that was just enabled in readUseDownloadDir. 

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.js#2280

As noted in #Comment 7 above, ‘Save to <provider>’ option is hidden behind prefs and needed for
shield study planned for 56  for a limited set of users (Bug 1399198).  Since shield study is not testing preferences UI,
my understanding is its a low risk bug. Thanks

Comment 13

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Based on comment 12 and comment 11, I will mark this as verified fixed, but file a separate bug for the glitch mentioned above.