Closed Bug 538595 Opened 10 years ago Closed 8 years ago

Clear Recent History should be able to clear offline cache

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 11

People

(Reporter: asqueella, Assigned: mayhemer)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

Depends on: 383014
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.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #572929 - Flags: review?(dcamp)
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+
(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 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)
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
Attached patch v1.1 + test (obsolete) — Splinter Review
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)
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-
Attached patch v1.2 + test (obsolete) — Splinter Review
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+
(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.
Attached patch v1.3 + testSplinter Review
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
https://hg.mozilla.org/mozilla-central/rev/5d01a082677e
Status: ASSIGNED → RESOLVED
Closed: 8 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)
You need to log in before you can comment on or make changes to this bug.