Closed Bug 1201365 Opened 5 years ago Closed 5 years ago

Remove offline data in the advanced preferences is broken

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox42 + verified
firefox43 --- verified

People

(Reporter: marco, Assigned: marco)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch fix_clear_offline_data (obsolete) — Splinter Review
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8656357 - Flags: review?(jaws)
Comment on attachment 8656357 [details] [diff] [review]
fix_clear_offline_data

Review of attachment 8656357 [details] [diff] [review]:
-----------------------------------------------------------------

browser/components/preferences/advanced.js was just removed today by bug 1140495. Only the in-content change is necessary.
Attachment #8656357 - Flags: review?(jaws) → review+
Carrying r+.

There isn't any test covering this, so I haven't sent the patch to try.
Attachment #8656357 - Attachment is obsolete: true
Attachment #8656476 - Flags: review+
Keywords: checkin-needed
(In reply to Marco Castelluccio [:marco] from comment #3)
> Created attachment 8656476 [details] [diff] [review]
> fix_clear_offline_data
> 
> Carrying r+.
> 
> There isn't any test covering this, so I haven't sent the patch to try.

Can you please file a new bug to add tests for this? Would be great if you attached a patch there too ;)
Flags: needinfo?(mar.castelluccio)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
> Can you please file a new bug to add tests for this? Would be great if you attached a patch there too ;)

Filed bug 1203354. Unfortunately I don't have time to do it at the moment, but maybe will take a stab at it in the future.
Flags: needinfo?(mar.castelluccio)
Should we uplift the fix to Aurora?
[Tracking Requested - why for this release]: Regression, users can't remove site from Options > Advanced > Network > Offline data.
(In reply to Marco Castelluccio [:marco] from comment #7)
> Should we uplift the fix to Aurora?

Please request approval for beta (42 has merged up), yes. I don't fully understand the additional , true) argument for the clearing call - is that going to work on 42, too?
Flags: needinfo?(mar.castelluccio)
(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Marco Castelluccio [:marco] from comment #7)
> > Should we uplift the fix to Aurora?
> 
> Please request approval for beta (42 has merged up), yes. I don't fully
> understand the additional , true) argument for the clearing call - is that
> going to work on 42, too?

It should work, as the third argument (exactHost) is present in http://mxr.mozilla.org/mozilla-beta/source/netwerk/base/nsIPermissionManager.idl as well.

I don't know if my patch applies cleanly to beta, I'll try tomorrow and publish a new patch in case it doesn't apply.
Flags: needinfo?(mar.castelluccio)
Comment on attachment 8656476 [details] [diff] [review]
fix_clear_offline_data

Approval Request Comment
[Feature/regressing bug #]: I'm not sure what regressed this.
[User impact if declined]: Users won't be able to clear offline data for websites from the "Advanced" preferences
[Describe test coverage new/current, TreeHerder]: No test coverage.
[Risks and why]: The patch is very simple, it simply adds a missing import and a missing parameter in a function.
[String/UUID change made/needed]: None.
Attachment #8656476 - Flags: approval-mozilla-beta?
Regression, tracking.
Attachment #8656476 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I tried on several builds to reproduce this issue but without any luck. However, I've checked on Firefox 44.0a1 (2015-09-30), Firefox 43.0a2 (2015-09-30) and Firefox 42 Beta 2 on Windows 7 64-bit and the “Clear Now” button from about:preferences#advanced works as expected.

Marco, since you've reproduced this bug, could you please confirm that it's fixed now?
Flags: needinfo?(mar.castelluccio)
I wrote the patch, so I tested it while working on it :D
Flags: needinfo?(mar.castelluccio)
Thanks Marco! Based on Comment 15 and Comment 16 I am marking this bug as Verified.
Status: RESOLVED → VERIFIED
Depends on: 1280775
You need to log in before you can comment on or make changes to this bug.