Last Comment Bug 784558 - [Page Thumbnails] single thumbnail removal doesn't work anymore when the corresponding site is removed from history
: [Page Thumbnails] single thumbnail removal doesn't work anymore when the corr...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on:
Blocks: 754671
  Show dependency treegraph
 
Reported: 2012-08-21 17:25 PDT by Tim Taubert [:ttaubert]
Modified: 2012-08-25 08:46 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected


Attachments
patch v1 (3.22 KB, patch)
2012-08-21 17:25 PDT, Tim Taubert [:ttaubert]
felipc: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review
patch v2 (5.77 KB, patch)
2012-08-24 13:25 PDT, Tim Taubert [:ttaubert]
felipc: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-08-21 17:25:47 PDT
Created attachment 654032 [details] [diff] [review]
patch v1

The last iteration of the patch for bug 754671 introduced the "removeFiles" method in the PageThumbsWorker. Unfortunately Storage.remove() relied on the old "removeFile" method and should now silently fail.

The test I wrote for exactly this was wrong so we didn't catch that. I re-added the "removeFile" method to remove single files and fixed the test as well.
Comment 1 Tim Taubert [:ttaubert] 2012-08-22 10:50:38 PDT
https://hg.mozilla.org/integration/fx-team/rev/5fb14f57f34a
Comment 2 Tim Taubert [:ttaubert] 2012-08-22 13:28:15 PDT
Comment on attachment 654032 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 754671
User impact if declined: removing a site from history doesn't remove its thumbnails
Testing completed (on m-c, etc.): should be in tomorrow's nightly
Risk to taking this patch (and alternatives if risky): low risk, need it for bug 754671 on Aurora
String or UUID changes made by this patch: none.

We need this patch on Aurora as well in combination with the patch from bug 754671.
Comment 3 Alex Keybl [:akeybl] 2012-08-22 15:46:50 PDT
Comment on attachment 654032 [details] [diff] [review]
patch v1

[Triage Comment]
We're letting bug 754671 ride the trains, so no need to uplift to Aurora 16.
Comment 4 Tim Taubert [:ttaubert] 2012-08-22 16:03:53 PDT
Bah, backed out because the test times out on Windows. Seems to be a file lock issue.

https://hg.mozilla.org/integration/fx-team/rev/fd6973289931
Comment 5 Tim Taubert [:ttaubert] 2012-08-23 15:05:45 PDT
Marking Fx 16 as unaffected because I fixed the backported patch from bug 754671 to just use "removeFiles" for now.
Comment 6 Tim Taubert [:ttaubert] 2012-08-24 12:58:59 PDT
Ok, I found the failure. Try looks green now:

https://tbpl.mozilla.org/?tree=Try&rev=b885539b32bb

Going to clean the patch up and upload it.
Comment 7 Tim Taubert [:ttaubert] 2012-08-24 13:25:18 PDT
Created attachment 655124 [details] [diff] [review]
patch v2

Fixed the windows timeout and cleaned up the test.

It was timing out on Windows because Windows sometimes locks the files and we need to re-try removing those files. The missing part was re-adding the URL to the browser history so that the history observer's onDeleteURI() is called again.
Comment 8 :Felipe Gomes (needinfo me!) 2012-08-24 18:12:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e186d2a171
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-25 08:46:45 PDT
https://hg.mozilla.org/mozilla-central/rev/89e186d2a171

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