236255 - Multiple duplicate entries added to popup whitelist

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Jon Henry]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040302 Firefox/0.8.0+

This is a Firefox spinoff of SeaMonkey bug 235454. In some situations, such as
the site linked to in the URL field, many duplicate entries can get added to the
popup whitelist. I've adapted the SeaMonkey patch to Firefox and it seems to work.

Reproducible: Always
Steps to Reproduce:
1. Visit the site linked in the URL field.
2. View the popup whitelist by clicking the icon in the statusbar.


Actual Results:  
The same entry is added dozens of times.

Expected Results:  
Only one entry per site should be in this list.

Comment 1

[Jon Henry]

Attachment: Proposed patch

Here is the patch.

Comment 2

[PikeUK]

This might depend on how the UI for Bug 176564 is done (assuming that bug isn't
WONTFIXed).

Comment 3

[Mike Connor [:mconnor]]

I realize this is a port of the patch for the seamonkey bug, but what is the
thinking in removing the first occurence and putting it at the end?  I would
think that it would be saner to just suppress adding further entries and then
popups appear in the order they initially would have appeared in.

Thoughts?

Comment 4

[Jon Henry]

Without actually speaking to mvl, who wrote the SeaMonkey patch, I guess that he
wanted the sites in "newest first" order which now that you mention it isn't
particularly logical. It also seems like it would be less efficient from a
performance standpoint. If you think it would be an improvement, I can redo the
patch to keep the entries in order and just not do anything when a dupe would be
added.

Comment 5

[Mike Connor [:mconnor]]

yeah, that seems more logical from where I sit, and slightly better perf-wise

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 142784 [details] [diff] [review]
Proposed patch

obsoleting patch, not what we wanted, but patch never got updated.  Also, I
think this is probably bitrotted/not valid with the post-0.9 fixes (someone
please verify)

Comment 7

[Phil Ringnalda (:philor)]

This is why it's a good idea to put testcases in bug attachments, not link to them. To the extent that crashme was actually just an automated version of the testcase in bug 235434, WFM.