Closed Bug 1621445 Opened 5 years ago Closed 3 years ago

Browser history not cleared during SeaMonkey shutdown

Categories

(SeaMonkey :: Bookmarks & History, defect, P1)

Product:
SeaMonkey
Component:
Bookmarks & History
Type:
defect

Tracking

(seamonkey2.53+ fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey 2.88
Tracking Flags:
Tracking Status
seamonkey2.53 + fixed

People

(Reporter: 1ponb, Assigned: frg)

References

(Blocks 2 open bugs)

Details

(Whiteboard: SM2.53.9)

Attachments

(8 files, 3 obsolete files)

Part 1: Move js code out of sanitize.xul pushed in comment #9
(deleted), patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-release+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
1621445-2-sanitizecanclear-2537.patch pushed in comment #22 Target 2.53.7
(deleted), patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-release+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
clearprivatedataUI.PNG
(deleted), image/png
iannbugzilla
: feedback+
rsx11m.pub
: feedback+
Details
1621445-3-sanitizecanclear-v1_3.patch [cc Target 2.85a1]
(deleted), patch
frg
: review+
frg
: approval-comm-release+
frg
: approval-comm-esr60+
Details | Diff | Splinter Review
1621445-4-sanitizeDialog.patch
(deleted), patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-release+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
1621445-6-notification-2539.patch
(deleted), patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-release+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
1621445-5-sanitizecanclear-v1_1-2539.patch
(deleted), patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-release+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
1621445-7-privatdataformat-2539.patch
(deleted), patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-release+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 SeaMonkey/2.53.1 Lightning/5.8.1

Steps to reproduce:

In SeaMonkey latest version 2.53.1, I create 5 profiles and in each profile my main setting is to set:
"Always ckear private date when I close Seamonkey"
with more option that it will erase all of 9 checked items (from "Browsing History", to "Authenticated Session", see menu Preferences>Privacy & Security>Private Data).

I browse some sites, say using profile1, and when I close it, a form called "Clear Private Data" is displayed that warn me before everything deleted; and I hit 'Clear Private Data Now' button.
Then I open a browser again with the same profile (profile1). A bug is, all of the link since last used, are still remembered.

This issue not happened in SeaMonkey version 2.49.5, but now happened in latest version.

Wanted to let you know that I also found this issue for all version (from version 38 to now on) of firefox browser, that why I no longer interested with firefox and I don't want this issue happen to seamonkey.

PS: I'm is a thunderbird and seamonkey lover and currently always make a donation. But now thunderbird stop all donation, please also fix it because I'll glad to make donation

Actual results:

Cookies/History not removed in a Seamonkey (or FireFox) profile

Expected results:

I want Cookies/History is completely removed in a Seamonkey (or FireFox) profile

The current component "mailnews core" seems incorrect. IIUC you're reporting a bug related to history in SeaMonkey.

Component: Security → Bookmarks & History
Product: MailNews Core → SeaMonkey

Whats that means?

You can still donate to Thunderbird: https://www.thunderbird.net/en-US/

Group: mail-core-security

In general clearing history works but seems there is a now problem during profile shutdown. Seems places is already gone when this is called. As in later Firefox we probably need to register a shutdown blocker for the sanitizer. As far as I see it cookies are still cleared ok.

Status: UNCONFIRMED → NEW
status-seamonkey2.53: --- → affected
status-seamonkey2.57esr: --- → affected
Ever confirmed: true
Summary: cookies in multiple profiles → Browser history not cleared duing SeaMonkey shutdown

Maybe my English is not so good. I have found someelse that has the same issue: https://www.linuxquestions.org/questions/slackware-14/seamonkey-2-53-1-on-current-preferences-not-honored-4175670714/

After the latest update, I notice two user settings that are mis-behaving. One of them is clearing browser history (and other private data) when exiting Seamonkey.

I did ahem interrupt the "checking for plugin compatibility" message during the first launch after update, b/c I was trying to look up something for a family member on the phone, so that may have borked the settings temporarily.

However, even after going into Edit/Preferences, un-checking and re-checking every desired box under Privacy & Security/Private Data, including Browsing History, I'm still getting links showing as "visited" immediately after launch. The browsing history is not being cleared at exit.

How would I go about fixing this, without wiping all my preferences?

Is anyone else encountering this behavior?

How would I go about fixing this, without wiping all my preferences?

Nothing you can do unless you are good in javascript. Needs to be fixed in the code.

Is anyone else encountering this behavior?

Yes I can reproduce it or I wouldn't have set the bug to new status. Just not that easy to fix. Much changed in the backend. We are cutting 2.53.2 beta 1 soon. I will try to get it fixed for the final 2.53.2. In the mean time clearing history via "Tools->Clear Private Data" works.

Some preliminary alignment with TB and Fx.

Attachment #9136344 - Flags: review?(iann_bugzilla)
Attachment #9136344 - Flags: approval-comm-release?
Attachment #9136344 - Flags: approval-comm-esr60?
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Summary: Browser history not cleared duing SeaMonkey shutdown → Browser history not cleared during SeaMonkey shutdown
Attachment #9136344 - Attachment is patch: true
Comment on attachment 9136344 [details] [diff] [review] Part 1: Move js code out of sanitize.xul pushed in comment #9 [Triage Comment] LGTM r/a=me
Attachment #9136344 - Flags: review?(iann_bugzilla)
Attachment #9136344 - Flags: review+
Attachment #9136344 - Flags: approval-comm-release?
Attachment #9136344 - Flags: approval-comm-release+
Attachment #9136344 - Flags: approval-comm-esr60?
Attachment #9136344 - Flags: approval-comm-esr60+
Keywords: leave-open
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/444f141ebc17 Part 1: Move js code out of sanitize.xul. r=IanN
Attachment #9136344 - Attachment description: Part 1: Move js code out of sanitize.xul → Part 1: Move js code out of sanitize.xul pushed in comment #9

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (Backlog,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (Backlog,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (Backlog,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Severity: normal → S3

The severity of these bugs was changed, mistakenly, from normal to S3.

Because these bugs have a priority of --, indicating that they have not been previously triaged, these bugs should be changed to Severity of --.

Severity: S3 → --
Severity: -- → S1
Priority: -- → P1

(In reply to Frank-Rainer Grahl (:frg) from comment #6)

How would I go about fixing this, without wiping all my preferences?

Nothing you can do unless you are good in javascript. Needs to be fixed in the code.

Is anyone else encountering this behavior?

Yes I can reproduce it or I wouldn't have set the bug to new status. Just not that easy to fix. Much changed in the backend. We are cutting 2.53.2 beta 1 soon. I will try to get it fixed for the final 2.53.2. In the mean time clearing history via "Tools->Clear Private Data" works.

Sorry just checked this. You said " I will try to get it fixed for the final 2.53.2. In the mean time clearing history via "Tools->Clear Private Data" works." Is that working good for newest current version?

Is that working good for newest current version?

Yes this still works and I am sorry. For working on shutdown it needs some serious refactoring and I am constantly sidetracked. Have already worked a bit more on it but not yet ready. The work in progress patch is at the top of my local patch queue and an older version is in the unofficial releases too.

I'm very appreciate your hardwork, thank you. Why I ask that question is because I'm confused since https://www.seamonkey-project.org/releases/seamonkey2.53.5/ (in KNOWN ISSUES section) reads: "Clearing the browser history during shutdown does currently not work. Please use "Tools->Clear Private Data" as a workaround. The problem is tracked in bug 1621445".

Why I ask that question is because I'm confused
It does work in the dialog just not during shutdown. I am working on makeing both work but no ETA.

Blocks: 696757

Sorry I ask the same question again; may anyone can solve the bug 1621445 ? It is very important for me and I don't know how to solve this, however I can support this project as I can. Thank guys, help me

[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: This mostly does not work with async sanitizers. Most items are already hardcoded to return true.
Testing completed (on m-c, etc.): 2.53 (in Bills builds already for a while).
Risk to taking this patch (and alternatives if risky): no risk.
String changes made by this patch: --

Attachment #9198558 - Flags: review?(iann_bugzilla)
Attachment #9198558 - Flags: approval-comm-release?
Attachment #9198558 - Flags: approval-comm-esr60?

1ponb sorry. I will see that I find some time now to fixing it. Still no promise.

Depends on: 1688403

Comment on attachment 9198558 [details] [diff] [review]
1621445-2-sanitizecanclear-2537.patch pushed in comment #22 Target 2.53.7

[Triage Comment]
LGTM r/a=me

Attachment #9198558 - Flags: review?(iann_bugzilla)
Attachment #9198558 - Flags: review+
Attachment #9198558 - Flags: approval-comm-release?
Attachment #9198558 - Flags: approval-comm-release+
Attachment #9198558 - Flags: approval-comm-esr60?
Attachment #9198558 - Flags: approval-comm-esr60+
Blocks: SeaMonkey2.53Bugs
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/38d9599ef1a8 Part 2: Remove canClear support from the Sanitizer. r=IanN
Attachment #9198558 - Attachment description: 1621445-2-sanitizecanclear-2537.patch → 1621445-2-sanitizecanclear-2537.patch pushed in comment #22 Target 2.53.7
Attached patch WIP-1621445-3-sanitizecanclear.patch (obsolete) (deleted) — — Splinter Review

Part 3 Split to clear preferences into dialog (manually) and onShutdown.

Attached image clearprivatedataUI.PNG (deleted) —

UI as implemented in part 3.
Cleckbox for ask before clearing is gone. I kept the l10n label and key for now. Need to think if it can be resurrected during shutdown if the dialog is shown in a shutdown step before profile shutdown and can set a pref depending on ok/cancel.
For the manual clearing I would always show the dialog. I found this a bit on the dangerous side if I user accidently clicks tools->clear private data.

And now to the fun part 4 to make it actually work on shutdown :)

The In shutdown column will only appear if the checkbox for clearing on shutdown is checked.

Attachment #9201633 - Flags: feedback?(rsx11m.pub)
Attachment #9201633 - Flags: feedback?(iann_bugzilla)

Comment on attachment 9201633 [details]
clearprivatedataUI.PNG

So far so good.
If you add flex="1" to each of the inner groupboxes then it might look better too - can you try that and add a image of it?
Still not sure about the number of groupboxes there is on the panel.

Flags: needinfo?(frgrahl)
Attachment #9201633 - Flags: feedback?(iann_bugzilla) → feedback+
Attachment #9201633 - Flags: feedback?(rsx11m.pub) → feedback+
Comment on attachment 9201632 [details] [diff] [review] WIP-1621445-3-sanitizecanclear.patch >+++ b/suite/components/pref/content/pref-privatedata.xul > >+ <groupbox id="clearDataSettungs" align="start" flex="1"> "clearDataSettings" perhaps? Also remove the align and flex attributes. >+++ b/suite/locales/en-US/chrome/common/pref/pref-privatedata.dtd > <!ENTITY askBeforeClear.label "Ask me before clearing private data"> >-<!ENTITY askBeforeClear.accesskey "k"> >+<!ENTITY askBeforeClear.accesskey "m"> Perhaps leave this as "k" >+++ b/suite/locales/en-US/chrome/common/sanitize.dtd > <!ENTITY itemHistory.label "Browsing History"> > <!ENTITY itemHistory.accesskey "B"> >+<!ENTITY itemHistoryS.accesskey "r"> > <!ENTITY itemUrlBar.label "Location Bar History"> > <!ENTITY itemUrlBar.accesskey "L"> >+<!ENTITY itemUrlBarS.accesskey "o"> This conflicts with Offline Website Data, perhaps "t" instead? > <!ENTITY itemDownloads.label "Download History"> > <!ENTITY itemDownloads.accesskey "D"> >+<!ENTITY itemDownloadsS.accesskey "l"> This conflicts with Location Bar History, perhaps "y" instead? > <!ENTITY itemFormSearchHistory.label "Saved Form and Search History"> > <!ENTITY itemFormSearchHistory.accesskey "F"> >+<!ENTITY itemFormSearchHistoryS.accesskey "y"> Perhaps make this one "m" > <!ENTITY itemCache.label "Cache"> > <!ENTITY itemCache.accesskey "a"> >+<!ENTITY itemCacheS.accesskey "h"> This conflicts with Help button, perhaps "x" instead? > <!ENTITY itemCookies.label "Cookies"> > <!ENTITY itemCookies.accesskey "C"> >+<!ENTITY itemCookiesS.accesskey "k"> Perhaps make this one "e" > <!ENTITY itemOfflineApps.label "Offline Website Data"> > <!ENTITY itemOfflineApps.accesskey "O"> >+<!ENTITY itemOfflineAppsS.accesskey "b"> This conflicts with Browsing History, perhaps "i" instead? > <!ENTITY itemPasswords.label "Saved Passwords"> > <!ENTITY itemPasswords.accesskey "P"> >+<!ENTITY itemPasswordsS.accesskey "v"> > <!ENTITY itemSessions.label "Authenticated Sessions"> > <!ENTITY itemSessions.accesskey "S"> >+<!ENTITY itemSessionsS.accesskey "i"> Perhaps make this one "u"? I think I'd prefer the second group box being disabled rather than hidden (not as simple to do I suspect).
Attached patch 1621445-3-sanitizecanclear-v1_2.patch (obsolete) (deleted) — — Splinter Review

Part 3 migration and pref ui.

Hope I fixed all the access keys. I needed to rename cache to browser cache. Using x would show "Cache (X)" in the dialog.

[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: no working sanitizer
Testing completed (on m-c, etc.): 2.53.8b1 pre
Risk to taking this patch (and alternatives if risky): broken functionality
String changes made by this patch: various.

Attachment #9201632 - Attachment is obsolete: true
Flags: needinfo?(frgrahl)
Attachment #9208856 - Flags: review?(iann_bugzilla)
Attachment #9208856 - Flags: approval-comm-release?
Attachment #9208856 - Flags: approval-comm-esr60?
Comment on attachment 9208856 [details] [diff] [review] 1621445-3-sanitizecanclear-v1_2.patch >+++ b/suite/locales/en-US/chrome/common/sanitize.dtd >-<!ENTITY itemCache.label "Cache"> >+<!ENTITY itemCache.label "Browser Cache"> If you're changing the string, you also need to change the label so maybe change all to itemBrowserCache*.* > <!ENTITY itemCache.accesskey "a"> >+<!ENTITY itemCacheS.accesskey "r"> This clashes with "Browsing History", if you make it "e", then... > <!ENTITY itemCookies.label "Cookies"> > <!ENTITY itemCookies.accesskey "C"> >+<!ENTITY itemCookiesS.accesskey "e"> ...change this to "k" r/a=me with those changes.
Attachment #9208856 - Flags: review?(iann_bugzilla)
Attachment #9208856 - Flags: review+
Attachment #9208856 - Flags: approval-comm-release?
Attachment #9208856 - Flags: approval-comm-release+
Attachment #9208856 - Flags: approval-comm-esr60?
Attachment #9208856 - Flags: approval-comm-esr60+

All accesskeays are now unique. With "e" back as the accesskey for cache I backed out the change to itemBrowserChange. I removed the unused askBeforeClear access key for now. to not start another round of alphabet soup.

r/a+ from IanN retained

Attachment #9208856 - Attachment is obsolete: true
Attachment #9208930 - Flags: review+
Attachment #9208930 - Flags: approval-comm-release+
Attachment #9208930 - Flags: approval-comm-esr60+
Attached patch 1621445-4-sanitizeDialog.patch (deleted) — — Splinter Review

The sanitize dialog is a standalone dialog and I don't think it fits well in base and should be moved to a standalone component. Currently only directly exposed in browser but via the preferences dialog also in other components. l10n can sty in place in common imho.

For clarity I would prefer to rename it to SanitizerDialog.xul too. Any add-ons left using it will be left broken by the changes anyway and need to be adapted.

[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.8b1 pre
Risk to taking this patch (and alternatives if risky): trivial renames
String changes made by this patch: --

Attachment #9208932 - Flags: review?(iann_bugzilla)
Attachment #9208932 - Flags: approval-comm-release?
Attachment #9208932 - Flags: approval-comm-esr60?
Comment on attachment 9208932 [details] [diff] [review] 1621445-4-sanitizeDialog.patch >+++ b/suite/components/sanitize/jar.mn >+comm.jar: >+% content communicator %content/communicator/ contentaccessible=yes >+ content/communicator/sanitizeDialog.xul (content/sanitizeDialog.xul) >+ content/communicator/sanitizeDialog.js (content/sanitizeDialog.js) Do you we need so much space between the first and second part of each line? It would be nice to have the line length < 80 characters. >+++ b/suite/components/sanitize/moz.build >+EXTRA_JS_MODULES += [ >+ 'Sanitizer.jsm', >+] >+ >+with Files('**'): >+ BUG_COMPONENT = ('SeaMonkey', 'General') Is General the best component? Perhaps Bookmarks & History? r/a=me with those addressed.
Attachment #9208932 - Flags: review?(iann_bugzilla)
Attachment #9208932 - Flags: review+
Attachment #9208932 - Flags: approval-comm-release?
Attachment #9208932 - Flags: approval-comm-release+
Attachment #9208932 - Flags: approval-comm-esr60?
Attachment #9208932 - Flags: approval-comm-esr60+
Version: Other Branch → Trunk
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/6e80d37104e2 Part 3: Rename privacy.item. prefs to privacy.clearOnShutdown. r=IanN
Attachment #9208930 - Attachment description: 1621445-3-sanitizecanclear-v1_3.patch → 1621445-3-sanitizecanclear-v1_3.patch [cc Target 2.85a1]

Unfortunately the bug 1621445 unsolved for the current version, hope it will good in the next version.

tracking-seamonkey2.53: --- → ?
tracking-seamonkey2.57esr: --- → ?
Keywords: leave-open
Whiteboard: SM2.53.9
Attached patch 1621445-5-sanitizecanclear-2539.patch (obsolete) (deleted) — — Splinter Review

Landed this for the next 2.53.9b1 pre
The patch is basically a mixup of Fx 56 and 60. sanitizer.jsm is a copy of the 60 version plus our additions for passwords and location bar and minus telemetry.

The dialog now contains the time range but I removed the silly drop down in Fx.
The time range is not honored by all sanitizers and all data will be deleted. This is now how it is in Fx too.
The height will jump if you switch the range back and for to evrything. Same as in Fx. Hint to do it better appreciated.

One additional item site settings has been added. Trying to keep the access keys distinct is a loosing game so I gave up. They will cycle if you use them and that is ok for me.

suite/modules/OfflineAppCacheHelper.jsm has been added. I thought first to split this but it is a 1K file and trivial. Was moved to toolkit in Fx later and deleted in 90 so you will no longer find it there. It was also named offlineAppCache.jsm which I found confusing given the function it exposes and the lower case.

2.57 might need an addtional patch on top. I think the prefpane stuff gone from it. Will test and fix later.

Tested the dialog and the shutdown function and unlike the current code seems to do the job.

Attachment #9224151 - Flags: feedback?(rsx11m.pub)
Attachment #9224151 - Flags: feedback?(iann_bugzilla)

Drive by fix. If you clear all permissions the notification binding will choke on it.

[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: error in the log
Testing completed (on m-c, etc.): 2.53.9
Risk to taking this patch (and alternatives if risky): trivial
String changes made by this patch: --

Attachment #9224152 - Flags: review?(iann_bugzilla)
Attachment #9224152 - Flags: approval-comm-release?
Attachment #9224152 - Flags: approval-comm-esr60?
No longer blocks: 696757
Comment on attachment 9208930 [details] [diff] [review] 1621445-3-sanitizecanclear-v1_3.patch [cc Target 2.85a1] >+++ b/suite/components/pref/content/pref-privatedata.js > /** >- * Sets the label of the "Clear Now..." button according to the >- * privacy.sanitize.promptOnSanitize preference. Read valueFromPreferences to >- * only change the button when the underlying pref changes, since in the case >- * of instantApply=false, the call to clearPrivateDataNow would result in the >- * dialog appearing when the user just unchecked the "Ask me" checkbox. >+ * > */ >+function updateClearOnShutdownBox(aDisable) { I didn't spot this previously but do we need an empty comment block? >+++ b/suite/components/pref/content/pref-privatedata.xul >+ <groupbox id="clearCpdBox" flex="1"> >+ <caption label="&clearData.cpd.label;"/> >+ <checkbox accesskey="&itemHistory.accesskey;" >+ preference="privacy.cpd.history" >+ label="&itemHistory.label;"/> Again, something I didn't spot previously but the order is usually label, accesskey then preference for all these checkboxes please.
Comment on attachment 9224151 [details] [diff] [review] 1621445-5-sanitizecanclear-2539.patch >+++ b/suite/components/pref/content/pref-privatedata.xul >@@ -126,16 +133,19 @@ >+ <checkbox accesskey="&itemSitePreferences.accesskey;" >+ preference="privacy.cpd.siteSettings" >+ label="&itemSitePreferences.label;"/> As mentioned elsewhere, label, accesskey then preference. >@@ -157,13 +167,16 @@ >+ <checkbox accesskey="&itemSitePreferencesS.accesskey;" >+ preference="privacy.clearOnShutdown.siteSettings" >+ label="&itemSitePreferences.label;"/> As mentioned elsewhere, label, accesskey then preference. >+++ b/suite/components/sanitize/Sanitizer.jsm If it doesn't break backporting for this file, can comments honour the 80 character limit and code lines that have a comment at the end, can the comment be moved to line before when it causes the line to break the 80 character limit? >+++ b/suite/components/sanitize/content/sanitizeDialog.js >+ window.resizeBy(0, warningBox.boxObject.height); >+ window.resizeBy(0, -warningBox.boxObject.height); Not sure I like the jumping around of the dialog box these cause. Also, isn't the warning still relevant even when clearing for a time period other than Everything? >+ /** >+ * If the panel that displays a warning when the duration is "Everything" is >+ * not set up, sets it up. Otherwise does nothing. >+ * >+ * @param aDontShowItemList Whether only the warning message should be updated. >+ * True means the item list visibility status should not >+ * be changed. >+ */ >+ prepareWarning(aDontShowItemList) { aDontShowItemsList doesn't seem to be used, can it be removed? >+ var warningStringID; >+ if (this.hasNonSelectedItems()) { >+ warningStringID = "sanitizeSelectedWarning"; >+ } else { >+ warningStringID = "sanitizeEverythingWarning2"; >+ } >+ >+ var warningDesc = document.getElementById("sanitizeEverythingWarning"); If you call the element sanitizeEverythingWarningDesc then you can avoid having to use sanitizeEverythingWarning2 for the string ID. >+ /** >+ * Check if all of the history items have been selected like the default status. >+ */ >+ hasNonSelectedItems() { >+ let checkboxes = document.querySelectorAll("#itemList > [preference]"); >+ for (let i = 0; i < checkboxes.length; ++i) { >+ let pref = document.getElementById(checkboxes[i].getAttribute("preference")); >+ if (!pref.value) >+ return true; >+ } >+ return false; Could the .some() or .every() method be used here instead? >+++ b/suite/components/sanitize/content/sanitizeDialog.xul >+ <separator class="thin"/> >+ >+ <vbox id="sanitizeEverythingWarningBox"> >+ <spacer flex="1"/> >+ <hbox align="center"> >+ <image id="sanitizeEverythingWarningIcon"/> >+ <vbox id="sanitizeEverythingWarningDescBox" flex="1"> >+ <description id="sanitizeEverythingWarning"/> As mentioned earlier change this to "sanitizeEverythingWarningDesc" >+ <description id="sanitizeEverythingUndoWarning">&sanitizeEverythingUndoWarning;</description> >+ </vbox> >+ </hbox> >+ <spacer flex="1"/> >+ </vbox> This block is indented too far. Also, would it look better with this block below the checkboxes? >+ </groupbox> Indentation for this incorrect, needs one more space. >+++ b/suite/locales/en-US/chrome/common/sanitize.properties >+# LOCALIZATION NOTE (sanitizeEverythingWarning2): Warning that appears when >+# "Time range to clear" is set to "Everything" in Clear Recent History dialog, >+# provided that the user has decided to clear all items. >+sanitizeEverythingWarning2=All history will be cleared. See above, this could now be called sanitizeEverythingWarning Is history a confusing term as it clears more than just history, so perhaps Private Data instead? >+# LOCALIZATION NOTE (sanitizeSelectedWarning): Warning that appears when >+# "Time range to clear" is set to "Everything" in Clear Recent History dialog, >+# provided that the user has decided only to clear some history items. >+sanitizeSelectedWarning=All selected items will be cleared. Again in the comment, same query about using the term history. From initial testing not spotted any issues. f+ with the issues above addressed/discussed.
Attachment #9224151 - Flags: feedback?(iann_bugzilla) → feedback+

Comment on attachment 9224152 [details] [diff] [review]
1621445-6-notification-2539.patch

[Triage Comment]
LGTM r/a=me

Attachment #9224152 - Flags: review?(iann_bugzilla)
Attachment #9224152 - Flags: review+
Attachment #9224152 - Flags: approval-comm-release?
Attachment #9224152 - Flags: approval-comm-release+
Attachment #9224152 - Flags: approval-comm-esr60?
Attachment #9224152 - Flags: approval-comm-esr60+

Comment on attachment 9224151 [details] [diff] [review]
1621445-5-sanitizecanclear-2539.patch

Looks good in general

--- a/suite/components/sanitize/Sanitizer.jsm
+++ b/suite/components/sanitize/Sanitizer.jsm
@@ -1,254 +1,981 @@
-/* -- indent-tabs-mode: nil; js-indent-level: 2 -- /
-/ vim: set ts=2 et sw=2 tw=80 filetype=javascript: /
+// -- indent-tabs-mode: nil; js-indent-level: 2 -*-

Why did you remove the "vim" line?
.jsm is not necessarily an always understood file extension...

Attachment #9224151 - Flags: feedback?(rsx11m.pub) → feedback+

I put the warning on top. Look best to me. Can put it below too as a test if needed.

I simplified the whole dialog logic. Thought about it and all the hubhub Fx does is imho not worth it. As suggested and I agree warning should always be displayed. The text flip flop with cycling everything and selected only complicates the logic and is not really needed. All the items are grouped together with no scrolling needed and if we specify that the selected items are cleared this is always correct.

If we need a distinction it is for items not honoring the time range but I think this should be done in a follow-up then.

Followup for the prefs might be needed for 2.57. Will test later.

We almost have no vim lines left. I copied the Mozilla esr 60 sanitizer file for this and it does not have it either. I would get either get rid of them over time or put them in all files.

Comments hopefully all adressed.
[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: sanitizer not working correctly
Testing completed (on m-c, etc.): 2-53-9
Risk to taking this patch (and alternatives if risky): Needs to be fixed sooner or later.
String changes made by this patch: various.

Attachment #9224151 - Attachment is obsolete: true
Attachment #9224198 - Flags: review?(iann_bugzilla)
Attachment #9224198 - Flags: approval-comm-release?
Attachment #9224198 - Flags: approval-comm-esr60?

Just reformatting for already landed patch.

Attachment #9224199 - Flags: review?(iann_bugzilla)
Attachment #9224199 - Flags: approval-comm-release?
Attachment #9224199 - Flags: approval-comm-esr60?

Comment on attachment 9224198 [details] [diff] [review]
1621445-5-sanitizecanclear-v1_1-2539.patch

[Triage Comment]
LGTM r/a=me

Attachment #9224198 - Flags: review?(iann_bugzilla)
Attachment #9224198 - Flags: review+
Attachment #9224198 - Flags: approval-comm-release?
Attachment #9224198 - Flags: approval-comm-release+
Attachment #9224198 - Flags: approval-comm-esr60?
Attachment #9224198 - Flags: approval-comm-esr60+

Comment on attachment 9224199 [details] [diff] [review]
1621445-7-privatdataformat-2539.patch

[Triage Comment]
LGTM r/a=me

Attachment #9224199 - Flags: review?(iann_bugzilla)
Attachment #9224199 - Flags: review+
Attachment #9224199 - Flags: approval-comm-release?
Attachment #9224199 - Flags: approval-comm-release+
Attachment #9224199 - Flags: approval-comm-esr60?
Attachment #9224199 - Flags: approval-comm-esr60+

Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/fc07c2258b50
Part 4: Rename sanitize.xul to sanitizeDialog. r=IanN
https://hg.mozilla.org/comm-central/rev/c30f8ea303bd
Part 5: Align Sanitzer with Firefox. r=IanN
https://hg.mozilla.org/comm-central/rev/bd1cc82c7bea
Part 6: Don't try to process changed permission after using clear all. r=IanN
https://hg.mozilla.org/comm-central/rev/5bf99524a7f6
Part 7: Reformat private data preferences dialog. r=IanN

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey 2.88
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/79a27a84a626 Part 8: Fix missing declaration. r=bustage-fix
Blocks: 1717379

Unfortunately this bug is unfixed for the current version, I appreciate your hard work for this, thank you. Hopefully it will solved soon in the next version.

Unfortunately this bug is unfixed for the current version

Which is expected because it has a target of 2.53.9 not 2.53.8.

If you need this now try the 2.53.9b1 prerelease build:
https://www.wg9s.com/comm-253/

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/d5f5da921ddcd306ee6dc05fa85a1cefb73aa491
Part 1: Move js code out of sanitize.xul. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/c7a6d2a2c63c1e811efaeff302356d4a982136f5
Part 2: Remove canClear support from the Sanitizer. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/ec54fa2f38e1eaefe92f3327c54e0eaa4c343d87
Part 3: Rename privacy.item. prefs to privacy.clearOnShutdown. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/fc7b5f5ac7c062692930a098724bc1707b05b265
Part 4: Rename sanitize.xul to sanitizeDialog. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/147ee7cd4b1f27fe93f83f23f7f6ff9ec6002445
Part 5: Align Sanitzer with Firefox. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/ebec7b6783de0c0c4137de582f16860345c41486
Part 6: Don't try to process changed permission after using clear all. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/9015da3a10c5c8b0370c5a247e9fbedd2f07b785
Part 7: Reformat private data preferences dialog. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/4ae8abc9bed95dfa4c46916d6868a3c6f4f1c51e
Part 8: Fix missing declaration. r=bustage-fix a=me

status-seamonkey2.53: affected → fixed
status-seamonkey2.57esr: affected → ---
tracking-seamonkey2.53: ? → +
tracking-seamonkey2.57esr: ? → ---
Blocks: 1728911
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: