Once user confirms to remove selected site data listed n Settings of Site Data, the following items should be removed for all sites: - persistent permission - cookies - HTTP Cache - appcache - IndexedDB - localStorage

(In reply to Fischer [:Fischer] from comment #0) > Once user confirms to remove selected site data listed n Settings of Site > Data, the following items should be removed for all sites: Correct the typo: the following items should be removed for all sites: - persistent permission - cookies - HTTP Cache - appcache - IndexedDB > - localStorage

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, Hi Jaws, This patch implements the function of "Remove Selected" button [1]. There are 2 cases: Case 1: Select all all sites to remove. See [2]. Case 2: Select partial sites to remove. See [3]. 2 test cases are added as well. Try is at [4]. Thanks. [1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637924 [2] attachment 8821980 [details]: select_and_remove_all_sites.png [3] attachment 8821981 [details]: select_and_remove_partial_sites.png [4] TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59bda38a7c28a6319818eecd53dd4e7a214bec92

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, Hi Jaws, Sorry for requesting the review twice. Forgot to update the mozreview on the 1st review request. ----------------------------------------------------------- This patch implements the function of "Remove Selected" button [1]. There are 2 cases: Case 1: Select all all sites to remove. See [2]. Case 2: Select partial sites to remove. See [3]. 2 test cases are added as well. Try is at [4]. Thanks. [1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637924 [2] attachment 8821980 [details]: select_and_remove_all_sites.png [3] attachment 8821981 [details]: select_and_remove_partial_sites.png [4] TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59bda38a7c28a6319818eecd53dd4e7a214bec92

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, https://reviewboard.mozilla.org/r/100970/#review103544 ::: browser/components/preferences/SiteDataManager.jsm:253 (Diff revision 2) > + if (!userContextId) { > + // Default identity is public. > + return false; > + } > + return !ContextualIdentityService.getIdentityFromId(userContextId).public; This can be rewritten as: > return userContextId && > !ContextualIdentityService.getIdentityFromId(userContextId).public; ::: browser/components/preferences/siteDataRemoveSelected.css:1 (Diff revision 2) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#SiteDataRemoveSelectedDialog { I believe it was a mistake to allow siteDataSettings.css to be added to /browser/components/preferences and it would be a mistake to add this file here also. Why do these two dialogs need special CSS files? The other dialogs like the Connection Settings dialog doesn't have a CSS file location in this directory. Can you look deeper at this? ::: browser/components/preferences/siteDataRemoveSelected.js:24 (Diff revision 2) > + let visibleItems = []; > + let itemsTable = new Map(); > + for (let [ baseDomain, hosts ] of hostsTable) { > + // In the beginning, only display base domains in the topmost level. > + visibleItems.push({ > + lv: 0, Please use 'level' instead of the 'lv' abbreviation. ::: browser/components/preferences/siteDataRemoveSelected.xul:32 (Diff revision 2) > + <!-- Only show this label on OS X because of no dialog title --> > + <label id="removing-label" > +#ifndef XP_MACOSX > + hidden="true" > +#endif > + >&removingDialog.title;</label> I don't see anything like this for other dialogs within the preferences. Why does this dialog need to act differently? ::: browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd:17 (Diff revision 2) > +<!ENTITY save.label "Save Changes"> > +<!ENTITY save.accesskey "a"> > +<!ENTITY cancel.label "Cancel"> > +<!ENTITY cancel.accesskey "C"> > +<!ENTITY removingDialog.title "Removing site data"> > +<!ENTITY removingDialog.description "Removing site data will also remove cookies. This may log you out of websites and remove offline web content. Are you sure you wnat to make the changes?"> spelling error with 'wnat' The language here is a bit confusing because it implies that removing cookies may remove offline web content. Is that the intention of the language? Is offline web content being removed not an explicit goal of removing site data?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8) Thanks for comments. Please see my replies and some questions below. > > + return !ContextualIdentityService.getIdentityFromId(userContextId).public; > > This can be rewritten as: > return userContextId && !ContextualIdentityService.getIdentityFromId(userContextId).public; > Will update, thanks. > ::: browser/components/preferences/siteDataRemoveSelected.css:1 > > +#SiteDataRemoveSelectedDialog { > > I believe it was a mistake to allow siteDataSettings.css to be added to > /browser/components/preferences and it would be a mistake to add this file > here also. > > Why do these two dialogs need special CSS files? The other dialogs like the > Connection Settings dialog doesn't have a CSS file location in this > directory. > > Can you look deeper at this? > OK, we can put the rules in siteDataRemoveSelected.css into siteDataSettings.css since only siteDataSettings.js is calling siteDataRemoveSelected-related stuff. About the file location, originally it was thought to put under "browser/components/preferences" like handlers.css [1]. And handlers.css is only for Application panel's xul binding. So for our case here, should we put the file to "browser/themes/shared/incontentprefs" ? [1] browser/components/preferences/handlers.css > ::: browser/components/preferences/siteDataRemoveSelected.js:24 > > + lv: 0, > > Please use 'level' instead of the 'lv' abbreviation. > Will update, thanks. > ::: browser/components/preferences/siteDataRemoveSelected.xul:32 > (Diff revision 2) > > + <!-- Only show this label on OS X because of no dialog title --> > > + <label id="removing-label" > > +#ifndef XP_MACOSX > > + hidden="true" > > +#endif > > + >&removingDialog.title;</label> > > I don't see anything like this for other dialogs within the preferences. Why > does this dialog need to act differently? > Because modal dialog title is present on Linux but not on Mac, add this label as title for Mac. See [1] for the details. This extra handling is to follow the standard "prompt" API, there is the similar handling in commonDialog.xul [2]. But if this handling was trivial and not necessary, we could remove it to reduce the complexity. (In fact, I am fine with both, quite trivial to me). How do you think? [1] attachment 8824879 [details]: modal_dialog_title_difference.png [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/content/commonDialog.xul#73 > ::: browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd:17 > > +<!ENTITY removingDialog.description "Removing site data will also remove cookies. This may log you out of websites and remove offline web content. Are you sure you wnat to make the changes?"> > > spelling error with 'wnat' > Sorry, will update. Thanks for spotting this. > The language here is a bit confusing because it implies that removing > cookies may remove offline web content. Is that the intention of the > language? Is offline web content being removed not an explicit goal of > removing site data? Thanks for pointing out this. Have passed it to the UX. They ware reviewing these strings with the copy writer.

(In reply to Fischer [:Fischer] from comment #10) > > ::: browser/components/preferences/siteDataRemoveSelected.css:1 > > > +#SiteDataRemoveSelectedDialog { > > > > I believe it was a mistake to allow siteDataSettings.css to be added to > > /browser/components/preferences and it would be a mistake to add this file > > here also. > > > > Why do these two dialogs need special CSS files? The other dialogs like the > > Connection Settings dialog doesn't have a CSS file location in this > > directory. > > > > Can you look deeper at this? > > > OK, we can put the rules in siteDataRemoveSelected.css into > siteDataSettings.css since only siteDataSettings.js is calling > siteDataRemoveSelected-related stuff. > About the file location, originally it was thought to put under > "browser/components/preferences" like handlers.css [1]. > And handlers.css is only for Application panel's xul binding. So for our > case here, should we put the file to "browser/themes/shared/incontentprefs" ? > > [1] browser/components/preferences/handlers.css Rules that are behavior related or connect XBL can be in /browser/components/preferences/, but rules that are about styling should be in /browser/themes/shared/incontentprefs. > > ::: browser/components/preferences/siteDataRemoveSelected.xul:32 > > (Diff revision 2) > > > + <!-- Only show this label on OS X because of no dialog title --> > > > + <label id="removing-label" > > > +#ifndef XP_MACOSX > > > + hidden="true" > > > +#endif > > > + >&removingDialog.title;</label> > > > > I don't see anything like this for other dialogs within the preferences. Why > > does this dialog need to act differently? > > > Because modal dialog title is present on Linux but not on Mac, add this > label as title for Mac. See [1] for the details. > This extra handling is to follow the standard "prompt" API, there is the > similar handling in commonDialog.xul [2]. > But if this handling was trivial and not necessary, we could remove it to > reduce the complexity. (In fact, I am fine with both, quite trivial to me). > How do you think? It's fine to keep it. > > The language here is a bit confusing because it implies that removing > > cookies may remove offline web content. Is that the intention of the > > language? Is offline web content being removed not an explicit goal of > > removing site data? > Thanks for pointing out this. Have passed it to the UX. They ware reviewing > these strings with the copy writer. Any update from the copy writer on this?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11) > > > The language here is a bit confusing because it implies that removing > > > cookies may remove offline web content. Is that the intention of the > > > language? Is offline web content being removed not an explicit goal of > > > removing site data? > > Thanks for pointing out this. Have passed it to the UX. They ware reviewing > > these strings with the copy writer. > > Any update from the copy writer on this? As far as I know, they are thinking new sentences such as "Removing site data will also remove cookies and offline web content. This may log you out of websites. Are you sure you want to make the changes?". So as to deliver message that removal of cookies and removal of offline web content are independent operations and gonna happen. Avoid misleading people to think "Remove cookies may remove offline web content btw". The review is still under process. I plan to revisit and update the strings then.

(In reply to Fischer [:Fischer] from comment #13) > As far as I know, they are thinking new sentences such as "Removing site > data will also remove cookies and offline web content. This may log you out > of websites. Are you sure you want to make the changes?". Okay, I like the direction of this text better.

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, https://reviewboard.mozilla.org/r/100970/#review106710 ::: browser/themes/shared/incontentprefs/siteDataSettings.css:13 (Diff revision 4) > +#sitesList { > + min-height: 20em; > +} > + > +#sitesList > richlistitem { > + -moz-binding: url("chrome://browser/content/preferences/siteListItem.xml#siteListItem"); -moz-binding rules shouldn't be defined in a /themes/ subdirectory. These are part of content behavior and would break if the theme changed. ::: browser/themes/shared/incontentprefs/siteDataSettings.css:25 (Diff revision 4) > +/** > + * Confirmation dialog of removing sites selected > + */ > +#SiteDataRemoveSelectedDialog { > + padding: 16px; > + -moz-binding: url("chrome://global/content/bindings/dialog.xml#dialog"); same for this line too.

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, https://reviewboard.mozilla.org/r/100970/#review106710 > -moz-binding rules shouldn't be defined in a /themes/ subdirectory. These are part of content behavior and would break if the theme changed. Have moved siteDataSettings.css from browser/themes/shared/incontentprefs to browser/components/preferences. I am not sure if it's good to split pure css rules and moz binding rules into two places. Maybe sort of not good for maintainance. But if split is a better convention. Please let me know, thanks.

(In reply to Fischer [:Fischer] from comment #17) > Comment on attachment 8821982 [details] > Bug 1312377 - Remove selected site data in Settings of Site Data > > https://reviewboard.mozilla.org/r/100970/#review106710 > > > -moz-binding rules shouldn't be defined in a /themes/ subdirectory. These are part of content behavior and would break if the theme changed. > > Have moved siteDataSettings.css from browser/themes/shared/incontentprefs to > browser/components/preferences. > I am not sure if it's good to split pure css rules and moz binding rules > into two places. Maybe sort of not good for maintainance. > But if split is a better convention. Please let me know, thanks. This is the convention that we use for other parts of the browser. See /browser/base/content/browser.css versus /browser/themes/windows/browser.css for example.

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, https://reviewboard.mozilla.org/r/100970/#review107540 ::: browser/components/preferences/siteDataSettings.css:28 (Diff revision 5) > +#contentContainer { > + font-size: 1.2em; > + margin-bottom: 10px; > +} I thought these types of rules would be moved to the /themes directory? Do you plan on uploading a new version of this patch?

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, https://reviewboard.mozilla.org/r/100970/#review107540 > I thought these types of rules would be moved to the /themes directory? Do you plan on uploading a new version of this patch? OK, split the moz binding rules into browser/components/preferences/siteDataSettings.css and the pure css style rules into browser/themes/shared/incontentprefs/siteDataSettings.css , thank you.

Comment on attachment 8821982 [details] Bug 1312377 - Remove selected site data in Settings of Site Data, https://reviewboard.mozilla.org/r/100970/#review107996 ::: browser/components/preferences/siteDataSettings.js:32 (Diff revision 7) > _list: null, > > init() { > + function setEventListener(id, eventType, callback) { > + document.getElementById(id) > + .addEventListener(eventType, callback.bind(gSiteDataSettings)); Instead of function.bind you can use arrow function syntax which will bind the function to the current 'this' instance. > document.getElementById(id) > .addEventListener(eventType, () => callback); ::: browser/components/preferences/siteDataSettings.js:111 (Diff revision 7) > + > + saveChanges() { > + let allowed = true; > + > + // Confirm user really wants to remove site data starts > + let removeds = []; please use "removals" instead of "removeds" ::: browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd:16 (Diff revision 7) > +<!ENTITY removeSelected.accesskey "r"> > +<!ENTITY save.label "Save Changes"> > +<!ENTITY save.accesskey "a"> > +<!ENTITY cancel.label "Cancel"> > +<!ENTITY cancel.accesskey "C"> > +<!ENTITY removingDialog.title "Removing site data"> The title for the dialog should be in Capital Case. So this should be "Removing Site Data"

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24) > Comment on attachment 8821982 [details] > Bug 1312377 - Remove selected site data in Settings of Site Data, > > https://reviewboard.mozilla.org/r/100970/#review107996 > > ::: browser/components/preferences/siteDataSettings.js:32 > (Diff revision 7) > > init() { > > + function setEventListener(id, eventType, callback) { > > + document.getElementById(id) > > + .addEventListener(eventType, callback.bind(gSiteDataSettings)); > > Instead of function.bind you can use arrow function syntax which will bind > the function to the current 'this' instance. > > > document.getElementById(id) > > .addEventListener(eventType, () => callback); The case here is a bit different. If calling `document.getElementById(id).addEventListener(eventType, () => callback());` then the callback function wouldn't be invoked with "this" of gSiteDataSettings, even though "this" is gSiteDataSettings inside that arrow function. So for the event such as, `setEventListener("save", "command", this.saveChanges);` the "saveChanges" wouldn't be executed with the proper "this". That is the reason binding callback with gSiteDataSettings. > ::: browser/components/preferences/siteDataSettings.js:111 > > + let removeds = []; > > please use "removals" instead of "removeds" Thanks, will update. > ::: browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd:16 > > +<!ENTITY removingDialog.title "Removing site data"> > > The title for the dialog should be in Capital Case. So this should be > "Removing Site Data" Thanks, will update.

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20cb1245a46b58f5c22acf5cae318861ccde6813

Autoland can't push this until all pending issues are marked as resolved in MozReview.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd41ffed554b Remove selected site data in Settings of Site Data, r=jaws