279710 - tidy up popup blocking code

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

not really much to do here, except removing some interface methods and fixing up
some implementation nits. {Get,Set}DefaultPermission is unused... the pref will
give that information if code wants to check it.

Comment 1

[[e-mail]]

Attachment: patch

dveditz, i'm not sure if you're still doing reviews or not, but this is an easy
one :)

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 172330 [details] [diff] [review]
patch

> nsPopupWindowManager::Observe(nsISupports *aSubject, 
>                               const char *aTopic,
>                               const PRUnichar *aData)
> {
>-  NS_LossyConvertUCS2toASCII pref(aData);
>-  if (pref.Equals(kPopupDisablePref)) {
>+  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
>+  NS_ASSERTION(!nsCRT::strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, aTopic),
>+               "unexpected topic - we only deal with pref changes!");
>+
>+  if (prefBranch) {

Doesn't this change mean we hit the pref hashtable for every pref change
anywhere in the browser? I don't know how often that actually happens but it
seems like we should keep the prefname check too.

>     // refresh our local copy of the "disable popups" pref
>     PRBool permission = PR_FALSE;
>+    prefBranch->GetBoolPref(kPopupDisablePref, &permission);

Should permission default to PR_TRUE, blocking popups?

> // {4275d3f4-752a-427a-b432-14d5dda1c20b}
> #define NS_POPUPWINDOWMANAGER_CID \
>- {0x4275d3f4, 0x752a, 0x427a, {0xb4, 0x32, 0x14, 0xd5, 0xdd, 0xa1, 0xc2, 0x0b}}
>+ {0x822bcd11, 0x6432, 0x48be, {0x9e, 0x9d, 0x36, 0xf7, 0x80, 0x4b, 0x77, 0x47}}

Please make the comment match

Comment 3

[[e-mail]]

(In reply to comment #2)
> Doesn't this change mean we hit the pref hashtable for every pref change
> anywhere in the browser? I don't know how often that actually happens but it
> seems like we should keep the prefname check too.

Woah... but we registered ourselves as an observer for just that pref, right?
Will that notification fire on _any_ pref change?

> Should permission default to PR_TRUE, blocking popups?

Hmm, given that the pref defaults to true, it probably should.

> Please make the comment match

Will do.

Comment 4

[[e-mail]]

http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/prefapi.cpp#862

the prefservice will only call our ::Observe for changes to that pref.

Comment 5

[[e-mail]]

Attachment: v2

fixed, except for prefname comparison.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 172384 [details] [diff] [review]
v2

r/sr=dveditz.

If we're registered for just that pref (and we are, I checked this time unlike
last time), why did the original code bother with the name compare?

Comment 7

[[e-mail]]

who knows. that's why it needed cleaning up ;)

checked in, thanks for the quick reviews.