Closed Bug 767693 Opened 12 years ago Closed 12 years ago

[New Tab Page] need to save block list when unblocking a site

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file)

Attached patch patch v1Splinter Review
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.
Attachment #636061 - Flags: review?(jaws)
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");
(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 on attachment 636061 [details] [diff] [review]
patch v1

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

OK, that makes sense now. Thanks for the clarification.
Attachment #636061 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/fx-team/rev/a6aeae2f1514
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/a6aeae2f1514
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: