The default bug view has changed. See this FAQ.

Clear Recent History should be able to clear offline cache

RESOLVED FIXED in Firefox 11

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: mayhemer)

Tracking

({dev-doc-complete})

Trunk
Firefox 11
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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)
(Reporter)

Updated

7 years ago
Depends on: 383014
(Reporter)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 572929 [details] [diff] [review]
v1
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #572929 - Flags: review?(dcamp)
(Assignee)

Updated

5 years ago
Attachment #572929 - Flags: ui-review?(limi)
Comment on attachment 572929 [details] [diff] [review]
v1

Fine by me.
Attachment #572929 - Flags: ui-review?(limi) → ui-review+

Comment 4

5 years ago
(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?
(Assignee)

Comment 5

5 years ago
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.
Attachment #572929 - Flags: review?(ehsan)
(Assignee)

Comment 6

5 years ago
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.
Depends on: 538588
(Assignee)

Comment 7

5 years ago
Created attachment 578614 [details] [diff] [review]
v1.1 + test

Includes test.  Deps on patch from bug 538588.
Attachment #572929 - Attachment is obsolete: true
Attachment #578614 - Flags: ui-review+
Attachment #578614 - Flags: superreview?(ehsan)
Attachment #578614 - Flags: review?(dcamp)
Attachment #572929 - Flags: review?(ehsan)
Attachment #572929 - Flags: review?(dcamp)

Updated

5 years ago
Attachment #578614 - Flags: review?(dcamp) → review+
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.
Attachment #578614 - Flags: superreview?(ehsan) → review-
(Assignee)

Comment 9

5 years ago
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.
Attachment #578614 - Attachment is obsolete: true
Attachment #578850 - Flags: ui-review+
Attachment #578850 - Flags: review?(gavin.sharp)
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?
Attachment #578850 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
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
Attachment #578850 - Attachment is obsolete: true
Attachment #578858 - Flags: ui-review+
Attachment #578858 - Flags: review+
No longer depends on: 383014
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d01a082677e
Target Milestone: --- → Firefox 11
https://hg.mozilla.org/mozilla-central/rev/5d01a082677e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.)
Keywords: dev-doc-needed
Doc updated:

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

(the one mentioned in comment 42 has been moved to that location)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.