While loading data, Site Data section should - display "loading message" to user - disable all buttons until data is loaded.

Comment on attachment 8855196 [details] Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data, Hi Gijs, This patch would display loading message and disable buttons while updaing site data, see the new specs [1]. Some items to explain: - Because of the Preferences Reorg, the Site Data part has been moved to the Privacy. So you could see: - updates in in-content/privacy.js and preferences/preferences.properties for the new Preferences - updates in in-content-old/advanced.js and preferences-old/preferences.properties for the old Preferences. - Because of the bug 1349051, the browser_advanced_siteData.js tests has been split into the browser_siteData.js and the browser_siteData2.js two tests. You could see the updates of the new test case in - in-content/tests/browser_siteData.js and in-content/tests/head.js for the new Preferences - in-content-old/tests/browser_advanced_siteData.js for the old Preferences. [1] https://mozilla.invisionapp.com/share/FGB2K7EKC#/screens [2] TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=add8589b3659273d8803dcf65cd272cdae263f40 Thanks

Comment on attachment 8855196 [details] Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data, https://reviewboard.mozilla.org/r/126830/#review129900 ::: browser/components/preferences/in-content-old/advanced.js:57 (Diff revision 1) > Services.obs.addObserver(this, "sitedatamanager:sites-updated", false); > + Services.obs.addObserver(this, "sitedatamanager:updating-sites", false); Can you file a followup to use a different mechanism for the storage manager to communicate with the preferences page? Sending a process-wide observer notification isn't the most efficient anyway, and if there were multiple instances they'd be getting each other's notifications. ::: browser/components/preferences/in-content-old/advanced.js:378 (Diff revision 1) > + clearButton.disabled = false; > + settingsButton.disabled = false; This is duplicated with `disableSiteData()`. Can you make the `disableSiteData()` function be a toggle function (`toggleSiteData(shouldShow)`) that assigns the (opposite of the?) value you pass it to the `disabled` properties, and factor out the label setting in it to the callsite? That'll reduce the code duplication and make this easier to read. Actually, for the label setting, you can have another utility method called `updateTotalDataSizeLabel`, and if called with -1 we can set it to this `loading` string, otherwise use the code that's currently in the `.then` here, lower down, to set the correct string. That would remove that duplication, too. ::: browser/components/preferences/in-content-old/advanced.js:810 (Diff revision 1) > showSecurityDevices() { > gSubDialog.open("chrome://pippki/content/device_manager.xul"); > }, > > observe(aSubject, aTopic, aData) { > if (AppConstants.MOZ_UPDATER) { Uhhh... this is going to break on updater-less builds (e.g. linux distro builds). We should move our stuff out of this if statement. Really, realistically, we should just get rid of the if statement. ::: browser/components/preferences/in-content-old/tests/browser_advanced_siteData.js:245 (Diff revision 1) > + let updatingPromise = promiseUpdatingSites(); > + Services.obs.notifyObservers(null, "sitedatamanager:updating-sites", null); > + yield updatingPromise; notifyObservers is sync, so you shouldn't need to yield here. ::: browser/components/preferences/in-content-old/tests/browser_advanced_siteData.js:249 (Diff revision 1) > + > + let updatingPromise = promiseUpdatingSites(); > + Services.obs.notifyObservers(null, "sitedatamanager:updating-sites", null); > + yield updatingPromise; > + is(clearBtn.disabled, true, "Should disable clear button while updating sites"); > + is(settingsButton.disabled, true, "Should disable settings button while updating sites"); Both here and in the other test I would expect an assertion that the label has been updated, too. ::: browser/components/preferences/in-content-old/tests/browser_advanced_siteData.js:251 (Diff revision 1) > + updatedPromise = promiseSitesUpdated(); > + Services.obs.notifyObservers(null, "sitedatamanager:sites-updated", null); > + yield updatedPromise; Same here, and same stuff in the other test, of course.

Comment on attachment 8855196 [details] Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data, (In reply to :Gijs from comment #3) > Comment on attachment 8855196 [details] > Bug 1348252 - Disable buttons and display loading message in Site Data > section while loading data > > https://reviewboard.mozilla.org/r/126830/#review129900 > > ::: browser/components/preferences/in-content-old/advanced.js:57 (Diff revision 1) > > + Services.obs.addObserver(this, "sitedatamanager:updating-sites", false); > > Can you file a followup to use a different mechanism for the storage manager > to communicate with the preferences page? Sending a process-wide observer > notification isn't the most efficient anyway, and if there were multiple > instances they'd be getting each other's notifications. OK, filed: Bug 1354170 - Should change the communication mechanism between SiteDataManager and Preferences page > > ::: browser/components/preferences/in-content-old/advanced.js:378 (Diff revision 1) > > + clearButton.disabled = false; > > + settingsButton.disabled = false; > > This is duplicated with `disableSiteData()`. > > Can you make the `disableSiteData()` function be a toggle function > (`toggleSiteData(shouldShow)`) that assigns the (opposite of the?) value you > pass it to the `disabled` properties, and factor out the label setting in it > to the callsite? That'll reduce the code duplication and make this easier to > read. > > Actually, for the label setting, you can have another utility method called > `updateTotalDataSizeLabel`, and if called with -1 we can set it to this > `loading` string, otherwise use the code that's currently in the `.then` > here, lower down, to set the correct string. That would remove that > duplication, too. > Thanks, updated. > ::: browser/components/preferences/in-content-old/advanced.js:810 (Diff revision 1) > > showSecurityDevices() { > > gSubDialog.open("chrome://pippki/content/device_manager.xul"); > > }, > > > > observe(aSubject, aTopic, aData) { > > if (AppConstants.MOZ_UPDATER) { > > Uhhh... this is going to break on updater-less builds (e.g. linux distro > builds). We should move our stuff out of this if statement. Really, > realistically, we should just get rid of the if statement. > Thanks for pointing out this. Removed this redundant `AppConstants.MOZ_UPDATER` condition here, since `updateReadPrefs` inside already got `AppConstants.MOZ_UPDATER` condition. > ::: > browser/components/preferences/in-content-old/tests/ > browser_advanced_siteData.js:245 > (Diff revision 1) > > + let updatingPromise = promiseUpdatingSites(); > > + Services.obs.notifyObservers(null, "sitedatamanager:updating-sites", null); > > + yield updatingPromise; > > notifyObservers is sync, so you shouldn't need to yield here. > Thanks, removed. > ::: > browser/components/preferences/in-content-old/tests/ > browser_advanced_siteData.js:249 > (Diff revision 1) > > + > > + let updatingPromise = promiseUpdatingSites(); > > + Services.obs.notifyObservers(null, "sitedatamanager:updating-sites", null); > > + yield updatingPromise; > > + is(clearBtn.disabled, true, "Should disable clear button while updating sites"); > > + is(settingsButton.disabled, true, "Should disable settings button while updating sites"); > > Both here and in the other test I would expect an assertion that the label has been updated, too. > Added assertions to check the label textContent, thanks. > ::: > browser/components/preferences/in-content-old/tests/ > browser_advanced_siteData.js:251 > (Diff revision 1) > > + updatedPromise = promiseSitesUpdated(); > > + Services.obs.notifyObservers(null, "sitedatamanager:sites-updated", null); > > + yield updatedPromise; > > Same here, and same stuff in the other test, of course. Thanks updated.

Comment on attachment 8855196 [details] Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data, https://reviewboard.mozilla.org/r/126830/#review130408 ::: browser/components/preferences/in-content-old/advanced.js:368 (Diff revision 2) > - let size = DownloadUtils.convertByteUnits(usage); > + if (shouldShow) { > + clearButton.disabled = false; > + settingsButton.disabled = false; > + } else { > + clearButton.disabled = true; > + settingsButton.disabled = true; > + } You don't need to branch (ie no if statement) here, just use: ```js clearButton.disabled = !shouldShow; settingsButton.disabled = !shouldShow; ``` ::: browser/components/preferences/in-content-old/advanced.js:822 (Diff revision 2) > + break; > + > - case "sitedatamanager:sites-updated": > + case "sitedatamanager:sites-updated": > - this.updateTotalSiteDataSize(); > + this.toggleSiteData(true); > + SiteDataManager.getTotalUsage() > + .then(usage => this.updateTotalDataSizeLabel(DownloadUtils.convertByteUnits(usage))); convertByteUnits will produce a string, and inside updateTotalDataSizeLabel you're assuming it's an int. This conversion should live inside updateTotalDataSizeLabel. ::: browser/components/preferences/in-content/privacy.js:1188 (Diff revision 2) > - let size = DownloadUtils.convertByteUnits(usage); > + if (shouldShow) { > + clearButton.disabled = false; > + settingsButton.disabled = false; > + } else { > + clearButton.disabled = true; > + settingsButton.disabled = true; > + } Same thing here.

Comment on attachment 8855196 [details] Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data, (In reply to :Gijs from comment #6) > Comment on attachment 8855196 [details] > Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data > > https://reviewboard.mozilla.org/r/126830/#review130408 > > ::: browser/components/preferences/in-content-old/advanced.js:368 (Diff revision 2) > > - let size = DownloadUtils.convertByteUnits(usage); > > + if (shouldShow) { > > + clearButton.disabled = false; > > + settingsButton.disabled = false; > > + } else { > > + clearButton.disabled = true; > > + settingsButton.disabled = true; > > + } > > You don't need to branch (ie no if statement) here, just use: > > ```js > clearButton.disabled = !shouldShow; > settingsButton.disabled = !shouldShow; > ``` > Thanks updated. > ::: browser/components/preferences/in-content-old/advanced.js:822 (Diff revision 2) > > + SiteDataManager.getTotalUsage() > > + .then(usage => this.updateTotalDataSizeLabel(DownloadUtils.convertByteUnits(usage))); > > convertByteUnits will produce a string, and inside updateTotalDataSizeLabel > you're assuming it's an int. This conversion should live inside > updateTotalDataSizeLabel. > Thanks, that should be right. Updated. > ::: browser/components/preferences/in-content/privacy.js:1188 (Diff revision 2) > > - let size = DownloadUtils.convertByteUnits(usage); > > + if (shouldShow) { > > + clearButton.disabled = false; > > + settingsButton.disabled = false; > > + } else { > > + clearButton.disabled = true; > > + settingsButton.disabled = true; > > + } > > Same thing here. Thanks updated.

Comment on attachment 8855196 [details] Bug 1348252 - Disable buttons and display loading message in Site Data section while loading data, https://reviewboard.mozilla.org/r/126830/#review130580 ::: browser/components/preferences/in-content-old/advanced.js:818 (Diff revision 3) > + break; > + > - case "sitedatamanager:sites-updated": > + case "sitedatamanager:sites-updated": > - this.updateTotalSiteDataSize(); > + this.toggleSiteData(true); > + SiteDataManager.getTotalUsage() > + .then(usage => this.updateTotalDataSizeLabel(usage)); And now you can just use: .then(this.updateTotalDataSizeLabel.bind(this))

(In reply to :Gijs from comment #9) > Comment on attachment 8855196 [details] > Bug 1348252 - Disable buttons and display loading message in Site Data > section while loading data, > > https://reviewboard.mozilla.org/r/126830/#review130580 > > ::: browser/components/preferences/in-content-old/advanced.js:818 (Diff revision 3) > > + SiteDataManager.getTotalUsage() > > + .then(usage => this.updateTotalDataSizeLabel(usage)); > > And now you can just use: > > .then(this.updateTotalDataSizeLabel.bind(this)) OK, updated. TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5ac114b38df0e5758a2296214f89c143439108f

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4ca0985e0a2 Disable buttons and display loading message in Site Data section while loading data, r=Gijs