Talked to antlam, and when "Browser History" is selected in the "Clear Private Data (on Quit)" AND the "Always restore tabs" pref is selected, we should clear the open tabs. Currently, tabs are not cleared when you clear browsing history (on Quit or manually).

To remove all confusion about this state, we could additionally remove the preference to "always/never restore tabs from last session" - the pref was defaulted to "always" in 48, and it looks like that's used at 2.8% constantly across every channel except aurora 48 (for which it decreased to 1.5%).

IMHO these two issues are orthogonal - I don't think this is a good move, and it's just as confusing as what we have now. It's entirely non-obvious that your current tabs are going to disappear when clearing history (moreover, clearing history from the "clear private data" setting doesn't close your tabs - why should this be different when quitting). The restore tabs pref is definitely confusing, but if we remove that, we should still have separate options to remove tabs, and to clear history. I.e. I think all of these scenarios are perfectly valid: - Clear all tabs but preserve history on "quit" - if you just want to finish a current browsing session. - Clear all tabs and clear history on "quit" - if you want to start completely afresh every time - Keep all tabs but clear history on "quit" - if you want to keep your currently active pages around, but discard all history - Keep tabs and history - that's the default If we change the behaviour here scenario 3 becomes impossible (unless you go into settings and clear history manually every time you're done browsing).

I think it makes complete sense that clearing your browser history should clear your tabs - if you set "clear history on quit" and then quit, and then come back, it *doesn't* make sense that your tabs would still be present because "clear history" implies a clean state. I agree that this is inconsistent with our behavior on manual close, so I think we should make that match this behavior. I think the scenarios may be cases that people might choose to have, but for simplicity's sake, I don't think we need to support #3. Antlam, let me know what you think - Andrzej brings up some good points about removing capability, and so if we should decide explicitly which cases we want to support.

(In reply to Andrzej Hunt :ahunt from comment #2) > IMHO these two issues are orthogonal - I don't think this is a good move, > and it's just as confusing as what we have now. > > It's entirely non-obvious that your current tabs are going to disappear when > clearing history (moreover, clearing history from the "clear private data" > setting doesn't close your tabs - why should this be different when > quitting). The restore tabs pref is definitely confusing, but if we remove > that, we should still have separate options to remove tabs, and to clear > history. So, let's take a step back here and focus on the root problem here. Currently, if you have "Restore tabs" turned on, your tabs will not close if you "clear private data". This is a problem because the most direct manifestation of a user's "private data" is front and center in the tabs tray. For users looking to be privacy-focused, "clearing" NEEDS to clear things. If this action is to be reliable and depended upon, it can feel broken if in fact nothing changes in the most visible parts of the UI. This is the core issue here. Our goal here is to simplify the mental model around clearing "things". One way (as you're suggesting), is to add an option to clear "Open tabs" in addition to this long list we're currently providing. Another way, is to consider that our users might not have this level of detail/separation in their minds when looking to clear their personal and private data from an application. If my "history" is visually represented in a tab, and in a list view. Clearing my "history" should clear those representations of it. With that, how hard is it to add "Open tabs" to this list Chenxia or Andrzej?

(In reply to Chenxia Liu [:liuche] from comment #1) > Created attachment 8756470 [details] > Screenshot: "Restore tabs" pref usage > > To remove all confusion about this state, we could additionally remove the > preference to "always/never restore tabs from last session" - the pref was > defaulted to "always" in 48, and it looks like that's used at 2.8% > constantly across every channel except aurora 48 (for which it decreased to > 1.5%). Can you share a link to the query you're using to generate this data? What is this percentage value you're referring to? Percentage of unique users who have flipped this preference? Barbara should be involved in any decision to remove a preference.

> Can you share a link to the query you're using to generate this data? What > is this percentage value you're referring to? Percentage of unique users who > have flipped this preference? These were just the clicks from the screenshot in percentage form (from the Fennec UI telemetry dashboard). > > Barbara should be involved in any decision to remove a preference.\ NI-ing her

(In reply to Chenxia Liu [:liuche] from comment #6) > > Can you share a link to the query you're using to generate this data? What > > is this percentage value you're referring to? Percentage of unique users who > > have flipped this preference? > > These were just the clicks from the screenshot in percentage form (from the > Fennec UI telemetry dashboard). Are you talking about this dashboard? https://people.mozilla.org/~mfinkle/uitelemetry/ I don't think that gets updated regularly, you should be using the re:dash dashboard: https://sql.telemetry.mozilla.org/queries/59 Percentage form is still confusing here, since the percentage changes depending on what you're displaying in the pivot table :)

I'm inclined to put this on hold for now as I don't see this being extremely important. Chenxia, are you basing your suggestion on quantitative data only or have people actually complained about it? If you wanted to pick this up, I'd be in favour of prioritizing privacy over convenience and therefore would vote for clear tabs in this incident.

Joining in. I'm actually complaining about this. It's non-intuitive and was forced on me without explicitly telling me. It broke the clearing of privacy settings. Perhaps people think this is another bug, or they can't find this bug, or they're not so technically inclined to sign up with bugzilla and write a bug report about it. If I weren't technically inclined, I'd simply badmouth firefox and switch browsers instead of going through all the trouble and filing a bug. I was referred to here from bug 1266594, after having filed one of my own. It was difficult enough to search bugzilla, and it's not immediately obvious that the core issue is that the new feature of restore tabs broke the privacy settings.

(In reply to Barbara Bermes [:barbara] - NI please! from comment #8) > I'm inclined to put this on hold for now as I don't see this being extremely > important. Chenxia, are you basing your suggestion on quantitative data only > or have people actually complained about it? > > If you wanted to pick this up, I'd be in favour of prioritizing privacy over > convenience and therefore would vote for clear tabs in this incident. Happy to pick this up. I completely agree that the current behaviour is extremely confusing and misleading. (Initially, I thought it was a bug with history clearing, until I discovered the "Restore tabs" pref).

Nevin, do you think someone in Taipei could take this on? It should be a nice little fix.

Hi Anthony. Please help verify my action item 1. If "Menu" -> "Setting" -> "Privacy" -> "Clear Private data on exit" is checked, only when you click "Quit" in menu item will work. Please view "Delete data on exit" section in (https://support.mozilla.org/en-US/kb/clear-your-browsing-history-and-other-personal-data). If this setting is checked, it'll ignore the "Menu" -> "Setting" -> "Advanced" -> "Restore Tab" -> "Always Restore" option and still close all tabs. 2. Click "Menu" -> "History" -> "CLEAR BROWSING HISTORY" will not close opening tabs( this is the behavior in desktop, see https://support.mozilla.org/en-US/kb/delete-browsing-search-download-history-firefox)

(In reply to Nevin Chen [:nechen] from comment #12) > Hi Anthony. > Please help verify my action item > 1. If "Menu" -> "Setting" -> "Privacy" -> "Clear Private data on exit" is > checked, only when you click "Quit" in menu item will work. Please view > "Delete data on exit" section in > (https://support.mozilla.org/en-US/kb/clear-your-browsing-history-and-other- > personal-data). If this setting is checked, it'll ignore the "Menu" -> > "Setting" -> "Advanced" -> "Restore Tab" -> "Always Restore" option and > still close all tabs. Yes, we want "close all tabs" to be included when a user Quits and has "Clear Private data on exit" enabled. But in comment 4, I wanted us to add "Open tabs" as an item in that dialog (checked ON by default as well). Can we do that? That would be the most obvious UX in this situation. It will also help us avoid the confusion you pointed out below as well. > 2. Click "Menu" -> "History" -> "CLEAR BROWSING HISTORY" will not close > opening tabs( this is the behavior in desktop, see > https://support.mozilla.org/en-US/kb/delete-browsing-search-download-history- > firefox) I don't think this bug includes anything about "History" -> "CLEAR BROWSING HISTORY". This is a separate issue for another bug (that can be avoided if we include "Open tabs" in the "Select which data to clear" and "Clear private data" dialog. tl;dr 1) Add "Open tabs" to the "Select which data to clear" (from the Clear private data on exit pref) and "Clear private data" (from the Clear private data pref) dialog. This item will control if tabs are closed when a user clears private data.

Comment on attachment 8822350 [details] Bug 1275662 - Add "Open tabs" as an item in [Clear Private data on exit] dialog. https://reviewboard.mozilla.org/r/101274/#review101806 This also makes the new "Open Tabs" item also show up in the "Settings -> Clear private data" menu, even though it doesn't do anything there.

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review101800 I don't think this is the correct solution - if I'm not mistaken, all our other history clearing functions run only when the user explicitly quits the browser, whereas this affects session restore even when the browser was merely killed by the OS. Besides, it doesn't even really forgets the tabs, but merely moves them into the recently closed tabs section, where they eventually get overwritten during the *next* startup. In fact, we do already have some infrastructure for making the session store really forget the current session on shutdown, which can e.g. be controlled [from here](https://dxr.mozilla.org/mozilla-central/rev/143bb4b9249e528e658f6ccc449991794b8675f8/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#552). Turning this into something along the lines of ```java if (clearObj.has("private.data.openTabs")) { res.put("dontSaveSession", true); } else if (clearObj.has("private.data.history")) { // existing code } ``` should properly solve this bug. Additionally, [the sanitizer](https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm) is now throwing a TypeError to the console each time private data is cleared with "Open tabs" checked, because it doesn't know how to handle the new `openTabs` data type. Since we clear the current session containing the tabs via the session store, there's no need for any special sanitizer handling, so in my opinion we could just extend [this switch here](https://dxr.mozilla.org/mozilla-central/rev/143bb4b9249e528e658f6ccc449991794b8675f8/mobile/android/chrome/content/browser.js#1458) to *not* pass along `openTabs` to the Sanitizer (with a suitable explaining comment why it's not necessary).

(In reply to Jan Henning [:JanH] from comment #17) > Comment on attachment 8822351 [details] > Bug 1275662 - Privacy setting(clear open tabs on exit) should have higher > priority. > > https://reviewboard.mozilla.org/r/101276/#review101800 > > I don't think this is the correct solution - if I'm not mistaken, all our > other history clearing functions run only when the user explicitly quits the > browser, whereas this affects session restore even when the browser was > merely killed by the OS. > Besides, it doesn't even really forgets the tabs, but merely moves them into > the recently closed tabs section, where they eventually get overwritten > during the *next* startup. > > In fact, we do already have some infrastructure for making the session store > really forget the current session on shutdown, which can e.g. be controlled > [from > here](https://dxr.mozilla.org/mozilla-central/rev/ > 143bb4b9249e528e658f6ccc449991794b8675f8/mobile/android/base/java/org/ > mozilla/gecko/GeckoApp.java#552). > Turning this into something along the lines of > ```java > if (clearObj.has("private.data.openTabs")) { > res.put("dontSaveSession", true); > } else if (clearObj.has("private.data.history")) { > // existing code > } > ``` > should properly solve this bug. > Additionally, [the > sanitizer](https://dxr.mozilla.org/mozilla-central/source/mobile/android/ > modules/Sanitizer.jsm) is now throwing a TypeError to the console each time > private data is cleared with "Open tabs" checked, because it doesn't know You are right. I'll submit a patch later. > how to handle the new `openTabs` data type. > Since we clear the current session containing the tabs via the session > store, there's no need for any special sanitizer handling, so in my opinion > we could just extend [this switch > here](https://dxr.mozilla.org/mozilla-central/rev/ > 143bb4b9249e528e658f6ccc449991794b8675f8/mobile/android/chrome/content/ > browser.js#1458) to *not* pass along `openTabs` to the Sanitizer (with a > suitable explaining comment why it's not necessary).

I'd ask Jan to look at those patches anyways, so I'll just redirect the review request to him. :)

Comment on attachment 8822350 [details] Bug 1275662 - Add "Open tabs" as an item in [Clear Private data on exit] dialog. https://reviewboard.mozilla.org/r/101274/#review102566

Tentatively marking this as depending on bug 1266594 since - some of the problems discovered there are interfering with this as well - it'll introduce a shutdown parameter to BrowserApp.sanitize, which might allow simplifying things while quitting

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review102576 Mainly I'd question the need to close all tabs even in the case were we're quitting the browser, since that'll automatically close all tabs anyway and we're already making sure that the session store won't save any data. Likewise, I still don't see any need to interfere with session restoring during startup. Otherwise, the basic idea is okay, but the actual implementation needs some more work. A tip: Besides watching the logcat for any unwanted (JS) errors, turning on the session store debug logging in about:config (browser.sessionstore.debug_logging) might also be helpful here to verify that things are working as expected. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:750 (Diff revision 2) > "Experiments:GetActive", > "Experiments:SetOverride", > "Experiments:ClearOverride", > "Feedback:MaybeLater", > "Sanitize:ClearHistory", > + "Sanitize:OpenTabs", In the handler for this you immediately call `runOnUiThread()`, so maybe this should rather be registered above? ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1459 (Diff revision 2) > "Experiments:GetActive", > "Experiments:SetOverride", > "Experiments:ClearOverride", > "Feedback:MaybeLater", > "Sanitize:ClearHistory", > + "Sanitize:OpenTabs", Ditto. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1907 (Diff revision 2) > + for (final Tab tab : tabs) { > + Tabs.getInstance().closeTabSilently(tab); > + } > + } > + }); > + callback.sendSuccess(null); I think you should only return success after you've actually closed all tabs, because this triggers the `Sanitizer` to send `browser:purge-session-tabs`, which causes the session store to forget all recently closed tabs. As it stands, this returns as soon as you've fired off the runnable, which could be too early. ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:547 (Diff revision 2) > res.put("sanitize", clearObj); > } catch (JSONException ex) { > Log.e(LOGTAG, "Error adding sanitize object", ex); > } > > - // If the user has opted out of session restore, and does want to clear history > + // If the user has opted out of session restore, and does want to clear history/tab This description doesn't quite fit the new behaviour, because the session restore setting only becomes relevant when *not* clearing open tabs. Possibly something along the lines of: If the user wants to clear open tabs, or else has opted out of session restore and does want to clear history, we also... ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:553 (Diff revision 2) > - final String sessionRestore = getSessionRestorePreference(getSharedPreferences()); > - try { > + try { > + if (clearObj.has("private.data.openTabs")) { > + res.put("dontSaveSession", true); > + } else if (clearObj.has("private.data.history")) { > + Nit: Whitespace? ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:556 (Diff revision 2) > + res.put("dontSaveSession", true); > + } else if (clearObj.has("private.data.history")) { > + > + final String sessionRestore = getSessionRestorePreference(getSharedPreferences()); > res.put("dontSaveSession", "quit".equals(sessionRestore)); > + Nit: Whitespace? ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1806 (Diff revision 2) > shouldRestore = true; > } else if (savedInstanceState != null || > getSessionRestorePreference(prefs).equals("always") || > getRestartFromIntent()) { > + > + // Privacy setting has higher priority here (bug 1275662) I still think this shouldn't be necessary, because if that option is enabled, we're already cleaning out all open tabs when the user hits "Quit", so there'll be nothing to restore on startup anyway. Besides, this'll break session restoring if the browser was merely backgrounded and then killed by the OS. None of our other data sanitizing options runs after a mere background kill, so I don't see why tabs should be handled differently. ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:394 (Diff revision 2) > > public synchronized void closeTab(Tab tab, boolean showUndoToast) { > closeTab(tab, getNextTab(tab), showUndoToast); > } > + // Close the tab without moving it to "Recently closed" tabs > + public synchronized void closeTabSilently(Tab tab) { Not telling Gecko about closing a tab is definitively the wrong solution to this problem - while it doesn't matter so much when quitting (because Gecko will be shutting down anyway), it's noticeable when going through *Settings -> Clear private data*. The tabs appear closed in the UI, but they're still loaded in Gecko, consuming resources and being *tracked by the session store* - if you kill and restart Firefox, they magically reappear. Instead, just call `closeTab(tab, false)` as usual from the `Sanitize:OpenTabs` handler because as long as you call `browser:purge-session-history` *afterwards*, that'll forget all closed tabs anyway. (For the record, for a true "silent" tab close, you'd need to adapt `closeTab()` to allow adding a new parameter to the `Tab:Closed` notification sent to Gecko and then pass that on through the [`TabClose` event](https://dxr.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f/mobile/android/chrome/content/browser.js#1243) (e.g. setting the tabIndex to -1), where it can then be handled by the session store, e.g. by just setting `_lastClosedTabIndex` to -1 and exiting early [here](https://dxr.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f/mobile/android/components/SessionStore.js#619) if a tabIndex < 0 was passed in.) ::: mobile/android/components/SessionStore.js:174 (Diff revision 2) > case "app-startup": > observerService.addObserver(this, "final-ui-startup", true); > observerService.addObserver(this, "domwindowopened", true); > observerService.addObserver(this, "domwindowclosed", true); > observerService.addObserver(this, "browser:purge-session-history", true); > + observerService.addObserver(this, "browser:purge-session-tab", true); session-tab**s** might be better ::: mobile/android/components/SessionStore.js:253 (Diff revision 2) > if (this._notifyClosedTabs) { > this._sendClosedTabsToJava(Services.wm.getMostRecentWindow("navigator:browser")); > } > break; > + > + case "browser:purge-session-tab": // catch sanitization Except for the missing `clearDisk()` call, this duplicates the code above, so it could be merged into one common handler by doing the extra work (deleting files) for one notification, then falling through (with an appropriate comment!) to the other handler for the common stuff. However you'll probably want to call `clearDisk()` anyway even when just cleaning up tabs, because otherwise you'll still have the backup copy of the session file containing all those tabs you wanted to clean up lingering around for a while. So best probably to have both notifications just fall through to the same code [1]. [1] If they're doing the same thing anyway, you could in theory just omit the `browser:purge-session-tab` notification and have the `Sanitizer` send `browser:purge-session-history` in both cases, but since there are some other parts of Gecko out there listening for the latter notification (and possibly add-ons, too), it's not a bad idea to have a separate notification in order to avoid triggering all the other stuff we do when cleaning the complete browsing history. ::: mobile/android/components/SessionStore.js:639 (Diff revision 2) > > onTabClose: function ss_onTabClose(aWindow, aBrowser, aTabIndex) { > if (this._maxTabsUndo == 0) { > return; > } > If you want to introduce a special function for closing tabs without adding them to the "Recently closed" list, this could be the place to handle it in the session store (see comment above). Alternatively, if you're using the normal `closeTab()` and want to explicitly close tabs even during shutdown, you still need to exit early from the session store's `onTabClose` (and set `_lastClosedTabIndex` to -1) by checking for `this._loadState <= STATE_QUITTING` here, otherwise you'll run into trouble a bit below because the window has already been removed from the session store (which is also why I'm saying that closing the tabs when quitting is actually not necessary). ::: mobile/android/components/SessionStore.js:1773 (Diff revision 2) > if (this._notifyClosedTabs) { > this._sendClosedTabsToJava(aWindow); > } > }, > > _sendClosedTabsToJava: function ss_sendClosedTabsToJava(aWindow) { If you still want to close all tabs even when quitting, you need to exit early here if `this._loadState <= STATE_QUITTING`. (Closing all tabs makes the home panel visible again, meanining it wants to receive closed tabs, which won't work because `removeWindow()` has been called.) ::: mobile/android/modules/Sanitizer.jsm:172 (Diff revision 2) > return true; > } > }, > > + > + openTabs: { While the Sanitizer is a good idea for handling *Settings -> Clear private data*, all in all I think that closing all tabs as well when quitting introduces some unnecessary complications. By passing `dontSaveSession` you're already ensuring that the current session data will be completely empty (no "Recently closed tabs" either) and the `browser:purge-session-tab` notification ensures that any other session files that might have been lying around are deleted as well. So additionally closing all tabs even though Gecko will be shutting down anyway is more or less unnecessary work that only complicates things. Bug 1266594 will introduce an `aShutdown` parameter in BrowserApp.sanitize(), so I'd still think there might be some benefit in adapting the [switch here](https://dxr.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f/mobile/android/chrome/content/browser.js#1458) - if `aShutdown` is true, you just call `Services.obs.notifyObservers(null, "browser:purge-session-tab", "");` directly without involving the sanitizer, so you don't generate a ton of additional events by closing all tabs even though Gecko is shutting down anyway. Else (when Gecko doesn't shut down), you can hand off to the Sanitizer as normal. ::: mobile/android/modules/Sanitizer.jsm:176 (Diff revision 2) > + > + openTabs: { > + clear: function () > + { > + return EventDispatcher.instance.sendRequestForResult({ type: "Sanitize:OpenTabs" }) > + .catch(e => Cu.reportError("Java-side history clearing failed: " + e)) Nit: This here is not history, it's tabs. ::: mobile/android/modules/Sanitizer.jsm:179 (Diff revision 2) > + { > + return EventDispatcher.instance.sendRequestForResult({ type: "Sanitize:OpenTabs" }) > + .catch(e => Cu.reportError("Java-side history clearing failed: " + e)) > + .then(function() { > + try { > + // clear history should also clear "Recently Closed" tabs in Android App Ditto, it's tabs, not general history you're clearing. ::: mobile/android/modules/Sanitizer.jsm:185 (Diff revision 2) > + Services.obs.notifyObservers(null, "browser:purge-session-tab", ""); > + } > + catch (e) { } > + > + try { > + var predictor = Cc["@mozilla.org/network/predictor;1"].getService(Ci.nsINetworkPredictor); Do you really want to clear the predictor on closing tabs? As long as we're not clearing the rest of the browsing history, I don't think this is necessary. ::: mobile/android/modules/Sanitizer.jsm:193 (Diff revision 2) > + }); > + }, > + > + get canClear() > + { > + // bug 347231: Always allow clearing history due to dependencies on Copy paste error: This comment is not applicable here.

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review102576 > Not telling Gecko about closing a tab is definitively the wrong solution to this problem - while it doesn't matter so much when quitting (because Gecko will be shutting down anyway), it's noticeable when going through *Settings -> Clear private data*. The tabs appear closed in the UI, but they're still loaded in Gecko, consuming resources and being *tracked by the session store* - if you kill and restart Firefox, they magically reappear. > > > Instead, just call `closeTab(tab, false)` as usual from the `Sanitize:OpenTabs` handler because as long as you call `browser:purge-session-history` *afterwards*, that'll forget all closed tabs anyway. > > (For the record, for a true "silent" tab close, you'd need to adapt `closeTab()` to allow adding a new parameter to the `Tab:Closed` notification sent to Gecko and then pass that on through the [`TabClose` event](https://dxr.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f/mobile/android/chrome/content/browser.js#1243) (e.g. setting the tabIndex to -1), where it can then be handled by the session store, e.g. by just setting `_lastClosedTabIndex` to -1 and exiting early [here](https://dxr.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f/mobile/android/components/SessionStore.js#619) if a tabIndex < 0 was passed in.) > as long as you call `browser:purge-session-history` afterwards Or of course `browser:purge-session-tabs`, as the case may be.

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review103354 ::: mobile/android/components/SessionStore.js:253 (Diff revision 2) > if (this._notifyClosedTabs) { > this._sendClosedTabsToJava(Services.wm.getMostRecentWindow("navigator:browser")); > } > break; > + > + case "browser:purge-session-tab": // catch sanitization Thanks for the advice! But if I do this, cleaning the tabs will also delete the history, am I corect?

> Thanks for the advice! > But if I do this, cleaning the tabs will also delete the history, am I > corect? Sorry I got it. I'll submit another patch later.

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review104598 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1898 (Diff revision 3) > SharedPreferences settings = getPreferences(Activity.MODE_PRIVATE); > settings.edit().putInt(getPackageName() + ".feedback_launch_count", 0).apply(); > break; > > + case "Sanitize:OpenTabs": > + runOnUiThread(new Runnable() { You call registerUiThreadListener() above, so you do not need to use runOnUiThread() here, because you are already on the UI thread. ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:32 (Diff revision 3) > import android.support.v4.app.LoaderManager; > import android.support.v4.app.LoaderManager.LoaderCallbacks; > import android.support.v4.content.Loader; > import android.support.v4.view.ViewPager; > import android.util.AttributeSet; > +import android.util.Log; nit: Not needed? ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:231 (Diff revision 3) > + if (lm.getLoader(LOADER_ID_CONFIG) != null ){ > + lm.getLoader(LOADER_ID_CONFIG).forceLoad(); > + }else{ nit: style: > if (...) { > ... > } else { > ... > } ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:237 (Diff revision 3) > + lm.getLoader(LOADER_ID_CONFIG).forceLoad(); > + }else{ > - lm.initLoader(LOADER_ID_CONFIG, null, mConfigLoaderCallbacks); > + lm.initLoader(LOADER_ID_CONFIG, null, mConfigLoaderCallbacks); > + } > + > + // Load list of panels from configuration This comment should be next to initLoader() I guess? ::: mobile/android/chrome/content/browser.js:1468 (Diff revision 3) > + case "openTabs": > + if (aShutdown === true) { > + Services.obs.notifyObservers(null, "browser:purge-session-tab", ""); > + break; > + } > + Do we want to fall-through for aShutdown == false? Maybe add a comment so that it's more obvious.

Comment on attachment 8822350 [details] Bug 1275662 - Add "Open tabs" as an item in [Clear Private data on exit] dialog. https://reviewboard.mozilla.org/r/101274/#review104600

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review104634 A few outstanding comments remain, but otherwise looking fine now. ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:230 (Diff revision 3) > // Don't show the tabs strip until we have the > // list of panels in place. > mTabStrip.setVisibility(View.INVISIBLE); > > - // Load list of panels from configuration > + > + // If HomeConfigLoader already exist, force load to select the current item Out of curiosity since I don't really know this code, could you give me a more detailed explanation in which scenario this is relevant to this bug? ::: mobile/android/chrome/content/browser.js:1465 (Diff revision 3) > promises.push(Sanitizer.clearItem("cookies")); > promises.push(Sanitizer.clearItem("sessions")); > break; > + case "openTabs": > + if (aShutdown === true) { > + Services.obs.notifyObservers(null, "browser:purge-session-tab", ""); You've missed the change to session-tab**s** here. ::: mobile/android/components/SessionStore.js:240 (Diff revision 3) > this.flushPendingState(); > this._loadState = STATE_QUITTING_FLUSHED; > > break; > case "browser:purge-session-history": // catch sanitization > log("browser:purge-session-history"); Turn this into `log(aTopic)` if handling both notifications with one common handler. ::: mobile/android/components/SessionStore.js:262 (Diff revision 3) > if (this._notifyClosedTabs) { > this._sendClosedTabsToJava(Services.wm.getMostRecentWindow("navigator:browser")); > } > break; > + > + case "browser:purge-session-tabs": // catch sanitization You're still duplicating almost all of the code above. At the very least, handle them together and just make `this._clearDisk()` dependent on `if (aTopic == "browser:purge-session-history)` (Case in point: you didn't copy over the changes from [Bug 1266594 part 2](https://hg.mozilla.org/mozilla-central/rev/cd8da5953744) regarding `STATE_QUITTING`/`STATE_QUITTING_FLUSHED`, which are relevant here as well). However within the session store, you really should treat both notifications the same and call `this._clearDisk()` when clearing tabs as well. Otherwise, the backup copy of the session file (possibly containing all those tabs we're trying to forget) will still be lying around and if e.g. Firefox crashes and the main session file gets damaged before the next update of the backup file (which is occurring only every few minutes), all those tabs we were supposed to forget will suddenly be restored from the backup. ::: mobile/android/components/SessionStore.js:1788 (Diff revision 3) > > _sendClosedTabsToJava: function ss_sendClosedTabsToJava(aWindow) { > if (!aWindow.__SSID) { > throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG); > } > - > + if (this._loadState <= STATE_QUITTING) { Thanks for adding this, but I've noticed I didn't think this quite through - this needs moving above the previous `if` statement to the very beginning, because the preceeding call to `removeWindow()` during shutdown will of course have deleted the window's `__SSID`, so currently we throw in the line above which of course isn't much better than just getting an unhandled exception further down the code. The difference in the logs will be noticeable if you're clearing history and/or tabs on exit and the currently selected tab is showing about:home when quitting. ::: mobile/android/modules/Sanitizer.jsm:182 (Diff revision 3) > + try { > + // clear "Recently Closed" tabs in Android App > + Services.obs.notifyObservers(null, "browser:purge-session-tabs", ""); > + } > + catch (e) { } > + Whitespace?

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review104634 > Out of curiosity since I don't really know this code, could you give me a more detailed explanation in which scenario this is relevant to this bug? When the user navigate from setting (Activity) back to BrowserApp(Activity), at that time all tabs are closed programatically and a new About:Home tab will be added. Since LoaderManager will only create the loader once, at this moment the loader won't load HomeConfig. So https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#498 won't be called. Thus I need to force run the loader to get the HomeConfig again and make HomePager/HomeScreen display the correct panel. If I don't do this, the tab will just show blank.

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review104634 > When the user navigate from setting (Activity) back to BrowserApp(Activity), at that time all tabs are closed programatically and a new About:Home tab will be added. Since LoaderManager will only create the loader once, at this moment the loader won't load HomeConfig. So https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#498 won't be called. Thus I need to force run the loader to get the HomeConfig again and make HomePager/HomeScreen display the correct panel. If I don't do this, the tab will just show blank. Thanks, I had susepcted it might have to do with something like that, but I wasn't really sure what exactly.

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review104846 One superfluous whitespace change slipped in and your commit message for this patch needs tidying up. ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:561 (Diff revision 5) > + > + } > - } catch (JSONException ex) { > + } catch (JSONException ex) { > - Log.e(LOGTAG, "Error adding session restore data", ex); > + Log.e(LOGTAG, "Error adding session restore data", ex); > - } > + } > - } > + Whitespace

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review105280 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1917 (Diff revision 6) > + final Iterable<Tab> tabs = Tabs.getInstance().getTabsInOrder(); > + for (final Tab tab : tabs) { > + Tabs.getInstance().closeTab(tab, false); > + } nit: We have this snippet in a couple of places now. I wonder why we do not just add a Tabs.getInstance().closeAll() method. ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:228 (Diff revision 6) > + // If HomeConfigLoader already exist, force load to select the current item > + if (lm.getLoader(LOADER_ID_CONFIG) != null) { > + lm.getLoader(LOADER_ID_CONFIG).forceLoad(); > + } else { > - // Load list of panels from configuration > + // Load list of panels from configuration > - lm.initLoader(LOADER_ID_CONFIG, null, mConfigLoaderCallbacks); > + lm.initLoader(LOADER_ID_CONFIG, null, mConfigLoaderCallbacks); > + } > This seems to make sense but I wonder how this is related to this bug / patch?

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review105280 > This seems to make sense but I wonder how this is related to this bug / patch? I'd already asked that - the answer is [here](https://bugzilla.mozilla.org/show_bug.cgi?id=1275662#c33).

Comment on attachment 8822351 [details] Bug 1275662 - Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. https://reviewboard.mozilla.org/r/101276/#review105280 > nit: We have this snippet in a couple of places now. I wonder why we do not just add a Tabs.getInstance().closeAll() method. I'll create another bug for this refactoring cause there are too many and may not direct related to this bug.

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6dad16396f74 Add "Open tabs" as an item in [Clear Private data on exit] dialog. r=JanH,sebastian https://hg.mozilla.org/integration/autoland/rev/8bfeb8f4a4f5 Close all tabs and clean up session files when "Open Tabs" is selected in "Clear private data on exit" or "Clear private data" pref. r=JanH,sebastian

https://hg.mozilla.org/mozilla-central/rev/6dad16396f74 https://hg.mozilla.org/mozilla-central/rev/8bfeb8f4a4f5

Can we add a localization note explaining that "Open" here is not an action, like a button, but an adjective? Explaining that the string is used for a pref to clear open tabs would be enough to clear any confusion.

Yes..I think we should. Can you help give me an example how to do that? Thank you!

Sorry, I realize I must have picked reporter instead of assignee for the NI. You can see the format in the same dtd file https://hg.mozilla.org/mozilla-central/file/6dad16396f74/mobile/android/base/locales/en-US/android_strings.dtd#l85 Possible comment: <!-- Localization note (pref_private_data_openTabs): Open tabs is an option in the Clear Private Data dialog and refers to currently open tabs. -->

Looks like mozreview won't let me add a new patch to the closed review. Actually I think an additional patch for this minor fix should be supported... Anyway, I'm opening a new bug 1333006 for this

Verified as fixed in build 54.0a1 and 53.0a2 (2017-02-15); Device: Asus ZenPad 8 (Android 6.0.1). "Open tabs" option is displayed in Privacy -> Clear private data on exit (Quit Menu), and also in Settings -> Clear private data. With "Always restore" option enabled and with "Open tabs" checked, the tabs were not restored. And with "Always restore" option enabled and "Open tabs" unchecked, the tabs were restored.