The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

unspecified
Firefox 16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
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");
(Assignee)

Comment 2

5 years ago
(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+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/a6aeae2f1514
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a6aeae2f1514
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.