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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Preferences
VERIFIED FIXED
2 months ago
a month ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox56 verified, firefox57 verified)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Updated

2 months ago
Assignee: nobody → pdahiya
(Assignee)

Updated

2 months ago
Blocks: 1387153
(Assignee)

Comment 2

2 months ago
Created attachment 8898557 [details] [diff] [review]
WIP patch to handle unspecified browser.download.dir
(Assignee)

Updated

2 months ago
Blocks: 1357160
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8898557 - Attachment is obsolete: true
(Assignee)

Comment 4

2 months ago
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 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!
Attachment #8898636 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 6

2 months ago
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/549b756fc868
Cloud Storage - Handle browser.download.dir unspecified in new profile r=Gijs
(Assignee)

Comment 7

2 months ago
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
Attachment #8898636 - Flags: approval-mozilla-beta?

Comment 8

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/549b756fc868
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
status-firefox56: --- → affected
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.
Attachment #8898636 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9f0bb349eced
status-firefox56: affected → fixed
Flags: qe-verify+
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?
Flags: needinfo?(pdahiya)
(Assignee)

Comment 12

a month ago
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
Flags: needinfo?(pdahiya)
Based on comment 12 and comment 11, I will mark this as verified fixed, but file a separate bug for the glitch mentioned above.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: fixed → verified
Flags: qe-verify+
Blocks: 1400154
You need to log in before you can comment on or make changes to this bug.