Last Comment Bug 538595 - Clear Recent History should be able to clear offline cache
: Clear Recent History should be able to clear offline cache
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 436248 538588
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-08 08:26 PST by Nickolay_Ponomarev
Modified: 2012-03-24 10:45 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.82 KB, patch)
2011-11-08 11:57 PST, Honza Bambas (:mayhemer)
limi: ui‑review+
Details | Diff | Review
v1.1 + test (11.47 KB, patch)
2011-12-02 09:33 PST, Honza Bambas (:mayhemer)
dcamp: review+
gavin.sharp: review-
honzab.moz: ui‑review+
Details | Diff | Review
v1.2 + test (9.10 KB, patch)
2011-12-03 10:53 PST, Honza Bambas (:mayhemer)
gavin.sharp: review+
honzab.moz: ui‑review+
Details | Diff | Review
v1.3 + test (8.72 KB, patch)
2011-12-03 13:56 PST, Honza Bambas (:mayhemer)
honzab.moz: review+
honzab.moz: ui‑review+
Details | Diff | Review

Description Nickolay_Ponomarev 2010-01-08 08:26:28 PST
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 2010-01-08 08:58:32 PST
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) 2011-11-08 11:57:13 PST
Created attachment 572929 [details] [diff] [review]
v1
Comment 3 Alex Limi (:limi) — Firefox UX Team 2011-11-15 15:56:09 PST
Comment on attachment 572929 [details] [diff] [review]
v1

Fine by me.
Comment 4 Dave Camp (:dcamp) 2011-11-22 15:57:22 PST
(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) 2011-11-30 16:09:56 PST
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) 2011-12-01 15:56:32 PST
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) 2011-12-02 09:33:01 PST
Created attachment 578614 [details] [diff] [review]
v1.1 + test

Includes test.  Deps on patch from bug 538588.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-02 11:20:56 PST
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) 2011-12-03 10:53:33 PST
Created attachment 578850 [details] [diff] [review]
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: gavin@gavinsharp.com] 2011-12-03 11:10:03 PST
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) 2011-12-03 13:20:55 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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) 2011-12-03 13:56:17 PST
Created attachment 578858 [details] [diff] [review]
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 14 Marco Bonardo [::mak] 2011-12-19 05:40:13 PST
https://hg.mozilla.org/mozilla-central/rev/5d01a082677e
Comment 15 Daniel Holbert [:dholbert] 2012-02-29 16:52:17 PST
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] 2012-03-24 10:45:18 PDT
Doc updated:

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

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

Note You need to log in before you can comment on or make changes to this bug.