Last Comment Bug 767693 - [New Tab Page] need to save block list when unblocking a site
: [New Tab Page] need to save block list when unblocking a site
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks: 735987
  Show dependency treegraph
 
Reported: 2012-06-23 04:23 PDT by Tim Taubert [:ttaubert]
Modified: 2012-06-30 15:17 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (2.78 KB, patch)
2012-06-23 04:23 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-06-23 04:23:38 PDT
Created attachment 636061 [details] [diff] [review]
patch v1

When bug 735987 was fixed I forgot to save the modified block list to disk after unblocking a site. This can lead to the issue described in bug 735987 again when restarting the browser because the 'unblock' modification is lost.
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-06-26 01:20:31 PDT
Comment on attachment 636061 [details] [diff] [review]
patch v1

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

::: browser/base/content/test/newtab/browser_newtab_bug735987.js
@@ +18,5 @@
>    checkGrid("0,99p,1,2,3,4,5,6,7");
>  
> +  NewTabUtils.blockedLinks.resetCache();
> +  yield addNewTabPageTab();
> +  checkGrid("0,99p,1,2,3,4,5,6,7");

I don't see a |block| between the |simulateDrop| on line 17 and the |resetCache| on line 20, so I'm not sure if this test is actually reproducing the problem that this patch is trying to fix.

Shouldn't this be:
> yield simulateDrop(1);
> checkGrid("0,99p,1,2,3,4,5,6,7");
> NewTabUtils.blockedLinks.block("about:blank#99p");
> NewTabUtils.blockedLinks.resetCache();
> yield addNewTabPageTab();
> checkGrid("0,99p,1,2,3,4,5,6,7");
Comment 2 Tim Taubert [:ttaubert] 2012-06-26 06:25:02 PDT
(In reply to Jared Wein [:jaws] from comment #1)
> I don't see a |block| between the |simulateDrop| on line 17 and the
> |resetCache| on line 20, so I'm not sure if this test is actually
> reproducing the problem that this patch is trying to fix.

I checked that the test is failing without the patch applied.

The block() call from line 14 is important. This should be undone with the simulateDrop() call from line 17. We check this by trying to block again on line 24 and see if we were successful.

The only difference here is that I simulated a user restarting the browser by just clearing the BlockedLinks cache and opening a new about:newtab instance.

> Shouldn't this be:
> > yield simulateDrop(1);
> > checkGrid("0,99p,1,2,3,4,5,6,7");
> > NewTabUtils.blockedLinks.block("about:blank#99p");
> > NewTabUtils.blockedLinks.resetCache();
> > yield addNewTabPageTab();
> > checkGrid("0,99p,1,2,3,4,5,6,7");

To explain again, we're checking if unblock() is working when dropping a blocked link onto the grid. block() is definitely working as assured by other tests.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-06-26 12:40:52 PDT
Comment on attachment 636061 [details] [diff] [review]
patch v1

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

OK, that makes sense now. Thanks for the clarification.
Comment 4 Tim Taubert [:ttaubert] 2012-06-26 12:47:46 PDT
https://hg.mozilla.org/integration/fx-team/rev/a6aeae2f1514
Comment 5 Tim Taubert [:ttaubert] 2012-06-30 15:17:03 PDT
https://hg.mozilla.org/mozilla-central/rev/a6aeae2f1514

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