538595 - Clear Recent History should be able to clear offline cache

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Nickolay_Ponomarev]

From bug 499654 comment 8. There's code in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#171 with no UI for clearing offline cache and DOM storage data:
https://developer.mozilla.org/en/Offline_resources_in_Firefox#Storage_location_and_clearing_the_offline_cache
https://developer.mozilla.org/en/DOM/Storage#Storage_location_and_clearing_the_data

We should either kill the code or add back the UI.
(BTW bug 436248 claims the code is also broken)

Comment 1

[Nickolay_Ponomarev]

other issues with the code that could be re-enabled here are in dependencies.

Another thing to remember is that currently Clear Recent History -> Cookies clears the DOM storage (although not always - bug 527667). The currently disabled code also clears DOM storage, so if it's enabled as is, it will be confusing. On the other hand, if it's changed, the logic in the Preferences window (Options -> Advanced -> Network -> Offline data -> Remove) should be tweaked too.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 3

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 572929 [details] [diff] [review]
v1

Fine by me.

Comment 4

[Dave Camp (:dcamp)]

(In reply to Nickolay_Ponomarev from comment #0)

> We should either kill the code or add back the UI.
> (BTW bug 436248 claims the code is also broken)

Are there any automated tests for this functionality?

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 572929 [details] [diff] [review]
v1

Needs also browser peer review, Ehsan, feel free to forward to anyone else.

I will write test for the app cache/user data clearing.


Looks like this gets closer related to bug 538588.  Making the code I want to test here shared between modules and also the test seems quit reasonable now.  See bug 397719 as mentioned in bug 538588 comment 13.


However, bug 397719 seems to be quit old and this bug and bug 538588 are Q4 goals that need to get in ASAP.  So I would rather go a followup way for sharing.

Comment 6

[Honza Bambas (:mayhemer)]

This should land after bug 538588.  We also want to add the removal of offline privileges to the shared module (to fix bug 383014).  The clear recent history dialog is not doing it right now.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1.1 + test

Includes test.  Deps on patch from bug 538588.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 578614 [details] [diff] [review]
v1.1 + test

>diff --git a/browser/base/content/offlineCache.jsm b/browser/base/content/offlineCache.jsm

> let OfflineCacheHelper = {
>   clear: function() {

>+    // Remove all privileges for offline-apps

Why are we including this here? For the sanitize dialog, this is handled separately by the "site specific settings" option (siteSettings), so I don't think having this option also clear permissions makes sense.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1.2 + test

Aha, wasn't aware.  I wanted to be consistent with the Advanced/Clear button, but there is now a discussion to change its behavior too.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 578850 [details] [diff] [review]
v1.2 + test

>diff --git a/browser/base/content/sanitizeDialog.css b/browser/base/content/sanitizeDialog.css

>+#itemList {
>+  padding-bottom: 0.25em;
>+}

Why is this needed? Assuming it is needed, this should probably live in the platform-specific theme CSS file rather than this content CSS file.

>diff --git a/browser/base/content/test/browser_sanitizeDialog.js b/browser/base/content/test/browser_sanitizeDialog.js

>+  function () {

>+    // Give www.example.com privileges to store offline data
>+    var pm = Cc["@mozilla.org/permissionmanager;1"]
>+             .getService(Ci.nsIPermissionManager);
>+    pm.add(URI, "offline-app", Ci.nsIPermissionManager.ALLOW_ACTION);
>+    pm.add(URI, "offline-app", Ci.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);

This should probably go in the next test, where it actually gets cleared? Or is it needed for setItem/openCacheEntry to work?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #10)
> >+#itemList {
> >+  padding-bottom: 0.25em;
> >+}
> 
> Why is this needed? Assuming it is needed, this should probably live in the
> platform-specific theme CSS file rather than this content CSS file.

I can see a scroll bar that disappears after clicking one of the items.  This avoids that scroll bar to appear and has actually no affect on the dialog's layout.

I'll try to find the proper CSS and move it there.  This is AFAIR specific to Windows.

> 
> >diff --git a/browser/base/content/test/browser_sanitizeDialog.js b/browser/base/content/test/browser_sanitizeDialog.js
> 
> >+  function () {
> 
> >+    // Give www.example.com privileges to store offline data
> >+    var pm = Cc["@mozilla.org/permissionmanager;1"]
> >+             .getService(Ci.nsIPermissionManager);
> >+    pm.add(URI, "offline-app", Ci.nsIPermissionManager.ALLOW_ACTION);
> >+    pm.add(URI, "offline-app", Ci.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);
> 
> This should probably go in the next test, where it actually gets cleared? Or
> is it needed for setItem/openCacheEntry to work?

It's obviously needed for the first test.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1.3 + test

only thing changed: removed the bottom padding, it seems like the scroll bar is not any more appearing with the newly added item

Comment 13

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d01a082677e

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/5d01a082677e

Comment 15

[Daniel Holbert [:dholbert]]

Adding dev-doc-needed, since https://developer.mozilla.org/en/Offline_resources_in_Firefox explicitly calls out this bug as being an existing issue.

(Once Firefox 11 is released, I assume we'll want to update that MDN article.)

Comment 16

[Eric Shepherd [:sheppy]]

Doc updated:

https://developer.mozilla.org/en/HTML/Using_the_application_cache

(the one mentioned in comment 42 has been moved to that location)